[PATCH v2 2/6] libcamera: mali-c55: Limit ISP input size

[Jacopo Mondi]

Hi Kieran

On Tue, Jun 25, 2024 at 09:23:49PM GMT, Kieran Bingham wrote:
> Quoting Jacopo Mondi (2024-06-25 20:04:15)
> > The Mali-C55 ISP has an input size limit of 640x480.
> >
> > Filter out resolutions smaller than this when selecting the
> > sensor format. While at it, rename 'maxYuvSize' to a more
> > appropriate 'minSensorSize'.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/mali-c55/mali-c55.cpp | 37 ++++++++++++--------
> >  1 file changed, 23 insertions(+), 14 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > index 1c1fef2337f0..91f2ebd6fd26 100644
> > --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > @@ -79,6 +79,7 @@ const std::map<libcamera::PixelFormat, unsigned int> maliC55FmtToCode = {
> >         { formats::SGRBG16, MEDIA_BUS_FMT_SGRBG16_1X16 },
> >  };
> >
> > +constexpr Size kMaliC55MinInputSize = { 640, 480 };
> >  constexpr Size kMaliC55MinSize = { 128, 128 };
> >  constexpr Size kMaliC55MaxSize = { 8192, 8192 };
> >  constexpr unsigned int kMaliC55ISPInternalFormat = MEDIA_BUS_FMT_RGB121212_1X36;
> > @@ -265,13 +266,16 @@ PixelFormat MaliC55CameraData::adjustRawFormat(const PixelFormat &rawFmt) const
> >         return rawFmt;
> >  }
> >
> > -Size MaliC55CameraData::adjustRawSizes(const PixelFormat &rawFmt, const Size &rawSize) const
> > +Size MaliC55CameraData::adjustRawSizes(const PixelFormat &rawFmt, const Size &size) const
> >  {
> >         /* Just make sure the format is supported. */
> >         auto it = maliC55FmtToCode.find(rawFmt);
> >         if (it == maliC55FmtToCode.end())
> >                 return {};
> >
> > +       /* Expand the RAW size to the minimum ISP input size. */
> > +       Size rawSize = size.expandedTo(kMaliC55MinInputSize);
> > +
> >         /* Check if the size is natively supported. */
> >         unsigned int rawCode = it->second;
> >         const auto rawSizes = sizes(rawCode);
> > @@ -282,14 +286,14 @@ Size MaliC55CameraData::adjustRawSizes(const PixelFormat &rawFmt, const Size &ra
> >         /* Or adjust it to the closest supported size. */
> >         uint16_t distance = std::numeric_limits<uint16_t>::max();
> >         Size bestSize;
> > -       for (const Size &size : rawSizes) {
> > +       for (const Size &sz : rawSizes) {
> >                 uint16_t dist = std::abs(static_cast<int>(rawSize.width) -
> > -                                        static_cast<int>(size.width)) +
> > +                                        static_cast<int>(sz.width)) +
> >                                 std::abs(static_cast<int>(rawSize.height) -
> > -                                        static_cast<int>(size.height));
> > +                                        static_cast<int>(sz.height));
> >                 if (dist < distance) {
> >                         dist = distance;
> > -                       bestSize = size;
> > +                       bestSize = sz;
> >                 }
> >         }
> >
> > @@ -376,8 +380,13 @@ CameraConfiguration::Status MaliC55CameraConfiguration::validate()
> >                 frPipeAvailable = false;
> >         }
> >
> > -       /* Adjust processed streams. */
> > -       Size maxYuvSize;
> > +       /*
> > +        * Adjust processed streams.
> > +        *
> > +        * Compute the minimum sensor size to be later used to select the
> > +        * sensor configuration.
> > +        */
> > +       Size minSensorSize = kMaliC55MinInputSize;
> >         for (StreamConfiguration &config : config_) {
> >                 if (isFormatRaw(config.pixelFormat))
> >                         continue;
> > @@ -399,8 +408,8 @@ CameraConfiguration::Status MaliC55CameraConfiguration::validate()
> >                         status = Adjusted;
> >                 }
> >
> > -               if (maxYuvSize < size)
> > -                       maxYuvSize = size;
> > +               if (minSensorSize < size)
> > +                       minSensorSize = size;
>
> This doesn't read well to me yet. Or maybe I'm tired, or it needs a comment.

A comment like ?

        * Compute the minimum sensor size to be later used to select the
        * sensor configuration.
>
> If the miniumSensorSize is smaller than size we make the
> miniumSensorSize bigger...

minSensorSize is the minimum required sensor output size. As the ISP
cannot upscale:

- Initialize it to the min ISP input size
- Walk all the YUV streams and search for the largest YUV size

Then search for a sensor mode large enough
>
> isn't a miniumSensorSize a constant? What is size here? The context
> isn't very helpful as you've changed a different function above to also
> have a parameter passed in now called size ... but I think that's a
> 'different' size to this one...

That change is self-contained. The function is not called from here.

>
> >
> >                 if (frPipeAvailable) {
> >                         config.setStream(const_cast<Stream *>(&data_->frStream_));
> > @@ -416,7 +425,7 @@ CameraConfiguration::Status MaliC55CameraConfiguration::validate()
> >         if (rawConfig) {
> >                 const auto it = maliC55FmtToCode.find(rawConfig->pixelFormat);
> >                 sensorFormat_.code = it->second;
> > -               sensorFormat_.size = rawConfig->size;
> > +               sensorFormat_.size = rawConfig->size.expandedTo(minSensorSize);
> >
> >                 return status;
> >         }
> > @@ -431,13 +440,13 @@ CameraConfiguration::Status MaliC55CameraConfiguration::validate()
> >         Size bestSize;
> >         for (const auto &size : sizes) {
> >                 /* Skip sensor sizes that are smaller than the max YUV size. */
>
> Should this comment change too ?

Ah yes, maybe yes.

>
> > -               if (maxYuvSize.width > size.width ||
> > -                   maxYuvSize.height > size.height)
> > +               if (minSensorSize.width > size.width ||
> > +                   minSensorSize.height > size.height)
> >                         continue;
> >
> > -               uint16_t dist = std::abs(static_cast<int>(maxYuvSize.width) -
> > +               uint16_t dist = std::abs(static_cast<int>(minSensorSize.width) -
> >                                          static_cast<int>(size.width)) +
> > -                               std::abs(static_cast<int>(maxYuvSize.height) -
> > +                               std::abs(static_cast<int>(minSensorSize.height) -
> >                                          static_cast<int>(size.height));
> >                 if (dist < distance) {
> >                         dist = distance;
> > --
> > 2.45.2
> >