[libcamera-devel] [PATCH v4 21/31] RFC: libcamera: pipeline_handlers: Add preAllocateBuffers

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Mar 20, 2019 at 05:30:45PM +0100, Jacopo Mondi wrote:
> Add a preAllocateBuffers virtual method to the PipelineHandler base
> class and call it before performing per-stream memory allocation.
> 
> Implement the method in the IPU3 pipeline handler to perform memory
> allocation on the CIO2 unit and the ImgU input and stat video devices.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/camera.cpp                 | 10 ++++-
>  src/libcamera/include/pipeline_handler.h |  5 +++
>  src/libcamera/pipeline/ipu3/ipu3.cpp     | 54 ++++++++++++++----------
>  src/libcamera/pipeline_handler.cpp       | 21 ++++++++-
>  4 files changed, 66 insertions(+), 24 deletions(-)
> 
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 8ee9cc086616..8020dff8f8ea 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -455,6 +455,8 @@ int Camera::configureStreams(std::map<Stream *, StreamConfiguration> &config)
>   */
>  int Camera::allocateBuffers()
>  {
> +	int ret;
> +
>  	if (disconnected_)
>  		return -ENODEV;
>  
> @@ -467,8 +469,14 @@ int Camera::allocateBuffers()
>  		return -EINVAL;
>  	}
>  
> +	ret = pipe_->preAllocateBuffers(this, activeStreams_);
> +	if (ret) {
> +		LOG(Camera, Error) << "Buffers pre-allocation failed";
> +		return ret;
> +	}
> +

I'm not a big fan of pre/post hooks. I think this should the
PipelineHandler::allocateBuffers() and PipelineHandler::freeBuffers()
methods are not correctly designed. It may be better to give them the
list of active streams, moving the loop from Camera to the pipeline
handlers. That way pipeline handlers could perform any extra step they
require, and even handle dependencies between allocations for different
streams if the need arises.

