[PATCH v4 2/2] libcamera: pipelines: Draw control delays from CameraSensor properties

[Jacopo Mondi]

Hi Dan

On Tue, Nov 19, 2024 at 01:03:01PM +0000, Daniel Scally wrote:
> Rather than hard coding default delays for control values in the
> pipeline handlers, pick up the ones defined in the CameraSensor
> properties.
>
> Signed-off-by: Daniel Scally <dan.scally at ideasonboard.com>
> ---
> Changes in v4:
>
> 	- Used the struct reference rather than values directly
>
> Changes in v3:
>
> 	- Fixed some broken alignment
>
> Changes in v2:
>
> 	- Switched to use the new getSensorDelays() function
>
>  src/libcamera/pipeline/ipu3/ipu3.cpp     | 10 +++-------
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 11 ++++-------
>  src/libcamera/pipeline/simple/simple.cpp |  6 ++++--
>  3 files changed, 11 insertions(+), 16 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 0069d5e2..f4e33830 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1077,14 +1077,10 @@ int PipelineHandlerIPU3::registerCameras()
>  		if (ret)
>  			continue;
>
> -		/*
> -		 * \todo Read delay values from the sensor itself or from a
> -		 * a sensor database. For now use generic values taken from
> -		 * the Raspberry Pi and listed as 'generic values'.
> -		 */
> +		const CameraSensorProperties::SensorDelays &delays = cio2->sensor()->sensorDelays();

camera_sensor_properties.h is included by camera_sensor.h, but I
wonder if we should include it explicitly

>  		std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
> -			{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },
> -			{ V4L2_CID_EXPOSURE, { 2, false } },
> +			{ V4L2_CID_ANALOGUE_GAIN, { delays.gainDelay, false } },
> +			{ V4L2_CID_EXPOSURE, { delays.exposureDelay, false } },
>  		};
>
>  		data->delayedCtrls_ =
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 6c6d711f..9118d893 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -24,6 +24,7 @@
>  #include <libcamera/control_ids.h>
>  #include <libcamera/formats.h>
>  #include <libcamera/framebuffer.h>
> +#include <libcamera/property_ids.h>

leftover possibily

>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
>  #include <libcamera/transform.h>
> @@ -1239,14 +1240,10 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>  	/* Initialize the camera properties. */
>  	data->properties_ = data->sensor_->properties();
>
> -	/*
> -	 * \todo Read delay values from the sensor itself or from a
> -	 * a sensor database. For now use generic values taken from
> -	 * the Raspberry Pi and listed as generic values.
> -	 */
> +	const CameraSensorProperties::SensorDelays &delays = data->sensor_->sensorDelays();
>  	std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
> -		{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },
> -		{ V4L2_CID_EXPOSURE, { 2, false } },
> +		{ V4L2_CID_ANALOGUE_GAIN, { delays.gainDelay, false } },
> +		{ V4L2_CID_EXPOSURE, { delays.exposureDelay, false } },
>  	};
>
>  	data->delayedCtrls_ =
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 41fdf84c..9e759bf8 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -26,6 +26,7 @@
>
>  #include <libcamera/camera.h>
>  #include <libcamera/control_ids.h>
> +#include <libcamera/property_ids.h>

ditto

>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
>
> @@ -1290,9 +1291,10 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>  	if (outputCfgs.empty())
>  		return 0;
>
> +	const CameraSensorProperties::SensorDelays &delays = data->sensor_->sensorDelays();
>  	std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
> -		{ V4L2_CID_ANALOGUE_GAIN, { 2, false } },
> -		{ V4L2_CID_EXPOSURE, { 2, false } },
> +		{ V4L2_CID_ANALOGUE_GAIN, { delays.gainDelay, false } },

I wonder if we shouldn't make the 'priorityWrite' argument internal to
DelayedControls, it actually only applies to VBLANK. Nothing of
concern of this patch though.

> +		{ V4L2_CID_EXPOSURE, { delays.exposureDelay, false } },

Includes apart, this looks good to me.

Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

>  	};
>  	data->delayedCtrls_ =
>  		std::make_unique<DelayedControls>(data->sensor_->device(),
> --
> 2.34.1
>