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

[Jacopo Mondi]

Hi Stefan

On Mon, Oct 28, 2024 at 04:40:15PM +0100, Stefan Klug wrote:
> 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

That's a possibility. I'm not sure if we have decided or not we're
going to allow Request pre-queing or not (it's a rather invasive API
change) but in that case each single IPA should go and check if a
Requests have been queued before start.

> 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.
>

Weeeeeeel, moving forward we're going to see Request underrun more
often, I don't think we should consider that a WARN, but I'm sure
we're going to discuss it.


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