[libcamera-devel] [RFC] libcamera: ipu3: Always use sensor full frame size

[Tomasz Figa]

On Thu, Sep 10, 2020 at 12:25 PM Niklas Söderlund
<niklas.soderlund at ragnatech.se> wrote:
>
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2020-09-09 15:10:14 +0200, Jacopo Mondi wrote:
> > When calculating the pipeline configuration for the IPU3 platform,
> > libcamera tries to be smart and select the smaller sensor frame
> > resolution larger enough to accommodate the stream sizes
> > requested by the application.
> >
> > While this seems to make a lot of sense, in practice optimizing the
> > selected sensor resolution makes the pipeline configuration calculation
> > process fail in multiple occasions, or results in stalls in the capture
> > process.
> >
> > As a trivial example, capturing with cam with the following command
> > line result in a stall:
> > $ cam -swidth=1280,height=720 -swidth=640,height=480 -c1 -C
> >
> > Likewise, the Android HAL supported format enumeration fails in
> > reporting smaller resolutions as supported, when used with the OV5670
> > sensor.
> >
> > 320x240:
> > DEBUG IPU3 ipu3.cpp:192 CIO2 configuration: 648x486-SGRBG10_IPU3
> > ERROR IPU3 imgu.cpp:408 Failed to calculate pipe configuration
> > ERROR IPU3 ipu3.cpp:299 Failed to calculate pipe configuration: unsupported resolutions.
> >
> > 640x480:
> > DEBUG IPU3 ipu3.cpp:192 CIO2 configuration: 648x486-SGRBG10_IPU3
> > ERROR IPU3 imgu.cpp:408 Failed to calculate pipe configuration
> > ERROR IPU3 ipu3.cpp:299 Failed to calculate pipe configuration: unsupported resolutions.
> >
> > Furthermore the reference .xml files used for the IPU3 camera
> > configuration on the ChromeOS platform restrict the number of sensor
> > resolution to be used for the OV5670 sensor to 2 from the 6 supported by
> > the driver [1].
> >
> > The selection criteria of the correct CIO2 mode are not specified, and
> > for the time being, always use the sensor maximum resolution at the
> > expense of frame rate and bus bandwidth allows the pipeline to successfully
> > support smaller modes for the OV5670 sensor and solves pipeline stalls
> > when capturing with both sensors.
> >
> > [1] See the <sensor_modes> enumeration in:
> > https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/master/baseboard-poppy/media-libs/cros-camera-hal-configs-poppy/files/gcss/graph_settings_ov5670.xml
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
>
> I love how the IPU3 configuration depends on a xml file ;-)

However it sounds, I believe some static configuration is unavoidable,
especially for software which is expected to be used in production and
maintain stable operating conditions. Linux has a lot of configuration
as well, either compile-time or runtime (sysfs, proc) for the same
reason.

As we noticed in another thread, about Android stream mapping, there
are multiple configurations existing to achieve the same final
parameters requested, even though the end result might not be exactly
the same. For example, scaling in one part of the pipeline or another
could have different effect on quality, with the choice between one or
another depending on business decisions of a particular integrator.

So as much as we want to avoid configuration files, I think that we
actually have to start thinking of how to make it possible to use such
configuration if one wants.

Best regards,
Tomasz

> Given that
> our current configuration depends on this and assumes the larger
> resolution I think this is the right move.
>
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +--------
> >  1 file changed, 1 insertion(+), 8 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 2d881fe28f98..488e9fff299e 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -155,14 +155,12 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> >       unsigned int rawCount = 0;
> >       unsigned int yuvCount = 0;
> >       Size maxYuvSize;
> > -     Size maxRawSize;
> >
> >       for (const StreamConfiguration &cfg : config_) {
> >               const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
> >
> >               if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {
> >                       rawCount++;
> > -                     maxRawSize.expandTo(cfg.size);
> >               } else {
> >                       yuvCount++;
> >                       maxYuvSize.expandTo(cfg.size);
> > @@ -174,18 +172,13 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> >               return Invalid;
> >       }
> >
> > -     if (maxRawSize.isNull())
> > -             maxRawSize = maxYuvSize.alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,
> > -                                                 IMGU_OUTPUT_HEIGHT_MARGIN)
> > -                                    .boundedTo(data_->cio2_.sensor()->resolution());
> > -
> >       /*
> >        * Generate raw configuration from CIO2.
> >        *
> >        * The output YUV streams will be limited in size to the maximum
> >        * frame size requested for the RAW stream.
> >        */
> > -     cio2Configuration_ = data_->cio2_.generateConfiguration(maxRawSize);
> > +     cio2Configuration_ = data_->cio2_.generateConfiguration({});
> >       if (!cio2Configuration_.pixelFormat.isValid())
> >               return Invalid;
> >
> > --
> > 2.28.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel