[libcamera-devel] [PATCH v1 2/4] ipa: ipu3: ipa_context: Extend FCQueue::get()
Jean-Michel Hautbois
jeanmichel.hautbois at ideasonboard.com
Mon Jun 13 16:39:59 CEST 2022
Hi Umang, Jacopo, Kieran,
On 11/06/2022 16:40, Jacopo Mondi via libcamera-devel wrote:
> Hi Kieran
>
> On Fri, Jun 10, 2022 at 08:20:32PM +0100, Kieran Bingham wrote:
>> Hi Jacopo,
>>
>> Quoting Jacopo Mondi via libcamera-devel (2022-06-09 17:20:54)
>>> Hi Umang,
>>>
>>> On Thu, Jun 09, 2022 at 01:09:28AM +0200, Umang Jain wrote:
>>>> 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
>>>
>>> I missed the call to clear() in the constructor and I thought this
>>> worked because of the clear() call in IPA::configure(). Sorry, this is
>>> ok I guess.
>>>
>>>> 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?
>>>>
>>>
>>> I would say that it depends on how the kernel driver counts the frame
>>> numbers. If the MIPI CSI-2 frame number is used it will count from 1
>>> and increment per-virtual channel. If the driver maintains an internal
>>> counter it should be reset at every start_stream/stop_stream session,
>>> but I don't this there are guarantees if not looking at the driver and
>>> making sure it does the right thing ?
>>>
>>> However I now also recall Kieran had a patch on his FrameSequence
>>> series to restart counting from 0 the frame sequence number in
>>> libcamera, do we assume they will always start from 0 because of this?
>>>
>>
>> Yes, I have a patch that will ensure frame numbers always start at zero
>> from the V4L2VideoDevice.
>>
>> I think this is important to keep things consistent, regardless of what
>> the kernel does or doesn't do correctly.. So the first frame from the
>> CSI2 receiver should always be zero from any call to start().
>>
>> We have it in development branches, but I don't think I've posted it to
>> the list yet. I'll try to make sure I do that on Monday, but you'll
>> probably find it in the code camp development branch before that if
>> you're interested.
>>
>>
>>
>>>>> 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.
>>>
>>> Frame drops had been biting us hard enough already. I think we should
>>> take them into account from the beginning in order to avoid having to
>>> re-think how to handle them later..
>>>
>>> That's why I think we need to define how mapping the frame number to
>>> the intended slot and do so uniquely in both "get" and "push". The
>>> current "frame % kMaxFrameContexts" is enough (it will only cause
>>> problems if the number of dropped frames in a kMaxFrameContexts period
>>> is larger than the queue depth I think).
>>
>> Yes, a missed frame should still consume it's slot. Lets not try to keep
>> all slots used - that will get a little messy I think. Just indexing the
>> slot based on the frame should be sufficient - if we get more than
>> kMaxFrameContexts of frame drop - then we have bigger issues and things
>> will be quite wrong (in PFC terms) but if we can detect it, we should
>> set any PFC error flag (and frame/request underrun flag?) and continue.
>>
>>>>
>>>>>
>>>>> (also being this a LIFO, isn't the head the most recent and the
>>>
>>> Sorry, I clearly meant FIFO
>>>
>>>>> 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).
>>>>
>>>
>>> Mine was mostly a comment on the general concept, not as much as the
>>> documentation of what you've done :) My thought was that in a stack
>>> (LIFO) you pop the head and in a queue (FIFO) you pop the tail (and
>>> consequentlly adavance the head when you push a new context).
>>>
>>> Basically this:
>>> https://softwareengineering.stackexchange.com/questions/144477/on-a-queue-which-end-is-the-head
>>>
>>> However STL seems to use "front" and "back" for queues [2] as
>>> synonymous for your head and tail, so I guess it's fine the way you
>>> have it (you could use "front" and "back" to stay closer to STL)
>>>
>>> [2] https://en.cppreference.com/w/cpp/container/queue
>>>
>>>>>
>>>>> 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.
>>>
>>> Right. Again careful that if frame numbers are numbered using the
>>> CSI-2 frame number, it will count from 1.
>>
>> Where have you got this '1' from ? Is that 'just' the IPU3 CIO2
>
> From the CSI-2 specification.
>
> I admit I have an old version: "Version 1.01.00 r0.04 2-Apr-2009"
> Section 9.8.1 "Frame Synchonization Packets"
>
> ------------------------------------------------------------------------------
> For FS and FE synchronization packets the Short Packet Data Field
> shall contain a 16-bit frame number. This frame number shall be the
> same for the FS and FE synchronization packets corresponding to a
> given frame.
>
> The 16-bit frame number, when used, shall be non-zero to distinguish
> it from the use-case where frame number is inoperative and remains set
> to zero.
>
> The behavior of the 16-bit frame number shall be as one of the
> following
> •Frame number is always zero – frame number is inoperative.
> •Frame number increments by 1 for every FS packet with the same
> Virtual Channel and is periodically reset to one e.g. 1, 2, 1, 2, 1,
> 2, 1, 2 or 1, 2, 3, 4, 1, 2, 3, 4
> ------------------------------------------------------------------------------
>
>> receiver?
>>
>
> As far as I can see IPU3, RPi and RKISP1 use a counter and not the
> CSI-2 frame number
>
The sequence number beeing forced to 0 in Kieran's patch [1] is
correcting the CSI-2 issue. But I agree with Jacopo, we have another
one, because we set the frame number to the request->sequence() and when
we are in a stop/start it won't be reset to 0.
Quoting d874b3e34173811fa89b68b4b71f22182bc5fd98:
"When requests are queued, they are given a sequential number to track
the order in which requests are queued to a camera. This number counts
all requests given to a camera through its lifetime, and is not reset to
zero between camera stop/start sequences.
It can be used to support debugging and identifying the flow of requests
through a pipeline, but does not guarantee to represent the sequence
number of any images in the stream. The sequence number is stored as an
unsigned integer and will wrap when overflowed.
"
Thus, I think IPU3Frames is not using the correct counter...
[1]: https://patchwork.libcamera.org/project/libcamera/list/?series=3173
>> It's precisely why I created the patch to V4L2VideoDevice to make all
>> devices consistent.
>>
>>>> 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
>>>
>>> Well, you have this to find out if a frame context is the "right" one
>>>
>>> IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);
>>> if (frame != frameContext.frame)
>>> LOG(IPAIPU3, Warning)
>>> << "Got wrong frame context for frame " << frame;
>>>
>>> But the distinction between a "push" and a "get" is just
>>>
>>> if (frame <= tail_->frame)
>>>
>>> which could be very well realized with integers.
>>
>> A frame count in the tail and head variables could indeed be easy and
>> clear enough. Probably easier all round as you don't have to care for
>> wrap around.
>>
>>
>>>
>>> Anyway, just a suggestion to have the implementation possibly simpler
>>>
>>>
>>>> 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)
>>>
>>> Yeah, I was using the notion I had in my head. Must be biased by the
>>> dog metaphore in the link above :)
>>>
>>>>
>>>>> - 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.
>>>>
>>>
>>> I don't think that's an issue as head and tail will simply allow you
>>> get the context and from there you can do whatever you want.
>>> Similarly, from the frame context you can get the frame number, so
>>> yes, whatever you prefer for now
>>>
>>>>>
>>>>> 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.
>>>>
>>
>> Yes, a '.get()' should certainly be available to algorithms to 'get' the
>> FC based on an index. The question here seems to be around 'what happens
>> if the algorithm 'gets' the slot - and it hadn't yet been filled in by
>> the queue request call.
>>
>> Well. ... first - the .get() call should hopefully know that ... and
>> will have to set the PFC error / underrun flag.
>>
>> But we may have to see what data is expected to be referenced, as it may
>> be 'uninitialised'. But this is where we really need to be clear on what
>> state is in the FCQ vs in the algorithm itself. To me the FCQ is
>> going to store 'commands' (i.e. controls). There will be some state
>> around the algorithm, but I expect that there should also be private
>> state for the algorithm for auto controls too - so it should be able to
>> remember any current filter positions or such ...
>>
>> I'm certain I'm not explaining what's in my head above clearly there -
>> so dig deeper and push me where it's not clear and I'll try to reword or
>> draw something up (which will be part of the PFC design doc I'm working
>> on anyway).
>>
>> But tl;dr; - I believe an algorithm should still be able to run if there
>> is no FCQ slot filled with a request - but it should be an error state.
>>
>>
>>
>>> In case of requests underrun the algorithms would try to access a
>>> non-existing frame context, hence their first "get()" will have to
>>> create one. Indeed having a get that creates entries blurries the line
>>> between a push and a get enough to justify using a single function.
>>>
>>>>>
>>>>> 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?
>>>>
>>>
>>> As I said I was thinking about other components outside of the IPAs
>>> that could benefit from this as well. But I agree getting to have a
>>> first user is the first step to later eventually generalize.
>>>
>>>>>
>>>>> sorry, a lot of questions actually..
>>>>
>>>>
>>>> No issues ;-)
>>>>
>>>
>>> So it seems to me that the only remaining concerns are actually:
>>>
>>> 1) Map uniquely the frame number to the slot index
>>
>> Yes, a frame number and a slot in the FCQ should be directly mappable.
>>
>>>
>>> 2) Clarify if it can be assumed frames always start from 0. This is
>>> desirable as in the current model where frames start being
>>> produced when enough buffers have been queued, the FCQ will be
>>> filled with requests numbered from 0 on, while the first frame
>>> could potentially have an arbitrary value. Is that handled by
>>> Kieran's series from december ? (sorry, I lost track of all the
>>> moving parts).
>>
>> Yes, I believe we work on the assumption all frames start at zero (after
>> any call to start(), so a stop() / start() resets to Frame/Request 0.
>
> Could you have a look at my other reply about the fact we use request
> numbers as frame numbers ?
>
> Thanks
> j
>
>>
>>
>> --
>> Kieran
>>
>>
>>
>>
>>>
>>> Thanks
>>> j
>>>
>>>>>
>>>>>> + 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