[libcamera-devel] [RFC PATCH v2 2/3] ipa: ipu3: ipa_context: Extend FCQueue::get()
Umang Jain
umang.jain at ideasonboard.com
Tue May 31 15:50:32 CEST 2022
Hi Paul,
Thank you for the review.
On 5/31/22 09:41, paul.elder at 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 at 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
>>
More information about the libcamera-devel
mailing list