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

[Quentin Schulz]

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