[libcamera-devel,1/2] ipa: ipu3: Extend ipu3 ipa interface for sensor controls

14465 diff mbox series

From: Han-Lin Chen <hanlinchen@chromium.org>

IPU3Event and IPU3Action use single ControlList for both libcamera and
V4L2 controls, and it's content could be either one based on the
context.  Extend IPU3Event and IPU3Action for sensor V4L2 controls, and
preserve the original one for only libcamera Controls to make the
content of an event more specific.

Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
 include/libcamera/ipa/ipu3.mojom     | 2 ++
 src/ipa/ipu3/ipu3.cpp                | 2 +-
 src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-
 3 files changed, 4 insertions(+), 2 deletions(-)

Hi Han-Lin,

As you can see, I missed an introduction in the mail with the patch I 
sent :-/.

I would like the sensor controls part to be integrated very soon, so I 
have amended your commit, to remove the lens specific part, and a bit of 
the commit message too...

On 05/11/2021 09:39, Jean-Michel Hautbois wrote:
> From: Han-Lin Chen <hanlinchen@chromium.org>
> 
> IPU3Event and IPU3Action use single ControlList for both libcamera and
> V4L2 controls, and it's content could be either one based on the
> context.  Extend IPU3Event and IPU3Action for sensor V4L2 controls, and
> preserve the original one for only libcamera Controls to make the
> content of an event more specific.
> 
> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>   include/libcamera/ipa/ipu3.mojom     | 2 ++
>   src/ipa/ipu3/ipu3.cpp                | 2 +-
>   src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-
>   3 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
> index 2f254ed4..16e3462e 100644
> --- a/include/libcamera/ipa/ipu3.mojom
> +++ b/include/libcamera/ipa/ipu3.mojom
> @@ -23,11 +23,13 @@ struct IPU3Event {
>   	int64 frameTimestamp;
>   	uint32 bufferId;
>   	libcamera.ControlList controls;
> +	libcamera.ControlList sensorControls;
>   };
>   
>   struct IPU3Action {
>   	IPU3Operations op;
>   	libcamera.ControlList controls;
> +	libcamera.ControlList sensorControls;
>   };
>   
>   struct IPAConfigInfo {
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 5c51607d..6775570e 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -646,7 +646,7 @@ void IPAIPU3::setControls(unsigned int frame)
>   	ControlList ctrls(ctrls_);
>   	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));
>   	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));
> -	op.controls = ctrls;
> +	op.sensorControls = ctrls;
>   
>   	queueFrameAction.emit(frame, op);
>   }
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index eb714aa6..8816efc5 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1248,7 +1248,7 @@ void IPU3CameraData::queueFrameAction(unsigned int id,
>   {
>   	switch (action.op) {
>   	case ipa::ipu3::ActionSetSensorControls: {
> -		const ControlList &controls = action.controls;
> +		const ControlList &controls = action.sensorControls;
>   		delayedCtrls_->push(controls);
>   		break;
>   	}
>

Quoting Jean-Michel Hautbois (2021-11-05 08:39:54)
> From: Han-Lin Chen <hanlinchen@chromium.org>
> 
> IPU3Event and IPU3Action use single ControlList for both libcamera and
> V4L2 controls, and it's content could be either one based on the
> context.  Extend IPU3Event and IPU3Action for sensor V4L2 controls, and
> preserve the original one for only libcamera Controls to make the
> content of an event more specific.
> 
> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>  include/libcamera/ipa/ipu3.mojom     | 2 ++
>  src/ipa/ipu3/ipu3.cpp                | 2 +-
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-
>  3 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
> index 2f254ed4..16e3462e 100644
> --- a/include/libcamera/ipa/ipu3.mojom
> +++ b/include/libcamera/ipa/ipu3.mojom
> @@ -23,11 +23,13 @@ struct IPU3Event {
>         int64 frameTimestamp;
>         uint32 bufferId;
>         libcamera.ControlList controls;
> +       libcamera.ControlList sensorControls;

It makes me wonder if we shouldn't be more explicit over the type of
controls stored in a ControlList.

controls are libcamera controls, while sensorControls are (I think) V4L2
controls..

But that's not a blocker for this patch. That's a larger change to the
control frameworks...

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


>  };
>  
>  struct IPU3Action {
>         IPU3Operations op;
>         libcamera.ControlList controls;
> +       libcamera.ControlList sensorControls;
>  };
>  
>  struct IPAConfigInfo {
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 5c51607d..6775570e 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -646,7 +646,7 @@ void IPAIPU3::setControls(unsigned int frame)
>         ControlList ctrls(ctrls_);
>         ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));
>         ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));
> -       op.controls = ctrls;
> +       op.sensorControls = ctrls;
>  
>         queueFrameAction.emit(frame, op);
>  }
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index eb714aa6..8816efc5 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1248,7 +1248,7 @@ void IPU3CameraData::queueFrameAction(unsigned int id,
>  {
>         switch (action.op) {
>         case ipa::ipu3::ActionSetSensorControls: {
> -               const ControlList &controls = action.controls;
> +               const ControlList &controls = action.sensorControls;
>                 delayedCtrls_->push(controls);
>                 break;
>         }
> -- 
> 2.32.0
>

Quoting Jean-Michel Hautbois (2021-11-05 08:43:21)
> Hi Han-Lin,
> 
> As you can see, I missed an introduction in the mail with the patch I 
> sent :-/.
> 


It's marked 1/2, was there a second patch as well?
--
Kieran

Hi,

On 05/11/2021 14:21, Kieran Bingham wrote:
> Quoting Jean-Michel Hautbois (2021-11-05 08:43:21)
>> Hi Han-Lin,
>>
>> As you can see, I missed an introduction in the mail with the patch I
>> sent :-/.
>>
> 
> 
> It's marked 1/2, was there a second patch as well?

The second one is the same as the original "[PATCH v2 2/4] ipu3: ipa: 
Report effective sensor" 
(<20211029115917.2467936-2-hanlinchen@chromium.org>)

Sorry for that...

JM