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

[paul.elder at ideasonboard.com]

On Thu, May 19, 2022 at 04:16:42PM +0200, 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.
> 
> 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>

Oops, I forgot:

Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>

I think this patch as-is is totally fine. Assuming merge() behaves
properly, this does exactly what we want. The "trigger" (though not
really trigger) controls for AE we can implement them
differently/separately/on top, so this patch is fine.

> 
> ---
> 
> 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?
> 
> 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) };
>  }
>  
>  /**
> -- 
> 2.25.1
>