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

[Umang Jain]

Hi Jacopo,

On 04/10/24 3:58 pm, Jacopo Mondi wrote:
> Hi Umang
>
> On Fri, Oct 04, 2024 at 03:17:34PM GMT, Umang Jain wrote:
>> 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.
> Sorry, I'm confused. Aren't you confirming me that the resulting
> cfg->size cannot be supported (with this patch I mean) ?

So user passes a relatively high resolution, supported by the sensor 
(but not with ISP)

	cam -c1 -srole=raw,width=5472,height=3648

so cfg-size here is 5472x3648.

So without this patch, you would right now get:

	ERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12
	Failed to configure camera
	Failed to start camera session

With this patch patched, you would get:

	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
....

So What has happened:

User has requested a raw stream with 5472x3648. It was found that this 
can't be supported hence,
the camera configuration is adjusted to a (lower) but maximum supported 
resolution and delivers raw
frames with 4096x3072.

One can argue that we should never adjust camera configuration to a 
lower size than cfg->size (i.e. what the user asked).

But then,

* \var CameraConfiguration::Adjusted
* The configuration has been adjusted to a valid configuration

and

  * \retval CameraConfigutation::Adjusted The configuration has been 
adjusted
  * and is now valid. Parameters may have changed for any stream, and 
stream
  * configurations may have been removed. The caller shall check the
  * configuration carefully.


>
>> Have you noticed the failure case I mentioned in the commit message?
> While the commmit message suggests this patch makes it valid ?
>
> What am I missing ? :)
>
>>> 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
>>>>