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

22017 diff mbox series

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@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(-)

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@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@ideasonboard.com>

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