[libcamera-devel] [PATCH] libcamera: pipeline: rkisp1: Do not wait for param buffer it it's not used

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Wed, Mar 25, 2020 at 11:23:55AM +0100, Niklas Söderlund wrote:
> If the parameter buffer is not filled in by the IPA and therefor not

s/therefor/therefore/

> used, mark it as already dequeued. If not a request without a parameter

s/If not/Otherwise,/ (or "If not,")

> buffer will never complete.

I'm not sure to follow you there, parameters buffers are not provided in
requests. Is this about the IPA not providing a parameters buffer in
time ? Doesn't that introduce a bad race condition if we don't provide a
parameters buffer for every request ?

> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 2f909cef7c75ba0f..6f4eafa1523a4a39 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -351,12 +351,15 @@ protected:
>  		if (!info)
>  			LOG(RkISP1, Fatal) << "Frame not known";
>  
> -		if (info->paramFilled)
> +		if (info->paramFilled) {
>  			pipe_->param_->queueBuffer(info->paramBuffer);
> -		else
> +		} else {
> +			/* No need to dequeue param if it's never queued. */
> +			info->paramDequeued = true;
>  			LOG(RkISP1, Error)
>  				<< "Parameters not ready on time for frame "
>  				<< frame() << ", ignore parameters.";

How often does this occur ?

> +		}

I think the logic should be simplified. Do we really have a need to
dequeue the parameters buffer to complete the request ? It seems we
should be able to just dequeue them when they're signalled, and put them
in a list of free parameters buffer for later use.

>  
>  		pipe_->stat_->queueBuffer(info->statBuffer);
>  		pipe_->video_->queueBuffer(info->videoBuffer);

-- 
Regards,

Laurent Pinchart