[libcamera-devel] [PATCH 2/2] libcamera: pipeline-handler: Consider max in-flight requests constraint

[Jacopo Mondi]

Hi Umang,

On Mon, Dec 12, 2022 at 08:32:56PM +0530, Umang Jain via libcamera-devel wrote:
> This patch introduces a new constraint in the pipeline handler base
> class which allows various derived pipeline handlers to set and
> restrict the number of maximum in-flight requests that can be queued to
> the underlying components (for e.g. the IPA).

It's my understanding that the max number of in-flight requests is
what the properties::PipelineDepth property expresses.

Can't we access that property at queue request time, or maybe better we
could cache it at camera registration time, since it's constant ?

Also, can this feature be tested in test/ ? Probably not, as it
requires a compliant pipeline handler ? Maybe vimc can be instrumented
?

>
> The pipeline handler is now equipped with the responsibility of not to
> over queue the requests to the underlying layers. The derived
> pipeline handler (or even IPA) can also have various kind of requests
> queue(arrays, queues or ring-buffer) hence, it might be an issue where
> the application queues the  requests at a rate where these kind of
> queues can over-flow. The patch ensures that the base PipelineHandler
> will never let the requests overflow to underlying layers, once the
> derived pipeline handler sets its maximum capacity of handling
> in-flight requests using PipelineHandler::setMaxQueueRequests().
>
> The queue request management introduced in the pipeline handler base
> class is now used by the IPU3 and RkISP1 pipeline handlers. This will
> prevent over-writing of frame contexts (i.e. FCQueue) in these two
> pipeline handlers.
>
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>  include/libcamera/internal/pipeline_handler.h |  4 ++
>  src/libcamera/pipeline/ipu3/ipu3.cpp          |  1 +
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  1 +
>  src/libcamera/pipeline_handler.cpp            | 51 ++++++++++++++++++-
>  4 files changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index ec4f662d..83f8bd9f 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -70,6 +70,7 @@ public:
>  protected:
>  	void registerCamera(std::shared_ptr<Camera> camera);
>  	void hotplugMediaDevice(MediaDevice *media);
> +	void setMaxQueueRequests(uint32_t maxRequests);
>
>  	virtual int queueRequestDevice(Camera *camera, Request *request) = 0;
>  	virtual void stopDevice(Camera *camera) = 0;
> @@ -97,6 +98,9 @@ private:
>  	Mutex lock_;
>  	unsigned int useCount_ LIBCAMERA_TSA_GUARDED_BY(lock_);
>
> +	uint32_t maxQueueRequests_;
> +	uint32_t requestsQueueCounter_;
> +
>  	friend class PipelineHandlerFactoryBase;
>  };
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index e4d79ea4..d1d42f78 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -422,6 +422,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)
>  	: PipelineHandler(manager), cio2MediaDev_(nullptr), imguMediaDev_(nullptr)
>  {
> +	setMaxQueueRequests(ipa::ipu3::kMaxFrameContexts);
>  }
>
>  std::unique_ptr<CameraConfiguration>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index eb9ad65c..a48adba9 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -599,6 +599,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
>  	: PipelineHandler(manager), hasSelfPath_(true)
>  {
> +	setMaxQueueRequests(ipa::rkisp1::kMaxFrameContexts);
>  }
>
>  std::unique_ptr<CameraConfiguration>
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index cfade490..103f9db0 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -67,7 +67,8 @@ LOG_DEFINE_CATEGORY(Pipeline)
>   * through the PipelineHandlerFactoryBase::create() function.
>   */
>  PipelineHandler::PipelineHandler(CameraManager *manager)
> -	: manager_(manager), useCount_(0)
> +	: manager_(manager), useCount_(0), maxQueueRequests_(0),
> +	  requestsQueueCounter_(0)
>  {
>  }
>
> @@ -428,6 +429,9 @@ void PipelineHandler::doQueueRequest(Request *request)
>  	Camera::Private *data = camera->_d();
>  	data->queuedRequests_.push_back(request);
>
> +	if (maxQueueRequests_)
> +		requestsQueueCounter_++;
> +
>  	request->_d()->sequence_ = data->requestSequence_++;
>
>  	if (request->_d()->cancelled_) {
> @@ -455,6 +459,10 @@ void PipelineHandler::doQueueRequests()
>  		if (!request->_d()->prepared_)
>  			break;
>
> +		if (maxQueueRequests_ &&
> +		    requestsQueueCounter_ >= maxQueueRequests_)
> +			break;
> +
>  		doQueueRequest(request);
>  		waitingRequests_.pop();
>  	}
> @@ -531,6 +539,9 @@ void PipelineHandler::completeRequest(Request *request)
>  		ASSERT(!req->hasPendingBuffers());
>  		data->queuedRequests_.pop_front();
>  		camera->requestComplete(req);
> +
> +		if (maxQueueRequests_)
> +			requestsQueueCounter_--;
>  	}
>  }
>
> @@ -647,6 +658,44 @@ void PipelineHandler::disconnect()
>   * constant for the whole lifetime of the pipeline handler.
>   */
>
> +/**
> + * \var PipelineHandler::maxQueueRequests_
> + * \brief Maximum number of in-flight requests that can be queued
> + *
> + * A hardware can handle a certain number of maximum requests at a given
> + * point. If such a constraint exists, set maxQueueRequests_ via
> + * \a setMaxQueueRequests() in the derived pipeline handler.
> + *
> + * The derived pipeline handler can choose not to define such constraint as
> + * well. In that case, the derived pipeline handler can avoid setting
> + * \a setMaxQueueReqeuests(), hence \a maxQueueRequests_ and
> + * \a requestsQueueCounter_ will be 0.
> + */
> +
> +/**
> + * \var PipelineHandler::requestsQueueCounter_
> + * \brief Number of requests queued to the underlying hardware
> + *
> + * If \a setMaxQueueRequests() is set by the derived pipeline handler,
> + * requestsQueueCounter_ reflects the number of requests queued
> + * to the underlying hardware by the pipeline handler.
> + */
> +
> +/**
> + * \brief Sets the maximum number of requests that can be queued
> + * \param[in] maxRequests Maximum number of in-flight requests
> + *
> + * A hardware can handle a certain number of requests at a given point.
> + * This function sets the maximum number of in-flight requests that can
> + * be queued to the hardware by the pipeline handler. Each derived pipeline
> + * handler should set the maximum number of in-flight requests it can handle
> + * at a given point using this function, if at all such a constraint exists.
> + */
> +void PipelineHandler::setMaxQueueRequests(uint32_t maxRequests)
> +{
> +	maxQueueRequests_ = maxRequests;
> +}
> +
>  /**
>   * \fn PipelineHandler::name()
>   * \brief Retrieve the pipeline handler name
> --
> 2.38.1
>