[PATCH 2/2] libcamera: rpi: Draw sensor delays from CameraSensorProperties
Naushir Patuck
naush at raspberrypi.com
Wed Dec 18 09:33:41 CET 2024
Hi Dan,
On Tue, 17 Dec 2024 at 22:46, Dan Scally <dan.scally at ideasonboard.com> wrote:
>
> Hi Naush, David
>
> On 17/12/2024 16:56, Laurent Pinchart wrote:
> > 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).
>
>
> Laurent's eagle-eyes spotted that actually the OV5647 driver doesn't seem to configure a 2 frame
> delay. The datasheet says bits [5:4] of register 0x3503 control the delay, with 11 being the setting
> for 2 frames but in the driver those are set to 00 (which should be no delay). Do you happen to know
> where this 2-frame delay value comes from?
This is one where the datasheet and the sensor behavior simply don't
seem to match. We got to these values empirically by adjusting
settings and looking for expected changes in the statistics. Not sure
what a 0 delay as described by the datasheet would even mean for a
rolling shutter sensor!
Regards,
Naush
>
> >
> >>> - 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 ¶ms, 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;
More information about the libcamera-devel
mailing list