[libcamera-devel] [PATCH v4 09/31] libcamera: ipu3: Implement buffer allocation

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Mar 20, 2019 at 05:30:33PM +0100, Jacopo Mondi wrote:
> Implement buffer allocation in IPU3 pipeline handlers.
> As the pipeline handler supports a single stream, preprare two buffer

s/preprare/prepare/

> 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 | 48 +++++++++++++++++++++++++---
>  1 file changed, 43 insertions(+), 5 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 994c95692dd4..659a7ca4829f 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -63,6 +63,9 @@ public:
>  	V4L2Device *viewfinder;
>  	V4L2Device *stat;
>  	/* \todo Add param video device for 3A tuning */
> +
> +	BufferPool vfPool;
> +	BufferPool statPool;
>  };
>  
>  struct CIO2Device {
> @@ -81,6 +84,8 @@ struct CIO2Device {
>  	V4L2Device *output;
>  	V4L2Subdevice *csi2;
>  	V4L2Subdevice *sensor;
> +
> +	BufferPool pool;
>  };
>  
>  class PipelineHandlerIPU3 : public PipelineHandler
> @@ -108,6 +113,8 @@ public:
>  private:
>  	static constexpr unsigned int IPU3_IMGU_COUNT = 2;
>  	static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> +	static constexpr unsigned int IPU3_CIO2_BUFFER_COUNT = 4;
> +	static constexpr unsigned int IPU3_IMGU_BUFFER_COUNT = 4;
>  
>  	class IPU3CameraData : public CameraData
>  	{
> @@ -301,16 +308,47 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
>  
>  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)
> -		return -EINVAL;
> +	/* 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;
> +	}
> +
> +	/* Prepare the buffer pools for viewfinder and stat. */
> +	data->imgu->vfPool.createBuffers(IPU3_IMGU_BUFFER_COUNT);

Don't you need the same number of buffers for viewfinder and stat than
for output ?

> +	ret = viewfinder->exportBuffers(&data->imgu->vfPool);
> +	if (ret) {
> +		LOG(IPU3, Error) << "Failed to reserve ImgU viewfinder memory";
> +		return ret;
> +	}
> +
> +	data->imgu->statPool.createBuffers(IPU3_IMGU_BUFFER_COUNT);
> +	ret = stat->exportBuffers(&data->imgu->statPool);
> +	if (ret) {
> +		LOG(IPU3, Error) << "Failed to reserve ImgU stat memory";
> +		return ret;
> +	}
>  
> -	int ret = cio2->exportBuffers(&stream->bufferPool());
> +	/* Export ImgU output buffers to the stream's pool. */
> +	ret = output->exportBuffers(&stream->bufferPool());
>  	if (ret) {
> -		LOG(IPU3, Error) << "Failed to request memory";
> +		LOG(IPU3, Error) << "Failed to reserve ImgU output memory";
>  		return ret;
>  	}

This looks good overall, I just wonder if it would make sense to move
code to the CIO2Device and ImgUDevice classes (creating
allocateBuffers() methods there).

>  

-- 
Regards,

Laurent Pinchart