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

[Quentin Schulz]

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?

Cheers,
Quentin