[libcamera-devel] [PATCH v2 08/10] libcamera: pipeline: Rework PipelineHandler::queueRequest()

[Niklas Söderlund]

Expecting a pipeline handler implementation of queueRequest() to call
the base class queueRequest() at the correct point have lead to
different behaviors between the pipelines.

Fix this by splitting queueRequest() into a base class implementation
which handles the bookkeeping and a new queueRequestHardware() that is
to be implemented by pipeline handlers and only deals with committing the
request to the hardware.

Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
---
 src/libcamera/include/pipeline_handler.h |  4 ++-
 src/libcamera/pipeline/ipu3/ipu3.cpp     |  6 ++--
 src/libcamera/pipeline/rkisp1/rkisp1.cpp |  7 ++---
 src/libcamera/pipeline/uvcvideo.cpp      |  6 ++--
 src/libcamera/pipeline/vimc.cpp          |  6 ++--
 src/libcamera/pipeline_handler.cpp       | 35 +++++++++++++++++-------
 6 files changed, 37 insertions(+), 27 deletions(-)

diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
index 241af58beec0eb13..ad7d08e681d0f28e 100644
--- a/src/libcamera/include/pipeline_handler.h
+++ b/src/libcamera/include/pipeline_handler.h
@@ -78,7 +78,7 @@ public:
 	virtual int start(Camera *camera) = 0;
 	virtual void stop(Camera *camera) = 0;
 
-	virtual int queueRequest(Camera *camera, Request *request);
+	int queueRequest(Camera *camera, Request *request);
 
 	bool completeBuffer(Camera *camera, Request *request, Buffer *buffer);
 	void completeRequest(Camera *camera, Request *request);
@@ -90,6 +90,8 @@ protected:
 			    std::unique_ptr<CameraData> data);
 	void hotplugMediaDevice(MediaDevice *media);
 
+	virtual int queueRequestHardware(Camera *camera, Request *request) = 0;
+
 	CameraData *cameraData(const Camera *camera);
 
 	CameraManager *manager_;
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index 1c5fccf694289ae9..ad223d9101bdc6ed 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -220,7 +220,7 @@ public:
 	int start(Camera *camera) override;
 	void stop(Camera *camera) override;
 
-	int queueRequest(Camera *camera, Request *request) override;
+	int queueRequestHardware(Camera *camera, Request *request) override;
 
 	bool match(DeviceEnumerator *enumerator) override;
 
@@ -756,7 +756,7 @@ void PipelineHandlerIPU3::stop(Camera *camera)
 	data->rawBuffers_.clear();
 }
 
-int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)
+int PipelineHandlerIPU3::queueRequestHardware(Camera *camera, Request *request)
 {
 	int error = 0;
 
@@ -769,8 +769,6 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)
 			error = ret;
 	}
 
-	PipelineHandler::queueRequest(camera, request);
-
 	return error;
 }
 
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index b21cf92435e7bbc9..f5d19dc047aa5a31 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -184,7 +184,7 @@ public:
 	int start(Camera *camera) override;
 	void stop(Camera *camera) override;
 
-	int queueRequest(Camera *camera, Request *request) override;
+	int queueRequestHardware(Camera *camera, Request *request) override;
 
 	bool match(DeviceEnumerator *enumerator) override;
 
@@ -810,13 +810,12 @@ void PipelineHandlerRkISP1::stop(Camera *camera)
 	activeCamera_ = nullptr;
 }
 
-int PipelineHandlerRkISP1::queueRequest(Camera *camera, Request *request)
+int PipelineHandlerRkISP1::queueRequestHardware(Camera *camera,
+						Request *request)
 {
 	RkISP1CameraData *data = cameraData(camera);
 	Stream *stream = &data->stream_;
 
-	PipelineHandler::queueRequest(camera, request);
-
 	RkISP1FrameInfo *info = data->frameInfo_.create(data->frame_, request,
 							stream);
 	if (!info)
diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
index 45448d6f8c05ddf5..76c2d377fb160ce4 100644
--- a/src/libcamera/pipeline/uvcvideo.cpp
+++ b/src/libcamera/pipeline/uvcvideo.cpp
@@ -72,7 +72,7 @@ public:
 	int start(Camera *camera) override;
 	void stop(Camera *camera) override;
 
-	int queueRequest(Camera *camera, Request *request) override;
+	int queueRequestHardware(Camera *camera, Request *request) override;
 
 	bool match(DeviceEnumerator *enumerator) override;
 
@@ -262,7 +262,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
 	return ret;
 }
 
