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

[Laurent Pinchart]

Hi Niklas,

On Tue, Mar 24, 2020 at 01:33:32PM +0100, Niklas Söderlund wrote:
> On 2020-03-23 14:26:06 +0200, Laurent Pinchart 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
> > 
> > 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.

I think the interface between the camera and pipeline handler should
remain as simple with possible, with high-level entry points. We can
then implement helpers, in the form of classes deriving from
PipelineHandler, or classes they implement helper objects (similar in
concept to the V4L2BufferCache for instance) to avoid code duplication.
It's a bit early to tell what form this will take, we probably need to
look at two examples at least.

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