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

[Nícolas F. R. A. Prado]

Hi Jacopo,

On Mon, Jul 05, 2021 at 10:09:28AM +0200, Jacopo Mondi wrote:
> 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() ?

Oops, indeed, totally forgot about that... I'll fix it for v2.

Thanks,
Nícolas

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