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

[Stefan Klug]

Hi Jacopo,

On Mon, Oct 28, 2024 at 11:39:13AM +0100, Jacopo Mondi wrote:
> Hi Stefan
> 
> On Mon, Oct 28, 2024 at 11:16:23AM +0100, Stefan Klug wrote:
> > 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?
> >
> 
> The only entry point (in the current implementation) for alloc() is
> queueRequest. So if we set controls at start() when no request is
> queued we will always get(0) before alloc(0).
> 
> I think this should change going forward once we rework the IPA to
> work based on when a frame/stats are available instead of being
> clocked by the user submitted requests, but for now I think this is
> what we have. Or did you mean something completely different I have
> missed ?

My thought was to call alloc(0) in start() before setControls(0) if no
request was queued. So that the edge case is handled outside of the
FCQueue and the FCQueue can consistently warn for any context that
wasn't alloced before a get(). 

Or is there another reason to silence the warning in FCQueue? As imho
getting a frame without prior alloc should warn in any case.

Cheers,
Stefan

> 
> Thanks
>   j
> 
> > Cheers,
> > Stefan
> >
> > > +
> > > +			return frameContext;
> > > +		}
> > > +
> > >  		if (frame == frameContext.frame)
> > >  			return frameContext;
> > >
> > > --
> > > 2.47.0
> > >