-int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)
+int PipelineHandlerUVC::queueRequestHardware(Camera *camera, Request *request)
 {
 	UVCCameraData *data = cameraData(camera);
 	Buffer *buffer = request->findBuffer(&data->stream_);
@@ -281,8 +281,6 @@ int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)
 	if (ret < 0)
 		return ret;
 
-	PipelineHandler::queueRequest(camera, request);
-
 	return 0;
 }
 
diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
index e6ab6a0858247549..13f5a3aca697f566 100644
--- a/src/libcamera/pipeline/vimc.cpp
+++ b/src/libcamera/pipeline/vimc.cpp
@@ -90,7 +90,7 @@ public:
 	int start(Camera *camera) override;
 	void stop(Camera *camera) override;
 
-	int queueRequest(Camera *camera, Request *request) override;
+	int queueRequestHardware(Camera *camera, Request *request) override;
 
 	bool match(DeviceEnumerator *enumerator) override;
 
@@ -323,7 +323,7 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)
 	return ret;
 }
 
-int PipelineHandlerVimc::queueRequest(Camera *camera, Request *request)
+int PipelineHandlerVimc::queueRequestHardware(Camera *camera, Request *request)
 {
 	VimcCameraData *data = cameraData(camera);
 	Buffer *buffer = request->findBuffer(&data->stream_);
@@ -342,8 +342,6 @@ int PipelineHandlerVimc::queueRequest(Camera *camera, Request *request)
 	if (ret < 0)
 		return ret;
 
-	PipelineHandler::queueRequest(camera, request);
-
 	return 0;
 }
 
diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
index 4349ca8957e403fe..c9e348b98da7b736 100644
--- a/src/libcamera/pipeline_handler.cpp
+++ b/src/libcamera/pipeline_handler.cpp
@@ -345,27 +345,42 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera)
  * \param[in] request The request to queue
  *
  * This method queues a capture request to the pipeline handler for processing.
- * The request contains a set of buffers associated with streams and a set of
- * parameters. The pipeline handler shall program the device to ensure that the
- * parameters will be applied to the frames captured in the buffers provided in
- * the request.
- *
- * Pipeline handlers shall override this method. The base implementation in the
- * PipelineHandler class keeps track of queued requests in order to ensure
- * completion of all requests when the pipeline handler is stopped with stop().
- * Requests completion shall be signalled by the pipeline handler using the
+ * The method keeps track of queued requests in order to ensure completion of
+ * all requests when the pipeline handler is stopped with stop(). Requests
+ * completion shall be signalled by the pipeline handler using the
  * completeRequest() method.
  *
  * \return 0 on success or a negative error code otherwise
  */
 int PipelineHandler::queueRequest(Camera *camera, Request *request)
 {
+	int ret;
+
 	CameraData *data = cameraData(camera);
 	data->queuedRequests_.push_back(request);
 
-	return 0;
+	ret = queueRequestHardware(camera, request);
+	if (ret)
+		data->queuedRequests_.remove(request);
+
+	return ret;
 }
 
+/**
+ * \fn PipelineHandler::queueRequestHardware()
+ * \brief Queue a request to the hardware
+ * \param[in] camera The camera to queue the request to
+ * \param[in] request The request to queue
+ *
+ * This method queues a capture request to the hardware for processing. The
+ * request contains a set of buffers associated with streams and a set of
+ * parameters. The pipeline handler shall program the device to ensure that the
+ * parameters will be applied to the frames captured in the buffers provided in
+ * the request.
+ *
+ * \return 0 on success or a negative error code otherwise
+ */
+
 /**
  * \brief Complete a buffer for a request
  * \param[in] camera The camera the request belongs to
-- 
2.24.0