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

[Laurent Pinchart]

Hi Quentin

On Tue, Apr 22, 2025 at 11:34:55AM +0200, Quentin Schulz wrote:
> On 4/4/25 11:15 AM, Quentin Schulz wrote:
> > On 4/4/25 9:46 AM, Kieran Bingham wrote:
> >> Quoting Jacopo Mondi (2025-04-04 07:56:33)
> >>> 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?

The Easter holidays took a bit of a toll on patch review. Thanks for
pinging. I'll review and reply to the patch.

-- 
Regards,

Laurent Pinchart