[PATCH] libcamera: rkisp1: Clamp stream configuration to ISP limit on raw path

[Jacopo Mondi]

Hi Umang

On Fri, Oct 04, 2024 at 10:23:38AM GMT, Umang Jain wrote:
> Commit 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not
> supported by the pipeline") introduced a mechanism to determine maximum
> supported sensor resolution and filter out resolutions that cannot be
> supported by the ISP.
>
> However, it missed to update the raw stream configuration path, where
> it should have clamped the raw stream configuration size to the maximum
> sensor supported resolution.
>
> This patch fixes the above issue and can be confirmed with IMX283
> on i.MX8MP:
>
> From:
> ($) cam -c1 -srole=raw,width=5472,height=3072
> INFO Camera camera.cpp:1197 configuring streams: (0) 5472x3648-SRGGB12
> ERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12
> Failed to configure camera
> Failed to start camera session
>
> To:
> ($) cam -c1 -srole=raw,width=5472,height=3072
> INFO Camera camera.cpp:1197 configuring streams: (0) 4096x3072-SRGGB12
> cam0: Capture until user interrupts by SIGINT
> 536.082380 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 25165824
> 536.182378 (10.00 fps) cam0-stream0 seq: 000001 bytesused: 25165824
> 536.282375 (10.00 fps) cam0-stream0 seq: 000002 bytesused: 25165824
> ...
>
> Fixes: 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline")
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> index da8d25c3..feb6d89f 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> @@ -316,9 +316,11 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
>  	if (isRaw) {
>  		/*
>  		 * Use the sensor output size closest to the requested stream
> -		 * size.
> +		 * size while ensuring the output size doesn't exceed ISP limits.
>  		 */
>  		uint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);
> +		cfg->size.boundTo(resolution);
> +
>  		V4L2SubdeviceFormat sensorFormat =
>  			sensor->getFormat({ mbusCode }, cfg->size);

CameraSensor::getFormat() returns a sensor resolution large enough to
accomodate the requested size, at least that's how I read the
following part of the documentation

 * \a size indicates the desired size at the output of the sensor. This function
 * selects the best media bus code and size supported by the sensor according
 * to the following criteria.
 *
 * - The desired \a size shall fit in the sensor output size to avoid the need
 *   to up-scale.

 And this part of the code

                if (sz.width < size.width || sz.height < size.height)
                        continue;

So, at least in my understanding "sensorFormat" could represent a size
larger than cfg->size. Is this your understanding as well ?

In the lines below this function we have

		minResolution = sensorFormat.size;
		maxResolution = sensorFormat.size;

        cfg->size.boundTo(maxResolution);
        cfg->size.expandTo(minResolution);

So if ((maxResolution = minResolution) > cfg->size) will the calls to
boundTo() followed by expandTo() enlarge cfg->size ?

Wouldn't it be better to use 'resolution' in

		V4L2SubdeviceFormat sensorFormat =
			sensor->getFormat({ mbusCode }, cfg->size);

instead of cfg->size ?

Thanks
  j

>
> --
> 2.45.2
>