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

[Jacopo Mondi]

Hi Niklas,

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.
>
> 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();
>  }
>
>  int CIO2Device::stop()
>  {
> -	return output_->streamOff();
> +	int ret;
> +
> +	ret = output_->streamOff();
> +
> +	/* 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();
>  	data->imgu_->freeBuffers(data);
>
>  	return 0;

Looks good to me
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j

> --
> 2.26.2
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel