[libcamera-devel] [PATCH v3 21/33] libcamera: pipeline: rkisp1: Destroy frame information before completing request

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Fri, Jan 10, 2020 at 08:37:56PM +0100, Niklas Söderlund wrote:
> It's common for applications to create and queue a new request in a
> previous request completion handler. When the new request gets queued to
> the RkISP1 pipeline handler it tries to find a parameters and statistic
> buffer to be used with the request. The problem is if the pipeline depth
> is already filled there are no internal buffers free to be used by the
> new request.
> 
> This was solved by allocation one more parameters and statistic buffer
> then the pipeline depth, this is waste full. Instead free the request
> that is about to be completed resources before it is signaled to the

"free the resources of the request that has completed"

> application, this way if the pipeline depth is full it can reuse the
> internal resources and the waste full allocation can be removed.

s/waste full/wasteful/

> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> * Changes since v2
> - Rewrote commit message.
> - Actually remove the wasteful allocation of extra buffers in this
>   commit instead of depending on the switch to FrameBuffer to do it for
>   us.

Ah this makes much more sense to me now :-)

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

> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 979b670e4cb75512..607ff85539a22324 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -671,14 +671,14 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,
>  	if (ret)
>  		return ret;
>  
> -	paramPool_.createBuffers(stream->configuration().bufferCount + 1);
> +	paramPool_.createBuffers(stream->configuration().bufferCount);
>  	ret = param_->exportBuffers(&paramPool_);
>  	if (ret) {
>  		video_->releaseBuffers();
>  		return ret;
>  	}
>  
> -	statPool_.createBuffers(stream->configuration().bufferCount + 1);
> +	statPool_.createBuffers(stream->configuration().bufferCount);
>  	ret = stat_->exportBuffers(&statPool_);
>  	if (ret) {
>  		param_->releaseBuffers();
> @@ -686,13 +686,13 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,
>  		return ret;
>  	}
>  
> -	for (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {
> +	for (unsigned int i = 0; i < stream->configuration().bufferCount; i++) {
>  		data->ipaBuffers_.push_back({ .id = RKISP1_PARAM_BASE | i,
>  					      .planes = paramPool_.buffers()[i].planes() });
>  		paramBuffers_.push(new Buffer(i));
>  	}
>  
> -	for (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {
> +	for (unsigned int i = 0; i < stream->configuration().bufferCount; i++) {
>  		data->ipaBuffers_.push_back({ .id = RKISP1_STAT_BASE | i,
>  					      .planes = statPool_.buffers()[i].planes() });
>  		statBuffers_.push(new Buffer(i));
> @@ -981,9 +981,9 @@ void PipelineHandlerRkISP1::tryCompleteRequest(Request *request)
>  	if (!info->paramDequeued)
>  		return;
>  
> -	completeRequest(activeCamera_, request);
> -
>  	data->frameInfo_.destroy(info->frame);
> +
> +	completeRequest(activeCamera_, request);
>  }
>  
>  void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)

-- 
Regards,

Laurent Pinchart