[libcamera-devel] [PATCH] pipeline: rpi: Move flip handling validation code

[Jacopo Mondi]

Hi Naush

On Tue, Oct 17, 2023 at 01:02:54PM +0100, Naushir Patuck wrote:
> Hi Jacopo,
>
> On Tue, 17 Oct 2023 at 12:54, Jacopo Mondi
> <jacopo.mondi at ideasonboard.com> wrote:
> >
> > Hi Naush
> >
> > On Fri, Oct 13, 2023 at 10:10:03AM +0100, Naushir Patuck via libcamera-devel wrote:
> > > Move the handling of Bayer order changes due to flips to run before
> > > platformValidate(). This removes the need for this code to be split
> > > between platformValidate() and validate() as it is right now.
> > >
> > > Also add some validation to ensure the vc4 pipeline handler only
> > > supports CSI2 packing or no packing.
> > >
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > > ---
> > >  .../pipeline/rpi/common/pipeline_base.cpp     | 43 +++++++++++++------
> > >  src/libcamera/pipeline/rpi/vc4/vc4.cpp        | 15 ++++---
> > >  2 files changed, 38 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > index 825eae80d014..7c88b87e0608 100644
> > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > @@ -231,16 +231,12 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> > >               }
> > >       }
> > >
> > > -     /* Do any platform specific fixups. */
> > > -     status = data_->platformValidate(this);
> > > -     if (status == Invalid)
> > > -             return Invalid;
> > > -
> > > -     /* Further fixups on the RAW streams. */
> > > +     /* Start with some initial generic RAW stream adjustments. */
> > >       for (auto &raw : rawStreams_) {
> > > -             int ret = raw.dev->tryFormat(&raw.format);
> > > -             if (ret)
> > > -                     return Invalid;
> > > +             StreamConfiguration *rawStream = raw.cfg;
> > > +
> > > +             /* Adjust the RAW stream to match the computed sensor format. */
> > > +             BayerFormat sensorBayer = BayerFormat::fromMbusCode(sensorFormat_.mbus_code);
> > >
> > >               /*
> > >                * Some sensors change their Bayer order when they are h-flipped
> > > @@ -249,12 +245,33 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> > >                * Note how we must fetch the "native" (i.e. untransformed) Bayer
> > >                * order, because the sensor may currently be flipped!
> > >                */
> > > -             BayerFormat bayer = BayerFormat::fromPixelFormat(raw.cfg->pixelFormat);
> > >               if (data_->flipsAlterBayerOrder_) {
> > > -                     bayer.order = data_->nativeBayerOrder_;
> > > -                     bayer = bayer.transform(combinedTransform_);
> > > +                     sensorBayer.order = data_->nativeBayerOrder_;
> > > +                     sensorBayer = sensorBayer.transform(combinedTransform_);
> > > +             }
> > > +
> > > +             /* Apply the sensor adjusted Bayer order to the user request. */
> > > +             BayerFormat cfgBayer = BayerFormat::fromPixelFormat(rawStream->pixelFormat);
> > > +             cfgBayer.order = sensorBayer.order;
> > > +
> > > +             if (rawStream->pixelFormat != cfgBayer.toPixelFormat()) {
> > > +                     rawStream->pixelFormat = cfgBayer.toPixelFormat();
> > > +                     status = Adjusted;
> > >               }
> > > -             raw.cfg->pixelFormat = bayer.toPixelFormat();
> > > +     }
> > > +
> > > +     /* Do any platform specific fixups. */
> > > +     Status st = data_->platformValidate(this);
> > > +     if (st == Invalid)
> > > +             return Invalid;
> > > +     else if (st == Adjusted)
> > > +             status = Adjusted;
> > > +
> > > +     /* Further fixups on the RAW streams. */
> > > +     for (auto &raw : rawStreams_) {
> > > +             int ret = raw.dev->tryFormat(&raw.format);
> > > +             if (ret)
> > > +                     return Invalid;
> > >
> > >               if (RPi::PipelineHandlerBase::updateStreamConfig(raw.cfg, raw.format))
> > >                       status = Adjusted;
> > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > > index 233473e2fe2b..425ab9ae6ea5 100644
> > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > > @@ -409,16 +409,17 @@ CameraConfiguration::Status Vc4CameraData::platformValidate(RPi::RPiCameraConfig
> > >
> > >               /* Adjust the RAW stream to match the computed sensor format. */
> > >               StreamConfiguration *rawStream = rawStreams[0].cfg;
> > > -             BayerFormat rawBayer = BayerFormat::fromMbusCode(rpiConfig->sensorFormat_.mbus_code);
> > > +             BayerFormat rawBayer = BayerFormat::fromPixelFormat(rawStream->pixelFormat);
> > >
> > > -             /* Handle flips to make sure to match the RAW stream format. */
> > > -             if (flipsAlterBayerOrder_)
> > > -                     rawBayer = rawBayer.transform(rpiConfig->combinedTransform_);
> > > +             /* Apply the sensor bitdepth. */
> > > +             rawBayer.bitDepth = BayerFormat::fromMbusCode(rpiConfig->sensorFormat_.mbus_code).bitDepth;
> > >
> > > -             /* Apply the user requested packing. */
> > > -             rawBayer.packing = BayerFormat::fromPixelFormat(rawStream->pixelFormat).packing;
> > > -             PixelFormat rawFormat = rawBayer.toPixelFormat();
> > > +             /* Default to CSI2 packing if the user request is unsupported. */
> > > +             if (rawBayer.packing != BayerFormat::Packing::CSI2 &&
> > > +                 rawBayer.packing != BayerFormat::Packing::None)
> > > +                     rawBayer.packing = BayerFormat::Packing::CSI2;
> >
> > Do you need to return Adjusted in this case ?
>
> Yes we do, that (I think!) is handled in the if() clause below where
> we convert from Bayer back to PixelFormat and do the comparison with
> the config PixelFormat.

Oh indeed, tha packing information gets reflected in the PixelFormat
returned by
             PixelFormat rawFormat = rawBayer.toPixelFormat();

Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

Thanks
 j

>
> Regards,
> Naush
>
>
> >
> > Thanks
> >   j
> >
> > >
> > > +             PixelFormat rawFormat = rawBayer.toPixelFormat();
> > >               if (rawStream->pixelFormat != rawFormat ||
> > >                   rawStream->size != rpiConfig->sensorFormat_.size) {
> > >                       rawStream->pixelFormat = rawFormat;
> > > --
> > > 2.34.1
> > >