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

[David Plowman]

Hi Naush

Thanks for this patch!

On Fri, 22 Oct 2021 at 12:55, Naushir Patuck <naush at raspberrypi.com> 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>

Looks like a significant improvement to me. To be honest, I'm a bit
baffled how complicated the previous version was. Did I write it, or
something?

Review-by: David Plowman <david.plowman at raspberrypi.com>

Thanks!
David

> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 61 +++++++------------
>  1 file changed, 22 insertions(+), 39 deletions(-)
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 5aaf24436f27..8fb6197ec283 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -639,16 +639,35 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>         /* First calculate the best sensor mode we can use based on the user request. */
>         SensorMode sensorMode = findBestMode(data->sensorFormats_, rawStream ? sensorSize : maxSize);
>         V4L2SubdeviceFormat sensorFormat = toV4L2SubdeviceFormat(sensorMode);
> -       V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorMode);
>
>         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);
> +               sensorMode.first = BayerFormat::fromMbusCode(sensorFormat.mbus_code).toPixelFormat();
> +       }
> +
> +       V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorMode);
>         ret = data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat);
>         if (ret)
>                 return ret;
> @@ -657,10 +676,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;
> @@ -881,23 +896,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);
>
> @@ -1311,10 +1309,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;
> @@ -1365,17 +1359,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);
>
> --
> 2.25.1
>