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

[Jean-Michel Hautbois]

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 ?
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 ?

> 
> 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.

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