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

[Niklas Söderlund]

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.
---
 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);
+
 		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 */
-- 
2.30.0