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

[Umang Jain]

Hi Jacopo

On 04/10/24 3:07 pm, Jacopo Mondi wrote:
> 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 ?

"sensorFormat" could represent a size larger than cfg->size

and

could also represent a size large than max supported resolution (i.e. 
'resolution' variable in this case)

For e.g. in IMX283 case it would,  5472x3648

>
> In the lines below this function we have
>
> 		minResolution = sensorFormat.size;
> 		maxResolution = sensorFormat.size;

 From the above e.g. of IMX283 minResolution = maxResolution = 5472x3648
>
>          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 ?

so cfg->size is 5472x3648 here, which can't be supported.

Have you noticed the failure case I mentioned in the commit message?
>
> Wouldn't it be better to use 'resolution' in
>
> 		V4L2SubdeviceFormat sensorFormat =
> 			sensor->getFormat({ mbusCode }, cfg->size);
>
> instead of cfg->size ?

No, otherwise it will always pick the highest sensor output size.

For e.g. I request -srole=raw,width=1920,height=1080

"cfg->size" will pick 2736x1824 for IMX283

"resolution" will always pick 4096x3072.
>
> Thanks
>    j
>
>> --
>> 2.45.2
>>