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

[Umang Jain]

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.

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.

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