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

683 diff mbox series

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@jmondi.org>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 47 ++++++++++++++++++++++++----
 1 file changed, 41 insertions(+), 6 deletions(-)

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@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)