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

[Laurent Pinchart]

On Tue, Dec 17, 2024 at 04:46:52PM +0000, Kieran Bingham wrote:
> 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.

It could be worth keeping the comment. I think AR0521 suffers from a
similar issue (but the driver doesn't currently run it in a mode where
the gain delay is predictable).

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

-- 
Regards,

Laurent Pinchart