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

[Jacopo Mondi]

Hi Kieran

On Fri, Apr 04, 2025 at 08:46:11AM +0100, 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....

If the driver supports ENUM_FRAMESIZES the actual limits should be
used, or am I missing something ?

>
> 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....
> --
> Kieran
>
> >
> > Thanks
> >   j
> >
> > > Signed-off-by: Quentin Schulz <quentin.schulz at cherry.de>
> > > ---
> > >  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 21 +++++++++++++------
> > >  src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  3 ++-
> > >  2 files changed, 17 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > > index eee5b09e..d43f31e1 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > > @@ -54,8 +54,11 @@ const std::map<PixelFormat, uint32_t> formatToMediaBus = {
> > >
> > >  } /* namespace */
> > >
> > > -RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats)
> > > -     : name_(name), running_(false), formats_(formats), link_(nullptr)
> > > +RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats,
> > > +                    const Size &minResolution, const Size &maxResolution)
> > > +     : name_(name), running_(false), formats_(formats),
> > > +       minResolution_(minResolution), maxResolution_(maxResolution),
> > > +       link_(nullptr)
> > >  {
> > >  }
> > >
> > > @@ -517,10 +520,12 @@ void RkISP1Path::stop()
> > >  }
> > >
> > >  /*
> > > - * \todo Remove the hardcoded formats once all users will have migrated to a
> > > - * recent enough kernel.
> > > + * \todo Remove the hardcoded resolutions and formats once all users will have
> > > + * migrated to a recent enough kernel.
> > >   */
> > >  namespace {
> > > +constexpr Size RKISP1_RSZ_MP_SRC_MIN{ 32, 16 };
> > > +constexpr Size RKISP1_RSZ_MP_SRC_MAX{ 4416, 3312 };
> > >  constexpr std::array<PixelFormat, 18> RKISP1_RSZ_MP_FORMATS{
> > >       formats::YUYV,
> > >       formats::NV16,
> > > @@ -542,6 +547,8 @@ constexpr std::array<PixelFormat, 18> RKISP1_RSZ_MP_FORMATS{
> > >       formats::SRGGB12,
> > >  };
> > >
> > > +constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 };
> > > +constexpr Size RKISP1_RSZ_SP_SRC_MAX{ 1920, 1920 };
> > >  constexpr std::array<PixelFormat, 8> RKISP1_RSZ_SP_FORMATS{
> > >       formats::YUYV,
> > >       formats::NV16,
> > > @@ -555,12 +562,14 @@ constexpr std::array<PixelFormat, 8> RKISP1_RSZ_SP_FORMATS{
> > >  } /* namespace */
> > >
> > >  RkISP1MainPath::RkISP1MainPath()
> > > -     : RkISP1Path("main", RKISP1_RSZ_MP_FORMATS)
> > > +     : RkISP1Path("main", RKISP1_RSZ_MP_FORMATS,
> > > +                  RKISP1_RSZ_MP_SRC_MIN, RKISP1_RSZ_MP_SRC_MAX)
> > >  {
> > >  }
> > >
> > >  RkISP1SelfPath::RkISP1SelfPath()
> > > -     : RkISP1Path("self", RKISP1_RSZ_SP_FORMATS)
> > > +     : RkISP1Path("self", RKISP1_RSZ_SP_FORMATS,
> > > +                  RKISP1_RSZ_SP_SRC_MIN, RKISP1_RSZ_SP_SRC_MAX)
> > >  {
> > >  }
> > >
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > > index 2a1ef0ab..430181d3 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > > @@ -34,7 +34,8 @@ struct V4L2SubdeviceFormat;
> > >  class RkISP1Path
> > >  {
> > >  public:
> > > -     RkISP1Path(const char *name, const Span<const PixelFormat> &formats);
> > > +     RkISP1Path(const char *name, const Span<const PixelFormat> &formats,
> > > +                const Size &minResolution, const Size &maxResolution);
> > >
> > >       bool init(MediaDevice *media);
> > >