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

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2020-03-23 14:26:06 +0200, Laurent Pinchart wrote:
> 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.

Good idea, I will try and improve on this for v1.

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

I'm all for starting work on this topic now. But do you think it really 
should be solved in individual pipeline handlers and not on a framework 
level?

When I thought about this in the past I always imaged the pipeline to 
report its min and max pipeline depth and then having the framework 
doing the heavy lifting of keeping and flushing a queue. If so I think 
it the behavior here is correct, but if you think pipelines shall deal 
with this it should be reworked.

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

I'm all for dropping it ;-)

> 
> > +		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_ = {};

Nice idea.

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

Not really, I added it as I think this driver is quiet messy and 
breaking things apart could be a good thing. Will drop for v1 and then 
we can refactor on top to make it more readable.

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

-- 
Regards,
Niklas Söderlund