| Message ID | 20241016170348.715993-3-jacopo.mondi@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo On 16/10/2024 18:03, 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@ideasonboard.com> Reviewed-by: Daniel Scally <dan.scally@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; > } > > 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); > + > + return frameContext; > + } > + > if (frame == frameContext.frame) > return frameContext; >
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@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 >
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@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 > >
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@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 > > >
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@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 > > > >
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@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@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 > > > > >
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; } 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); + + return frameContext; + } + if (frame == frameContext.frame) return frameContext;
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@ideasonboard.com> --- src/ipa/libipa/fc_queue.h | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)