| Message ID | 20220527191740.242300-3-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, On Fri, May 27, 2022 at 09:17:39PM +0200, Umang Jain via libcamera-devel wrote: > Extend the FCQueue::get() to return a IPAFrameContext for a > non-existent frame. The .get() should be able to figure out if a > existent frame context is asked for, or to create a new frame context > based on the next available position. To keep track of next available > position in the queue, use of head and tail pointer are used. s/use of// s/pointer/pointers/ > > We specifically want to have access to the queue through the s/have/only allow/ ? > .get() function hence operator[] is deleted for FCQueue as > part of this patch as well. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/ipa/ipu3/ipa_context.cpp | 81 +++++++++++++++++++++++++++++++++--- > src/ipa/ipu3/ipa_context.h | 5 +++ > src/ipa/ipu3/ipu3.cpp | 4 +- > 3 files changed, 83 insertions(+), 7 deletions(-) > > diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp > index e5010e3f..dcce6b48 100644 > --- a/src/ipa/ipu3/ipa_context.cpp > +++ b/src/ipa/ipu3/ipa_context.cpp > @@ -217,32 +217,96 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls) > * \brief Analogue gain multiplier > */ > > +/** > + * \class FCQueue > + * \brief A FIFO circular queue holding IPAFrameContext(s) > + * > + * FCQueue holds all the IPAFrameContext(s) related to frames required > + * to be processed by the IPA at a given point. A IPAFrameContext is created s/A/An/ > + * on the first call FCQueue::get(frame) for that frame. Subsequent calls to > + * FCQueue::get() with the same frame number shall return the IPAFrameContext > + * previously created until the frame is marked as complete through > + * FCQueue::completeFrame(frame). > + */ > + > +/** > + * \var FCQueue::head > + * \brief A pointer to the a IPAFrameContext next expected to complete > + */ > + > +/** > + * \var FCQueue::tail > + * \brief A pointer to the latest IPAFrameContext created > + */ > + > /** > * \brief FCQueue constructor > */ > FCQueue::FCQueue() > + : head(nullptr), tail(nullptr) > { > clear(); > } > > /** > - * Retrieve the IPAFrameContext for the frame > + * \brief Retrieve the IPAFrameContext for the frame > * \param[in] frame Frame number for which the IPAFrameContext needs to > * retrieved > * > + * This functions returns the IPAFrameContext for the desired frame. If the s/functions/function/ > + * frame context does not exists in the queue, the next available reference s/exists/exist/ > + * of the position in the queue, is returned. It is the responsibility of the s/,// > + * caller to fill the correct IPAFrameContext parameters of newly returned > + * IPAFrameContext. > + * > * \return Reference to the IPAFrameContext > */ > IPAFrameContext &FCQueue::get(uint32_t frame) > { > - IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts); > + if (frame <= tail->frame) { > + IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts); > + if (frame != frameContext.frame) > + LOG(IPAIPU3, Warning) > + << "Got wrong frame context for frame " << frame; > + return frameContext; > + } else { > + /* > + * Frame context doesn't exist yet so get the next available > + * position in the queue. > + */ > + tail = &this->at((tail->frame + 1) % kMaxFrameContexts); > + tail->frame = frame; If we're overflowing the queue, don't we want to avoid overwriting tail? > > - if (frame != frameContext.frame) { > + /* Avoid over-queuing of frame contexts */ > + if (tail->frame > kMaxFrameContexts && > + (tail->frame - head->frame) >= kMaxFrameContexts) { > + LOG(IPAIPU3, Error) << "FCQueue is full!"; > + > + /* \todo: Determine return reference? or make it fatal? */ iirc we discussed that the IPA should be notified if the get() fails so it could handle it somehow (like saving it in another queue). So... what about returning a pointer? Or is that dangerous :/ > + } > + > + return *tail; > + } > +} > + > +/** > + * \brief Notifies the FCQueue that a frame has been completed > + * \param[in] frame The completed frame number > + */ > +void FCQueue::completeFrame(uint32_t frame) > +{ > + if (head->frame != frame) > LOG(IPAIPU3, Warning) > - << "Got wrong frame context for frame" << frame > - << " or frame context doesn't exist yet"; > + << "Frame " << frame << " completed out-of-sync?"; > + > + if (frame > tail->frame) { > + LOG(IPAIPU3, Error) > + << "Completing a frame " << frame > + << " not present in the queue is disallowed"; > + return; > } > > - return frameContext; > + head = &this->get(frame + 1); This ought to work fine even with frame drop right...? Because the application still has to queue requests if it wants frames... so the skipped ones would still be completed... so the +1 increment will be correct. > } > > /** > @@ -252,6 +316,11 @@ void FCQueue::clear() > { > IPAFrameContext initFrameContext; > this->fill(initFrameContext); > + > + /* Intialise 0th index to frame 0 */ > + this->at(0).frame = 0; > + tail = &this->at(0); > + head = tail; > } > > } /* namespace ipa::ipu3 */ > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h > index 61454b41..b36ec61d 100644 > --- a/src/ipa/ipu3/ipa_context.h > +++ b/src/ipa/ipu3/ipa_context.h > @@ -93,9 +93,14 @@ class FCQueue : public std::array<IPAFrameContext, kMaxFrameContexts> > { > public: > FCQueue(); > + FCQueue &operator[](const FCQueue &) = delete; > > void clear(); > + void completeFrame(uint32_t frame); > IPAFrameContext &get(uint32_t frame); > + > + IPAFrameContext *head; > + IPAFrameContext *tail; Should these have the _ suffix (because this is a class and not a struct)? Paul > }; > > struct IPAContext { > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > index c48d2f62..e09c5d05 100644 > --- a/src/ipa/ipu3/ipu3.cpp > +++ b/src/ipa/ipu3/ipu3.cpp > @@ -605,6 +605,8 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, > */ > > metadataReady.emit(frame, ctrls); > + > + context_.frameContexts.completeFrame(frame); > } > > /** > @@ -618,7 +620,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, > void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls) > { > /* \todo Start processing for 'frame' based on 'controls'. */ > - context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls }; > + context_.frameContexts.get(frame) = { frame, controls }; > } > > /** > -- > 2.31.1 >
Hi Paul, Thank you for the review. On 5/31/22 09:41, paul.elder@ideasonboard.com wrote: > Hi Umang, > > On Fri, May 27, 2022 at 09:17:39PM +0200, Umang Jain via libcamera-devel wrote: >> Extend the FCQueue::get() to return a IPAFrameContext for a >> non-existent frame. The .get() should be able to figure out if a >> existent frame context is asked for, or to create a new frame context >> based on the next available position. To keep track of next available >> position in the queue, use of head and tail pointer are used. > s/use of// > > s/pointer/pointers/ > >> We specifically want to have access to the queue through the > s/have/only allow/ ? > >> .get() function hence operator[] is deleted for FCQueue as >> part of this patch as well. >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> src/ipa/ipu3/ipa_context.cpp | 81 +++++++++++++++++++++++++++++++++--- >> src/ipa/ipu3/ipa_context.h | 5 +++ >> src/ipa/ipu3/ipu3.cpp | 4 +- >> 3 files changed, 83 insertions(+), 7 deletions(-) >> >> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp >> index e5010e3f..dcce6b48 100644 >> --- a/src/ipa/ipu3/ipa_context.cpp >> +++ b/src/ipa/ipu3/ipa_context.cpp >> @@ -217,32 +217,96 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls) >> * \brief Analogue gain multiplier >> */ >> >> +/** >> + * \class FCQueue >> + * \brief A FIFO circular queue holding IPAFrameContext(s) >> + * >> + * FCQueue holds all the IPAFrameContext(s) related to frames required >> + * to be processed by the IPA at a given point. A IPAFrameContext is created > s/A/An/ > >> + * on the first call FCQueue::get(frame) for that frame. Subsequent calls to >> + * FCQueue::get() with the same frame number shall return the IPAFrameContext >> + * previously created until the frame is marked as complete through >> + * FCQueue::completeFrame(frame). >> + */ >> + >> +/** >> + * \var FCQueue::head >> + * \brief A pointer to the a IPAFrameContext next expected to complete >> + */ >> + >> +/** >> + * \var FCQueue::tail >> + * \brief A pointer to the latest IPAFrameContext created >> + */ >> + >> /** >> * \brief FCQueue constructor >> */ >> FCQueue::FCQueue() >> + : head(nullptr), tail(nullptr) >> { >> clear(); >> } >> >> /** >> - * Retrieve the IPAFrameContext for the frame >> + * \brief Retrieve the IPAFrameContext for the frame >> * \param[in] frame Frame number for which the IPAFrameContext needs to >> * retrieved >> * >> + * This functions returns the IPAFrameContext for the desired frame. If the > s/functions/function/ > >> + * frame context does not exists in the queue, the next available reference > s/exists/exist/ > >> + * of the position in the queue, is returned. It is the responsibility of the > s/,// > >> + * caller to fill the correct IPAFrameContext parameters of newly returned >> + * IPAFrameContext. >> + * >> * \return Reference to the IPAFrameContext >> */ >> IPAFrameContext &FCQueue::get(uint32_t frame) >> { >> - IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts); >> + if (frame <= tail->frame) { >> + IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts); >> + if (frame != frameContext.frame) >> + LOG(IPAIPU3, Warning) >> + << "Got wrong frame context for frame " << frame; >> + return frameContext; >> + } else { >> + /* >> + * Frame context doesn't exist yet so get the next available >> + * position in the queue. >> + */ >> + tail = &this->at((tail->frame + 1) % kMaxFrameContexts); >> + tail->frame = frame; > If we're overflowing the queue, don't we want to avoid overwriting tail? I haven't particularly decided on how should be we handle overflowing of requests. I just" detect it" for now, and print an error (as mentioned in the \todo below) so yes, it is a matter of discussion and some design thoughts. I don't have a very good options right now > >> >> - if (frame != frameContext.frame) { >> + /* Avoid over-queuing of frame contexts */ >> + if (tail->frame > kMaxFrameContexts && >> + (tail->frame - head->frame) >= kMaxFrameContexts) { >> + LOG(IPAIPU3, Error) << "FCQueue is full!"; >> + >> + /* \todo: Determine return reference? or make it fatal? */ > iirc we discussed that the IPA should be notified if the get() fails so > it could handle it somehow (like saving it in another queue). So... > what about returning a pointer? Or is that dangerous :/ I don't recall the discussion of maintaining yet another queue in the IPA. Yes, we should notify if the .get() fails but what's will it get plumbed to? Since we return a reference, we can't return a nullptr type reference. We will have to return something, or change the function signature (overall). Maybe if we want to notify .get() failed then how about `bool FcQueue::get(uint32_t frame, IPAFrameContext &outFrameContext)` ? Just bouncing thoughts... But are we sure that in some cases, the IPA::queueRequest() can be overflown with requests? I need to check that probability because the IPU3 pipeline handler itself maintains two queue for requests I think which are: std::queue<Request *> pendingRequests_; std::queue<Request *> processingRequests_; So everything goes through pendingRequests_ first and then to processingRequests_. I see that before the request is popped from pendingRequests_ it does get queued to IPA. So maybe we can surely overflow in the IPA itself.... > >> + } >> + >> + return *tail; >> + } >> +} >> + >> +/** >> + * \brief Notifies the FCQueue that a frame has been completed >> + * \param[in] frame The completed frame number >> + */ >> +void FCQueue::completeFrame(uint32_t frame) >> +{ >> + if (head->frame != frame) >> LOG(IPAIPU3, Warning) >> - << "Got wrong frame context for frame" << frame >> - << " or frame context doesn't exist yet"; >> + << "Frame " << frame << " completed out-of-sync?"; >> + >> + if (frame > tail->frame) { >> + LOG(IPAIPU3, Error) >> + << "Completing a frame " << frame >> + << " not present in the queue is disallowed"; >> + return; >> } >> >> - return frameContext; >> + head = &this->get(frame + 1); > This ought to work fine even with frame drop right...? Because the > application still has to queue requests if it wants frames... so the > skipped ones would still be completed... so the +1 increment will be > correct. Yes it should, that was more or less why we went with ring-buffer implementation rather than a queue And we don't care to cleanup since its a fixed-sized ring buffer. Things will get over-written as requests come in > >> } >> >> /** >> @@ -252,6 +316,11 @@ void FCQueue::clear() >> { >> IPAFrameContext initFrameContext; >> this->fill(initFrameContext); >> + >> + /* Intialise 0th index to frame 0 */ >> + this->at(0).frame = 0; >> + tail = &this->at(0); >> + head = tail; >> } >> >> } /* namespace ipa::ipu3 */ >> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h >> index 61454b41..b36ec61d 100644 >> --- a/src/ipa/ipu3/ipa_context.h >> +++ b/src/ipa/ipu3/ipa_context.h >> @@ -93,9 +93,14 @@ class FCQueue : public std::array<IPAFrameContext, kMaxFrameContexts> >> { >> public: >> FCQueue(); >> + FCQueue &operator[](const FCQueue &) = delete; >> >> void clear(); >> + void completeFrame(uint32_t frame); >> IPAFrameContext &get(uint32_t frame); >> + >> + IPAFrameContext *head; >> + IPAFrameContext *tail; > Should these have the _ suffix (because this is a class and not a > struct)? Ah the _ suffix are for private members I think. I haven't made head and tail private here. I think they need to be private! > > > Paul > >> }; >> >> struct IPAContext { >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >> index c48d2f62..e09c5d05 100644 >> --- a/src/ipa/ipu3/ipu3.cpp >> +++ b/src/ipa/ipu3/ipu3.cpp >> @@ -605,6 +605,8 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, >> */ >> >> metadataReady.emit(frame, ctrls); >> + >> + context_.frameContexts.completeFrame(frame); >> } >> >> /** >> @@ -618,7 +620,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, >> void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls) >> { >> /* \todo Start processing for 'frame' based on 'controls'. */ >> - context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls }; >> + context_.frameContexts.get(frame) = { frame, controls }; >> } >> >> /** >> -- >> 2.31.1 >>
Hi Umang, Thanks for the patch ! On 27/05/2022 21:17, Umang Jain via libcamera-devel wrote: > Extend the FCQueue::get() to return a IPAFrameContext for a > non-existent frame. The .get() should be able to figure out if a > existent frame context is asked for, or to create a new frame context > based on the next available position. To keep track of next available > position in the queue, use of head and tail pointer are used. I don't know if it is the easier way for us :-). I like the idea that, when get() can't find the frame, it creates one, but as we see it later in the code, it also introduces a difficulty: what should we do when the queue is full, and we can't create a new one ? I think it makes it a "too-smart" ring buffer, maybe ? AFAIK, the frameContext is created when we enter the process() call. A simple approach would be to have a set() when we enter process(), and the algorithms will only call get(). This way set() would be responsible of managing the size ? > > We specifically want to have access to the queue through the > .get() function hence operator[] is deleted for FCQueue as > part of this patch as well. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/ipa/ipu3/ipa_context.cpp | 81 +++++++++++++++++++++++++++++++++--- > src/ipa/ipu3/ipa_context.h | 5 +++ > src/ipa/ipu3/ipu3.cpp | 4 +- > 3 files changed, 83 insertions(+), 7 deletions(-) > > diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp > index e5010e3f..dcce6b48 100644 > --- a/src/ipa/ipu3/ipa_context.cpp > +++ b/src/ipa/ipu3/ipa_context.cpp > @@ -217,32 +217,96 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls) > * \brief Analogue gain multiplier > */ > > +/** > + * \class FCQueue > + * \brief A FIFO circular queue holding IPAFrameContext(s) > + * > + * FCQueue holds all the IPAFrameContext(s) related to frames required > + * to be processed by the IPA at a given point. A IPAFrameContext is created > + * on the first call FCQueue::get(frame) for that frame. Subsequent calls to > + * FCQueue::get() with the same frame number shall return the IPAFrameContext > + * previously created until the frame is marked as complete through > + * FCQueue::completeFrame(frame). > + */ > + > +/** > + * \var FCQueue::head > + * \brief A pointer to the a IPAFrameContext next expected to complete > + */ > + > +/** > + * \var FCQueue::tail > + * \brief A pointer to the latest IPAFrameContext created > + */ > + > /** > * \brief FCQueue constructor > */ > FCQueue::FCQueue() > + : head(nullptr), tail(nullptr) > { > clear(); > } > > /** > - * Retrieve the IPAFrameContext for the frame > + * \brief Retrieve the IPAFrameContext for the frame > * \param[in] frame Frame number for which the IPAFrameContext needs to > * retrieved > * > + * This functions returns the IPAFrameContext for the desired frame. If the > + * frame context does not exists in the queue, the next available reference > + * of the position in the queue, is returned. It is the responsibility of the > + * caller to fill the correct IPAFrameContext parameters of newly returned > + * IPAFrameContext. > + * > * \return Reference to the IPAFrameContext > */ > IPAFrameContext &FCQueue::get(uint32_t frame) > { > - IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts); > + if (frame <= tail->frame) { > + IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts); > + if (frame != frameContext.frame) > + LOG(IPAIPU3, Warning) > + << "Got wrong frame context for frame " << frame; > + return frameContext; > + } else { > + /* > + * Frame context doesn't exist yet so get the next available > + * position in the queue. > + */ > + tail = &this->at((tail->frame + 1) % kMaxFrameContexts); > + tail->frame = frame; > > - if (frame != frameContext.frame) { > + /* Avoid over-queuing of frame contexts */ > + if (tail->frame > kMaxFrameContexts && > + (tail->frame - head->frame) >= kMaxFrameContexts) { > + LOG(IPAIPU3, Error) << "FCQueue is full!"; > + > + /* \todo: Determine return reference? or make it fatal? */ > + } > + > + return *tail; > + } > +} > + > +/** > + * \brief Notifies the FCQueue that a frame has been completed > + * \param[in] frame The completed frame number > + */ > +void FCQueue::completeFrame(uint32_t frame) > +{ > + if (head->frame != frame) > LOG(IPAIPU3, Warning) > - << "Got wrong frame context for frame" << frame > - << " or frame context doesn't exist yet"; > + << "Frame " << frame << " completed out-of-sync?"; > + > + if (frame > tail->frame) { > + LOG(IPAIPU3, Error) > + << "Completing a frame " << frame > + << " not present in the queue is disallowed"; > + return; > } > > - return frameContext; > + head = &this->get(frame + 1); > } > > /** > @@ -252,6 +316,11 @@ void FCQueue::clear() > { > IPAFrameContext initFrameContext; > this->fill(initFrameContext); > + > + /* Intialise 0th index to frame 0 */ > + this->at(0).frame = 0; > + tail = &this->at(0); > + head = tail; > } > > } /* namespace ipa::ipu3 */ > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h > index 61454b41..b36ec61d 100644 > --- a/src/ipa/ipu3/ipa_context.h > +++ b/src/ipa/ipu3/ipa_context.h > @@ -93,9 +93,14 @@ class FCQueue : public std::array<IPAFrameContext, kMaxFrameContexts> > { > public: > FCQueue(); > + FCQueue &operator[](const FCQueue &) = delete; > > void clear(); > + void completeFrame(uint32_t frame); > IPAFrameContext &get(uint32_t frame); > + > + IPAFrameContext *head; > + IPAFrameContext *tail; If you want it to be public, I think having a tail() and a head() function would be cleaner. Those pointer should be private with _ suffixes. JM > }; > > struct IPAContext { > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > index c48d2f62..e09c5d05 100644 > --- a/src/ipa/ipu3/ipu3.cpp > +++ b/src/ipa/ipu3/ipu3.cpp > @@ -605,6 +605,8 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, > */ > > metadataReady.emit(frame, ctrls); > + > + context_.frameContexts.completeFrame(frame); > } > > /** > @@ -618,7 +620,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, > void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls) > { > /* \todo Start processing for 'frame' based on 'controls'. */ > - context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls }; > + context_.frameContexts.get(frame) = { frame, controls }; > } > > /**
Hi Jean-Michel, On 6/1/22 08:38, Jean-Michel Hautbois wrote: > Hi Umang, > > Thanks for the patch ! > > On 27/05/2022 21:17, Umang Jain via libcamera-devel wrote: >> Extend the FCQueue::get() to return a IPAFrameContext for a >> non-existent frame. The .get() should be able to figure out if a >> existent frame context is asked for, or to create a new frame context >> based on the next available position. To keep track of next available >> position in the queue, use of head and tail pointer are used. > > I don't know if it is the easier way for us :-). I like the idea that, > when get() can't find the frame, it creates one, but as we see it > later in the code, it also introduces a difficulty: what should we do > when the queue is full, and we can't create a new one ? Yes, but it's an edge case - that we need to handle. It's not like we should drop the idea of ring-buffer overall. > I think it makes it a "too-smart" ring buffer, maybe ? > > AFAIK, the frameContext is created when we enter the process() call. A > simple approach would be to have a set() when we enter process(), and > the algorithms will only call get(). This way set() would be > responsible of managing the size ? No, we just cannot assume this - that were it's created Any algorithm depending on it's "look-ahead", shall be free to create a frameContext and populate it. > >> >> We specifically want to have access to the queue through the >> .get() function hence operator[] is deleted for FCQueue as >> part of this patch as well. >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> src/ipa/ipu3/ipa_context.cpp | 81 +++++++++++++++++++++++++++++++++--- >> src/ipa/ipu3/ipa_context.h | 5 +++ >> src/ipa/ipu3/ipu3.cpp | 4 +- >> 3 files changed, 83 insertions(+), 7 deletions(-) >> >> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp >> index e5010e3f..dcce6b48 100644 >> --- a/src/ipa/ipu3/ipa_context.cpp >> +++ b/src/ipa/ipu3/ipa_context.cpp >> @@ -217,32 +217,96 @@ IPAFrameContext::IPAFrameContext(uint32_t id, >> const ControlList &reqControls) >> * \brief Analogue gain multiplier >> */ >> +/** >> + * \class FCQueue >> + * \brief A FIFO circular queue holding IPAFrameContext(s) >> + * >> + * FCQueue holds all the IPAFrameContext(s) related to frames required >> + * to be processed by the IPA at a given point. A IPAFrameContext is >> created >> + * on the first call FCQueue::get(frame) for that frame. Subsequent >> calls to >> + * FCQueue::get() with the same frame number shall return the >> IPAFrameContext >> + * previously created until the frame is marked as complete through >> + * FCQueue::completeFrame(frame). >> + */ >> + >> +/** >> + * \var FCQueue::head >> + * \brief A pointer to the a IPAFrameContext next expected to complete >> + */ >> + >> +/** >> + * \var FCQueue::tail >> + * \brief A pointer to the latest IPAFrameContext created >> + */ >> + >> /** >> * \brief FCQueue constructor >> */ >> FCQueue::FCQueue() >> + : head(nullptr), tail(nullptr) >> { >> clear(); >> } >> /** >> - * Retrieve the IPAFrameContext for the frame >> + * \brief Retrieve the IPAFrameContext for the frame >> * \param[in] frame Frame number for which the IPAFrameContext >> needs to >> * retrieved >> * >> + * This functions returns the IPAFrameContext for the desired frame. >> If the >> + * frame context does not exists in the queue, the next available >> reference >> + * of the position in the queue, is returned. It is the >> responsibility of the >> + * caller to fill the correct IPAFrameContext parameters of newly >> returned >> + * IPAFrameContext. >> + * >> * \return Reference to the IPAFrameContext >> */ >> IPAFrameContext &FCQueue::get(uint32_t frame) >> { >> - IPAFrameContext &frameContext = this->at(frame % >> kMaxFrameContexts); >> + if (frame <= tail->frame) { >> + IPAFrameContext &frameContext = this->at(frame % >> kMaxFrameContexts); >> + if (frame != frameContext.frame) >> + LOG(IPAIPU3, Warning) >> + << "Got wrong frame context for frame " << frame; >> + return frameContext; >> + } else { >> + /* >> + * Frame context doesn't exist yet so get the next available >> + * position in the queue. >> + */ >> + tail = &this->at((tail->frame + 1) % kMaxFrameContexts); >> + tail->frame = frame; >> - if (frame != frameContext.frame) { >> + /* Avoid over-queuing of frame contexts */ >> + if (tail->frame > kMaxFrameContexts && >> + (tail->frame - head->frame) >= kMaxFrameContexts) { >> + LOG(IPAIPU3, Error) << "FCQueue is full!"; >> + >> + /* \todo: Determine return reference? or make it fatal? */ >> + } >> + >> + return *tail; >> + } >> +} >> + >> +/** >> + * \brief Notifies the FCQueue that a frame has been completed >> + * \param[in] frame The completed frame number >> + */ >> +void FCQueue::completeFrame(uint32_t frame) >> +{ >> + if (head->frame != frame) >> LOG(IPAIPU3, Warning) >> - << "Got wrong frame context for frame" << frame >> - << " or frame context doesn't exist yet"; >> + << "Frame " << frame << " completed out-of-sync?"; >> + >> + if (frame > tail->frame) { >> + LOG(IPAIPU3, Error) >> + << "Completing a frame " << frame >> + << " not present in the queue is disallowed"; >> + return; >> } >> - return frameContext; >> + head = &this->get(frame + 1); >> } >> /** >> @@ -252,6 +316,11 @@ void FCQueue::clear() >> { >> IPAFrameContext initFrameContext; >> this->fill(initFrameContext); >> + >> + /* Intialise 0th index to frame 0 */ >> + this->at(0).frame = 0; >> + tail = &this->at(0); >> + head = tail; >> } >> } /* namespace ipa::ipu3 */ >> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h >> index 61454b41..b36ec61d 100644 >> --- a/src/ipa/ipu3/ipa_context.h >> +++ b/src/ipa/ipu3/ipa_context.h >> @@ -93,9 +93,14 @@ class FCQueue : public std::array<IPAFrameContext, >> kMaxFrameContexts> >> { >> public: >> FCQueue(); >> + FCQueue &operator[](const FCQueue &) = delete; >> void clear(); >> + void completeFrame(uint32_t frame); >> IPAFrameContext &get(uint32_t frame); >> + >> + IPAFrameContext *head; >> + IPAFrameContext *tail; > > If you want it to be public, I think having a tail() and a head() > function would be cleaner. Those pointer should be private with _ > suffixes. Don't think these pointers and/or tail() head(), are needed to be public. Will make it private. Down the line, we can have helpers like, if necessary: FCQueue::isFull() and FCQueue::isEmpty() which will operate on head and tail. > > JM > >> }; >> struct IPAContext { >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >> index c48d2f62..e09c5d05 100644 >> --- a/src/ipa/ipu3/ipu3.cpp >> +++ b/src/ipa/ipu3/ipu3.cpp >> @@ -605,6 +605,8 @@ void IPAIPU3::processStatsBuffer(const uint32_t >> frame, >> */ >> metadataReady.emit(frame, ctrls); >> + >> + context_.frameContexts.completeFrame(frame); >> } >> /** >> @@ -618,7 +620,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t >> frame, >> void IPAIPU3::queueRequest(const uint32_t frame, const ControlList >> &controls) >> { >> /* \todo Start processing for 'frame' based on 'controls'. */ >> - context_.frameContexts[frame % kMaxFrameContexts] = { frame, >> controls }; >> + context_.frameContexts.get(frame) = { frame, controls }; >> } >> /**
diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp index e5010e3f..dcce6b48 100644 --- a/src/ipa/ipu3/ipa_context.cpp +++ b/src/ipa/ipu3/ipa_context.cpp @@ -217,32 +217,96 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls) * \brief Analogue gain multiplier */ +/** + * \class FCQueue + * \brief A FIFO circular queue holding IPAFrameContext(s) + * + * FCQueue holds all the IPAFrameContext(s) related to frames required + * to be processed by the IPA at a given point. A IPAFrameContext is created + * on the first call FCQueue::get(frame) for that frame. Subsequent calls to + * FCQueue::get() with the same frame number shall return the IPAFrameContext + * previously created until the frame is marked as complete through + * FCQueue::completeFrame(frame). + */ + +/** + * \var FCQueue::head + * \brief A pointer to the a IPAFrameContext next expected to complete + */ + +/** + * \var FCQueue::tail + * \brief A pointer to the latest IPAFrameContext created + */ + /** * \brief FCQueue constructor */ FCQueue::FCQueue() + : head(nullptr), tail(nullptr) { clear(); } /** - * Retrieve the IPAFrameContext for the frame + * \brief Retrieve the IPAFrameContext for the frame * \param[in] frame Frame number for which the IPAFrameContext needs to * retrieved * + * This functions returns the IPAFrameContext for the desired frame. If the + * frame context does not exists in the queue, the next available reference + * of the position in the queue, is returned. It is the responsibility of the + * caller to fill the correct IPAFrameContext parameters of newly returned + * IPAFrameContext. + * * \return Reference to the IPAFrameContext */ IPAFrameContext &FCQueue::get(uint32_t frame) { - IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts); + if (frame <= tail->frame) { + IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts); + if (frame != frameContext.frame) + LOG(IPAIPU3, Warning) + << "Got wrong frame context for frame " << frame; + return frameContext; + } else { + /* + * Frame context doesn't exist yet so get the next available + * position in the queue. + */ + tail = &this->at((tail->frame + 1) % kMaxFrameContexts); + tail->frame = frame; - if (frame != frameContext.frame) { + /* Avoid over-queuing of frame contexts */ + if (tail->frame > kMaxFrameContexts && + (tail->frame - head->frame) >= kMaxFrameContexts) { + LOG(IPAIPU3, Error) << "FCQueue is full!"; + + /* \todo: Determine return reference? or make it fatal? */ + } + + return *tail; + } +} + +/** + * \brief Notifies the FCQueue that a frame has been completed + * \param[in] frame The completed frame number + */ +void FCQueue::completeFrame(uint32_t frame) +{ + if (head->frame != frame) LOG(IPAIPU3, Warning) - << "Got wrong frame context for frame" << frame - << " or frame context doesn't exist yet"; + << "Frame " << frame << " completed out-of-sync?"; + + if (frame > tail->frame) { + LOG(IPAIPU3, Error) + << "Completing a frame " << frame + << " not present in the queue is disallowed"; + return; } - return frameContext; + head = &this->get(frame + 1); } /** @@ -252,6 +316,11 @@ void FCQueue::clear() { IPAFrameContext initFrameContext; this->fill(initFrameContext); + + /* Intialise 0th index to frame 0 */ + this->at(0).frame = 0; + tail = &this->at(0); + head = tail; } } /* namespace ipa::ipu3 */ diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h index 61454b41..b36ec61d 100644 --- a/src/ipa/ipu3/ipa_context.h +++ b/src/ipa/ipu3/ipa_context.h @@ -93,9 +93,14 @@ class FCQueue : public std::array<IPAFrameContext, kMaxFrameContexts> { public: FCQueue(); + FCQueue &operator[](const FCQueue &) = delete; void clear(); + void completeFrame(uint32_t frame); IPAFrameContext &get(uint32_t frame); + + IPAFrameContext *head; + IPAFrameContext *tail; }; struct IPAContext { diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp index c48d2f62..e09c5d05 100644 --- a/src/ipa/ipu3/ipu3.cpp +++ b/src/ipa/ipu3/ipu3.cpp @@ -605,6 +605,8 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, */ metadataReady.emit(frame, ctrls); + + context_.frameContexts.completeFrame(frame); } /** @@ -618,7 +620,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls) { /* \todo Start processing for 'frame' based on 'controls'. */ - context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls }; + context_.frameContexts.get(frame) = { frame, controls }; } /**
Extend the FCQueue::get() to return a IPAFrameContext for a non-existent frame. The .get() should be able to figure out if a existent frame context is asked for, or to create a new frame context based on the next available position. To keep track of next available position in the queue, use of head and tail pointer are used. We specifically want to have access to the queue through the .get() function hence operator[] is deleted for FCQueue as part of this patch as well. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- src/ipa/ipu3/ipa_context.cpp | 81 +++++++++++++++++++++++++++++++++--- src/ipa/ipu3/ipa_context.h | 5 +++ src/ipa/ipu3/ipu3.cpp | 4 +- 3 files changed, 83 insertions(+), 7 deletions(-)