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

[Jacopo Mondi]

Hi Tomasz,
        +Laurent

On Fri, Sep 11, 2020 at 08:48:52PM +0200, Tomasz Figa wrote:
> 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.

I, sadly, agree.

In the meantime for this platform, I'll push this patch (maybe in a
slighter different form).

Thanks
   j

>
> 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