[libcamera-devel] [PATCH v1 3/4] ipa: ipu3: Prevent over queuing of requests

[Jacopo Mondi]

Hi Umang,

On Fri, Jun 10, 2022 at 10:32:03AM +0200, Umang Jain wrote:
> Hi Jacopo,
>
> On 6/10/22 09:49, Jacopo Mondi wrote:
> > Hi Umang,
> >
> > On Fri, Jun 03, 2022 at 03:22:58PM +0200, Umang Jain via libcamera-devel wrote:
> > > The IPAIPU3 processes each IPAFrameContext present in the FCQueue
> > > ring buffer. If the application queues the requests at a pace where
> > > IPAFrameContext(s) can get potentially over-written without getting
> > > processed by the IPA, it would lead to un-desirable effects.
> > >
> > > Hence, inspect the FCQueue if it is full and reject any further
> > > queuing of requests. This requires changes to the IPAIPU3 mojom
> > > interface. The IPAIPU3::queueRequest() cannot be [async] anymore
> > > since we need to return a success/failure value.
> > >
> > > The rejection by the IPA doesn't mean it cannot be queued
> > > later. The request is still preserved in
> > > IPU3CameraData::pendingRequests_. It shall be queued sequentially
> > > on subsequent calls to  IPU3CameraData::queuePendingRequests().
> > >
> > > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > > ---
> > > I am not super-happy with changing the interface and dropping [async]
> > > but this is what I've for now. Would appreciate any other ideas...
> > I feel like this change handles the issue at the very bottom of the
> > call stack, once a request has been queued from the ph to the IPA.
> >
> > I wonder if we shouldn't try to tackle the problem from the top
> > instead, which means at the time a request is queued to the camera by
> > the application.
> >
> > The lenght of the FCQueue represents the maximum number of requests
> > in-flight at a time, iow the number of requests queued to the camera
> > and not yet completed.
> >
> > Would it make sense to make this a global camera property and have the
> > base pipeline handler class defer queueing requests when too many are
> > in-flight ?
>
>
> In the last call, I brought up this issue. However, it was geared towards a
> solution to avoid dropping [async] from the mojom interface.

I was clearly distracted, sorry

>
> The design I have in mind and shared in the call as well is basically that,
> the PH should be responsible to queue up requests to the IPA - in a way that
> it doesn't do over-queuing.
>
> The way it should be done is - communicating the size of the FCQueue to the
> PH during init/configure phase (or make a global in mojom interface where
> it's shared to both parts - not sure yet, I haven't decided). The PH will
> queue requests to the IPA, increment a counter. We already have a callback
> from IPA to PH when a frame processing completes - so decrement the counter.
> The max the counter can reach is FCQueue.size() - beyond that it should just
> stop queuing and let the request sit in PipelineHandler::pendingRequests_
> queue. The queuePendingRequests() is a recurring call on every request queue
> by the application - so as long as the counter < FCQueue.size() - it will
> keep queuing and stop when it's > FCQueue.size(). As frames get completed,
> counter will decrement and queuing shall start again.

I agree on the principle, but I wonder if this couldn't be done at the
base PipelineHandler class level, where we have a waitingRequests_
queue already. Right now it only serves to handle waiting on fences,
but it could be probably be instrumented to also take a max number of
requests in flight counter.

If the length of FCQ can be made a property of the camera system (not
sure if a proper application visibile libcamera::property or else) and
the parameter can be shared with the PH base class (it could even be a
base class member each derived PH class have to override as an
opt-in mechanism) the PipelineHandler::doQueueRequests() function
can be made aware of this. We have PipelineHandler::completeRequest()
as well, which is the request completion point where the counter could
be decreased and waiting requests queued again.

>
> > Thanks
> >     j
> >
> > > ---
> > >   include/libcamera/ipa/ipu3.mojom     |  2 +-
> > >   src/ipa/ipu3/ipu3.cpp                | 11 +++++++++--
> > >   src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +++++++++-
> > >   3 files changed, 19 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
> > > index d1b1c6b8..3faffd9c 100644
> > > --- a/include/libcamera/ipa/ipu3.mojom
> > > +++ b/include/libcamera/ipa/ipu3.mojom
> > > @@ -30,7 +30,7 @@ interface IPAIPU3Interface {
> > >   	mapBuffers(array<libcamera.IPABuffer> buffers);
> > >   	unmapBuffers(array<uint32> ids);
> > >
> > > -	[async] queueRequest(uint32 frame, libcamera.ControlList controls);
> > > +	queueRequest(uint32 frame, libcamera.ControlList controls) => (int32 ret);
> > >   	[async] fillParamsBuffer(uint32 frame, uint32 bufferId);
> > >   	[async] processStatsBuffer(uint32 frame, int64 frameTimestamp,
> > >   				   uint32 bufferId, libcamera.ControlList sensorControls);
> > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > > index 1d6ee515..63aa9b56 100644
> > > --- a/src/ipa/ipu3/ipu3.cpp
> > > +++ b/src/ipa/ipu3/ipu3.cpp
> > > @@ -145,7 +145,7 @@ public:
> > >   	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > >   	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > >
> > > -	void queueRequest(const uint32_t frame, const ControlList &controls) override;
> > > +	int queueRequest(const uint32_t frame, const ControlList &controls) override;
> > >   	void fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;
> > >   	void processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp,
> > >   				const uint32_t bufferId,
> > > @@ -616,11 +616,18 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
> > >    *
> > >    * Parse the request to handle any IPA-managed controls that were set from the
> > >    * application such as manual sensor settings.
> > > + *
> > > + * \return 0 if successful, -EPERM otherwise.
> > >    */
> > > -void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
> > > +int IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
> > >   {
> > > +	if (context_.frameContexts.isFull())
> > > +		return -EPERM;
> > > +
> > >   	/* \todo Start processing for 'frame' based on 'controls'. */
> > >   	*context_.frameContexts.get(frame) = { frame, controls };
> > > +
> > > +	return 0;
> > >   }
> > >
> > >   /**
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index fd989e61..d69e28d4 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -874,7 +874,15 @@ void IPU3CameraData::queuePendingRequests()
> > >
> > >   		info->rawBuffer = rawBuffer;
> > >
> > > -		ipa_->queueRequest(info->id, request->controls());
> > > +		/*
> > > +		 * Queueing request to the IPA can fail in cases like over-flowing
> > > +		 * its queue. Hence, keep the request preserved in pendingRequests_
> > > +		 * if a failure occurs.
> > > +		 */
> > > +		if (ipa_->queueRequest(info->id, request->controls()) != 0) {
> > > +			frameInfos_.remove(info);
> > > +			break;
> > > +		}
> > >
> > >   		pendingRequests_.pop();
> > >   		processingRequests_.push(request);
> > > --
> > > 2.31.1
> > >