[PATCH 2/2] libcamera: rpi: Draw sensor delays from CameraSensorProperties
Dan Scally
dan.scally at ideasonboard.com
Tue Dec 17 23:46:01 CET 2024
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?
>
>>> - 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