[libcamera-devel] [PATCH v3 2/4] ipa: ipu3: ipa_context: Extend FCQueue::get()
Umang Jain
umang.jain at ideasonboard.com
Mon Jun 20 15:49:34 CEST 2022
Hi Jacopo
On 6/20/22 16:03, Jacopo Mondi wrote:
> Hi Umang
>
> On Mon, Jun 20, 2022 at 02:10:22AM +0530, 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 and return its IPAFrameContext.
>>
>> We specifically want to have access to the FCQueue through the
>> .get() public function hence the class FCQueue needs to be now
>> privately inherited from std::vector<>.
>>
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>> ---
>> src/ipa/ipu3/ipa_context.cpp | 97 +++++++++++++++++++++++++++++++++---
>> src/ipa/ipu3/ipa_context.h | 10 +++-
>> src/ipa/ipu3/ipu3.cpp | 6 ++-
>> 3 files changed, 103 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
>> index 36d20b1b..60c58b97 100644
>> --- a/src/ipa/ipu3/ipa_context.cpp
>> +++ b/src/ipa/ipu3/ipa_context.cpp
>> @@ -222,15 +222,34 @@ 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().
>> + */
>> +
>> +/**
>> + * \var FCQueue::oldestFC_
>> + * \brief Frame number of the oldest IPAFrameContext in the FCQueue
>> + */
>> +
>> +/**
>> + * \var FCQueue::latestFC_
>> + * \brief Frame number of the latest IPAFrameContext in the FCQueue
>> + */
>> +
> Let's open the bikeshedding on names.
> head and tail confused me at the beginning but I think they're more
> appropriate. Other ideas ?
>
>> +/**
>> + * \var FCQueue::isFull_
>> + * \brief Flag set when the FCQueue is full
>> */
>>
>> /**
>> * \brief FCQueue constructor
>> */
>> FCQueue::FCQueue()
>> + : latestFC_(0), oldestFC_(0), isFull_(false)
>> {
>> - reset();
>> }
>>
>> /**
>> @@ -238,19 +257,70 @@ 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, a new slot for the corresponding
>> + * \a frame is created and 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 (latestFC_ && frame <= latestFC_) {
>> + IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);
>> + if (frame != frameContext.frame)
>> + LOG(IPAIPU3, Warning)
>> + << "Got wrong frame context for frame " << frame;
>> +
>> + return &frameContext;
>> + } else {
> You can save one indendation level by dropping the else
Oh yes that would be possible.
>
>> + if (isFull_) {
> You know, I would be tempted to turn this into a function
>
> bool FCQueue::full(unsigned int frame)
> {
> isFull_ = (frame - oldestFC_) >= kMaxFrameContexts;
> return isFull_;
> }
>
> Which does the right calculations and sets the internal flag (if you
> need to expose a public isFull() method), so that you only have to
> update the counters on get() and complete()
Yes we can do that, not sure how would it be beneficial though.
For the isFull() method, I don't think passing a frame number to know
whether the FCQueue is full or not - might be a deviation from the
convention.
>
>> + LOG(IPAIPU3, Warning)
> I was about to suggest to use Error, as the caller should really check
> the returned value and this could potentially cause a segfault, but
> maybe warning is enough ?
Error should be Okay
>
>> + << "Cannot create frame context for frame " << frame
>> + << " since FCQueue is full";
>> +
>> + return nullptr;
>> + }
>> +
>> + latestFC_ = frame;
>> +
>> + /* Check if the queue is full to avoid over-queueing later */
>> + if (oldestFC_ && latestFC_ - oldestFC_ >= kMaxFrameContexts - 1)
> Doesn't this work even with !oldestFC_, ie can you remove the first
It has to do with the fact frame numbers doesn't start from '0' like in
CTS (back to back test runs)
So on every start() we call FCQueue::reset() and oldestFC_ gets
assigned again to '0'
When the "first" .get() happens, the incoming frame number maybe 244 so
the check
+ if (latestFC_ - oldestFC_ >= kMaxFrameContexts - 1)
will be true and hence isFull_ gets set on first frame itself.
When the first frame (#244) gets completed, oldestFC_ will be set from 0
to 244 directly,
so from there it will do the right thing
So, for now we need the initial part of the check as well - until
Kieran's patch makes sure
every frame starts from '0' I believe.
> part of the condition. Also, I think you do >= kMaxFrameContexts - 1
> to prepare the flag for the 'next' frame, however as we use the frame
what's wrong with that? You set the last IPAFrameContext slot in the
FCQueue - see whether FCQueue full - and set the flag there itself.
> id to identify the slot position and there's no guarantee that the
> 'next' frame id will be + 1 (because of drops). This makes me think
The gap / drop handling will probably halt this entire development.
Yes, we need to handle drops and it will requiring FCQueue adjustments,
but I cannot develop once we some sort of handling policy or
discussion/mechanisms plan.
> checking before with a function that takes the frame number to insert
> is better.
>
>> + isFull_ = true;
>>
>> - if (frame != frameContext.frame) {
>> + return &this->at(latestFC_ % kMaxFrameContexts);
>> + }
>> +}
>> +
>> +/**
>> + * \brief Notifies the FCQueue that a frame has been completed
>> + * \param[in] frame The completed frame number
>> + */
>> +void FCQueue::completeFrame(uint32_t frame)
>> +{
>> + if (oldestFC_ != 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 > latestFC_) {
> This mean something is very wrong. Is it worth an ASSERT() ?
could be upgraded.
>
>> + LOG(IPAIPU3, Error)
>> + << "Completing a frame " << frame
>> + << " not present in the queue is disallowed";
>> + return;
>> }
>>
>> - return &frameContext;
>> + /**
>> + * \todo If there are 'gaps' in FCQueue due to frame drops, the below
>> + * oldestFC_ = frame + 1 will be be wrong.
>> + * We need to set oldestFC_ to the next valid entry, jumping the drop
>> + * gap slots.
>> + */
>> + oldestFC_ = frame + 1;
> This looks like
>
>
> FCQueue = [...] [frame] [gap] [frame+1] [...]
>
> gap is a leftover from a previous iteration so it will certainly have
> an id < frame, right ? Can't we inspect the 'next' positions until we
> don't find an id > frame ?
That's what is being biting us here. We can't really know if the next
slot is a valid frame context or not, or just a 'gap'
If we have a mechanism to know if the next slot is valid IPAFrameContext
or just a gap, I could have jumped the oldestFC_ to point to next
"valid" one.
> This now rises the question what happens when we have an underrun, ie
> the just completed frame is the latest one in the queue.
>
> I think you're current code doesn't handle that and sets oldestFC_
> past latestFC_ unconditionally. Should we not advance oldestFC_ ? The
> disance between latestFC_ and oldestFC_ will be 0, which I guess is
> what we want.
We need a discussion here.
If algorithms keeps computing values for next frames, the slots will get
filled up so the latestFC_ will be advancing I believe.
When application doesn't queue requests enough, we will face underrun so
oldestFC_ will stop at a point will the last request is processed by the
IPA (and the last call to completeFrame)
So it might very well happen practically, that distance between
latestFC_ and oldestFC_ doesn't go to zero and the queue reports full.
I think we need to differentiate between IPAFrameContexts (whether it
was been created by a Algorithm's look-ahead .get() call or a
queueRequest() call) to really handle advance corner-cases.
I also think all this handling should go on top instead of trying to
handle it in this series, when we hook up "look-ahead" computation of
the Algorithms.
>
> Could something like this work ?
>
> for (unsigned int i = 0; i < kMaxFrameContexts; ++i) {
> const IPAFrameContext &next = this->at(frame + i % kMaxFrameContexts);
>
> if (next.frame > frame || next.frame == latestFC_) {
> oldestFC_ = next.frame;
> break;
> }
> }
>
> I wish we could unit-test this in detail to catch the corner cases.
>
>> +
>> + if (isFull_)
>> + isFull_ = false;
>> }
>>
>> /**
>> @@ -262,12 +332,23 @@ void FCQueue::setSize(uint32_t size)
>> resize(size);
>> }
>>
>> +/**
>> + * \fn FCQueue::isFull()
>> + * \brief Checks whether the frame context queue is full
>> + * \return True of FCQueue is full, False otherwise
>> + */
> This is never used afaict. When do you envision users of the queue to
> use this function ?
I use it for debugging, knowing when the queue is full might be helpful.
Not sure if I can give you a proper use case right now.
>
>> +
>> /**
>> * \brief Clear the FCQueue by resetting all the entries in the ring-buffer
>> */
>> void FCQueue::reset()
>> {
>> clear();
>> +
>> + isFull_ = false;
>> +
>> + latestFC_ = 0;
>> + oldestFC_ = 0;
>> }
>>
>> } /* namespace ipa::ipu3 */
>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
>> index e487d34b..52a0b5e7 100644
>> --- a/src/ipa/ipu3/ipa_context.h
>> +++ b/src/ipa/ipu3/ipa_context.h
>> @@ -89,15 +89,23 @@ struct IPAFrameContext {
>> ControlList frameControls;
>> };
>>
>> -class FCQueue : public std::vector<IPAFrameContext>
>> +class FCQueue : private std::vector<IPAFrameContext>
>> {
>> public:
>> FCQueue();
>>
>> + void completeFrame(uint32_t frame);
>> void reset();
>> void setSize(uint32_t size);
>>
>> IPAFrameContext *get(uint32_t frame);
>> + bool isFull() { return isFull_; }
>> +
>> +private:
>> + uint32_t latestFC_;
>> + uint32_t oldestFC_;
>> +
>> + bool isFull_;
>> };
>>
>> struct IPAContext {
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index d770c254..15070fa0 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -334,6 +334,8 @@ int IPAIPU3::init(const IPASettings &settings,
>> */
>> int IPAIPU3::start()
>> {
>> + context_.frameContexts.reset();
>> +
>> /*
>> * Set the sensors V4L2 controls before the first frame to ensure that
>> * we have an expected and known configuration from the start.
>> @@ -605,6 +607,8 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>> */
>>
>> metadataReady.emit(frame, ctrls);
>> +
>> + context_.frameContexts.completeFrame(frame);
>> }
>>
>> /**
>> @@ -618,7 +622,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