[libcamera-devel] [PATCH v3 09/14] libcamera: ipu3: Register camera controls

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Oct 21, 2020 at 04:36:30PM +0200, Jacopo Mondi wrote:
> Register controls for the IPU3 pipeline handler. The only supported
> Camera control is currently the pipeline depth control.
> 
> Report the minimum and maximum values the pipeline handler supports
> for the pipeline processing stages.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>

How about squashing this with 10/14 ?

> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index af47739d8d4f..a90683ea523a 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -12,6 +12,7 @@
>  #include <vector>
>  
>  #include <libcamera/camera.h>
> +#include <libcamera/control_ids.h>
>  #include <libcamera/formats.h>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
> @@ -40,6 +41,10 @@ static constexpr unsigned int IMGU_OUTPUT_HEIGHT_ALIGN = 4;
>  static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64;
>  static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32;
>  
> +static const ControlInfoMap IPU3Controls = {
> +	{ &controls::draft::PipelineDepth, ControlInfo(2, 3) },
> +};
> +
>  class IPU3CameraData : public CameraData
>  {
>  public:
> @@ -771,6 +776,9 @@ int PipelineHandlerIPU3::registerCameras()
>  		/* Initialize the camera properties. */
>  		data->properties_ = cio2->sensor()->properties();
>  
> +		/* Initialze the camera controls. */
> +		data->controlInfo_ = IPU3Controls;

Hmmmm... This control can only be reported in metadata, it can't be set
in requests. This isn't something that needs to be addressed in this
series, but do you have any idea on how we should handle that to avoid
giving the impression to applications that they can set the pipeline
depth ? Or maybe we shouldn't handle this at compile time or runtime,
just document it ?

> +
>  		/**
>  		 * \todo Dynamically assign ImgU and output devices to each
>  		 * stream and camera; as of now, limit support to two cameras

-- 
Regards,

Laurent Pinchart