[libcamera-devel,18/22] ipa: ipu3: Move the sensor update controls

14487 diff mbox series

We want the setControls() to use the cached exposure_ and gain_ values,
so move the frame context update of those variables into
frameCompleted() call instead.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
 src/ipa/ipu3/ipu3.cpp | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Quoting Jean-Michel Hautbois (2021-11-08 13:13:46)
> We want the setControls() to use the cached exposure_ and gain_ values,
> so move the frame context update of those variables into
> frameCompleted() call instead.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index b60ab7e7..9b20e8ab 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -534,6 +534,10 @@ void IPAIPU3::frameStarted([[maybe_unused]] unsigned int frame)
>  
>  void IPAIPU3::frameCompleted([[maybe_unused]] unsigned int frame)
>  {
> +       /* Apply the exposure and gain updated values */
> +       exposure_ = context_.frameContext->agc.exposure;
> +       gain_ = camHelper_->gainCode(context_.frameContext->agc.gain);
> +

Following this through I fear we might have to split these values more.

We need to store Exposure and Gain (EG from here) in several places, and
a couple of different points when it's calculated.


For frame X

- processControls:
	EG 'might' be set as manual requested values here
	from the request. Otherwise they have no use afaict.
	If they are set in here, then they should be stored in the
	IPU3IPA state to be used when calling FillParams (next)

- FillParams:
	EG needs to be the 'desired next EG values for this frame'
	These values were calculated on the previous iteration of
	processStatistics if it has happened or some sane initial
	defaults if we are just starting up from frame 0.

- processStatistics:
	The metadata needs to retrieve the EG that was used (from
	delayedControls or sensor metadata?) , and make
	sure those values are set in the outgoing request metadata.

	The algorithms need to compare the EG that was used for this
	frame against the statistics of the frame.
	Then ... the algorithms will generate 'new' EG values. (EG++)

	At this point, I think EG++ needs to be stored (not in the
	FrameContext) probably in the IPU3IPA state or such ready to be
	set in the next FillParams.

Is my understanding of the sequences correct above?


Looking at IPAIPU3::parseStatistics(), I can see a call to
setControls() after the algorithms run, so perhaps there isn't any
waiting around for fillParams to occur, and they get set quickly which
throws my understanding of the sequencing out a bit.

>         /*
>          * Remove the pointer from the queue, it should not be accessed
>          * anymore and delete it.
> @@ -711,10 +715,6 @@ void IPAIPU3::setControls(unsigned int frame)
>         IPU3Action op;
>         op.op = ActionSetSensorControls;
>  
> -       /* Apply the exposure and gain updated values */
> -       exposure_ = context_.frameContext->agc.exposure;
> -       gain_ = camHelper_->gainCode(context_.frameContext->agc.gain);
> -
>         ControlList ctrls(ctrls_);
>         ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));
>         ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));
> -- 
> 2.32.0
>