[libcamera-devel] [PATCH] libcamera: pipeline: rkisp1: Avoid race when processing parameter buffers

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Mon, Jan 18, 2021 at 03:06:15PM +0100, Niklas Söderlund wrote:
> When moving the pipeline away from the Timeline design it was discovered
> that the design of queuing the buffers to the device as soon as possible
> was not the best idea. The parameter buffers where queued to the device
> before the IPA had processed them and this violates the V4L2 API.
> 
> Fix this by waiting to queue any buffer to the device until the IPA have
> filled in the parameters buffer.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> Hello,
> 
> This patch is based on-top of '[PATCH v5 0/8] libcamera: Add helper for
> controls that take effect with a delay' as the change is much cleaner
> on-top of the removal of the Timeline.

It should probably be squashed in the appropriate patch from that
series.

> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 41 ++++++++----------------
>  1 file changed, 13 insertions(+), 28 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 58ae8f98a5c683d5..e85979a719b7ced6 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -51,7 +51,6 @@ struct RkISP1FrameInfo {
>  	FrameBuffer *mainPathBuffer;
>  	FrameBuffer *selfPathBuffer;
>  
> -	bool paramFilled;
>  	bool paramDequeued;
>  	bool metadataProcessed;
>  };
> @@ -219,7 +218,6 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req
>  	info->mainPathBuffer = mainPathBuffer;
>  	info->selfPathBuffer = selfPathBuffer;
>  	info->statBuffer = statBuffer;
> -	info->paramFilled = false;
>  	info->paramDequeued = false;
>  	info->metadataProcessed = false;
>  
> @@ -322,9 +320,20 @@ void RkISP1CameraData::queueFrameAction(unsigned int frame,
>  		break;
>  	}
>  	case RKISP1_IPA_ACTION_PARAM_FILLED: {
> +		PipelineHandlerRkISP1 *pipe = static_cast<PipelineHandlerRkISP1 *>(pipe_);
>  		RkISP1FrameInfo *info = frameInfo_.find(frame);
> -		if (info)
> -			info->paramFilled = true;
> +		if (!info)
> +			break;
> +
> +		pipe->param_->queueBuffer(info->paramBuffer);
> +		pipe->stat_->queueBuffer(info->statBuffer);
> +
> +		if (info->mainPathBuffer)
> +			mainPath_->queueBuffer(info->mainPathBuffer);
> +
> +		if (info->selfPathBuffer)
> +			selfPath_->queueBuffer(info->selfPathBuffer);
> +

The IPA generates RKISP1_IPA_ACTION_PARAM_FILLED when the request is
queued to the IPA, so the buffers will be queued early to the driver,
ensuring we won't have a queue underrun.

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

However, this means that the IPA will fill the parameters buffer with
data that is based on much earlier frames. We'll need to find a proper
middleground here, waiting as long as possible before asking the IPA for
parameters, but not too long to ensure we won't get underruns.

>  		break;
>  	}
>  	case RKISP1_IPA_ACTION_METADATA:
> @@ -852,15 +861,6 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
>  	op.controls = { request->controls() };
>  	data->ipa_->processEvent(op);
>  
> -	param_->queueBuffer(info->paramBuffer);
> -	stat_->queueBuffer(info->statBuffer);
> -
> -	if (info->mainPathBuffer)
> -		mainPath_.queueBuffer(info->mainPathBuffer);
> -
> -	if (info->selfPathBuffer)
> -		selfPath_.queueBuffer(info->selfPathBuffer);
> -
>  	data->frame_++;
>  
>  	return 0;
> @@ -1009,7 +1009,6 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
>  	mainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);
>  	selfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);
>  	stat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);
> -	isp_->frameStart.connect(this, &PipelineHandlerRkISP1::frameStart);
>  	param_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);
>  
>  	/*
> @@ -1097,20 +1096,6 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
>  	data->ipa_->processEvent(op);
>  }
>  
> -void PipelineHandlerRkISP1::frameStart(uint32_t sequence)
> -{
> -	if (!activeCamera_)
> -		return;
> -
> -	RkISP1CameraData *data = cameraData(activeCamera_);
> -
> -	RkISP1FrameInfo *info = data->frameInfo_.find(sequence);
> -	if (!info || !info->paramFilled)
> -		LOG(RkISP1, Info)
> -			<< "Parameters not ready on time for frame "
> -			<< sequence;
> -}
> -
>  REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1)
>  
>  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart