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

[Laurent Pinchart]

Hi Jacopo,

On Wed, Mar 25, 2020 at 12:13:39PM +0100, Jacopo Mondi wrote:
> 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.

Everything runs in a single thread, so there's no need to.

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

It would need to be a copy, not a reference, which would be expensive
for queues that store large elements.

> Anyway, looks good to me, will wait for v1 for a tag.
> 
> > +
> > +	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();
> >  }
> >

-- 
Regards,

Laurent Pinchart