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

[Jacopo Mondi]

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 ?

Thanks
  j

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