[libcamera-devel] [PATCH 09/10] libcamera: ipu3: cio2: Hide buffer allocation and freeing from users

[Laurent Pinchart]

Hi Niklas,

On Sat, Jun 06, 2020 at 04:08:41PM +0200, Niklas Söderlund wrote:
> On 2020-06-06 01:15:03 +0300, Laurent Pinchart wrote:
> > On Tue, Jun 02, 2020 at 02:32:25PM +0200, Jacopo Mondi wrote:
> > > On Tue, Jun 02, 2020 at 03:39:08AM +0200, Niklas Söderlund wrote:
> > > > The allocation and freeing of buffers for the CIO2 is handled in the
> > > > IPU3 pipeline handlers start() and stop() functions. These functions
> > > > also calls CIO2Device start() and stop() at the appropriate times so
> > > > move the CIO2 buffer allocation/freeing inside the CIO2Device and reduce
> > > > the complexity of the exposed interface.
> > 
> > Given the overall structure of the libcamera internal APIs I think
> > that's fine, but I think we'll likely have to reintroduce intermediate
> > steps before start() and after stop() to handle buffer allocation, as
> > those are expensive operations. This would then get in the way. It's OK
> > for now though.
> > 
> > > > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > > ---
> > > >  src/libcamera/pipeline/ipu3/cio2.cpp | 53 +++++++++++-----------------
> > > >  src/libcamera/pipeline/ipu3/cio2.h   |  2 --
> > > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +-----
> > > >  3 files changed, 21 insertions(+), 44 deletions(-)
> > > >
> > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> > > > index 63a46959f3cac06a..895848d2b3a1fba9 100644
> > > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> > > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> > > > @@ -199,44 +199,12 @@ CIO2Device::generateConfiguration(const PixelFormat desiredPixelFormat,
> > > >  	return cfg;
> > > >  }
> > > >
> > > > -/**
> > > > - * \brief Allocate frame buffers for the CIO2 output
> > > > - *
> > > > - * Allocate frame buffers in the CIO2 video device to be used to capture frames
> > > > - * from the CIO2 output. The buffers are stored in the CIO2Device::buffers_
> > > > - * vector.
> > > > - *
> > > > - * \return Number of buffers allocated or negative error code
> > > > - */
> > > > -int CIO2Device::allocateBuffers()
> > > > -{
> > > > -	int ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_);
> > > > -	if (ret < 0)
> > > > -		return ret;
> > > > -
> > > > -	for (std::unique_ptr<FrameBuffer> &buffer : buffers_)
> > > > -		availableBuffers_.push(buffer.get());
> > > > -
> > > > -	return ret;
> > > > -}
> > > > -
> > > >  int CIO2Device::exportBuffers(unsigned int count,
> > > >  			      std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > > >  {
> > > >  	return output_->exportBuffers(count, buffers);
> > > >  }
> > > >
> > > > -void CIO2Device::freeBuffers()
> > > > -{
> > > > -	/* The default std::queue constructor is explicit with gcc 5 and 6. */
> > > > -	availableBuffers_ = std::queue<FrameBuffer *>{};
> > > > -
> > > > -	buffers_.clear();
> > > > -
> > > > -	if (output_->releaseBuffers())
> > > > -		LOG(IPU3, Error) << "Failed to release CIO2 buffers";
> > > > -}
> > > > -
> > > >  FrameBuffer *CIO2Device::getBuffer()
> > > >  {
> > > >  	if (availableBuffers_.empty()) {
> > > > @@ -258,12 +226,31 @@ void CIO2Device::putBuffer(FrameBuffer *buffer)
> > > >
> > > >  int CIO2Device::start()
> > > >  {
> > > > +	int ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	for (std::unique_ptr<FrameBuffer> &buffer : buffers_)
> > > > +		availableBuffers_.push(buffer.get());
> > > > +
> > > >  	return output_->streamOn();
> > 
> > Aren't you leaking the buffers if this call fails ?
> > 
> > > >  }
> > > >
> > > >  int CIO2Device::stop()
> > > >  {
> > > > -	return output_->streamOff();
> > > > +	int ret;
> > > > +
> > > > +	ret = output_->streamOff();
> > 
> > You can merge the two lines.
> > 
> > > > +
> > > > +	/* The default std::queue constructor is explicit with gcc 5 and 6. */
> > > > +	availableBuffers_ = std::queue<FrameBuffer *>{};
> > > 
> > > I'm surprised std::queue does not have a reset method or something
> > > similar.
> > > 
> > > > +
> > > > +	buffers_.clear();
> > > > +
> > > > +	if (output_->releaseBuffers())
> > > > +		LOG(IPU3, Error) << "Failed to release CIO2 buffers";
> > > > +
> > > > +	return ret;
> > > >  }
> > > >
> > > >  int CIO2Device::queueBuffer(FrameBuffer *buffer)
> > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
> > > > index 465c0778f27e4066..032b91c082889a63 100644
> > > > --- a/src/libcamera/pipeline/ipu3/cio2.h
> > > > +++ b/src/libcamera/pipeline/ipu3/cio2.h
> > > > @@ -44,10 +44,8 @@ public:
> > > >  	StreamConfiguration generateConfiguration(const PixelFormat desiredPixelFormat = {},
> > > >  						  const Size desiredSize = {}) const;
> > > >
> > > > -	int allocateBuffers();
> > > >  	int exportBuffers(unsigned int count,
> > > >  			  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> > > > -	void freeBuffers();
> > > >
> > > >  	FrameBuffer *getBuffer();
> > > >  	void putBuffer(FrameBuffer *buffer);
> > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > index 2d636f0944587a17..636c1c54627b5777 100644
> > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > @@ -653,24 +653,17 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,
> > > >  int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
> > > >  {
> > > >  	IPU3CameraData *data = cameraData(camera);
> > > > -	CIO2Device *cio2 = &data->cio2_;
> > > >  	ImgUDevice *imgu = data->imgu_;
> > > >  	unsigned int bufferCount = 0;
> > > >  	int ret;
> > > >
> > > > -	ret = cio2->allocateBuffers();
> > > > -	if (ret < 0)
> > > > -		return ret;
> > > > -
> > > >  	bufferCount = std::max<unsigned int>(data->outStream_.configuration().bufferCount, bufferCount);
> > > >  	bufferCount = std::max<unsigned int>(data->vfStream_.configuration().bufferCount, bufferCount);
> > > >  	bufferCount = std::max<unsigned int>(data->rawStream_.configuration().bufferCount, bufferCount);
> > > >
> > > >  	ret = imgu->allocateBuffers(data, bufferCount);
> > > > -	if (ret < 0) {
> > > > -		cio2->freeBuffers();
> > > > +	if (ret < 0)
> > > >  		return ret;
> > > > -	}
> > > >
> > > >  	return 0;
> > > >  }
> > > > @@ -679,7 +672,6 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)
> > > >  {
> > > >  	IPU3CameraData *data = cameraData(camera);
> > > >
> > > > -	data->cio2_.freeBuffers();
> > 
> > The CIO2 buffers are now freed when stopping the CIO2, which happens
> > before stopping the IMGU. Is there a risk they could still be in use by
> > the IMGU hardware ? V4L2 buffer orphaning may help us here, but handling
> > this issue explicitly may be better.
> 
> Yes the buffer may be used by the ImgU when the buffer is freed but as 
> you point out buffer orphaning takes care of this. We already depend on 
> buffer orphaning working properly and 10/10 in this series makes us even 
> more depending on it. In this case we could make the stop sequencing 
> more complex to eliminate the need to depend on buffer orphaning here 
> but IMHO it's not worth it.

Could we solve this by stopping the ImgU first ?

> > > >  	data->imgu_->freeBuffers(data);
> > > >
> > > >  	return 0;
> > > 
> > > Looks good to me
> > > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

-- 
Regards,

Laurent Pinchart