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

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

Commit Message

Dan Scally Nov. 15, 2024, 7:46 a.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>
---
Changes in v3:

	- Fixed some broken alignment 

Changes in v2:

	- Switched to use the new getSensorDelays() function

 src/libcamera/pipeline/ipu3/ipu3.cpp     | 12 +++++-------
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 13 ++++++-------
 src/libcamera/pipeline/simple/simple.cpp |  8 ++++++--
 3 files changed, 17 insertions(+), 16 deletions(-)

Comments

Kieran Bingham Nov. 15, 2024, 12:12 p.m. UTC | #1 
Quoting Daniel Scally (2024-11-15 07:46:28)
> 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 v3:
> 
>         - Fixed some broken alignment 
> 
> Changes in v2:
> 
>         - Switched to use the new getSensorDelays() function
> 
>  src/libcamera/pipeline/ipu3/ipu3.cpp     | 12 +++++-------
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 13 ++++++-------
>  src/libcamera/pipeline/simple/simple.cpp |  8 ++++++--
>  3 files changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 0069d5e2..4d6b86b5 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1077,14 +1077,12 @@ 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'.
> -                */
> +               uint8_t exposureDelay, gainDelay, vblankDelay, hblankDelay;
> +               cio2->sensor()->getSensorDelays(exposureDelay, gainDelay,
> +                                               vblankDelay, hblankDelay);

This probably makes me think passing the struct would be easier!

>                 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 } },

Should we map all known control delays already even if they're not used ?

Same comments apply below too.

--
Kieran

>                 };
>  
>                 data->delayedCtrls_ =
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 6c6d711f..42600908 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>
> @@ -1239,14 +1240,12 @@ 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.
> -        */
> +       uint8_t exposureDelay, gainDelay, vblankDelay, hblankDelay;
> +       data->sensor_->getSensorDelays(exposureDelay, gainDelay, vblankDelay,
> +                                      hblankDelay);
>         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 41fdf84c..20f395fb 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>
>  
> @@ -1290,9 +1291,12 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>         if (outputCfgs.empty())
>                 return 0;
>  
> +       uint8_t exposureDelay, gainDelay, vblankDelay, hblankDelay;
> +       data->sensor_->getSensorDelays(exposureDelay, gainDelay, vblankDelay,
> +                                      hblankDelay);
>         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
>
Laurent Pinchart Nov. 18, 2024, 1:38 a.m. UTC | #2 
On Fri, Nov 15, 2024 at 12:12:46PM +0000, Kieran Bingham wrote:
> Quoting Daniel Scally (2024-11-15 07:46:28)
> > 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 v3:
> > 
> >         - Fixed some broken alignment 
> > 
> > Changes in v2:
> > 
> >         - Switched to use the new getSensorDelays() function
> > 
> >  src/libcamera/pipeline/ipu3/ipu3.cpp     | 12 +++++-------
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 13 ++++++-------
> >  src/libcamera/pipeline/simple/simple.cpp |  8 ++++++--
> >  3 files changed, 17 insertions(+), 16 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 0069d5e2..4d6b86b5 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -1077,14 +1077,12 @@ 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'.
> > -                */
> > +               uint8_t exposureDelay, gainDelay, vblankDelay, hblankDelay;
> > +               cio2->sensor()->getSensorDelays(exposureDelay, gainDelay,
> > +                                               vblankDelay, hblankDelay);
> 
> This probably makes me think passing the struct would be easier!
> 
> >                 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 } },
> 
> Should we map all known control delays already even if they're not used ?
> 
> Same comments apply below too.
> 
> >                 };

Should the CameraSensor class report the map instead of just the delays
?

> >  
> >                 data->delayedCtrls_ =
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 6c6d711f..42600908 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>
> > @@ -1239,14 +1240,12 @@ 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.
> > -        */
> > +       uint8_t exposureDelay, gainDelay, vblankDelay, hblankDelay;
> > +       data->sensor_->getSensorDelays(exposureDelay, gainDelay, vblankDelay,
> > +                                      hblankDelay);
> >         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 41fdf84c..20f395fb 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>
> >  
> > @@ -1290,9 +1291,12 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
> >         if (outputCfgs.empty())
> >                 return 0;
> >  
> > +       uint8_t exposureDelay, gainDelay, vblankDelay, hblankDelay;
> > +       data->sensor_->getSensorDelays(exposureDelay, gainDelay, vblankDelay,
> > +                                      hblankDelay);
> >         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(),

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index 0069d5e2..4d6b86b5 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -1077,14 +1077,12 @@  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'.
-		 */
+		uint8_t exposureDelay, gainDelay, vblankDelay, hblankDelay;
+		cio2->sensor()->getSensorDelays(exposureDelay, gainDelay,
+						vblankDelay, hblankDelay);
 		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 6c6d711f..42600908 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>
@@ -1239,14 +1240,12 @@  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.
-	 */
+	uint8_t exposureDelay, gainDelay, vblankDelay, hblankDelay;
+	data->sensor_->getSensorDelays(exposureDelay, gainDelay, vblankDelay,
+				       hblankDelay);
 	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 41fdf84c..20f395fb 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>
 
@@ -1290,9 +1291,12 @@  int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
 	if (outputCfgs.empty())
 		return 0;
 
+	uint8_t exposureDelay, gainDelay, vblankDelay, hblankDelay;
+	data->sensor_->getSensorDelays(exposureDelay, gainDelay, vblankDelay,
+				       hblankDelay);
 	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(),