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

Message ID 20190417135858.23914-4-jacopo@jmondi.org
State Accepted
Headers show
Series
  • libcamera: Framework changes to prepare for multiple streams support
Related show

Commit Message

Jacopo Mondi April 17, 2019, 1:58 p.m. UTC 
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@jmondi.org>
---
 src/libcamera/camera.cpp             |  1 -
 src/libcamera/pipeline/ipu3/ipu3.cpp | 19 ++++++++++++++-----
 2 files changed, 14 insertions(+), 6 deletions(-)

Comments

Niklas Söderlund April 17, 2019, 3:35 p.m. UTC | #1 
Hi Jacopo,

Thanks for your work.

On 2019-04-17 15:58:53 +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@jmondi.org>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  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();
> +
> +	return ret;
>  }
>  
>  int PipelineHandlerIPU3::freeBuffers(Camera *camera,
> -- 
> 2.21.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart April 18, 2019, 12:47 p.m. UTC | #2 
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@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@ideasonboard.com>

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

Patch
diff mbox series 

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();
+
+	return ret;
 }
 
 int PipelineHandlerIPU3::freeBuffers(Camera *camera,