[libcamera-devel] [PATCH v4 10/31] libcamera: ipu3: Implement buffer release

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Mar 20, 2019 at 05:30:34PM +0100, Jacopo Mondi wrote:
> Release buffers on all video devices in the pipeline.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 30 +++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 659a7ca4829f..965794494a4e 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -158,6 +158,7 @@ private:
>  	void registerCameras();
>  
>  	ImgUDevice imgus_[IPU3_IMGU_COUNT];
> +

Please move this to the appropriate patch if needed, or drop it.

>  	std::shared_ptr<MediaDevice> cio2MediaDev_;
>  	std::shared_ptr<MediaDevice> imguMediaDev_;
>  };
> @@ -358,13 +359,32 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)
>  int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream)
>  {
>  	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;

I'm really not a big fan of all these local variables, here and in the
previous patches :-( Maybe creating helpers in the CIO2Device and
ImgUDevice classes would help.

> +	int ret;
>  
> -	int ret = cio2->releaseBuffers();
> -	if (ret) {
> -		LOG(IPU3, Error) << "Failed to release memory";
> -		return ret;
> -	}
> +	ret = output->releaseBuffers();
> +	if (ret)
> +		LOG(IPU3, Error) << "Failed to release ImgU output memory";

memory or buffers ? Same below.

Apart from that the patch looks good to me.

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

> +
> +	ret = stat->releaseBuffers();
> +	if (ret)
> +		LOG(IPU3, Error) << "Failed to release ImgU stat memory";
> +
> +	ret = viewfinder->releaseBuffers();
> +	if (ret)
> +		LOG(IPU3, Error) << "Failed to release ImgU viewfinder memory";
> +
> +	ret = input->releaseBuffers();
> +	if (ret)
> +		LOG(IPU3, Error) << "Failed to release ImgU input memory";
> +
> +	ret = cio2->releaseBuffers();
> +	if (ret)
> +		LOG(IPU3, Error) << "Failed to release CIO2 memory";
>  
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart