[PATCH v2] libcamera: software_isp: Clean up pending requests on stop

[Milan Zamazal]

PipelineHandler::stop() calls stopDevice() method to perform pipeline
specific cleanup and then completes waiting requests.  If any queued
requests remain, an assertion error is raised.

Software ISP stores request buffers in
SimpleCameraData::conversionQueue_ and queues them as V4L2 signals
bufferReady.  stopDevice() cleanup forgets to clean up the buffers and
their requests from conversionQueue_, possibly resulting in the
assertion error.

This patch fixes the omission.  The request must be also canceled, which
requires introducing a little PipelineHandler::cancelRequest helper in
order to be able to access the private cancel() method.

The problem wasn't very visible when
SimplePipelineHandler::kNumInternalBuffers (the number of buffers
allocated in V4L2) was equal to the number of buffers exported from
software ISP.  But when the number of the exported buffers was increased
by one in commit abe2ec64f9e4e97bbdfe3a50372611bd7b5315c2, the assertion
error started pop up in some environments.  Increasing the number of the
buffers much more, e.g. to 9, makes the problem very reproducible.

Each pipeline uses its own mechanism to track the requests to clean up
and it can't be excluded that similar omissions are present in other
places.  But there is no obvious way to make a common cleanup for all
the pipelines (except for doing it instead of raising the assertion
error, which is probably undesirable, in order not to hide incomplete
pipeline specific cleanups).

Bug: https://bugs.libcamera.org/show_bug.cgi?id=234
Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
---
 include/libcamera/internal/pipeline_handler.h |  1 +
 src/libcamera/pipeline/simple/simple.cpp      | 14 +++++++++++
 src/libcamera/pipeline_handler.cpp            | 23 +++++++++++++------
 3 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
index 0d3808036..fb28a18d0 100644
--- a/include/libcamera/internal/pipeline_handler.h
+++ b/include/libcamera/internal/pipeline_handler.h
@@ -60,6 +60,7 @@ public:
 
 	bool completeBuffer(Request *request, FrameBuffer *buffer);
 	void completeRequest(Request *request);
+	void cancelRequest(Request *request);
 
 	std::string configurationFile(const std::string &subdir,
 				      const std::string &name) const;
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index 3ddce71d3..e862ef00f 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -284,6 +284,7 @@ public:
 	std::vector<std::unique_ptr<FrameBuffer>> conversionBuffers_;
 	std::queue<std::map<const Stream *, FrameBuffer *>> conversionQueue_;
 	bool useConversion_;
+	void clearIncompleteRequests();
 
 	std::unique_ptr<Converter> converter_;
 	std::unique_ptr<SoftwareIsp> swIsp_;
@@ -897,6 +898,18 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)
 		pipe->completeRequest(request);
 }
 
+void SimpleCameraData::clearIncompleteRequests()
+{
+	while (!conversionQueue_.empty()) {
+		for (auto &item : conversionQueue_.front()) {
+			FrameBuffer *outputBuffer = item.second;
+			Request *request = outputBuffer->request();
+			pipe()->cancelRequest(request);
+		}
+		conversionQueue_.pop();
+	}
+}
+
 void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
 {
 	swIsp_->processStats(frame, bufferId,
@@ -1407,6 +1420,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
 	video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);
 
 	data->conversionBuffers_.clear();
+	data->clearIncompleteRequests();
 
 	releasePipeline(data);
 }
diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
index e59404691..c9cb11f0f 100644
--- a/src/libcamera/pipeline_handler.cpp
+++ b/src/libcamera/pipeline_handler.cpp
@@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)
 	while (!waitingRequests_.empty()) {
 		Request *request = waitingRequests_.front();
 		waitingRequests_.pop();
-
-		request->_d()->cancel();
-		completeRequest(request);
+		cancelRequest(request);
 	}
 
 	/* Make sure no requests are pending. */
@@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)
 	}
 
 	int ret = queueRequestDevice(camera, request);
-	if (ret) {
-		request->_d()->cancel();
-		completeRequest(request);
-	}
+	if (ret)
+		cancelRequest(request);
 }
 
 /**
@@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)
 	}
 }
 
+/**
+ * \brief Cancel request and signal its completion
+ * \param[in] request The request to cancel
+ *
+ * This function cancels the request in addition to its completion. The same
+ * rules as for completeRequest() apply.
+ */
+void PipelineHandler::cancelRequest(Request *request)
+{
+	request->_d()->cancel();
+	completeRequest(request);
+}
+
 /**
  * \brief Retrieve the absolute path to a platform configuration file
  * \param[in] subdir The pipeline handler specific subdirectory name
-- 
2.44.1