| Message ID | 20250403180902.484863-1-foss+libcamera@0leil.net |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Quentin On Thu, Apr 03, 2025 at 08:09:00PM +0200, Quentin Schulz wrote: > From: Quentin Schulz <quentin.schulz@cherry.de> > > This reverts commit e85c7ddd38ce8456ab01c2a73baf9e788f6a462e. > > Linux kernel predating 6.4 (specifically commit 7cfb35d3a800 ("media: > rkisp1: Implement ENUM_FRAMESIZES") do not have the ioctl in rkisp1 > driver required to dynamically query the resizer limits. > > Because of that, maxResolution and minResolution are both {0, 0} > (default value for Size objects) which means filterSensorResolution() > will create an entry for the sensor in sensorSizesMap_ but because the > sensor resolution cannot fit inside the min and max resolution of the > rkisp1, no size is put into this entry in sensorSizesMap_. > On the next call to filterSensorResolution(), > sensorSizesMap_.find(sensor) will return the entry but when attempting > to call back() on iter->second, it'll trigger an assert because the size > array is empty. > > Linux kernel 6.1 is supported until December 2027, so it seems premature > to get rid of those hard-coded resizer limits before this happens. > > Let's keep the hard-coded resizer limits by default, they can still be > queried if necessary. So I presume you hit LOG(RkISP1, Info) << "Failed to enumerate supported formats and sizes, using defaults"; > > Fixes: 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline") Not sure about the fixes tag here, but indeed if this fixes an issue on an older but not-yet-dead kernel at the cheap cost of a having a few defaults here, then: Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de> > --- > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 21 +++++++++++++------ > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 ++- > 2 files changed, 17 insertions(+), 7 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > index eee5b09e..d43f31e1 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > @@ -54,8 +54,11 @@ const std::map<PixelFormat, uint32_t> formatToMediaBus = { > > } /* namespace */ > > -RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats) > - : name_(name), running_(false), formats_(formats), link_(nullptr) > +RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats, > + const Size &minResolution, const Size &maxResolution) > + : name_(name), running_(false), formats_(formats), > + minResolution_(minResolution), maxResolution_(maxResolution), > + link_(nullptr) > { > } > > @@ -517,10 +520,12 @@ void RkISP1Path::stop() > } > > /* > - * \todo Remove the hardcoded formats once all users will have migrated to a > - * recent enough kernel. > + * \todo Remove the hardcoded resolutions and formats once all users will have > + * migrated to a recent enough kernel. > */ > namespace { > +constexpr Size RKISP1_RSZ_MP_SRC_MIN{ 32, 16 }; > +constexpr Size RKISP1_RSZ_MP_SRC_MAX{ 4416, 3312 }; > constexpr std::array<PixelFormat, 18> RKISP1_RSZ_MP_FORMATS{ > formats::YUYV, > formats::NV16, > @@ -542,6 +547,8 @@ constexpr std::array<PixelFormat, 18> RKISP1_RSZ_MP_FORMATS{ > formats::SRGGB12, > }; > > +constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 }; > +constexpr Size RKISP1_RSZ_SP_SRC_MAX{ 1920, 1920 }; > constexpr std::array<PixelFormat, 8> RKISP1_RSZ_SP_FORMATS{ > formats::YUYV, > formats::NV16, > @@ -555,12 +562,14 @@ constexpr std::array<PixelFormat, 8> RKISP1_RSZ_SP_FORMATS{ > } /* namespace */ > > RkISP1MainPath::RkISP1MainPath() > - : RkISP1Path("main", RKISP1_RSZ_MP_FORMATS) > + : RkISP1Path("main", RKISP1_RSZ_MP_FORMATS, > + RKISP1_RSZ_MP_SRC_MIN, RKISP1_RSZ_MP_SRC_MAX) > { > } > > RkISP1SelfPath::RkISP1SelfPath() > - : RkISP1Path("self", RKISP1_RSZ_SP_FORMATS) > + : RkISP1Path("self", RKISP1_RSZ_SP_FORMATS, > + RKISP1_RSZ_SP_SRC_MIN, RKISP1_RSZ_SP_SRC_MAX) > { > } > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > index 2a1ef0ab..430181d3 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > @@ -34,7 +34,8 @@ struct V4L2SubdeviceFormat; > class RkISP1Path > { > public: > - RkISP1Path(const char *name, const Span<const PixelFormat> &formats); > + RkISP1Path(const char *name, const Span<const PixelFormat> &formats, > + const Size &minResolution, const Size &maxResolution); > > bool init(MediaDevice *media); >
Quoting Jacopo Mondi (2025-04-04 07:56:33) > Hi Quentin > > On Thu, Apr 03, 2025 at 08:09:00PM +0200, Quentin Schulz wrote: > > From: Quentin Schulz <quentin.schulz@cherry.de> > > > > This reverts commit e85c7ddd38ce8456ab01c2a73baf9e788f6a462e. > > > > Linux kernel predating 6.4 (specifically commit 7cfb35d3a800 ("media: > > rkisp1: Implement ENUM_FRAMESIZES") do not have the ioctl in rkisp1 > > driver required to dynamically query the resizer limits. > > > > Because of that, maxResolution and minResolution are both {0, 0} > > (default value for Size objects) which means filterSensorResolution() > > will create an entry for the sensor in sensorSizesMap_ but because the > > sensor resolution cannot fit inside the min and max resolution of the > > rkisp1, no size is put into this entry in sensorSizesMap_. > > On the next call to filterSensorResolution(), > > sensorSizesMap_.find(sensor) will return the entry but when attempting > > to call back() on iter->second, it'll trigger an assert because the size > > array is empty. > > > > Linux kernel 6.1 is supported until December 2027, so it seems premature > > to get rid of those hard-coded resizer limits before this happens. > > > > Let's keep the hard-coded resizer limits by default, they can still be > > queried if necessary. > > So I presume you hit > > LOG(RkISP1, Info) > << "Failed to enumerate supported formats and sizes, using defaults"; > > > > > Fixes: 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline") > > Not sure about the fixes tag here, but indeed if this fixes an issue > on an older but not-yet-dead kernel at the cheap cost of a having a > few defaults here, then: > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Rather annoyingly, - I think we introduced this because different hardware has different limits, and so we moved the limits to the kernel (where we wish they were all along). Bringing the hardcoded single limits in this patch may potentially restrict platforms maximum size capabilities - such as the i.MX8MP for instance.... I don't object to making sure we can still support 6.1 - but we would then need to revisit how to handle the platform specific differences across all of the platforms supported by the RkISP1.... -- Kieran > > Thanks > j > > > Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de> > > --- > > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 21 +++++++++++++------ > > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 ++- > > 2 files changed, 17 insertions(+), 7 deletions(-) > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > index eee5b09e..d43f31e1 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > @@ -54,8 +54,11 @@ const std::map<PixelFormat, uint32_t> formatToMediaBus = { > > > > } /* namespace */ > > > > -RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats) > > - : name_(name), running_(false), formats_(formats), link_(nullptr) > > +RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats, > > + const Size &minResolution, const Size &maxResolution) > > + : name_(name), running_(false), formats_(formats), > > + minResolution_(minResolution), maxResolution_(maxResolution), > > + link_(nullptr) > > { > > } > > > > @@ -517,10 +520,12 @@ void RkISP1Path::stop() > > } > > > > /* > > - * \todo Remove the hardcoded formats once all users will have migrated to a > > - * recent enough kernel. > > + * \todo Remove the hardcoded resolutions and formats once all users will have > > + * migrated to a recent enough kernel. > > */ > > namespace { > > +constexpr Size RKISP1_RSZ_MP_SRC_MIN{ 32, 16 }; > > +constexpr Size RKISP1_RSZ_MP_SRC_MAX{ 4416, 3312 }; > > constexpr std::array<PixelFormat, 18> RKISP1_RSZ_MP_FORMATS{ > > formats::YUYV, > > formats::NV16, > > @@ -542,6 +547,8 @@ constexpr std::array<PixelFormat, 18> RKISP1_RSZ_MP_FORMATS{ > > formats::SRGGB12, > > }; > > > > +constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 }; > > +constexpr Size RKISP1_RSZ_SP_SRC_MAX{ 1920, 1920 }; > > constexpr std::array<PixelFormat, 8> RKISP1_RSZ_SP_FORMATS{ > > formats::YUYV, > > formats::NV16, > > @@ -555,12 +562,14 @@ constexpr std::array<PixelFormat, 8> RKISP1_RSZ_SP_FORMATS{ > > } /* namespace */ > > > > RkISP1MainPath::RkISP1MainPath() > > - : RkISP1Path("main", RKISP1_RSZ_MP_FORMATS) > > + : RkISP1Path("main", RKISP1_RSZ_MP_FORMATS, > > + RKISP1_RSZ_MP_SRC_MIN, RKISP1_RSZ_MP_SRC_MAX) > > { > > } > > > > RkISP1SelfPath::RkISP1SelfPath() > > - : RkISP1Path("self", RKISP1_RSZ_SP_FORMATS) > > + : RkISP1Path("self", RKISP1_RSZ_SP_FORMATS, > > + RKISP1_RSZ_SP_SRC_MIN, RKISP1_RSZ_SP_SRC_MAX) > > { > > } > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > > index 2a1ef0ab..430181d3 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > > @@ -34,7 +34,8 @@ struct V4L2SubdeviceFormat; > > class RkISP1Path > > { > > public: > > - RkISP1Path(const char *name, const Span<const PixelFormat> &formats); > > + RkISP1Path(const char *name, const Span<const PixelFormat> &formats, > > + const Size &minResolution, const Size &maxResolution); > > > > bool init(MediaDevice *media); > >
Hi Kieran On Fri, Apr 04, 2025 at 08:46:11AM +0100, Kieran Bingham wrote: > Quoting Jacopo Mondi (2025-04-04 07:56:33) > > Hi Quentin > > > > On Thu, Apr 03, 2025 at 08:09:00PM +0200, Quentin Schulz wrote: > > > From: Quentin Schulz <quentin.schulz@cherry.de> > > > > > > This reverts commit e85c7ddd38ce8456ab01c2a73baf9e788f6a462e. > > > > > > Linux kernel predating 6.4 (specifically commit 7cfb35d3a800 ("media: > > > rkisp1: Implement ENUM_FRAMESIZES") do not have the ioctl in rkisp1 > > > driver required to dynamically query the resizer limits. > > > > > > Because of that, maxResolution and minResolution are both {0, 0} > > > (default value for Size objects) which means filterSensorResolution() > > > will create an entry for the sensor in sensorSizesMap_ but because the > > > sensor resolution cannot fit inside the min and max resolution of the > > > rkisp1, no size is put into this entry in sensorSizesMap_. > > > On the next call to filterSensorResolution(), > > > sensorSizesMap_.find(sensor) will return the entry but when attempting > > > to call back() on iter->second, it'll trigger an assert because the size > > > array is empty. > > > > > > Linux kernel 6.1 is supported until December 2027, so it seems premature > > > to get rid of those hard-coded resizer limits before this happens. > > > > > > Let's keep the hard-coded resizer limits by default, they can still be > > > queried if necessary. > > > > So I presume you hit > > > > LOG(RkISP1, Info) > > << "Failed to enumerate supported formats and sizes, using defaults"; > > > > > > > > Fixes: 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline") > > > > Not sure about the fixes tag here, but indeed if this fixes an issue > > on an older but not-yet-dead kernel at the cheap cost of a having a > > few defaults here, then: > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Rather annoyingly, - I think we introduced this because different > hardware has different limits, and so we moved the limits to the kernel > (where we wish they were all along). > > Bringing the hardcoded single limits in this patch may potentially > restrict platforms maximum size capabilities - such as the i.MX8MP for > instance.... If the driver supports ENUM_FRAMESIZES the actual limits should be used, or am I missing something ? > > I don't object to making sure we can still support 6.1 - but we would > then need to revisit how to handle the platform specific differences > across all of the platforms supported by the RkISP1.... > -- > Kieran > > > > > Thanks > > j > > > > > Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de> > > > --- > > > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 21 +++++++++++++------ > > > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 ++- > > > 2 files changed, 17 insertions(+), 7 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > > index eee5b09e..d43f31e1 100644 > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > > @@ -54,8 +54,11 @@ const std::map<PixelFormat, uint32_t> formatToMediaBus = { > > > > > > } /* namespace */ > > > > > > -RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats) > > > - : name_(name), running_(false), formats_(formats), link_(nullptr) > > > +RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats, > > > + const Size &minResolution, const Size &maxResolution) > > > + : name_(name), running_(false), formats_(formats), > > > + minResolution_(minResolution), maxResolution_(maxResolution), > > > + link_(nullptr) > > > { > > > } > > > > > > @@ -517,10 +520,12 @@ void RkISP1Path::stop() > > > } > > > > > > /* > > > - * \todo Remove the hardcoded formats once all users will have migrated to a > > > - * recent enough kernel. > > > + * \todo Remove the hardcoded resolutions and formats once all users will have > > > + * migrated to a recent enough kernel. > > > */ > > > namespace { > > > +constexpr Size RKISP1_RSZ_MP_SRC_MIN{ 32, 16 }; > > > +constexpr Size RKISP1_RSZ_MP_SRC_MAX{ 4416, 3312 }; > > > constexpr std::array<PixelFormat, 18> RKISP1_RSZ_MP_FORMATS{ > > > formats::YUYV, > > > formats::NV16, > > > @@ -542,6 +547,8 @@ constexpr std::array<PixelFormat, 18> RKISP1_RSZ_MP_FORMATS{ > > > formats::SRGGB12, > > > }; > > > > > > +constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 }; > > > +constexpr Size RKISP1_RSZ_SP_SRC_MAX{ 1920, 1920 }; > > > constexpr std::array<PixelFormat, 8> RKISP1_RSZ_SP_FORMATS{ > > > formats::YUYV, > > > formats::NV16, > > > @@ -555,12 +562,14 @@ constexpr std::array<PixelFormat, 8> RKISP1_RSZ_SP_FORMATS{ > > > } /* namespace */ > > > > > > RkISP1MainPath::RkISP1MainPath() > > > - : RkISP1Path("main", RKISP1_RSZ_MP_FORMATS) > > > + : RkISP1Path("main", RKISP1_RSZ_MP_FORMATS, > > > + RKISP1_RSZ_MP_SRC_MIN, RKISP1_RSZ_MP_SRC_MAX) > > > { > > > } > > > > > > RkISP1SelfPath::RkISP1SelfPath() > > > - : RkISP1Path("self", RKISP1_RSZ_SP_FORMATS) > > > + : RkISP1Path("self", RKISP1_RSZ_SP_FORMATS, > > > + RKISP1_RSZ_SP_SRC_MIN, RKISP1_RSZ_SP_SRC_MAX) > > > { > > > } > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > > > index 2a1ef0ab..430181d3 100644 > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > > > @@ -34,7 +34,8 @@ struct V4L2SubdeviceFormat; > > > class RkISP1Path > > > { > > > public: > > > - RkISP1Path(const char *name, const Span<const PixelFormat> &formats); > > > + RkISP1Path(const char *name, const Span<const PixelFormat> &formats, > > > + const Size &minResolution, const Size &maxResolution); > > > > > > bool init(MediaDevice *media); > > >
Hi Jacopo, On 4/4/25 8:56 AM, Jacopo Mondi wrote: > Hi Quentin > > On Thu, Apr 03, 2025 at 08:09:00PM +0200, Quentin Schulz wrote: >> From: Quentin Schulz <quentin.schulz@cherry.de> >> >> This reverts commit e85c7ddd38ce8456ab01c2a73baf9e788f6a462e. >> >> Linux kernel predating 6.4 (specifically commit 7cfb35d3a800 ("media: >> rkisp1: Implement ENUM_FRAMESIZES") do not have the ioctl in rkisp1 >> driver required to dynamically query the resizer limits. >> >> Because of that, maxResolution and minResolution are both {0, 0} >> (default value for Size objects) which means filterSensorResolution() >> will create an entry for the sensor in sensorSizesMap_ but because the >> sensor resolution cannot fit inside the min and max resolution of the >> rkisp1, no size is put into this entry in sensorSizesMap_. >> On the next call to filterSensorResolution(), >> sensorSizesMap_.find(sensor) will return the entry but when attempting >> to call back() on iter->second, it'll trigger an assert because the size >> array is empty. >> >> Linux kernel 6.1 is supported until December 2027, so it seems premature >> to get rid of those hard-coded resizer limits before this happens. >> >> Let's keep the hard-coded resizer limits by default, they can still be >> queried if necessary. > > So I presume you hit > > LOG(RkISP1, Info) > << "Failed to enumerate supported formats and sizes, using defaults"; > Correct. On v0.4.0 (latest version in Buildroot): # cam -c1 -C15 -F --stream pixelformat=NV12,height=1440,width=1920 --strict-formats [0:02:21.164554400] [2523] INFO Camera camera_manager.cpp:327 libcamera v0.4.0 [0:02:21.189272566] [2524] ERROR V4L2 v4l2_videodevice.cpp:1208 /dev/video0[17:cap]: Unable to enumerate frame sizes: Inappropriate ioctl for device [0:02:21.189448733] [2524] INFO RkISP1 rkisp1_path.cpp:88 Failed to enumerate supported formats and sizes, using defaults [0:02:21.189828483] [2524] ERROR V4L2 v4l2_videodevice.cpp:1208 /dev/video1[19:cap]: Unable to enumerate frame sizes: Inappropriate ioctl for device [0:02:21.189896150] [2524] INFO RkISP1 rkisp1_path.cpp:88 Failed to enumerate supported formats and sizes, using defaults [0:02:21.197233900] [2524] WARN CameraSensor camera_sensor_legacy.cpp:501 'ov5675 2-0036': No sensor delays found in static properties. Assuming unverified defaults. [0:02:21.206403316] [2524] WARN RkISP1Agc agc.cpp:46 'AeMeteringMode' parameter not found in tuning file [0:02:21.206555566] [2524] WARN RkISP1Agc agc.cpp:69 No metering modes read from tuning file; defaulting to matrix [0:02:21.206653858] [2524] ERROR Interpolator interpolator.h:50 yaml object must be a list [0:02:21.206697025] [2524] WARN RkISP1Awb awb.cpp:61 Failed to parse 'colourGains' parameter from tuning file; manual colour temperature will not work properly /home/qschulz/work/cobra/build-pp1516-debug/host/opt/ext-toolchain/aarch64-none-linux-gnu/include/c++/14.2.1/bits/stl_vector.h:1237: std::vector<_Tp, _Alloc>::reference std::vector<_Tp, _Alloc>::back() [with _Tp = libcamera::Size; _Alloc = std::allocator<libcamera::Size>; reference = libcamera::Size&]: Assertion '!this->empty()' failed. Aborted >> >> Fixes: 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline") > > Not sure about the fixes tag here, but indeed if this fixes an issue > on an older but not-yet-dead kernel at the cheap cost of a having a > few defaults here, then: > Fair, me neither. It's just that the part that triggers the assert without this revert is added in that patch. I'm not too bothered if there isn't a Fixes: tag, so up to you. Cheers, Quentin
Hi Kieran, On 4/4/25 9:46 AM, Kieran Bingham wrote: > Quoting Jacopo Mondi (2025-04-04 07:56:33) >> Hi Quentin >> >> On Thu, Apr 03, 2025 at 08:09:00PM +0200, Quentin Schulz wrote: >>> From: Quentin Schulz <quentin.schulz@cherry.de> >>> >>> This reverts commit e85c7ddd38ce8456ab01c2a73baf9e788f6a462e. >>> >>> Linux kernel predating 6.4 (specifically commit 7cfb35d3a800 ("media: >>> rkisp1: Implement ENUM_FRAMESIZES") do not have the ioctl in rkisp1 >>> driver required to dynamically query the resizer limits. >>> >>> Because of that, maxResolution and minResolution are both {0, 0} >>> (default value for Size objects) which means filterSensorResolution() >>> will create an entry for the sensor in sensorSizesMap_ but because the >>> sensor resolution cannot fit inside the min and max resolution of the >>> rkisp1, no size is put into this entry in sensorSizesMap_. >>> On the next call to filterSensorResolution(), >>> sensorSizesMap_.find(sensor) will return the entry but when attempting >>> to call back() on iter->second, it'll trigger an assert because the size >>> array is empty. >>> >>> Linux kernel 6.1 is supported until December 2027, so it seems premature >>> to get rid of those hard-coded resizer limits before this happens. >>> >>> Let's keep the hard-coded resizer limits by default, they can still be >>> queried if necessary. >> >> So I presume you hit >> >> LOG(RkISP1, Info) >> << "Failed to enumerate supported formats and sizes, using defaults"; >> >>> >>> Fixes: 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline") >> >> Not sure about the fixes tag here, but indeed if this fixes an issue >> on an older but not-yet-dead kernel at the cheap cost of a having a >> few defaults here, then: >> >> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Rather annoyingly, - I think we introduced this because different > hardware has different limits, and so we moved the limits to the kernel > (where we wish they were all along). > > Bringing the hardcoded single limits in this patch may potentially > restrict platforms maximum size capabilities - such as the i.MX8MP for > instance.... > The support for the i.MX8MP was added in kernel 6.9 in 9f9cd26aec84 ("media: rkisp1: Add match data for i.MX8MP ISP") if I'm not mistaken. That would be after the commit that added the plumbing for the ioctl for detecting the frame sizes, so it would anyway (hopefully, haven't thoroughly checked the code) use the ioctl to set the min and max resolution. > I don't object to making sure we can still support 6.1 - but we would > then need to revisit how to handle the platform specific differences > across all of the platforms supported by the RkISP1.... So I would say we are safe from "other SoCs implementing RkISP" because they were added after the ioctl was added? Cheers, Quentin
Hi all, On 4/4/25 11:15 AM, Quentin Schulz wrote: > Hi Kieran, > > On 4/4/25 9:46 AM, Kieran Bingham wrote: >> Quoting Jacopo Mondi (2025-04-04 07:56:33) >>> Hi Quentin >>> >>> On Thu, Apr 03, 2025 at 08:09:00PM +0200, Quentin Schulz wrote: >>>> From: Quentin Schulz <quentin.schulz@cherry.de> >>>> >>>> This reverts commit e85c7ddd38ce8456ab01c2a73baf9e788f6a462e. >>>> >>>> Linux kernel predating 6.4 (specifically commit 7cfb35d3a800 ("media: >>>> rkisp1: Implement ENUM_FRAMESIZES") do not have the ioctl in rkisp1 >>>> driver required to dynamically query the resizer limits. >>>> >>>> Because of that, maxResolution and minResolution are both {0, 0} >>>> (default value for Size objects) which means filterSensorResolution() >>>> will create an entry for the sensor in sensorSizesMap_ but because the >>>> sensor resolution cannot fit inside the min and max resolution of the >>>> rkisp1, no size is put into this entry in sensorSizesMap_. >>>> On the next call to filterSensorResolution(), >>>> sensorSizesMap_.find(sensor) will return the entry but when attempting >>>> to call back() on iter->second, it'll trigger an assert because the >>>> size >>>> array is empty. >>>> >>>> Linux kernel 6.1 is supported until December 2027, so it seems >>>> premature >>>> to get rid of those hard-coded resizer limits before this happens. >>>> >>>> Let's keep the hard-coded resizer limits by default, they can still be >>>> queried if necessary. >>> >>> So I presume you hit >>> >>> LOG(RkISP1, Info) >>> << "Failed to enumerate supported formats and sizes, >>> using defaults"; >>> >>>> >>>> Fixes: 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not >>>> supported by the pipeline") >>> >>> Not sure about the fixes tag here, but indeed if this fixes an issue >>> on an older but not-yet-dead kernel at the cheap cost of a having a >>> few defaults here, then: >>> >>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >> >> Rather annoyingly, - I think we introduced this because different >> hardware has different limits, and so we moved the limits to the kernel >> (where we wish they were all along). >> >> Bringing the hardcoded single limits in this patch may potentially >> restrict platforms maximum size capabilities - such as the i.MX8MP for >> instance.... >> > > The support for the i.MX8MP was added in kernel 6.9 in 9f9cd26aec84 > ("media: rkisp1: Add match data for i.MX8MP ISP") if I'm not mistaken. > That would be after the commit that added the plumbing for the ioctl for > detecting the frame sizes, so it would anyway (hopefully, haven't > thoroughly checked the code) use the ioctl to set the min and max > resolution. > >> I don't object to making sure we can still support 6.1 - but we would >> then need to revisit how to handle the platform specific differences >> across all of the platforms supported by the RkISP1.... > > So I would say we are safe from "other SoCs implementing RkISP" because > they were added after the ioctl was added? > Any feedback? I'm not sure what needs to be done in order for this patch to be merged? Cheers, Quentin
Hi Quentin On Tue, Apr 22, 2025 at 11:34:55AM +0200, Quentin Schulz wrote: > On 4/4/25 11:15 AM, Quentin Schulz wrote: > > On 4/4/25 9:46 AM, Kieran Bingham wrote: > >> Quoting Jacopo Mondi (2025-04-04 07:56:33) > >>> On Thu, Apr 03, 2025 at 08:09:00PM +0200, Quentin Schulz wrote: > >>>> From: Quentin Schulz <quentin.schulz@cherry.de> > >>>> > >>>> This reverts commit e85c7ddd38ce8456ab01c2a73baf9e788f6a462e. > >>>> > >>>> Linux kernel predating 6.4 (specifically commit 7cfb35d3a800 ("media: > >>>> rkisp1: Implement ENUM_FRAMESIZES") do not have the ioctl in rkisp1 > >>>> driver required to dynamically query the resizer limits. > >>>> > >>>> Because of that, maxResolution and minResolution are both {0, 0} > >>>> (default value for Size objects) which means filterSensorResolution() > >>>> will create an entry for the sensor in sensorSizesMap_ but because the > >>>> sensor resolution cannot fit inside the min and max resolution of the > >>>> rkisp1, no size is put into this entry in sensorSizesMap_. > >>>> On the next call to filterSensorResolution(), > >>>> sensorSizesMap_.find(sensor) will return the entry but when attempting > >>>> to call back() on iter->second, it'll trigger an assert because the > >>>> size > >>>> array is empty. > >>>> > >>>> Linux kernel 6.1 is supported until December 2027, so it seems > >>>> premature > >>>> to get rid of those hard-coded resizer limits before this happens. > >>>> > >>>> Let's keep the hard-coded resizer limits by default, they can still be > >>>> queried if necessary. > >>> > >>> So I presume you hit > >>> > >>> LOG(RkISP1, Info) > >>> << "Failed to enumerate supported formats and sizes, > >>> using defaults"; > >>> > >>>> > >>>> Fixes: 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not > >>>> supported by the pipeline") > >>> > >>> Not sure about the fixes tag here, but indeed if this fixes an issue > >>> on an older but not-yet-dead kernel at the cheap cost of a having a > >>> few defaults here, then: > >>> > >>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > >> > >> Rather annoyingly, - I think we introduced this because different > >> hardware has different limits, and so we moved the limits to the kernel > >> (where we wish they were all along). > >> > >> Bringing the hardcoded single limits in this patch may potentially > >> restrict platforms maximum size capabilities - such as the i.MX8MP for > >> instance.... > > > > The support for the i.MX8MP was added in kernel 6.9 in 9f9cd26aec84 > > ("media: rkisp1: Add match data for i.MX8MP ISP") if I'm not mistaken. > > That would be after the commit that added the plumbing for the ioctl for > > detecting the frame sizes, so it would anyway (hopefully, haven't > > thoroughly checked the code) use the ioctl to set the min and max > > resolution. > > > >> I don't object to making sure we can still support 6.1 - but we would > >> then need to revisit how to handle the platform specific differences > >> across all of the platforms supported by the RkISP1.... > > > > So I would say we are safe from "other SoCs implementing RkISP" because > > they were added after the ioctl was added? > > Any feedback? I'm not sure what needs to be done in order for this patch > to be merged? The Easter holidays took a bit of a toll on patch review. Thanks for pinging. I'll review and reply to the patch.
Hi Quentin, Thank you for the patch. On Thu, Apr 03, 2025 at 08:09:00PM +0200, Quentin Schulz wrote: > From: Quentin Schulz <quentin.schulz@cherry.de> > > This reverts commit e85c7ddd38ce8456ab01c2a73baf9e788f6a462e. > > Linux kernel predating 6.4 (specifically commit 7cfb35d3a800 ("media: > rkisp1: Implement ENUM_FRAMESIZES") do not have the ioctl in rkisp1 > driver required to dynamically query the resizer limits. > > Because of that, maxResolution and minResolution are both {0, 0} > (default value for Size objects) which means filterSensorResolution() > will create an entry for the sensor in sensorSizesMap_ but because the > sensor resolution cannot fit inside the min and max resolution of the > rkisp1, no size is put into this entry in sensorSizesMap_. > On the next call to filterSensorResolution(), > sensorSizesMap_.find(sensor) will return the entry but when attempting > to call back() on iter->second, it'll trigger an assert because the size > array is empty. > > Linux kernel 6.1 is supported until December 2027, so it seems premature > to get rid of those hard-coded resizer limits before this happens. You noticed our secret plan to push people towards mainline :-) Jokes aside, we do our best to support older kernels. I won't commit to supporting v6.1 on all platforms until end of 2027 (or worse, until the end of the CIP support, which will be in 2033) if it would make our life much harder, but in this specific case the effort is low. > Let's keep the hard-coded resizer limits by default, they can still be > queried if necessary. They will always be queried if the ENUM_FRAMESIZES ioctl is implemented, so I would write this as Let's restore the hard-coded resizer limits as fallbacks, limits are still queried from the driver on recent enough kernels. > Fixes: 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline") > Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de> > --- > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 21 +++++++++++++------ > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 ++- > 2 files changed, 17 insertions(+), 7 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > index eee5b09e..d43f31e1 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > @@ -54,8 +54,11 @@ const std::map<PixelFormat, uint32_t> formatToMediaBus = { > > } /* namespace */ > > -RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats) > - : name_(name), running_(false), formats_(formats), link_(nullptr) > +RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats, > + const Size &minResolution, const Size &maxResolution) > + : name_(name), running_(false), formats_(formats), > + minResolution_(minResolution), maxResolution_(maxResolution), > + link_(nullptr) > { > } > > @@ -517,10 +520,12 @@ void RkISP1Path::stop() > } > > /* > - * \todo Remove the hardcoded formats once all users will have migrated to a > - * recent enough kernel. > + * \todo Remove the hardcoded resolutions and formats once all users will have > + * migrated to a recent enough kernel. We can now be a bit more precise: * \todo Remove the hardcoded resolutions and formats once kernels older than * v6.4 will stop receiving LTS support (scheduled for December 2027 for v6.1). I can make those small changes when applying the patch if you're fine with them. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > */ > namespace { > +constexpr Size RKISP1_RSZ_MP_SRC_MIN{ 32, 16 }; > +constexpr Size RKISP1_RSZ_MP_SRC_MAX{ 4416, 3312 }; > constexpr std::array<PixelFormat, 18> RKISP1_RSZ_MP_FORMATS{ > formats::YUYV, > formats::NV16, > @@ -542,6 +547,8 @@ constexpr std::array<PixelFormat, 18> RKISP1_RSZ_MP_FORMATS{ > formats::SRGGB12, > }; > > +constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 }; > +constexpr Size RKISP1_RSZ_SP_SRC_MAX{ 1920, 1920 }; > constexpr std::array<PixelFormat, 8> RKISP1_RSZ_SP_FORMATS{ > formats::YUYV, > formats::NV16, > @@ -555,12 +562,14 @@ constexpr std::array<PixelFormat, 8> RKISP1_RSZ_SP_FORMATS{ > } /* namespace */ > > RkISP1MainPath::RkISP1MainPath() > - : RkISP1Path("main", RKISP1_RSZ_MP_FORMATS) > + : RkISP1Path("main", RKISP1_RSZ_MP_FORMATS, > + RKISP1_RSZ_MP_SRC_MIN, RKISP1_RSZ_MP_SRC_MAX) > { > } > > RkISP1SelfPath::RkISP1SelfPath() > - : RkISP1Path("self", RKISP1_RSZ_SP_FORMATS) > + : RkISP1Path("self", RKISP1_RSZ_SP_FORMATS, > + RKISP1_RSZ_SP_SRC_MIN, RKISP1_RSZ_SP_SRC_MAX) > { > } > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > index 2a1ef0ab..430181d3 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > @@ -34,7 +34,8 @@ struct V4L2SubdeviceFormat; > class RkISP1Path > { > public: > - RkISP1Path(const char *name, const Span<const PixelFormat> &formats); > + RkISP1Path(const char *name, const Span<const PixelFormat> &formats, > + const Size &minResolution, const Size &maxResolution); > > bool init(MediaDevice *media); >
Hi Laurent, On 4/22/25 12:07 PM, Laurent Pinchart wrote: > Hi Quentin, > > Thank you for the patch. > > On Thu, Apr 03, 2025 at 08:09:00PM +0200, Quentin Schulz wrote: >> From: Quentin Schulz <quentin.schulz@cherry.de> >> >> This reverts commit e85c7ddd38ce8456ab01c2a73baf9e788f6a462e. >> >> Linux kernel predating 6.4 (specifically commit 7cfb35d3a800 ("media: >> rkisp1: Implement ENUM_FRAMESIZES") do not have the ioctl in rkisp1 >> driver required to dynamically query the resizer limits. >> >> Because of that, maxResolution and minResolution are both {0, 0} >> (default value for Size objects) which means filterSensorResolution() >> will create an entry for the sensor in sensorSizesMap_ but because the >> sensor resolution cannot fit inside the min and max resolution of the >> rkisp1, no size is put into this entry in sensorSizesMap_. >> On the next call to filterSensorResolution(), >> sensorSizesMap_.find(sensor) will return the entry but when attempting >> to call back() on iter->second, it'll trigger an assert because the size >> array is empty. >> >> Linux kernel 6.1 is supported until December 2027, so it seems premature >> to get rid of those hard-coded resizer limits before this happens. > > You noticed our secret plan to push people towards mainline :-) > And I can only regret airing this plan publicly as it is a good one ;) To be fair, the only product we have that currently needs this patch will have its kernel updated and not require this patch anymore. So, soon not a problem for me anymore :) > Jokes aside, we do our best to support older kernels. I won't commit to > supporting v6.1 on all platforms until end of 2027 (or worse, until the > end of the CIP support, which will be in 2033) if it would make our life > much harder, but in this specific case the effort is low. > Why I sent the revert is to let people know that this could be enough to get libcamera to work on those old(er) kernels. It's always nice to see some patches available, even if they may not be merged because upstream has a specific policy. I understand your position as supporting multiple kernel versions simply doesn't scale well and possibly hinder "innovation" in the code :) >> Let's keep the hard-coded resizer limits by default, they can still be >> queried if necessary. > > They will always be queried if the ENUM_FRAMESIZES ioctl is implemented, > so I would write this as > > Let's restore the hard-coded resizer limits as fallbacks, limits are > still queried from the driver on recent enough kernels. > I think the point is "the hard-coded resizer limits [...] can still be queried if necessary", reading between the lines meaning ENUM_FRAMESIZES ioctl being implemented will make hard-coded resizer limits unnecessary, thus, not queried. Your suggestion works for me, I could/would add "actual" before the second "limits" but eh, doesn't matter much. [...] >> /* >> - * \todo Remove the hardcoded formats once all users will have migrated to a >> - * recent enough kernel. >> + * \todo Remove the hardcoded resolutions and formats once all users will have >> + * migrated to a recent enough kernel. > > We can now be a bit more precise: > > * \todo Remove the hardcoded resolutions and formats once kernels older than > * v6.4 will stop receiving LTS support (scheduled for December 2027 for v6.1). > > I can make those small changes when applying the patch if you're fine > with them. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Works for me! Thanks! Quentin
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp index eee5b09e..d43f31e1 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp @@ -54,8 +54,11 @@ const std::map<PixelFormat, uint32_t> formatToMediaBus = { } /* namespace */ -RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats) - : name_(name), running_(false), formats_(formats), link_(nullptr) +RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats, + const Size &minResolution, const Size &maxResolution) + : name_(name), running_(false), formats_(formats), + minResolution_(minResolution), maxResolution_(maxResolution), + link_(nullptr) { } @@ -517,10 +520,12 @@ void RkISP1Path::stop() } /* - * \todo Remove the hardcoded formats once all users will have migrated to a - * recent enough kernel. + * \todo Remove the hardcoded resolutions and formats once all users will have + * migrated to a recent enough kernel. */ namespace { +constexpr Size RKISP1_RSZ_MP_SRC_MIN{ 32, 16 }; +constexpr Size RKISP1_RSZ_MP_SRC_MAX{ 4416, 3312 }; constexpr std::array<PixelFormat, 18> RKISP1_RSZ_MP_FORMATS{ formats::YUYV, formats::NV16, @@ -542,6 +547,8 @@ constexpr std::array<PixelFormat, 18> RKISP1_RSZ_MP_FORMATS{ formats::SRGGB12, }; +constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 }; +constexpr Size RKISP1_RSZ_SP_SRC_MAX{ 1920, 1920 }; constexpr std::array<PixelFormat, 8> RKISP1_RSZ_SP_FORMATS{ formats::YUYV, formats::NV16, @@ -555,12 +562,14 @@ constexpr std::array<PixelFormat, 8> RKISP1_RSZ_SP_FORMATS{ } /* namespace */ RkISP1MainPath::RkISP1MainPath() - : RkISP1Path("main", RKISP1_RSZ_MP_FORMATS) + : RkISP1Path("main", RKISP1_RSZ_MP_FORMATS, + RKISP1_RSZ_MP_SRC_MIN, RKISP1_RSZ_MP_SRC_MAX) { } RkISP1SelfPath::RkISP1SelfPath() - : RkISP1Path("self", RKISP1_RSZ_SP_FORMATS) + : RkISP1Path("self", RKISP1_RSZ_SP_FORMATS, + RKISP1_RSZ_SP_SRC_MIN, RKISP1_RSZ_SP_SRC_MAX) { } diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h index 2a1ef0ab..430181d3 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h @@ -34,7 +34,8 @@ struct V4L2SubdeviceFormat; class RkISP1Path { public: - RkISP1Path(const char *name, const Span<const PixelFormat> &formats); + RkISP1Path(const char *name, const Span<const PixelFormat> &formats, + const Size &minResolution, const Size &maxResolution); bool init(MediaDevice *media);