[6/6] libcamera: pipelines: Draw control delays from CameraSensor properties
diff mbox series

Message ID 20241031160741.253855-7-dan.scally@ideasonboard.com
State Superseded
Headers show
Series
  • Centralise common functions in IPA modules
Related show

Commit Message

Dan Scally Oct. 31, 2024, 4:07 p.m. UTC 
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>
---

I note that the simple pipeline handler had a Gain delay of 2 rather than the
standard 1...I don't think the difference from the norm is correct but thought
I would highlight it in case I'm mistaken.

 src/libcamera/pipeline/ipu3/ipu3.cpp     | 11 ++++-------
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 12 +++++-------
 src/libcamera/pipeline/simple/simple.cpp |  7 +++++--
 3 files changed, 14 insertions(+), 16 deletions(-)

Comments

Jacopo Mondi Nov. 4, 2024, 11:29 a.m. UTC | #1 
Hi Dan

On Thu, Oct 31, 2024 at 04:07:41PM +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>
> ---
>
> I note that the simple pipeline handler had a Gain delay of 2 rather than the
> standard 1...I don't think the difference from the norm is correct but thought
> I would highlight it in case I'm mistaken.
>

Well, I guess the "default" value where just some semi-random numbers,
so I don't think one is more correct than the other ?

>  src/libcamera/pipeline/ipu3/ipu3.cpp     | 11 ++++-------
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 12 +++++-------
>  src/libcamera/pipeline/simple/simple.cpp |  7 +++++--
>  3 files changed, 14 insertions(+), 16 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 29d3c6c1..ff58ba28 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1077,14 +1077,11 @@ 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'.
> -		 */
> +		unsigned int gainDelay = data->properties_.get(properties::GainDelay).value();
> +		unsigned int exposureDelay = data->properties_.get(properties::ExposureDelay).value();

Maybe we don't even need controls:: for these but the CameraSensor
class could expose delayes are read from the static properties ?

After all we don't need to pass these delays across the ph/IPA
boundaries as DelayedControls is initialized on the ph side.

What od you think ?


