[libcamera-devel] [PATCH v4 04/12] libcamera: ipu3: Add multiple stream memory management

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Apr 09, 2019 at 09:25:40PM +0200, Jacopo Mondi wrote:
> Perform allocation and setup of memory sharing between the CIO2 output
> and the ImgU input and allocate memory for each active stream.

No SoB ?

> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 65 +++++++++++++++++++++-------
>  1 file changed, 49 insertions(+), 16 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 527213a8970a..f5d768f9f87f 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -91,6 +91,7 @@ public:
>  
>  	BufferPool vfPool_;
>  	BufferPool statPool_;
> +	BufferPool outPool_;
>  };
>  
>  class CIO2Device
> @@ -380,13 +381,23 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
>  	return 0;
>  }
>  
> +/**
> + * \todo Clarify if 'viewfinder' and 'stat' nodes have to be set up and
> + * started even if not in use. As of now, if not properly configured and
> + * enabled, the ImgU processing pipeline stalls.
> + *
> + * In order to be able to start the 'viewfinder' and 'stat' nodes, we need
> + * memory to be reserved.
> + */
>  int PipelineHandlerIPU3::allocateBuffers(Camera *camera,
>  					 const std::set<Stream *> &streams)
>  {
>  	IPU3CameraData *data = cameraData(camera);
> -	Stream *stream = *streams.begin();
> +	IPU3Stream *outStream = &data->outStream_;
> +	IPU3Stream *vfStream = &data->vfStream_;
>  	CIO2Device *cio2 = &data->cio2_;
>  	ImgUDevice *imgu = data->imgu_;
> +
>  	int ret;
>  
>  	/* Share buffers between CIO2 output and ImgU input. */
> @@ -398,28 +409,49 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera,
>  	if (ret)
>  		return ret;
>  
> -	/* Export ImgU output buffers to the stream's pool. */
> -	ret = imgu->exportBuffers(&imgu->output_, &stream->bufferPool());
> -	if (ret)
> -		return ret;
> -
>  	/*
> -	 * Reserve memory in viewfinder and stat output devices. Use the
> -	 * same number of buffers as the ones requested for the output
> -	 * stream.
> +	 * Use for the stat's internal pools the same number of buffer as

s/pools/pool/

With this fixed,

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

> +	 * for the input pool.
> +	 * \todo To be revised when we'll actually use the stat node.
>  	 */
> -	unsigned int bufferCount = stream->bufferPool().count();
> -
> -	imgu->viewfinder_.pool->createBuffers(bufferCount);
> -	ret = imgu->exportBuffers(&imgu->viewfinder_, imgu->viewfinder_.pool);
> -	if (ret)
> -		return ret;
> -
> +	unsigned int bufferCount = pool->count();
>  	imgu->stat_.pool->createBuffers(bufferCount);
>  	ret = imgu->exportBuffers(&imgu->stat_, imgu->stat_.pool);
>  	if (ret)
>  		return ret;
>  
> +	/* Allocate buffers for each active stream. */
> +	for (Stream *s : streams) {
> +		IPU3Stream *stream = static_cast<IPU3Stream *>(s);
> +		ImgUDevice::ImgUOutput *dev = stream->device_;
> +
> +		ret = imgu->exportBuffers(dev, &stream->bufferPool());
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/*
> +	 * Allocate buffers also on non-active outputs; use the same number
> +	 * of buffers as the active ones.
> +	 */
> +	if (!outStream->active_) {
> +		bufferCount = vfStream->bufferPool().count();
> +		outStream->device_->pool->createBuffers(bufferCount);
> +		ret = imgu->exportBuffers(outStream->device_,
> +					  outStream->device_->pool);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (!vfStream->active_) {
> +		bufferCount = outStream->bufferPool().count();
> +		vfStream->device_->pool->createBuffers(bufferCount);
> +		ret = imgu->exportBuffers(vfStream->device_,
> +					  vfStream->device_->pool);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -780,6 +812,7 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)
>  
>  	output_.pad = PAD_OUTPUT;
>  	output_.name = "output";
> +	output_.pool = &outPool_;
>  
>  	viewfinder_.dev = V4L2Device::fromEntityName(media,
>  						     name_ + " viewfinder");

-- 
Regards,

Laurent Pinchart