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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Sat, Jun 06, 2020 at 05:04:35PM +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

s/calls/call/

> move the CIO2 buffer allocation/freeing inside the CIO2Device and reduce
> the complexity of the exposed interface.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> * Changes since v1
> - Fix potential leaking of buffers if start fails.
> ---
>  src/libcamera/pipeline/ipu3/cio2.cpp | 63 +++++++++++++---------------
>  src/libcamera/pipeline/ipu3/cio2.h   |  2 -
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +----
>  3 files changed, 30 insertions(+), 45 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index 2399be8de974ff92..9298f10d8d8c07f7 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -229,44 +229,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";
> -}

Should you keep this function and make it private, to be called from
start() and stop() ? One may wonder why we don't have a private
allocateBuffers() then, I'll leave that one up to you.

> -
>  FrameBuffer *CIO2Device::getBuffer()
>  {
>  	if (availableBuffers_.empty()) {
> @@ -288,12 +256,39 @@ void CIO2Device::putBuffer(FrameBuffer *buffer)
>  
>  int CIO2Device::start()
>  {
> -	return output_->streamOn();
> +	int ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_);
> +	if (ret < 0)
> +		return ret;
> +
> +	for (std::unique_ptr<FrameBuffer> &buffer : buffers_)
> +		availableBuffers_.push(buffer.get());
> +
> +	ret = output_->streamOn();
> +	if (ret) {
> +		/* The default std::queue constructor is explicit with gcc 5 and 6. */
> +		availableBuffers_ = std::queue<FrameBuffer *>{};
> +
> +		buffers_.clear();
> +
> +		output_->releaseBuffers();
> +	}
> +
> +	return ret;
>  }
>  
>  int CIO2Device::stop()
>  {
> -	return output_->streamOff();
> +	int ret = output_->streamOff();
> +
> +	/* 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";
> +
> +	return ret;
>  }
>  
>  int CIO2Device::queueBuffer(FrameBuffer *buffer)
> diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
> index 8c3d9dd24188ef1c..ffb401da6b576556 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.h
> +++ b/src/libcamera/pipeline/ipu3/cio2.h
> @@ -39,10 +39,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 55958a6c5e64dbf1..c95c1bfbce914801 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -646,15 +646,10 @@ 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;
>  	int ret;
>  
> -	ret = cio2->allocateBuffers();
> -	if (ret < 0)
> -		return ret;
> -
>  	bufferCount = std::max({
>  		data->outStream_.configuration().bufferCount,
>  		data->vfStream_.configuration().bufferCount,
> @@ -662,10 +657,8 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
>  	});
>  
>  	ret = imgu->allocateBuffers(data, bufferCount);
> -	if (ret < 0) {
> -		cio2->freeBuffers();
> +	if (ret < 0)
>  		return ret;
> -	}
>  
>  	return 0;
>  }
> @@ -674,7 +667,6 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)
>  {
>  	IPU3CameraData *data = cameraData(camera);
>  
> -	data->cio2_.freeBuffers();

As commented in the review of v1, the buffers are freed when stopping
the CIO2, which happens before stopping the ImgU. I think that's a bit
fragile. Could we stop the ImgU first to handle this ?

>  	data->imgu_->freeBuffers(data);
>  
>  	return 0;

-- 
Regards,

Laurent Pinchart