[libcamera-devel] [PATCH v1 3/4] ipa: ipu3: Prevent over queuing of requests
Umang Jain
umang.jain at ideasonboard.com
Fri Jun 10 11:03:35 CEST 2022
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).
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 ?
>
>>> 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