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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tue, Jun 23, 2020 at 04:09:29AM +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 call 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 at ragnatech.se>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> * Changes since v2
> - Stop IMGU before CIO2
> 
> * 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 | 14 ++-----
>  3 files changed, 32 insertions(+), 47 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index 3d7348782b0fa6cb..efaa460b246697a6 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -207,44 +207,12 @@ CIO2Device::generateConfiguration(const Size &desiredSize) const
>  	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()) {
> @@ -266,12 +234,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";
> +

This duplicates code, I don't think it's nice :-( Is there an issue with
keeping CIO2Device::freeBuffers() and making it private ?

> +	return ret;
>  }
>  
>  int CIO2Device::queueBuffer(FrameBuffer *buffer)
> diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
> index cc898f13fd73f865..405e6c03755367c4 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.h
> +++ b/src/libcamera/pipeline/ipu3/cio2.h
> @@ -37,10 +37,8 @@ public:
>  
>  	StreamConfiguration generateConfiguration(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 2d1ec707ea4b08fe..4818027e8db1f7a3 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -658,15 +658,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,
> @@ -674,10 +669,8 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
>  	});
>  
>  	ret = imgu->allocateBuffers(data, bufferCount);
> -	if (ret < 0) {
> -		cio2->freeBuffers();
> +	if (ret < 0)
>  		return ret;
> -	}
>  
>  	return 0;
>  }
> @@ -686,7 +679,6 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)
>  {
>  	IPU3CameraData *data = cameraData(camera);
>  
> -	data->cio2_.freeBuffers();
>  	data->imgu_->freeBuffers(data);
>  
>  	return 0;
> @@ -731,10 +723,10 @@ error:
>  void PipelineHandlerIPU3::stop(Camera *camera)
>  {
>  	IPU3CameraData *data = cameraData(camera);
> -	int ret;
> +	int ret = 0;
>  
> -	ret = data->cio2_.stop();
>  	ret |= data->imgu_->stop();
> +	ret |= data->cio2_.stop();
>  	if (ret)
>  		LOG(IPU3, Warning) << "Failed to stop camera "
>  				   << camera->name();

-- 
Regards,

Laurent Pinchart