[libcamera-devel] [PATCH] libcamera: pipeline: uvcvideo: Add internal request queue

[Jacopo Mondi]

Hi Nicolas,

On Thu, Jul 01, 2021 at 04:29:51PM -0300, Nícolas F. R. A. Prado wrote:
> Add an internal queue that stores requests until there are v4l2 buffer
> slots available. This avoids the need to cancel requests when there is a
> shortage of said buffers.
>
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> ---
>
> This is very similar to what 5a9d19210fad ("libcamera: pipeline: ipu3: Try
> queuing pending requests if a buffer is available") and 89dae5844964
> ("libcamera: pipeline: ipu3: Store requests in the case a buffer shortage") did
> for IPU3.
>
> I tested this with a new lc-compliance test from [1] which is a rebased version
> of the series in [2]. I'll resend this series after the googletest refactor
> series [3] is merged. But this patch for uvcvideo doesn't depend on that.
>
> [1] https://gitlab.collabora.com/nfraprado/libcamera/-/tree/lcc-lots-requests-v5
> [2] https://lists.libcamera.org/pipermail/libcamera-devel/2021-May/020150.html
> [3] https://lists.libcamera.org/pipermail/libcamera-devel/2021-June/021891.html
>
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 64 +++++++++++++++++---
>  1 file changed, 54 insertions(+), 10 deletions(-)
>
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 0f634b8da609..a51131dce8e0 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -10,6 +10,7 @@
>  #include <iomanip>
>  #include <math.h>
>  #include <memory>
> +#include <queue>
>  #include <tuple>
>
>  #include <libcamera/base/log.h>
> @@ -45,6 +46,11 @@ public:
>  			ControlInfoMap::Map *ctrls);
>  	void bufferReady(FrameBuffer *buffer);
>
> +	void queuePendingRequests();
> +	void cancelPendingRequests();

Am I wrong or this is not called in this patch (or any other related
patch I can find). Should you call it in stop() ?

Thanks
  j

> +
> +	std::queue<Request *> pendingRequests_;
> +
>  	std::unique_ptr<V4L2VideoDevice> video_;
>  	Stream stream_;
>  };
> @@ -79,12 +85,13 @@ public:
>
>  	bool match(DeviceEnumerator *enumerator) override;
>
> +	int processControls(UVCCameraData *data, Request *request);
> +
>  private:
>  	std::string generateId(const UVCCameraData *data);
>
>  	int processControl(ControlList *controls, unsigned int id,
>  			   const ControlValue &value);
> -	int processControls(UVCCameraData *data, Request *request);
>
>  	UVCCameraData *cameraData(const Camera *camera)
>  	{
> @@ -363,24 +370,59 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
>  	return ret;
>  }
>
> +void UVCCameraData::cancelPendingRequests()
> +{
> +	while (!pendingRequests_.empty()) {
> +		Request *request = pendingRequests_.front();
> +		FrameBuffer *buffer = request->findBuffer(&stream_);
> +
> +		buffer->cancel();
> +		pipe_->completeBuffer(request, buffer);
> +		pipe_->completeRequest(request);
> +
> +		pendingRequests_.pop();
> +	}
> +}
> +
> +void UVCCameraData::queuePendingRequests()
> +{
> +	PipelineHandlerUVC *pipe = static_cast<PipelineHandlerUVC *>(pipe_);
> +
> +	while (!pendingRequests_.empty()) {
> +		Request *request = pendingRequests_.front();
> +		FrameBuffer *buffer = request->findBuffer(&stream_);
> +
> +		int ret = pipe->processControls(this, request);
> +		if (ret < 0) {
> +			buffer->cancel();
> +			pipe_->completeBuffer(request, buffer);
> +			pipe_->completeRequest(request);
> +			pendingRequests_.pop();
> +			continue;
> +		}
> +
> +		/* If we're missing v4l2 buffer slots, try again later */
> +		ret = video_->queueBuffer(buffer);
> +		if (ret < 0)
> +			break;
> +
> +		pendingRequests_.pop();
> +	}
> +}
> +
>  int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request)
>  {
>  	UVCCameraData *data = cameraData(camera);
> -	FrameBuffer *buffer = request->findBuffer(&data->stream_);
> -	if (!buffer) {
> +
> +	if (!request->findBuffer(&data->stream_)) {
>  		LOG(UVC, Error)
>  			<< "Attempt to queue request with invalid stream";
>
>  		return -ENOENT;
>  	}
>
> -	int ret = processControls(data, request);
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = data->video_->queueBuffer(buffer);
> -	if (ret < 0)
> -		return ret;
> +	data->pendingRequests_.push(request);
> +	data->queuePendingRequests();
>
>  	return 0;
>  }
> @@ -668,6 +710,8 @@ void UVCCameraData::bufferReady(FrameBuffer *buffer)
>
>  	pipe_->completeBuffer(request, buffer);
>  	pipe_->completeRequest(request);
> +
> +	queuePendingRequests();
>  }
>
>  REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC)
> --
> 2.32.0
>