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

[Jacopo Mondi]

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 ?

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
>