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

[Stefan Klug]

Hi Jacopo,

On Mon, Oct 28, 2024 at 05:24:48PM +0100, Jacopo Mondi wrote:
> 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.

Oh, I forgot that queueRequest is only allowed after start() sorry.
That is something we maybe should discuss again. In my head it's always
queueRequests() -> start(). Now I understood that this is not a special
case but happens on every start and that also explains why you don't
want to warn. Sorry that it took so long on my side.

I still don't like the asymmetric handling of get(0) but don't see an
easy way around. Maybe we should completely remove the alloc() function
and always do initialization on demand. But I didn't think that through.

As it stands now:
Reviewed-by: Stefan Klug <stefan.klug at ideasonboard.com> 

Cheers,
Stefan

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