[libcamera-devel] [PATCH v1 2/4] ipa: ipu3: ipa_context: Extend FCQueue::get()
Umang Jain
umang.jain at ideasonboard.com
Thu Jun 9 01:09:28 CEST 2022
Hi Jacopo,
Thanks for the review.
On 6/8/22 15:56, Jacopo Mondi wrote:
> Hi Umang,
>
> On Fri, Jun 03, 2022 at 03:22:57PM +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.
>>
>> 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 at ideasonboard.com>
>> ---
>> src/ipa/ipu3/ipa_context.cpp | 95 +++++++++++++++++++++++++++++++++---
>> src/ipa/ipu3/ipa_context.h | 9 ++++
>> src/ipa/ipu3/ipu3.cpp | 4 +-
>> 3 files changed, 100 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
>> index 9f95a61c..2438d68d 100644
>> --- a/src/ipa/ipu3/ipa_context.cpp
>> +++ b/src/ipa/ipu3/ipa_context.cpp
>> @@ -222,13 +222,33 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
>> * \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.
>> + * to be processed by the IPA at a given point. An 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
>> + */
>> +
>> +/**
>> + * \var FCQueue::isFull_
>> + * \brief Flag set when the FCQueue is full
>> */
>>
>> /**
>> * \brief FCQueue constructor
>> */
>> FCQueue::FCQueue()
>> + : head_(nullptr), tail_(nullptr), isFull_(false)
>> {
>> clear();
>> }
>> @@ -238,21 +258,75 @@ FCQueue::FCQueue()
>> * \param[in] frame Frame number for which the IPAFrameContext needs to
>> * retrieved
>> *
>> - * \return Pointer to the IPAFrameContext
>> + * This function returns the IPAFrameContext for the desired frame. If the
>> + * frame context does not exist in the queue, the next available slot in the
>> + * queue is returned. It is the responsibility of the caller to fill the correct
>> + * IPAFrameContext parameters of newly returned IPAFrameContext.
>> + *
>> + * \return Pointer to the IPAFrameContext or nullptr if frame context couldn't
>> + * be created
>> */
>> IPAFrameContext *FCQueue::get(uint32_t frame)
>> {
>> - IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);
>> + if (frame <= tail_->frame) {
> How does this work ? The first get() you call after construction has tail_
> == nullptr. How come this doesn't segfault ? Is it because there's a
> call to clear() that initializes the pointers before usage ?
> Shouldn't the constructor take care of creating a usable object
> without requiring clear() to be called ?
Yes, bad naming. I think reset() would be more appropriate? The
constructor is
responsible yes (hence it calls clear() in the first place) so it was
the matter of
code-deduplication. We can discuss the naming conventions but tail_
shouldn't
be a nullptr before any .get() calls. So I do want proper initialization
plus, a
clear() or reset() to clear the ring buffer and resetting the tail_,
head_ etc.
>
> Also, are we sure using the frame number in tail_ and head_ is the best
> way to ensure that we actually track where we are in the queue ?
>
> I have a few worries:
>
> 1) are frame numbers always starting from 0 ?
It should be, no?
>
> 2) frame numbers are monotonically increasing, but can have gaps.
> When you create a new frame context you increase by one to get the
> next slot, but when you get an existing frame you compute the index
> by doing frame % kMaxFrameContexts
>
> IPAFrameContext *FCQueue::get(uint32_t frame) {
>
> if (frame <= tail_->frame)
> /* GET */
> IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);
> else
> /* PUSH */
> tail_ = &this->at((tail_->frame + 1) % kMaxFrameContexts);
> tail_->frame = frame;
>
> Isn't there a risk you get the wrong frame ?
Yes, I've realized I have made an error here after posting to the list.
I put the IPAFrameContext on the next slot immediate to tail_ , but that's
not correct. It's only correct if we ensure there are not any gaps
(majority of the
development has been done by assuming that there will not be gaps for now).
Gaps / Frame drops is yet another beast to handle. I guess I am keeping
it separate
from the "per-frame controls" planning for now. I discussed the same
with Kieran
the other day - that the frame-drop handling and per-frame controls need
to kept
separate for progress. Otherwise both half-baked designs, trying to
satisfying/handle
each other arbitrarily might create chicken-and-egg problems.
>
> (also being this a LIFO, isn't the head the most recent and the
> tail the oldest entry ? you seem to advance tail when you push a
> new frame)
No, head_ represents the oldest entry and tail_ represents the latest
IPAFrameContext
that got created (refer documentation in this patch).
>
> 2 ) tail_ gets initialized with an empty IPAFrameContext whose constructor
> is
> IPAFrameContext::IPAFrameContext() = default;
>
> hence its frame id is not intialized, or are POD types default
> initialized in C++ ?
We manually give it a value of '0' in clear(). It should work out from
there.
The .get() calls will over-write the frame number while creating the
IPAFrameContext(s),
so I don't think it will be a problem as such..
>
> Anyway, isn't it simpler to use plain counters for head and tail instead
> of pointers to the contained objects ? (This would maybe make
> complicated to generalize the libcamera::RingBuffer implementation
> maybe), but head_ and tail_ mainly serve for two purpose:
> - underflow detection (trying to complete a frame not yet queued)
> - overflow detection (trying to queue a frame overwrites a
> not-yet-consumed one)
Well, the main point for having these pointers is to know if a
IPAFrameContext exists in the first
place or not. The underflow/overflow checks comes later (... and we need
have to a
additional head_ for that, otherwise simply a tail_ would have been
sufficient for the former)
I agree with you that plain counters would be sufficient (and it doesn't
really make a difference
to me, either you store pointers OR frame numbers of oldest's and
latest's). It is just storing
a unique handles somehow.
>
> Can't we have head and tail simply follow the latest frame number that
> have been queued and the lastest frame number that has been consumed
> respectively ?
>
> Collision detection will simply be
> - undeflow: tail + 1 == head
rather should be head + 1 == tail (according to how tail and head are
used in this patch)
> - overflow (queue frame): head - tail == array size
>
> The actual position on the array can always be computed as (frame %
> kMaxFrameContexts)
>
> This doesn't however work well with gaps, as one frame gap means we're
> actually not using one slot, so a little space is wasted. Ofc as the
> number of gaps approaches the number of available slots, the risk of
> overflow increases. But gaps should be an exceptional events and with
> large enough buffers this shouldn't be a problem ?
To be honest, I don't think storing IPAFrameContext pointers vs frame
numbers
should be a major concern. I say this because intrinsically everything
revolves
around the frame number in both cases.
Going forward (and it's only my educated guess), storing head and tail frame
numbers will get limiting a bit. I need to see how Kieran's work on
merging/retaining
controls is going on. The idea is controls from latest IPAFrameContext
gets retained
into next-created IPAFrameContext(s) and so on... If you think about it,
the tail_->controls
will get copied into the next IPAFrameContext while being created. In
cases like this,
having IPAFrameContexts pointers are useful in my opinion.
>
> Also I wonder if a push/get interface wouldn't be simpler, with new
> reuests being pushed() and frames consumed with get(), but that's more
> an implementation detail maybe..
I do wondered that as well, but in my opinion having a push() + get()
interface also means,
each algorithm has to do various checks on their part. For e.g.
Algorithm1:
.get(X) <--- Failed
.push(XFrameContext)
.get(X) <---- Success and carry on
Algorithm2:
.get(X) <-- Success because Algorithm1 created XFrameContext
// but you still need to write failure path here containing
.push(XFrameContext)
.. and so on.
Also, various Algorithm might need to create different frame's
IPAFrameContext in a single run,
depending on their latencies.
So I think we are unnecessarily outsourcing the responsibility of
"pushing" the IPAFrameContext
to the algorithms. All this can be encapsulated in the .get(), no? I
thought we all agreed on
the .get() design, so hence this direction.
>
> IPA components cannot have tests right ? It would be nice to have a
> unit test for this component to make sure it behaves as intended
> instead of having to debug it "live"
I see that you have mentioned a desire to make a generic
libcamera::RingBuffer
class implementation. While I do agree on the point of tests, but I am
not looking
at this angle just yet. It feels to me a bit early to do so because
things can be very
specific w.r.t IPAs. Even if we do it "generically" right now, only IPU3
would be the
only user I think. There are other similar parts that we want to provide
a generic
implementation to all the IPAs for e.g. IPAContext, IPAFrameContexts and
so on,
so probably once we have a good usage across multiple IPAs, we would be
in a
better position to write a generic ring buffer implementation then and adapt
the IPAs on top of it?
>
> sorry, a lot of questions actually..
No issues ;-)
>
>> + IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);
>> + if (frame != frameContext.frame)
>> + LOG(IPAIPU3, Warning)
>> + << "Got wrong frame context for frame " << frame;
>> +
>> + return &frameContext;
>> + } else {
>> + if (isFull_) {
>> + LOG(IPAIPU3, Warning)
>> + << "Cannot create frame context for frame " << frame
>> + << " since FCQueue is full";
>> +
>> + return nullptr;
>> + }
>> +
>> + /*
>> + * 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;
>> +
>> + /* Check if the queue is full to avoid over-queueing later */
>> + if (tail_->frame - head_->frame >= kMaxFrameContexts - 1)
>> + isFull_ = true;
>>
>> - if (frame != frameContext.frame) {
>> + 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);
>> +
>> + if (isFull_)
>> + isFull_ = false;
>> }
>>
>> +/**
>> + * \fn FCQueue::isFull()
>> + * \brief Checks whether the frame context queue is full
>> + */
>> +
>> /**
>> * \brief Clear the FCQueue by resetting all the entries in the ring-buffer
>> */
>> @@ -260,6 +334,13 @@ void FCQueue::clear()
>> {
>> IPAFrameContext initFrameContext;
>> this->fill(initFrameContext);
>> +
>> + isFull_ = false;
>> +
>> + /* 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 56d281f6..475855da 100644
>> --- a/src/ipa/ipu3/ipa_context.h
>> +++ b/src/ipa/ipu3/ipa_context.h
>> @@ -93,9 +93,18 @@ 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);
>> + bool isFull() { return isFull_; }
>> +
>> +private:
>> + IPAFrameContext *head_;
>> + IPAFrameContext *tail_;
>> +
>> + bool isFull_;
>> };
>>
>> struct IPAContext {
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index 0843d882..1d6ee515 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
>>
More information about the libcamera-devel
mailing list