[libcamera-devel] [PATCH 05/10] libcamera: ipu3: Implement buffer allocation

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Feb 28, 2019 at 09:04:05PM +0100, Jacopo Mondi wrote:
> Implement buffer allocation in IPU3 pipeline handlers.
> As the pipeline handler supports a single stream, preprare two buffer
> pools for 'viewfinder' and 'stat' video devices, and export the 'output'
> video device buffers to the Stream's pool.
> 
> Share buffers between the CIO2 output and the ImgU input video devices,
> as the output of the former should immediately be provided to the
> latter for further processing.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 47 ++++++++++++++++++++++++----
>  1 file changed, 41 insertions(+), 6 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 1e89e57f628b..c7b7973952a0 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -47,6 +47,9 @@ struct ImguDevice {
>  	V4L2Device *viewfinder;
>  	V4L2Device *stat;
>  	/* TODO: add param video device for 3A tuning */
> +
> +	BufferPool vfPool;
> +	BufferPool statPool;
>  };
>  
>  struct Cio2Device {
> @@ -63,6 +66,8 @@ struct Cio2Device {
>  	V4L2Device *output;
>  	V4L2Subdevice *csi2;
>  	V4L2Subdevice *sensor;
> +
> +	BufferPool pool;
>  };
>  
>  class IPU3CameraData : public CameraData
> @@ -319,18 +324,48 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)
>  {
>  	const StreamConfiguration &cfg = stream->configuration();
>  	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;
> +	int ret;
>  
> -	if (!cfg.bufferCount)
> +	if (!cfg.bufferCount) {
> +		LOG(IPU3, Error)
> +			<< "Invalid number of buffers: "<< cfg.bufferCount;
>  		return -EINVAL;

Can this happen ? Shouldn't it be caught in upper layers ?

> -
> -	int ret = cio2->exportBuffers(&stream->bufferPool());
> -	if (ret) {
> -		LOG(IPU3, Error) << "Failed to request memory";
> -		return ret;
>  	}
>  
> +	/* Share buffers between CIO2 output and ImgU input. */
> +	data->cio2.pool.createBuffers(IPU3_BUF_NUM);

The number of buffers doesn't have to match between the internal pool
and the external pool. I would define a separate constant for this, even
if their values happen to be the same for now.

> +	ret = cio2->exportBuffers(&data->cio2.pool);
> +	if (ret)
> +		goto error_reserve_memory;
> +	input->importBuffers(&data->cio2.pool);

Can't this fail ?

> +
> +	/* Prepare the buffer pools for viewfinder and stat. */
> +	data->imgu->vfPool.createBuffers(IPU3_BUF_NUM);
> +	ret = viewfinder->exportBuffers(&data->imgu->vfPool);
> +	if (ret)
> +		goto error_reserve_memory;
> +
> +	data->imgu->statPool.createBuffers(IPU3_BUF_NUM);
> +	ret = stat->exportBuffers(&data->imgu->statPool);
> +	if (ret)
> +		goto error_reserve_memory;
> +
> +	/* Export ImgU output buffers to the stream's pool. */
> +	ret = output->exportBuffers(&stream->bufferPool());
> +	if (ret)
> +		goto error_reserve_memory;
> +
>  	return 0;
> +
> +error_reserve_memory:
> +	LOG(IPU3, Error) << "Failed to reserve memory";

I'd duplicate the error message instead of using a goto, in order to
print which memory allocation failed.

As we won't queue buffers for the viewfinder and stats video node (at
least for now), do we even need to allocate them ?

> +
> +	return ret;
>  }
>  
>  int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream)

-- 
Regards,

Laurent Pinchart