[libcamera-devel] [PATCH v1 3/4] ipa: ipu3: Prevent over queuing of requests
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Jun 16 00:53:07 CEST 2022
Quoting Jacopo Mondi via libcamera-devel (2022-06-10 10:26:48)
> Hi Umang,
>
> On Fri, Jun 10, 2022 at 11:03:35AM +0200, Umang Jain wrote:
> > Hi Jacopo,
> >
> > On 6/10/22 10:52, Jacopo Mondi wrote:
> > > 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.
> >
> >
> > 2 Points:
> >
> > - I haven't had a look at the PH base for this yet. But reading from your
> > explanation (thanks for the details), it seems it should be quite do-able.
> > So yes, we have two "similar" options now
> >
> > - Next points is basically its PH "base". So "generic" stuff - which might
> > apply to other PH and IPAs as well. I don't want to go into doing it
> > "generically" yet but touching the PH base for FCQueue (which is still IPU3
> > sepcific) seems a violation of the layers to me. So I am a bit resistant
> > going in that direction (for now).
>
> I do expect all pipeline handlers would potentially be interested in
> exposing a max number of in-flight requests and to have a mechanism
> that allows them automatically be guaranteed not to receive more, even
> more if they don't have to do anything to have that realized :)
>
> For IPU3 this is currently the size of the FCQ too, for other
> platforms might be different and could simply be set to the number of
> buffers requested on the video device.
>
> >
> > One option could be - to implement the solution PipelinehandlerIPU3 class
> > for now and include a \todo which basically summarises your solution in the
> > last reply. When we start moving these parts from the PH / IPA to more
> > "generic" location(s)- we can address that \todo as well. This way we don't
> > have things spread out between specific and generic, I think. The principle
> > in both is similar basically. What do you think ?
> >
>
> Doing the work in the IPU3 ph to then have to re-implement it in the
> base class seems like a work duplication to me, if the change can be
> self-contained and implemented in the base class already with the same
> effort.
>
> The mechanism could be made opt-in, conditional to the presence of a
> camera property or on the value of a base class member field which
> derived ph will override.
>
> I would be happy to help if you want this as not part of this series.
Preventing overflowing the IPA using the existing queue at the PH sounds
quite optimal to me too.
Let me know if I can help there in anyway too - or if we need to add it
to a discussion point.
If we 'know' that we'll prevent overflowing the FCQ, we don't have to
handle it in this IPU3 series.
I wonder if there are underflow issues still though...
--
Kieran
>
> > >
> > > > > 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
> > > > > >
More information about the libcamera-devel
mailing list