[libcamera-devel] [PATCH 1/3] libcamera: pipeline: vimc: Add internal request queue

[Laurent Pinchart]

Hi Nícolas,

Thank you for the patch.

On Mon, Jul 19, 2021 at 04:14:36PM -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>
> ---
>  src/libcamera/pipeline/vimc/vimc.cpp | 65 +++++++++++++++++++++++-----
>  1 file changed, 55 insertions(+), 10 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index a698427c4361..c9092bec9a74 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -9,6 +9,7 @@
>  #include <iomanip>
>  #include <map>
>  #include <math.h>
> +#include <queue>
>  #include <tuple>
>  
>  #include <linux/media-bus-format.h>
> @@ -53,6 +54,9 @@ public:
>  	int init();
>  	void bufferReady(FrameBuffer *buffer);
>  
> +	void queuePendingRequests();
> +	void cancelPendingRequests();
> +
>  	MediaDevice *media_;
>  	std::unique_ptr<CameraSensor> sensor_;
>  	std::unique_ptr<V4L2Subdevice> debayer_;
> @@ -62,6 +66,8 @@ public:
>  	Stream stream_;
>  
>  	std::unique_ptr<ipa::vimc::IPAProxyVimc> ipa_;
> +
> +	std::queue<Request *> pendingRequests_;
>  };
>  
>  class VimcCameraConfiguration : public CameraConfiguration
> @@ -94,9 +100,9 @@ public:
>  
>  	bool match(DeviceEnumerator *enumerator) override;
>  
> -private:
>  	int processControls(VimcCameraData *data, Request *request);

Instead of making this function public, shouldn't it be moved to the
VimcCameraData class ?

>  
> +private:
>  	VimcCameraData *cameraData(const Camera *camera)
>  	{
>  		return static_cast<VimcCameraData *>(
> @@ -335,6 +341,7 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlLis
>  void PipelineHandlerVimc::stop(Camera *camera)
>  {
>  	VimcCameraData *data = cameraData(camera);
> +	data->cancelPendingRequests();
>  	data->video_->streamOff();
>  	data->ipa_->stop();
>  	data->video_->releaseBuffers();
> @@ -383,21 +390,16 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)
>  int PipelineHandlerVimc::queueRequestDevice(Camera *camera, Request *request)
>  {
>  	VimcCameraData *data = cameraData(camera);
> -	FrameBuffer *buffer = request->findBuffer(&data->stream_);
> -	if (!buffer) {
> +
> +	if (!request->findBuffer(&data->stream_)) {
>  		LOG(VIMC, 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;
>  }
> @@ -531,6 +533,49 @@ void VimcCameraData::bufferReady(FrameBuffer *buffer)
>  
>  	pipe_->completeBuffer(request, buffer);
>  	pipe_->completeRequest(request);
> +
> +	queuePendingRequests();
> +}
> +
> +void VimcCameraData::queuePendingRequests()
> +{
> +	PipelineHandlerVimc *pipe = static_cast<PipelineHandlerVimc *>(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 */

s/later/later./

> +		ret = video_->queueBuffer(buffer);
> +		if (ret < 0)
> +			break;

This is problematic, because the controls will be applied already, and
they shouldn't. One possible way to fix this is to maintain a counter of
the available V4L2 buffer slots, to stop the loop right away without
having to rely on queueBuffer().

Another option is to consider that we don't implement per-frame control
yet anyway, as the controls are applied when the buffer is queued to the
driver, while we should delay them until the corresponding buffer gets
filled by the device, and ignore this issue for now given that per-frame
control will involve quite a bit of refactoring anyway.

Same comments for 2/3.

> +
> +		pendingRequests_.pop();
> +	}
> +}
> +
> +void VimcCameraData::cancelPendingRequests()
> +{
> +	while (!pendingRequests_.empty()) {
> +		Request *request = pendingRequests_.front();
> +		FrameBuffer *buffer = request->findBuffer(&stream_);
> +
> +		buffer->cancel();
> +		pipe_->completeBuffer(request, buffer);
> +		pipe_->completeRequest(request);
> +
> +		pendingRequests_.pop();
> +	}
>  }
>  
>  REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc)

-- 
Regards,

Laurent Pinchart