[PATCH 2/4] libipa: FCQueue: Make sure FrameContext#0 is initialized

[Stefan Klug]

Hi Jacopo,

Thank you for the patch. 

On Wed, Oct 16, 2024 at 07:03:43PM +0200, Jacopo Mondi wrote:
> Some IPA modules, like the RkISP1 one, call FCQueue::get(0) at
> IPA::start() time, before any frame context has been allocated with
> FCQueue::alloc() called at queueRequest() time.
> 
> The FCQueue implementation aims to detect when a FrameContext is get()
> before it is alloc()-ated, Warns about it, and initializes the
> FrameContext before returning it.
> 
> In case of frame#0, a get() preceding an alloc() call is not detected
> as the "frame == frameContext.frame" test returns success, as
> FrameContexts are zeroed by default.
> 
> As a result, the first returned FrameContext is not initialized.
> 
> Explicitly test for frame#0 to make sure the FrameContext is initialized
> if get(0) is called before alloc(0). To avoid re-initializing a frame
> context, in case alloc() has been called correctly before get(),
> introduce an "initialised" state variable that tracks the FrameContext
> initialisation state.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> ---
>  src/ipa/libipa/fc_queue.h | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h
> index b1e8bc1485d4..bfcce5a81356 100644
> --- a/src/ipa/libipa/fc_queue.h
> +++ b/src/ipa/libipa/fc_queue.h
> @@ -26,11 +26,13 @@ protected:
>  	virtual void init(const uint32_t frameNum)
>  	{
>  		frame = frameNum;
> +		initialised = true;

If I got it right, this only applies to the first initialization and not
when the frame context gets reused for another frame.

I believe we need to implement start controls anyways. I had a prototype
for that in:

c67de53b882e ("pipeline: rkisp1: Apply initial controls")

If I'm not mistaken we could do the the explicit alloc of the context
for frame 0 there.


>  	}
>  
>  private:
>  	template<typename T> friend class FCQueue;
>  	uint32_t frame;
> +	bool initialised = false;
>  };
>  
>  template<typename FrameContext>
> @@ -44,8 +46,10 @@ public:
>  
>  	void clear()
>  	{
> -		for (FrameContext &ctx : contexts_)
> +		for (FrameContext &ctx : contexts_) {
> +			ctx.initialised = false;
>  			ctx.frame = 0;
> +		}
>  	}
>  
>  	FrameContext &alloc(const uint32_t frame)
> @@ -89,6 +93,21 @@ public:
>  					    << " has been overwritten by "
>  					    << frameContext.frame;
>  
> +		if (frame == 0 && !frameContext.initialised) {
> +			/*
> +			 * If the IPA calls get() at start() time it will get an
> +			 * un-intialized FrameContext as the below "frame ==
> +			 * frameContext.frame" check will return success because
> +			 * FrameContexts are zeroed at creation time.
> +			 *
> +			 * Make sure the FrameContext gets initialised if get()
> +			 * is called before alloc() by the IPA for frame#0.
> +			 */
> +			frameContext.init(frame);

Wouldn't it be more consistent if we warn in the get(0) case as for the
other cases and ensure alloc get's called for frame 0?

Cheers,
Stefan

> +
> +			return frameContext;
> +		}
> +
>  		if (frame == frameContext.frame)
>  			return frameContext;
>  
> -- 
> 2.47.0
>