[PATCH] Revert "libcamera: rkisp1: Eliminate hard-coded resizer limits"

[Laurent Pinchart]

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 at 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 at 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 at 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);
>  

-- 
Regards,

Laurent Pinchart