[libcamera-devel] [RFC 5/6] libcamera: ipu3: Do not unconditionally queue buffers to CIO2

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Mon, Mar 16, 2020 at 03:41:45AM +0100, Niklas Söderlund wrote:
> Instead of unconditionally cycling buffers between the CIO2 and IMGU
> pick a buffer when a request is queued to the pipeline. This is needed
> if operations are the be applied to the buffer coming from CIO2 with

s/the be/to be/

> parameters coming from a Request.
> 
> The approach to pick a CIO2 buffer when a request is queued is similar
> to other pipelines, where parameters and statistic buffers are picked
> this way.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 69 +++++++++++++++++++++++-----
>  1 file changed, 58 insertions(+), 11 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 0c2a217c9ea8f6ba..dbb0c4328aa0efe5 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -8,6 +8,7 @@
>  #include <algorithm>
>  #include <iomanip>
>  #include <memory>
> +#include <queue>
>  #include <vector>
>  
>  #include <linux/drm_fourcc.h>
> @@ -119,6 +120,10 @@ public:
>  	int allocateBuffers();
>  	void freeBuffers();
>  
> +	FrameBuffer *getBuffer();
> +	void putBuffer(FrameBuffer *buffer);
> +	int queueBuffer(FrameBuffer *buffer);
> +
>  	int start();
>  	int stop();
>  
> @@ -130,6 +135,7 @@ public:
>  
>  private:
>  	std::vector<std::unique_ptr<FrameBuffer>> buffers_;
> +	std::queue<FrameBuffer *> availableBuffers_;
>  };
>  
>  class IPU3Stream : public Stream
> @@ -157,6 +163,8 @@ public:
>  	void imguInputBufferReady(FrameBuffer *buffer);
>  	void cio2BufferReady(FrameBuffer *buffer);
>  
> +	std::map<FrameBuffer *, Request *> cio2ToRequest_;

We have a Request pointer in FrameBuffer to track this. It's a private
field, pipeline handlers can't set it, but I think it's a good time to
fix that issue properly.

> +
>  	CIO2Device cio2_;
>  	ImgUDevice *imgu_;
>  
> @@ -717,11 +725,20 @@ void PipelineHandlerIPU3::stop(Camera *camera)
>  
>  int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
>  {
> +	IPU3CameraData *data = cameraData(camera);
> +	FrameBuffer *buffer;
>  	int error = 0;
>  
> +	/* Get a CIO2 buffer and track it it's used for this request. */
> +	buffer = data->cio2_.getBuffer();
> +	if (!buffer)
> +		return -EINVAL;

We can return an error for now, but we'll really have to handle this
better. If the application queues more request than the pipeline handler
internal depth, they need to be put in a queue somewhere. I wonder if we
should try address this already.

> +	data->cio2ToRequest_[buffer] = request;
> +	data->cio2_.queueBuffer(buffer);
> +
>  	for (auto it : request->buffers()) {
>  		IPU3Stream *stream = static_cast<IPU3Stream *>(it.first);
> -		FrameBuffer *buffer = it.second;
> +		buffer = it.second;
>  
>  		int ret = stream->device_->dev->queueBuffer(buffer);
>  		if (ret < 0)
> @@ -893,7 +910,9 @@ void IPU3CameraData::imguInputBufferReady(FrameBuffer *buffer)
>  	if (buffer->metadata().status == FrameMetadata::FrameCancelled)
>  		return;
>  
> -	cio2_.output_->queueBuffer(buffer);
> +	/* Remove association between CIO2 buffer an Request. */
> +	cio2ToRequest_.erase(buffer);
> +	cio2_.putBuffer(buffer);
>  }
>  
>  /**
> @@ -1416,30 +1435,58 @@ int CIO2Device::configure(const Size &size,
>  int CIO2Device::allocateBuffers()
>  {
>  	int ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_);
> -	if (ret < 0)
> +	if (ret < 0) {
>  		LOG(IPU3, Error) << "Failed to allocate CIO2 buffers";

There's already a message printed by the V4L2VideoDevice class, do we
need another one here ?

> +		return ret;
> +	}
> +
> +	for (std::unique_ptr<FrameBuffer> &buffer : buffers_)
> +		availableBuffers_.push(buffer.get());
>  
>  	return ret;
>  }
>  
>  void CIO2Device::freeBuffers()
>  {
> +	while (!availableBuffers_.empty())
> +		availableBuffers_.pop();
> +

How about

	availableBuffers_ = {};

?

>  	buffers_.clear();
>  
>  	if (output_->releaseBuffers())
>  		LOG(IPU3, Error) << "Failed to release CIO2 buffers";
>  }
>  
> +FrameBuffer *CIO2Device::getBuffer()
> +{
> +	if (availableBuffers_.empty()) {
> +		LOG(IPU3, Error) << "CIO2 buffer underrun";
> +		return nullptr;
> +	}
> +
> +	FrameBuffer *buffer = availableBuffers_.front();
> +
> +	availableBuffers_.pop();
> +
> +	return buffer;
> +}
> +
> +void CIO2Device::putBuffer(FrameBuffer *buffer)
> +{
> +	availableBuffers_.push(buffer);
> +}
> +
> +int CIO2Device::queueBuffer(FrameBuffer *buffer)
> +{
> +	int ret = output_->queueBuffer(buffer);
> +	if (ret)
> +		LOG(IPU3, Error) << "Failed to queue CIO2 buffer";
> +
> +	return ret;

Do we need this wrapper, can't we call
cio2_.output_->queueBuffer(buffer) in the caller like we used to ?

> +}
> +
>  int CIO2Device::start()
>  {
> -	for (const std::unique_ptr<FrameBuffer> &buffer : buffers_) {
> -		int ret = output_->queueBuffer(buffer.get());
> -		if (ret) {
> -			LOG(IPU3, Error) << "Failed to queue CIO2 buffer";
> -			return ret;
> -		}
> -	}
> -
>  	return output_->streamOn();
>  }
>  

-- 
Regards,

Laurent Pinchart