[libcamera-devel] [PATCH v7 3/8] libcamera: camera: Don't call freeBuffer() on allocateBuffer() error

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Apr 17, 2019 at 03:58:53PM +0200, Jacopo Mondi wrote:
> Do not assume the freeBuffer() function can handle allocateBuffer()
> method failures, as error handling and clean up should be performed
> by allocateBuffer() method itself.
> 
> Perform clean-up on allocations failures in the IPU3 pipeline handler,
> now that freeBuffers() is not called anymore.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/camera.cpp             |  1 -
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 19 ++++++++++++++-----
>  2 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 21caa24b90b5..2d0a80664214 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -650,7 +650,6 @@ int Camera::allocateBuffers()
>  	int ret = pipe_->allocateBuffers(this, activeStreams_);
>  	if (ret) {
>  		LOG(Camera, Error) << "Failed to allocate buffers";
> -		freeBuffers();
>  		return ret;
>  	}
>  
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index f96e8763bce9..6e093fc22259 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -314,6 +314,7 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *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. */
> @@ -323,31 +324,39 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera,
>  
>  	ret = imgu->importBuffers(pool);
>  	if (ret)
> -		return ret;
> +		goto error_free_cio2;
>  
>  	/* Export ImgU output buffers to the stream's pool. */
>  	ret = imgu->exportBuffers(&imgu->output_, &stream->bufferPool());
>  	if (ret)
> -		return ret;
> +		goto error_free_imgu;
>  
>  	/*
>  	 * Reserve memory in viewfinder and stat output devices. Use the
>  	 * same number of buffers as the ones requested for the output
>  	 * stream.
>  	 */
> -	unsigned int bufferCount = stream->bufferPool().count();
> +	bufferCount = stream->bufferPool().count();
>  
>  	imgu->viewfinder_.pool->createBuffers(bufferCount);
>  	ret = imgu->exportBuffers(&imgu->viewfinder_, imgu->viewfinder_.pool);
>  	if (ret)
> -		return ret;
> +		goto error_free_imgu;
>  
>  	imgu->stat_.pool->createBuffers(bufferCount);
>  	ret = imgu->exportBuffers(&imgu->stat_, imgu->stat_.pool);
>  	if (ret)
> -		return ret;
> +		goto error_free_imgu;
>  
>  	return 0;
> +
> +error_free_imgu:
> +	imgu->freeBuffers();
> +
> +error_free_cio2:
> +	cio2->freeBuffers();

Given that in the case of the IPU3 the freeBuffers() functions can be
called unconditionally even when buffers haven't been allocated, you
could have

error:
	freeBuffers();

Up to you.

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

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

-- 
Regards,

Laurent Pinchart