[PATCH 2/2] libcamera: rpi: Draw sensor delays from CameraSensorProperties

[Kieran Bingham]

Quoting Daniel Scally (2024-11-27 13:32:33)
> Now that we have camera sensor control application delay values in
> the CameraSensorProperties class, remove the duplicated definitions
> in the RPi IPA's CameraSensorHelpers and update the pipeline handler
> to use the values from CameraSensorProperties.
> 
> Signed-off-by: Daniel Scally <dan.scally at ideasonboard.com>
> ---
>  include/libcamera/ipa/raspberrypi.mojom           |  4 ----
>  src/ipa/rpi/cam_helper/cam_helper.cpp             | 13 -------------
>  src/ipa/rpi/cam_helper/cam_helper.h               |  7 -------
>  src/ipa/rpi/cam_helper/cam_helper_imx283.cpp      | 12 ------------
>  src/ipa/rpi/cam_helper/cam_helper_imx290.cpp      | 11 -----------
>  src/ipa/rpi/cam_helper/cam_helper_imx296.cpp      | 11 -----------
>  src/ipa/rpi/cam_helper/cam_helper_imx477.cpp      | 11 -----------
>  src/ipa/rpi/cam_helper/cam_helper_imx519.cpp      | 11 -----------
>  src/ipa/rpi/cam_helper/cam_helper_imx708.cpp      | 11 -----------
>  src/ipa/rpi/cam_helper/cam_helper_ov5647.cpp      | 15 ---------------
>  src/ipa/rpi/cam_helper/cam_helper_ov64a40.cpp     | 12 ------------
>  src/ipa/rpi/cam_helper/cam_helper_ov7251.cpp      | 12 ------------
>  src/ipa/rpi/cam_helper/cam_helper_ov9281.cpp      | 12 ------------
>  src/ipa/rpi/common/ipa_base.cpp                   | 14 ++------------
>  .../pipeline/rpi/common/pipeline_base.cpp         |  9 +++++----
>  15 files changed, 7 insertions(+), 158 deletions(-)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> index 0b92587d..e30c70bd 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -12,10 +12,6 @@ import "include/libcamera/ipa/core.mojom";
>  const uint32 MaxLsGridSize = 0x8000;
>  
>  struct SensorConfig {
> -       uint32 gainDelay;
> -       uint32 exposureDelay;
> -       uint32 vblankDelay;
> -       uint32 hblankDelay;
>         uint32 sensorMetadata;
>  };
>  
> diff --git a/src/ipa/rpi/cam_helper/cam_helper.cpp b/src/ipa/rpi/cam_helper/cam_helper.cpp
> index 6493e882..8c720652 100644
> --- a/src/ipa/rpi/cam_helper/cam_helper.cpp
> +++ b/src/ipa/rpi/cam_helper/cam_helper.cpp
> @@ -156,19 +156,6 @@ void CamHelper::setCameraMode(const CameraMode &mode)
>         }
>  }
>  
> -void CamHelper::getDelays(int &exposureDelay, int &gainDelay,
> -                         int &vblankDelay, int &hblankDelay) const
> -{
> -       /*
> -        * These values are correct for many sensors. Other sensors will
> -        * need to over-ride this function.
> -        */
> -       exposureDelay = 2;
> -       gainDelay = 1;
> -       vblankDelay = 2;
> -       hblankDelay = 2;
> -}

This matches "CameraSensorProperties::SensorDelays defaultSensorDelays"
in CameraSensorLegacy, so Ack here.

<snip because large diff>

> -void CamHelperOv5647::getDelays(int &exposureDelay, int &gainDelay,
> -                               int &vblankDelay, int &hblankDelay) const
> -{
> -       /*
> -        * We run this sensor in a mode where the gain delay is bumped up to
> -        * 2. It seems to be the only way to make the delays "predictable".
> -        */

We lose this comment as it's not in CameraSensorProperties, but the
values match and I think it's ok.

> -       exposureDelay = 2;
> -       gainDelay = 2;
> -       vblankDelay = 2;
> -       hblankDelay = 2;
> -}
> -

<snip>

> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index 5fce17e6..45e2a1d7 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -134,18 +134,8 @@ int32_t IpaBase::init(const IPASettings &settings, const InitParams &params, Ini
>                 return -EINVAL;
>         }
>  
> -       /*
> -        * Pass out the sensor config to the pipeline handler in order
> -        * to setup the staggered writer class.
> -        */
> -       int gainDelay, exposureDelay, vblankDelay, hblankDelay, sensorMetadata;
> -       helper_->getDelays(exposureDelay, gainDelay, vblankDelay, hblankDelay);
> -       sensorMetadata = helper_->sensorEmbeddedDataPresent();
> -
> -       result->sensorConfig.gainDelay = gainDelay;
> -       result->sensorConfig.exposureDelay = exposureDelay;
> -       result->sensorConfig.vblankDelay = vblankDelay;
> -       result->sensorConfig.hblankDelay = hblankDelay;
> +       /* Pass out the sensor metadata to the pipeline handler */
> +       int sensorMetadata = helper_->sensorEmbeddedDataPresent();
>         result->sensorConfig.sensorMetadata = sensorMetadata;
>  
>         /* Load the tuning file for this sensor. */
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index 9e2d9d23..398a0280 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> @@ -816,11 +816,12 @@ int PipelineHandlerBase::registerCamera(std::unique_ptr<RPi::CameraData> &camera
>          * Setup our delayed control writer with the sensor default
>          * gain and exposure delays. Mark VBLANK for priority write.
>          */
> +       const CameraSensorProperties::SensorDelays &delays = data->sensor_->sensorDelays();
>         std::unordered_map<uint32_t, RPi::DelayedControls::ControlParams> params = {
> -               { V4L2_CID_ANALOGUE_GAIN, { result.sensorConfig.gainDelay, false } },
> -               { V4L2_CID_EXPOSURE, { result.sensorConfig.exposureDelay, false } },
> -               { V4L2_CID_HBLANK, { result.sensorConfig.hblankDelay, false } },
> -               { V4L2_CID_VBLANK, { result.sensorConfig.vblankDelay, true } }
> +               { V4L2_CID_ANALOGUE_GAIN, { delays.gainDelay, false } },
> +               { V4L2_CID_EXPOSURE, { delays.exposureDelay, false } },
> +               { V4L2_CID_HBLANK, { delays.hblankDelay, false } },
> +               { V4L2_CID_VBLANK, { delays.vblankDelay, true } }

And that's all simplified and now unified so we have a single table of
truth for sensor delay values.

so:


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>


>         };
>         data->delayedCtrls_ = std::make_unique<RPi::DelayedControls>(data->sensor_->device(), params);
>         data->sensorMetadata_ = result.sensorConfig.sensorMetadata;
> -- 
> 2.34.1
>