>  	for (Stream *stream : activeStreams_) {
> -		int ret = pipe_->allocateBuffers(this, stream);
> +		ret = pipe_->allocateBuffers(this, stream);
>  		if (ret) {
>  			LOG(Camera, Error) << "Failed to allocate buffers";
>  			freeBuffers();
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> index acb376e07030..7ce5b67cc7fc 100644
> --- a/src/libcamera/include/pipeline_handler.h
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -57,6 +57,11 @@ public:
>  	virtual int configureStreams(Camera *camera,
>  				     std::map<Stream *, StreamConfiguration> &config) = 0;
>  
> +	virtual int preAllocateBuffers(Camera *camera,
> +				       const std::set<Stream *> &activeStreams)
> +	{
> +		return 0;
> +	}
>  	virtual int allocateBuffers(Camera *camera, Stream *stream) = 0;
>  	virtual int freeBuffers(Camera *camera, Stream *stream) = 0;
>  
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 0507fc380e68..5f323888d84f 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -105,6 +105,8 @@ public:
>  	int configureStreams(Camera *camera,
>  			     std::map<Stream *, StreamConfiguration> &config) override;
>  
> +	int preAllocateBuffers(Camera *camera,
> +			       const std::set<Stream *> &activeStreams) override;
>  	int allocateBuffers(Camera *camera, Stream *stream) override;
>  	int freeBuffers(Camera *camera, Stream *stream) override;
>  
> @@ -451,40 +453,48 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
>  	return 0;
>  }
>  
> -int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)
> +int PipelineHandlerIPU3::preAllocateBuffers(Camera *camera,
> +					    const std::set<Stream *> &activeStreams)
>  {
>  	IPU3CameraData *data = cameraData(camera);
> -	V4L2Device *viewfinder = data->imgu->viewfinder;
> -	V4L2Device *output = data->imgu->output;
>  	V4L2Device *input = data->imgu->input;
>  	V4L2Device *cio2 = data->cio2.output;
>  	V4L2Device *stat = data->imgu->stat;
>  	ImgUDevice *imgu = data->imgu;
>  	int ret;
>  
> -	if (data->cio2.pool.count() == 0) {
> -		/* Share buffers between CIO2 output and ImgU input. */
> -		data->cio2.pool.createBuffers(IPU3_CIO2_BUFFER_COUNT);
> -		ret = cio2->exportBuffers(&data->cio2.pool);
> -		if (ret) {
> -			LOG(IPU3, Error) << "Failed to reserve CIO2 memory";
> -			return ret;
> -		}
> +	/* Share buffers between CIO2 output and ImgU input. */
> +	data->cio2.pool.createBuffers(IPU3_CIO2_BUFFER_COUNT);
> +	ret = cio2->exportBuffers(&data->cio2.pool);
> +	if (ret) {
> +		LOG(IPU3, Error) << "Failed to reserve CIO2 memory";
> +		return ret;
> +	}
>  
> -		ret = input->importBuffers(&data->cio2.pool);
> -		if (ret) {
> -			LOG(IPU3, Error) << "Failed to import ImgU memory";
> -			return ret;
> -		}
> +	ret = input->importBuffers(&data->cio2.pool);
> +	if (ret) {
> +		LOG(IPU3, Error) << "Failed to import ImgU memory";
> +		return ret;
> +	}
>  
> -		imgu->statPool.createBuffers(IPU3_IMGU_BUFFER_COUNT);
> -		ret = stat->exportBuffers(&imgu->statPool);
> -		if (ret) {
> -			LOG(IPU3, Error) << "Failed to reserve ImgU stat memory";
> -			return ret;
> -		}
> +	imgu->statPool.createBuffers(IPU3_IMGU_BUFFER_COUNT);
> +	ret = stat->exportBuffers(&imgu->statPool);
> +	if (ret) {
> +		LOG(IPU3, Error) << "Failed to reserve ImgU stat memory";
> +		return ret;
>  	}
>  
> +	return 0;
> +}
> +
> +int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)
> +{
> +	IPU3CameraData *data = cameraData(camera);
> +	V4L2Device *viewfinder = data->imgu->viewfinder;
> +	V4L2Device *output = data->imgu->output;
> +	ImgUDevice *imgu = data->imgu;
> +	int ret;
> +
>  	if (isOutput(data, stream)) {
>  		/* Export ImgU output buffers to the stream's pool. */
>  		ret = output->exportBuffers(&stream->bufferPool());
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 1a858f2638ce..1f556ed789b6 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -189,6 +189,24 @@ PipelineHandler::~PipelineHandler()
>   * \return 0 on success or a negative error code otherwise
>   */
>  
> +/**
> + * \fn PipelineHandler::preAllocateBuffers()
> + * \brief Perform operations to prepare for memory allocation. Optional for
> + * pipeline handlers to implement
> + * \param[in] camera The camera the streams belongs to
> + * \param[in] activeStreams The set of active streams
> + *
> + * If operations to prepare for the per-stream memory allocation are required,
> + * this virtual method provides an entry point for pipeline handlers to do so.
> + *
> + * This method is optional to implement for pipeline handlers, and its intended
> + * caller is the Camera class, which calls it once before performing per-stream
> + * memory allocation by calling the PipelineHandler::allocateBuffers() method
> + * once per each active stream.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +
>  /**
>   * \fn PipelineHandler::allocateBuffers()
>   * \brief Allocate buffers for a stream
> @@ -198,7 +216,8 @@ PipelineHandler::~PipelineHandler()
>   * This method allocates buffers internally in the pipeline handler and
>   * associates them with the stream's buffer pool.
>   *
> - * The intended caller of this method is the Camera class.
> + * The intended caller of this method is the Camera class which calls it
> + * once per each active stream.
>   *
>   * \return 0 on success or a negative error code otherwise
>   */

-- 
Regards,

Laurent Pinchart