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

3971 diff mbox series

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.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Reviewed-by: Jacopo Mondi <jacopo@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(-)

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@ragnatech.se>
> Reviewed-by: Jacopo Mondi <jacopo@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;