>  		std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
> -			{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },
> -			{ V4L2_CID_EXPOSURE, { 2, false } },
> +			{ V4L2_CID_ANALOGUE_GAIN, { gainDelay, false } },
> +			{ V4L2_CID_EXPOSURE, { exposureDelay, false } },
>  		};
>
>  		data->delayedCtrls_ =
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index c7b0b392..43e1315f 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>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
>  #include <libcamera/transform.h>
> @@ -1240,14 +1241,11 @@ 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.
> -	 */
> +	unsigned int gainDelay = data->properties_.get(properties::GainDelay).value();
> +	unsigned int exposureDelay = data->properties_.get(properties::ExposureDelay).value();
>  	std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
> -		{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },
> -		{ V4L2_CID_EXPOSURE, { 2, false } },
> +		{ V4L2_CID_ANALOGUE_GAIN, { gainDelay, false } },
> +		{ V4L2_CID_EXPOSURE, { exposureDelay, false } },
>  	};
>
>  	data->delayedCtrls_ =
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 3ddce71d..20c97c37 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>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
>
> @@ -1286,9 +1287,11 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>  	if (outputCfgs.empty())
>  		return 0;
>
> +	unsigned int gainDelay = data->properties_.get(properties::GainDelay).value();
> +	unsigned int exposureDelay = data->properties_.get(properties::ExposureDelay).value();
>  	std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
> -		{ V4L2_CID_ANALOGUE_GAIN, { 2, false } },
> -		{ V4L2_CID_EXPOSURE, { 2, false } },
> +		{ V4L2_CID_ANALOGUE_GAIN, { gainDelay, false } },
> +		{ V4L2_CID_EXPOSURE, { exposureDelay, false } },
>  	};
>  	data->delayedCtrls_ =
>  		std::make_unique<DelayedControls>(data->sensor_->device(),
> --
> 2.30.2
>
Dan Scally Nov. 6, 2024, 9:43 a.m. UTC | #2 
Hi Jacopo

On 04/11/2024 11:29, Jacopo Mondi wrote:
> Hi Dan
>
> On Thu, Oct 31, 2024 at 04:07:41PM +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>
>> ---
>>
>> I note that the simple pipeline handler had a Gain delay of 2 rather than the
>> standard 1...I don't think the difference from the norm is correct but thought
>> I would highlight it in case I'm mistaken.
>>
> Well, I guess the "default" value where just some semi-random numbers,
> so I don't think one is more correct than the other ?
>
>>   src/libcamera/pipeline/ipu3/ipu3.cpp     | 11 ++++-------
>>   src/libcamera/pipeline/rkisp1/rkisp1.cpp | 12 +++++-------
>>   src/libcamera/pipeline/simple/simple.cpp |  7 +++++--
>>   3 files changed, 14 insertions(+), 16 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> index 29d3c6c1..ff58ba28 100644
>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> @@ -1077,14 +1077,11 @@ 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'.
>> -		 */
>> +		unsigned int gainDelay = data->properties_.get(properties::GainDelay).value();
>> +		unsigned int exposureDelay = data->properties_.get(properties::ExposureDelay).value();
> Maybe we don't even need controls:: for these but the CameraSensor
> class could expose delayes are read from the static properties ?
>
> After all we don't need to pass these delays across the ph/IPA
> boundaries as DelayedControls is initialized on the ph side.
>
> What od you think ?


That's fine by me too sure.

>
>
>>   		std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
>> -			{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },
>> -			{ V4L2_CID_EXPOSURE, { 2, false } },
>> +			{ V4L2_CID_ANALOGUE_GAIN, { gainDelay, false } },
>> +			{ V4L2_CID_EXPOSURE, { exposureDelay, false } },
>>   		};
>>
>>   		data->delayedCtrls_ =
>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> index c7b0b392..43e1315f 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>
>>   #include <libcamera/request.h>
>>   #include <libcamera/stream.h>
>>   #include <libcamera/transform.h>
>> @@ -1240,14 +1241,11 @@ 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.
>> -	 */
>> +	unsigned int gainDelay = data->properties_.get(properties::GainDelay).value();
>> +	unsigned int exposureDelay = data->properties_.get(properties::ExposureDelay).value();
>>   	std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
>> -		{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },
>> -		{ V4L2_CID_EXPOSURE, { 2, false } },
>> +		{ V4L2_CID_ANALOGUE_GAIN, { gainDelay, false } },
>> +		{ V4L2_CID_EXPOSURE, { exposureDelay, false } },
>>   	};
>>
>>   	data->delayedCtrls_ =
>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> index 3ddce71d..20c97c37 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>
>>   #include <libcamera/request.h>
>>   #include <libcamera/stream.h>
>>
>> @@ -1286,9 +1287,11 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>>   	if (outputCfgs.empty())
>>   		return 0;
>>
>> +	unsigned int gainDelay = data->properties_.get(properties::GainDelay).value();
>> +	unsigned int exposureDelay = data->properties_.get(properties::ExposureDelay).value();
>>   	std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
>> -		{ V4L2_CID_ANALOGUE_GAIN, { 2, false } },
>> -		{ V4L2_CID_EXPOSURE, { 2, false } },
>> +		{ V4L2_CID_ANALOGUE_GAIN, { gainDelay, false } },
>> +		{ V4L2_CID_EXPOSURE, { exposureDelay, false } },
>>   	};
>>   	data->delayedCtrls_ =
>>   		std::make_unique<DelayedControls>(data->sensor_->device(),
>> --
>> 2.30.2
>>

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index 29d3c6c1..ff58ba28 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -1077,14 +1077,11 @@  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'.
-		 */
+		unsigned int gainDelay = data->properties_.get(properties::GainDelay).value();
+		unsigned int exposureDelay = data->properties_.get(properties::ExposureDelay).value();
 		std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
-			{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },
-			{ V4L2_CID_EXPOSURE, { 2, false } },
+			{ V4L2_CID_ANALOGUE_GAIN, { gainDelay, false } },
+			{ V4L2_CID_EXPOSURE, { exposureDelay, false } },
 		};
 
 		data->delayedCtrls_ =
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index c7b0b392..43e1315f 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>
 #include <libcamera/request.h>
 #include <libcamera/stream.h>
 #include <libcamera/transform.h>
@@ -1240,14 +1241,11 @@  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.
-	 */
+	unsigned int gainDelay = data->properties_.get(properties::GainDelay).value();
+	unsigned int exposureDelay = data->properties_.get(properties::ExposureDelay).value();
 	std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
-		{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },
-		{ V4L2_CID_EXPOSURE, { 2, false } },
+		{ V4L2_CID_ANALOGUE_GAIN, { gainDelay, false } },
+		{ V4L2_CID_EXPOSURE, { exposureDelay, false } },
 	};
 
 	data->delayedCtrls_ =
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index 3ddce71d..20c97c37 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>
 #include <libcamera/request.h>
 #include <libcamera/stream.h>
 
@@ -1286,9 +1287,11 @@  int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
 	if (outputCfgs.empty())
 		return 0;
 
+	unsigned int gainDelay = data->properties_.get(properties::GainDelay).value();
+	unsigned int exposureDelay = data->properties_.get(properties::ExposureDelay).value();
 	std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
-		{ V4L2_CID_ANALOGUE_GAIN, { 2, false } },
-		{ V4L2_CID_EXPOSURE, { 2, false } },
+		{ V4L2_CID_ANALOGUE_GAIN, { gainDelay, false } },
+		{ V4L2_CID_EXPOSURE, { exposureDelay, false } },
 	};
 	data->delayedCtrls_ =
 		std::make_unique<DelayedControls>(data->sensor_->device(),