[libcamera-devel] [PATCH v3 4/8] libcamera: ipu3: Add multiple stream memory management

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

On 2019-04-03 17:07:31 +0200, Jacopo Mondi wrote:
> Perform allocation and setup of memory sharing betweent the CIO2 output
> and the ImgU input and allocate memory for each active stream.
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 78 ++++++++++++++++++++++------
>  1 file changed, 62 insertions(+), 16 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index a12e3f9a496d..f7e75fac1dfe 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
> @@ -428,13 +429,21 @@ 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();
>  	CIO2Device *cio2 = &data->cio2_;
>  	ImgUDevice *imgu = data->imgu_;
> +	unsigned int bufferCount;
>  	int ret;
>  
>  	/* Share buffers between CIO2 output and ImgU input. */
> @@ -446,28 +455,64 @@ 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 internal pools the same buffer number as the input
> +	 * one: it should match the pipeline lenght.
>  	 */
> -	unsigned int bufferCount = stream->bufferPool().count();
> -
> -	imgu->viewfinder_.pool->createBuffers(bufferCount);
> -	ret = imgu->exportBuffers(&imgu->viewfinder_, imgu->viewfinder_.pool);
> -	if (ret)
> -		return ret;
> -
> +	bufferCount = pool->count();
>  	imgu->stat_.pool->createBuffers(bufferCount);
>  	ret = imgu->exportBuffers(&imgu->stat_, imgu->stat_.pool);
>  	if (ret)
>  		return ret;
>  
> +	for (Stream *stream : streams) {
> +		if (isOutput(data, stream)) {
> +			/* Export output buffers to the stream's pool. */
> +			ret = imgu->exportBuffers(&imgu->output_,
> +						  &stream->bufferPool());
> +			if (ret)
> +				return ret;
> +
> +			if (isViewfinderActive(data))
> +				continue;
> +
> +			/*
> +			 * Reserve memory in viewfinder device if it is not
> +			 * part of the active streams list. Use the same
> +			 * number of buffers as the ones requested for the
> +			 * output stream.
> +			 */
> +			bufferCount = stream->bufferPool().count();
> +			imgu->viewfinder_.pool->createBuffers(bufferCount);
> +			ret = imgu->exportBuffers(&imgu->viewfinder_,
> +						  imgu->viewfinder_.pool);
> +			if (ret)
> +				return ret;
> +		} else if (isViewfinder(data, stream)) {
> +			/* Export viewfinder buffers to the stream's pool. */
> +			ret = imgu->exportBuffers(&imgu->viewfinder_,
> +						  &stream->bufferPool());
> +			if (ret)
> +				return ret;
> +
> +			if (isOutputActive(data))
> +				continue;
> +
> +			/*
> +			 * Reserve memory in output device if it is not part
> +			 * of the active streams list. Use the same number
> +			 * of buffers as the ones requested for the viewfinder
> +			 * stream.
> +			 */
> +			bufferCount = stream->bufferPool().count();
> +			imgu->output_.pool->createBuffers(bufferCount);
> +			ret = imgu->exportBuffers(&imgu->output_,
> +						  imgu->output_.pool);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +

This change looks ok to me but I wonder about the error path here. What 
happens if exportBuffers fails for the second stream? I'm only curious 
:-)

>  	return 0;
>  }
>  
> @@ -819,6 +864,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");
> -- 
> 2.21.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund