[libcamera-devel] [RFC PATCH v2 2/3] ipa: ipu3: ipa_context: Extend FCQueue::get()

[Umang Jain]

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 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
>> + * 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 };
>>   }
>>     /**