[libcamera-devel] [PATCH v4 8/9] libcamera: ipu3: cio2: Hide buffer allocation and freeing from users

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Fri, Jun 26, 2020 at 12:38:59AM +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 v3
> - Keep CIO2Device::freeBuffers() but make it private.
> 
> * Changes since v2
> - Stop IMGU before CIO2
> 
> * Changes since v1
> - Fix potential leaking of buffers if start fails.
> ---
>  src/libcamera/pipeline/ipu3/cio2.cpp | 62 +++++++++++++---------------
>  src/libcamera/pipeline/ipu3/cio2.h   |  4 +-
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 14 ++-----
>  3 files changed, 33 insertions(+), 47 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index 972787a83dede203..d698cd80cba212a1 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -207,44 +207,12 @@ CIO2Device::generateConfiguration(Size size) 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,27 @@ 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)
> +		freeBuffers();
> +
> +	return ret;
>  }
>  
>  int CIO2Device::stop()
>  {
> -	return output_->streamOff();
> +	int ret = output_->streamOff();
> +
> +	freeBuffers();
> +
> +	return ret;
>  }
>  
>  int CIO2Device::queueBuffer(FrameBuffer *buffer)
> @@ -279,6 +262,17 @@ int CIO2Device::queueBuffer(FrameBuffer *buffer)
>  	return output_->queueBuffer(buffer);
>  }
>  
> +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";
> +}

You don't have to move this function.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +
>  void CIO2Device::cio2BufferReady(FrameBuffer *buffer)
>  {
>  	bufferReady.emit(buffer);
> diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
> index 8cb90c9da7f19ecb..47c6f010eae6a64d 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.h
> +++ b/src/libcamera/pipeline/ipu3/cio2.h
> @@ -37,10 +37,8 @@ public:
>  
>  	StreamConfiguration generateConfiguration(Size size) const;
>  
> -	int allocateBuffers();
>  	int exportBuffers(unsigned int count,
>  			  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> -	void freeBuffers();
>  
>  	FrameBuffer *getBuffer();
>  	void putBuffer(FrameBuffer *buffer);
> @@ -54,6 +52,8 @@ public:
>  	Signal<FrameBuffer *> bufferReady;
>  
>  private:
> +	void freeBuffers();
> +
>  	void cio2BufferReady(FrameBuffer *buffer);
>  
>  	CameraSensor *sensor_;
> 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