[libcamera-devel] [PATCH v2 6/6] pipeline: raspberrypi: Apply sensor flips at the start of configure()

[Naushir Patuck]

Hi Laurent,

Thank you for your feedback.

On Mon, 25 Oct 2021 at 18:09, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Naush,
>
> Thank you for the patch.
>
> On Fri, Oct 22, 2021 at 03:39:07PM +0100, Naushir Patuck wrote:
> > Sensor flips might change the Bayer order of the requested format. The
> existing
> > code would set a sensor format along with the appropriate Unicam and ISP
> input
> > formats, but reset the latter two on start() once the flips had been
> requested.
> >
> > We can now set the sensor flips just after we set the sensor mode in
> > configure(), thereby not needing the second pair of format sets in
> start().
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > Review-by: David Plowman <david.plowman at raspberrypi.com>
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 60 +++++++------------
> >  1 file changed, 21 insertions(+), 39 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index c5e9607c7d95..ad6f1231bbf6 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -627,16 +627,34 @@ int PipelineHandlerRPi::configure(Camera *camera,
> CameraConfiguration *config)
> >
> >       /* First calculate the best sensor mode we can use based on the
> user request. */
> >       V4L2SubdeviceFormat sensorFormat =
> findBestMode(data->sensorFormats_, rawStream ? sensorSize : maxSize);
> > -     V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat,
> unpacked);
> >
> >       ret = data->sensor_->setFormat(&sensorFormat);
> >       if (ret)
> >               return ret;
> >
> >       /*
> > -      * Unicam image output format. The ISP input format gets set at
> start,
> > -      * just in case we have swapped bayer orders due to flips.
> > +      * Configure the H/V flip controls based on the combination of
> > +      * the sensor and user transform.
> >        */
> > +     if (data->supportsFlips_) {
> > +             const RPiCameraConfiguration *rpiConfig =
> > +                     static_cast<const RPiCameraConfiguration
> *>(config);
> > +             ControlList controls;
> > +
> > +             controls.set(V4L2_CID_HFLIP,
> > +
> static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::HFlip)));
> > +             controls.set(V4L2_CID_VFLIP,
> > +
> static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::VFlip)));
> > +             data->setSensorControls(controls);
> > +
> > +             /*
> > +              * IPA configure may have changed the sensor flips - hence
> the bayer
> > +              * order. So update the sensor format now.
> > +              */
> > +             data->sensor_->device()->getFormat(0, &sensorFormat);
> > +     }
>
> If you moved this whole block before the sensor_->setFormat() call,
> could you drop getFormat() ?
>

Yes I could!  Will fix this and all other suggested changes in a v3 shortly.

Thanks,
Naush

>
> > +
> > +     V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat,
> unpacked);
> >       ret = data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat);
> >       if (ret)
> >               return ret;
> > @@ -645,10 +663,6 @@ int PipelineHandlerRPi::configure(Camera *camera,
> CameraConfiguration *config)
> >                      << " - Selected sensor mode: " <<
> sensorFormat.toString()
> >                      << " - Selected unicam mode: " <<
> unicamFormat.toString();
> >
> > -     /*
> > -      * This format may be reset on start() if the bayer order has
> changed
> > -      * because of flips in the sensor.
> > -      */
> >       ret = data->isp_[Isp::Input].dev()->setFormat(&unicamFormat);
> >       if (ret)
> >               return ret;
> > @@ -871,23 +885,6 @@ int PipelineHandlerRPi::start(Camera *camera, const
> ControlList *controls)
> >               return ret;
> >       }
> >
> > -     /*
> > -      * IPA configure may have changed the sensor flips - hence the
> bayer
> > -      * order. Get the sensor format and set the ISP input now.
> > -      */
> > -     V4L2SubdeviceFormat sensorFormat;
> > -     data->sensor_->device()->getFormat(0, &sensorFormat);
> > -
> > -     V4L2DeviceFormat ispFormat;
> > -     ispFormat.fourcc =
> BayerFormat::fromMbusCode(sensorFormat.mbus_code).toV4L2PixelFormat();
> > -     ispFormat.size = sensorFormat.size;
> > -
> > -     ret = data->isp_[Isp::Input].dev()->setFormat(&ispFormat);
> > -     if (ret) {
> > -             stop(camera);
> > -             return ret;
> > -     }
> > -
> >       /* Enable SOF event generation. */
> >       data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);
> >
> > @@ -1301,10 +1298,6 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig
> *sensorConfig)
> >
> >  int RPiCameraData::configureIPA(const CameraConfiguration *config)
> >  {
> > -     /* We know config must be an RPiCameraConfiguration. */
> > -     const RPiCameraConfiguration *rpiConfig =
> > -             static_cast<const RPiCameraConfiguration *>(config);
> > -
> >       std::map<unsigned int, IPAStream> streamConfig;
> >       std::map<unsigned int, ControlInfoMap> entityControls;
> >       ipa::RPi::IPAConfig ipaConfig;
> > @@ -1355,17 +1348,6 @@ int RPiCameraData::configureIPA(const
> CameraConfiguration *config)
> >               return -EPIPE;
> >       }
> >
> > -     /*
> > -      * Configure the H/V flip controls based on the combination of
> > -      * the sensor and user transform.
> > -      */
> > -     if (supportsFlips_) {
> > -             controls.set(V4L2_CID_HFLIP,
> > -
> static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::HFlip)));
> > -             controls.set(V4L2_CID_VFLIP,
> > -
> static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::VFlip)));
> > -     }
> > -
> >       if (!controls.empty())
> >               setSensorControls(controls);
> >
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20211026/974469ed/attachment-0001.htm>