[libcamera-devel] [RFC PATCH] ipa: ipu3: Retain request controls in the IPA

[Umang Jain]

Hi Kieran,

On 5/19/22 16:16, Kieran Bingham via libcamera-devel wrote:
> To ensure consistency in processing control lists, an application does
> not have to continually set controls that were already set, but not
> expected to change.


So what is this implying...

If application sends in manual-exposure=100 just for frame 25th, what is 
it expected to do to "unset" it from frame 26th?

- Send in manual-exposure = 0 ?

- send in no manual-exposure, in that case, I assume that the retained 
control list (purpose of this patch) is to set exposure=100 for frame 
26th, which is probably right thing to do?, I am not sure...


>
> To ensure we keep that state, merge incoming request control lists with
> a stored ControlList that is owned by the IPA and store the state of
> that merged list to the per frame FrameContext so that algorithms know
> the full expected control state at that time.
>
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> ---
>
> RFC:
>
> 1)
>
> If ControlList.update() were implemented like .merge, but with
> overriding existing values, instead of leaving them then this could be
> simplified to :
>
> +	retainedControls_.update(controls);
> +	context_.frameContexts[frame % kMaxFrameContexts] = { frame, std::copy(retainedControls_) };
>
> Not yet sure if that's more efficient or if it doesn't make much
> difference.
>
> 2)
>
> This will not support triggered controls that are set once to
> 'fire'... Do we have / plan to have any of those?
>
> 3)
>
> Do we want to keep both a delta control list (the incoming controls) as
> well as the merged/retained list for full state so IPA's can know if
> something was changed?


I think so yes. Algorithms might need to look up the delta to know in 
advance whether or not a manual control is set for a frame or not. 
Accordingly, they might decide the relevant parameter(s)
For e.g. manual exposure but automatic gain.

>
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>   src/ipa/ipu3/ipu3.cpp | 23 ++++++++++++++++++++++-
>   1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 2f6bb672f7bb..89fc34f86e46 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -176,6 +176,9 @@ private:
>   
>   	/* Local parameter storage */
>   	struct IPAContext context_;
> +
> +	/* Control retention to maintain mode states */
> +	ControlList retainedControls_;
>   };
>   
>   /**
> @@ -456,6 +459,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>   
>   	/* Clean IPAActiveState at each reconfiguration. */
>   	context_.activeState = {};
> +	retainedControls_.clear();
> +
>   	IPAFrameContext initFrameContext;
>   	context_.frameContexts.fill(initFrameContext);
>   
> @@ -617,8 +622,24 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>    */
>   void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
>   {
> +	/*
> +	 * Controls are retained, to ensure any mode updates are persistant.
> +	 * We merge them into our class control storage before assigning to
> +	 * the frame context.
> +	 *
> +	 * \todo This will not support trigger controls and may need rework if
> +	 *	 we add any to prevent continually retriggering.
> +	 *
> +	 * \todo We may wish to store both the full merged control list, as well
> +	 *	 as the delta (\a controls) to facilitate algorithms identifying
> +	 *	 when things have changed.
> +	 */
> +	ControlList mergeControls = controls;
> +	mergeControls.merge(retainedControls_);
> +	retainedControls_ = mergeControls;
> +
>   	/* \todo Start processing for 'frame' based on 'controls'. */
> -	context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };
> +	context_.frameContexts[frame % kMaxFrameContexts] = { frame, std::move(mergeControls) };
>   }
>   
>   /**