[libcamera-devel,v1,5/8] ipa: rkisp1: Report and use sensor controls
diff mbox series

Message ID 20211119111654.68445-6-jeanmichel.hautbois@ideasonboard.com
State Superseded
Headers show
Series
  • Introduce AGC for RkISP1
Related show

Commit Message

Jean-Michel Hautbois Nov. 19, 2021, 11:16 a.m. UTC 
The pipeline handler populates a new sensorControls ControlList, to
have the effective exposure and gain values for the current frame. This
is done when a statistics buffer is received.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
 include/libcamera/ipa/rkisp1.mojom       | 2 ++
 src/ipa/rkisp1/rkisp1.cpp                | 2 +-
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 3 ++-
 3 files changed, 5 insertions(+), 2 deletions(-)

Comments

Kieran Bingham Nov. 19, 2021, 2:41 p.m. UTC | #1 
Quoting Jean-Michel Hautbois (2021-11-19 11:16:51)
> The pipeline handler populates a new sensorControls ControlList, to
> have the effective exposure and gain values for the current frame. This
> is done when a statistics buffer is received.
> 

I'm fine with this as is, but if we tackle the interface changes to be
more explicit we should do both RKISP and IPU3 at the same time I
suspect.

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

> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>  include/libcamera/ipa/rkisp1.mojom       | 2 ++
>  src/ipa/rkisp1/rkisp1.cpp                | 2 +-
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 3 ++-
>  3 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> index a6991d4f..c3a6d8e1 100644
> --- a/include/libcamera/ipa/rkisp1.mojom
> +++ b/include/libcamera/ipa/rkisp1.mojom
> @@ -21,11 +21,13 @@ struct RkISP1Event {
>         uint32 frame;
>         uint32 bufferId;
>         libcamera.ControlList controls;
> +       libcamera.ControlList sensorControls;
>  };
>  
>  struct RkISP1Action {
>         RkISP1Operations op;
>         libcamera.ControlList controls;
> +       libcamera.ControlList sensorControls;
>  };
>  
>  interface IPARkISP1Interface {
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index ddddd52d..d2f4380a 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -313,7 +313,7 @@ void IPARkISP1::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/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index db8856d3..6e9f1dad 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -333,7 +333,7 @@ void RkISP1CameraData::queueFrameAction(unsigned int frame,
>  {
>         switch (action.op) {
>         case ipa::rkisp1::ActionV4L2Set: {
> -               const ControlList &controls = action.controls;
> +               const ControlList &controls = action.sensorControls;
>                 delayedCtrls_->push(controls);
>                 break;
>         }
> @@ -1125,6 +1125,7 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
>         ev.op = ipa::rkisp1::EventSignalStatBuffer;
>         ev.frame = info->frame;
>         ev.bufferId = info->statBuffer->cookie();
> +       ev.sensorControls = data->delayedCtrls_->get(buffer->metadata().sequence);
>         data->ipa_->processEvent(ev);
>  }
>  
> -- 
> 2.32.0
>
Jean-Michel Hautbois Nov. 22, 2021, 2:19 p.m. UTC | #2 
Hi Kieran,

On 19/11/2021 15:41, Kieran Bingham wrote:
> Quoting Jean-Michel Hautbois (2021-11-19 11:16:51)
>> The pipeline handler populates a new sensorControls ControlList, to
>> have the effective exposure and gain values for the current frame. This
>> is done when a statistics buffer is received.
>>
> 
> I'm fine with this as is, but if we tackle the interface changes to be
> more explicit we should do both RKISP and IPU3 at the same time I
> suspect.

Yes, and as those are very similar, it can be a dedicated series. Well 
that's what I had in mind :-).

> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>> ---
>>   include/libcamera/ipa/rkisp1.mojom       | 2 ++
>>   src/ipa/rkisp1/rkisp1.cpp                | 2 +-
>>   src/libcamera/pipeline/rkisp1/rkisp1.cpp | 3 ++-
>>   3 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
>> index a6991d4f..c3a6d8e1 100644
>> --- a/include/libcamera/ipa/rkisp1.mojom
>> +++ b/include/libcamera/ipa/rkisp1.mojom
>> @@ -21,11 +21,13 @@ struct RkISP1Event {
>>          uint32 frame;
>>          uint32 bufferId;
>>          libcamera.ControlList controls;
>> +       libcamera.ControlList sensorControls;
>>   };
>>   
>>   struct RkISP1Action {
>>          RkISP1Operations op;
>>          libcamera.ControlList controls;
>> +       libcamera.ControlList sensorControls;
>>   };
>>   
>>   interface IPARkISP1Interface {
>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
>> index ddddd52d..d2f4380a 100644
>> --- a/src/ipa/rkisp1/rkisp1.cpp
>> +++ b/src/ipa/rkisp1/rkisp1.cpp
>> @@ -313,7 +313,7 @@ void IPARkISP1::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/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> index db8856d3..6e9f1dad 100644
>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> @@ -333,7 +333,7 @@ void RkISP1CameraData::queueFrameAction(unsigned int frame,
>>   {
>>          switch (action.op) {
>>          case ipa::rkisp1::ActionV4L2Set: {
>> -               const ControlList &controls = action.controls;
>> +               const ControlList &controls = action.sensorControls;
>>                  delayedCtrls_->push(controls);
>>                  break;
>>          }
>> @@ -1125,6 +1125,7 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
>>          ev.op = ipa::rkisp1::EventSignalStatBuffer;
>>          ev.frame = info->frame;
>>          ev.bufferId = info->statBuffer->cookie();
>> +       ev.sensorControls = data->delayedCtrls_->get(buffer->metadata().sequence);
>>          data->ipa_->processEvent(ev);
>>   }
>>   
>> -- 
>> 2.32.0
>>

Patch
diff mbox series 

diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
index a6991d4f..c3a6d8e1 100644
--- a/include/libcamera/ipa/rkisp1.mojom
+++ b/include/libcamera/ipa/rkisp1.mojom
@@ -21,11 +21,13 @@  struct RkISP1Event {
 	uint32 frame;
 	uint32 bufferId;
 	libcamera.ControlList controls;
+	libcamera.ControlList sensorControls;
 };
 
 struct RkISP1Action {
 	RkISP1Operations op;
 	libcamera.ControlList controls;
+	libcamera.ControlList sensorControls;
 };
 
 interface IPARkISP1Interface {
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
index ddddd52d..d2f4380a 100644
--- a/src/ipa/rkisp1/rkisp1.cpp
+++ b/src/ipa/rkisp1/rkisp1.cpp
@@ -313,7 +313,7 @@  void IPARkISP1::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/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index db8856d3..6e9f1dad 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -333,7 +333,7 @@  void RkISP1CameraData::queueFrameAction(unsigned int frame,
 {
 	switch (action.op) {
 	case ipa::rkisp1::ActionV4L2Set: {
-		const ControlList &controls = action.controls;
+		const ControlList &controls = action.sensorControls;
 		delayedCtrls_->push(controls);
 		break;
 	}
@@ -1125,6 +1125,7 @@  void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
 	ev.op = ipa::rkisp1::EventSignalStatBuffer;
 	ev.frame = info->frame;
 	ev.bufferId = info->statBuffer->cookie();
+	ev.sensorControls = data->delayedCtrls_->get(buffer->metadata().sequence);
 	data->ipa_->processEvent(ev);
 }