[libcamera-devel] [RFC PATCH 2/2] libcamera: PipelineHandler: Retry queuing a capture request

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Mon, Mar 29, 2021 at 11:26:04AM +0900, Hirokazu Honda wrote:
> PipelineHandler::queueRequestDevice() fails due to a buffer
> shortage. We should retry queuing a capture request later in the
> case.

I don't think this should be handled by the base PipelineHandler class,
but by individual pipeline handlers. There may be needs for pipeline
handlers to peek ahead in the queue of requests to get metadata needed
by the algorithms, with a queue depth larger than the number of buffers.
You queue those requests on waitedRequests_ so the pipeline handler can
have access to them, but the semantics isn't well defined, and I think
that will be quite error prone. Furthermore, the call to
tryQueueRequests() in PipelineHandler::completeRequest() will result in
the pipeline handler's queueRequestDevice() function being called from
the request completion handler, making the pipeline handler code
reentrant, and pipeline handlers are not prepared for that. This may
create race conditions, I'd rather not go that route.

This being said, duplicating request queues in all pipeline handlers
isn't a great idea either. We should share code, but that should be
implemented by helper classes that pipeline handlers can opt-in to use,
not in a mandatory middle layer. Now that we have multiple pipeline
handler implementations, it's time to start cleaning things up, and the
Raspberry Pi and IPU3 pipeline handlers are good candidates for code
sharing as they share a common hardware architecture.

If you want to fix this issue for the IPU3 pipeline handler without
depending on the creation of those helper classes, then the fix should
be implemented in the IPU3 pipeline handler itself in the meantime.

> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  include/libcamera/internal/pipeline_handler.h |  2 +
>  src/libcamera/pipeline_handler.cpp            | 57 +++++++++++++++++--
>  2 files changed, 54 insertions(+), 5 deletions(-)
> 
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index 763da63e..e6f771a6 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -44,6 +44,7 @@ public:
>  	virtual ~CameraData() = default;
>  
>  	PipelineHandler *pipe_;
> +	std::queue<Request *> waitedRequests_;
>  	std::deque<Request *> queuedRequests_;
>  	ControlInfoMap controlInfo_;
>  	ControlList properties_;
> @@ -99,6 +100,7 @@ protected:
>  	CameraManager *manager_;
>  
>  private:
> +	void tryQueueRequests(CameraData *data);
>  	void mediaDeviceDisconnected(MediaDevice *media);
>  	virtual void disconnect();
>  
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 4cb58084..18952a1b 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -70,6 +70,16 @@ LOG_DEFINE_CATEGORY(Pipeline)
>   * and remains valid until the instance is destroyed.
>   */
>  
> +/**
> + * \var CameraData::waitedRequests_
> + * \brief The queue of not yet queued request
> + *
> + * The queue of not yet queued request is used to track requests that are not
> + * queued in order to retry queueing them when a pipeline is ready to accept.
> + *
> + * \sa PipelineHandler::queueRequest(), PipelineHandler::tryQueueRequests().
> + */
> +
>  /**
>   * \var CameraData::queuedRequests_
>   * \brief The queue of queued and not yet completed request
> @@ -378,12 +388,44 @@ void PipelineHandler::queueRequest(Request *request)
>  
>  	Camera *camera = request->camera_;
>  	CameraData *data = cameraData(camera);
> -	data->queuedRequests_.push_back(request);
> +	data->waitedRequests_.push(request);
> +
> +	tryQueueRequests(data);
> +}
> +
> +/**
> + * \fn PipelineHandler::tryQueueRequests()
> + * \brief Queue requests that are not yet queued.
> + * \param[in] data The camera data whose waited requests are tried to queue.
> + *
> + * This tries to queue as many requests as possible in order of the
> + * waitedRequests_ in data. If queuing fails due to a buffer shortage, this
> + * method stops and the next call of this method starts from the request that
> + * fails in the previous call. On any other failures, the request is marked as
> + * complete and proceed the successive requests.
> + *
> + * \context This function shall be called from the CameraManager thread.
> + */
> +void PipelineHandler::tryQueueRequests(CameraData *data)
> +{
> +	while (!data->waitedRequests_.empty()) {
> +		Request *request = data->waitedRequests_.front();
> +		Camera *camera = request->camera_;
> +		ASSERT(data == cameraData(camera));
> +
> +		data->queuedRequests_.push_back(request);
> +		int ret = queueRequestDevice(camera, request);
> +		if (ret == -ENOBUFS || ret == -ENOMEM) {
> +			data->queuedRequests_.pop_back();
> +			break;
> +		}
>  
> -	int ret = queueRequestDevice(camera, request);
> -	if (ret) {
> -		request->result_ = ret;
> -		completeRequest(request);
> +		data->waitedRequests_.pop();
> +		if (ret) {
> +			request->result_ = ret;
> +			completeRequest(request);
> +			break;
> +		}
>  	}
>  }
>  
> @@ -440,6 +482,9 @@ bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)
>   * submission order, the pipeline handler may call it on any complete request
>   * without any ordering constraint.
>   *
> + * There might be requests that are waiting to be queued, this triggers
> + * tryQueueRequests().
> + *
>   * \context This function shall be called from the CameraManager thread.
>   */
>  void PipelineHandler::completeRequest(Request *request)
> @@ -459,6 +504,8 @@ void PipelineHandler::completeRequest(Request *request)
>  		data->queuedRequests_.pop_front();
>  		camera->requestComplete(req);
>  	}
> +
> +	tryQueueRequests(data);
>  }
>  
>  /**

-- 
Regards,

Laurent Pinchart