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

[Jacopo Mondi]

Hi Niklas,

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
> 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_;
> +
>  	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;

very minor nit: empty line

I don't have much to add to Laurent's review, I'm wondering if we need
some kind of locking when accessing the queue though.

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

OT:
Is it me or I'm rightfully puzzled by the fact std::queue::pop() does
not return an reference to the popped element?

Anyway, looks good to me, will wait for v1 for a tag.

Thanks
  j

> +
> +	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;
> +}
> +
>  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();
>  }
>
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel