[libcamera-devel] [PATCH 3/3] apps: cam: kms_sink: Introduce a requests tracking queue

[Umang Jain]

Currently the queue depth tracking DRM completed requests is
effectively 2, via queued_ and pending_ class members in KMSSink.
This patch introduces a queue which can track more requests thus giving
a higher queue depth.

The reason to introduce a higher queue depth is to avoid use-after-free
on KMSSink::stop() in cases where KMSSink class is frequently operated
under: start() -> configure() -> stop() cycles. As soon as the
KMSSink::stop() is called, it used to free the queued_ and pending_
requests, but a DRM request can still complete asynchronously (and
after the queued_ and pending_ are freed). This led to use-after-free
segfault in Device::pageFlipComplete() while emitting the
`requestComplete` signal on a (already freed) request.

In the design introduced in this patch, the requests already in the queue
are marked as 'expired' and not freed in KMSSink::stop(). This prevents
the use-after-free segfault in Device::pageFlipComplete(). The expired
requests are dropped from the queue when new requests come into the
queue and gets completed in the KMSSink::requestComplete() handler.

Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
---
 src/apps/cam/kms_sink.cpp | 73 +++++++++++++++++++++------------------
 src/apps/cam/kms_sink.h   | 11 +++---
 2 files changed, 47 insertions(+), 37 deletions(-)

diff --git a/src/apps/cam/kms_sink.cpp b/src/apps/cam/kms_sink.cpp
index 2aefec06..8305e6de 100644
--- a/src/apps/cam/kms_sink.cpp
+++ b/src/apps/cam/kms_sink.cpp
@@ -24,7 +24,8 @@
 #include "drm.h"
 
 KMSSink::KMSSink(const std::string &connectorName)
-	: connector_(nullptr), crtc_(nullptr), plane_(nullptr), mode_(nullptr)
+	: connector_(nullptr), crtc_(nullptr), plane_(nullptr), mode_(nullptr),
+	  firstFrame_(false)
 {
 	int ret = dev_.init();
 	if (ret < 0)
@@ -327,6 +328,8 @@ int KMSSink::start()
 
 	dev_.requestComplete.connect(this, &KMSSink::requestComplete);
 
+	firstFrame_ = true;
+
 	return 0;
 }
 
@@ -334,6 +337,13 @@ int KMSSink::stop()
 {
 	dev_.requestComplete.disconnect();
 
+	{
+		std::lock_guard<std::mutex> lock(lock_);
+		/* Expire all the DRM requests in the queue */
+		for (std::unique_ptr<Request> &req : requests_)
+			req->expired_ = true;
+	}
+
 	/* Display pipeline. */
 	DRM::AtomicRequest request(&dev_);
 
@@ -352,9 +362,6 @@ int KMSSink::stop()
 	}
 
 	/* Free all buffers. */
-	pending_.reset();
-	queued_.reset();
-	active_.reset();
 	buffers_.clear();
 
 	return FrameSink::stop();
@@ -450,13 +457,6 @@ bool KMSSink::setupComposition(DRM::FrameBuffer *drmBuffer)
 
 bool KMSSink::processRequest(libcamera::Request *camRequest)
 {
-	/*
-	 * Perform a very crude rate adaptation by simply dropping the request
-	 * if the display queue is full.
-	 */
-	if (pending_)
-		return true;
-
 	libcamera::FrameBuffer *buffer = camRequest->buffers().begin()->second;
 	auto iter = buffers_.find(buffer);
 	if (iter == buffers_.end())
@@ -469,7 +469,7 @@ bool KMSSink::processRequest(libcamera::Request *camRequest)
 		std::make_unique<DRM::AtomicRequest>(&dev_);
 	drmRequest->addProperty(plane_, "FB_ID", drmBuffer->id());
 
-	if (!active_ && !queued_) {
+	if (firstFrame_) {
 		/* Enable the display pipeline on the first frame. */
 		if (!setupComposition(drmBuffer)) {
 			std::cerr << "Failed to setup composition" << std::endl;
@@ -497,22 +497,22 @@ bool KMSSink::processRequest(libcamera::Request *camRequest)
 			drmRequest->addProperty(plane_, "COLOR_RANGE", *colorRange_);
 
 		flags |= DRM::AtomicRequest::FlagAllowModeset;
+		firstFrame_ = false;
 	}
 
-	pending_ = std::make_unique<Request>(std::move(drmRequest), camRequest);
+	std::unique_ptr<Request> pending =
+		std::make_unique<Request>(std::move(drmRequest), camRequest);
 
 	std::lock_guard<std::mutex> lock(lock_);
 
-	if (!queued_) {
-		int ret = pending_->drmRequest_->commit(flags);
-		if (ret < 0) {
-			std::cerr
-				<< "Failed to commit atomic request: "
-				<< strerror(-ret) << std::endl;
-			/* \todo Implement error handling */
-		}
-
-		queued_ = std::move(pending_);
+	int ret = pending->drmRequest_->commit(flags);
+	if (ret < 0) {
+		std::cerr
+			<< "Failed to commit atomic request: "
+			<< strerror(-ret) << std::endl;
+		/* \todo Implement error handling */
+	} else {
+		requests_.push_back(std::move(pending));
 	}
 
 	return false;
@@ -522,18 +522,25 @@ void KMSSink::requestComplete([[maybe_unused]] DRM::AtomicRequest *request)
 {
 	std::lock_guard<std::mutex> lock(lock_);
 
-	assert(queued_ && queued_->drmRequest_.get() == request);
+	std::unique_ptr<Request> &headReq = requests_.front();
 
-	/* Complete the active request, if any. */
-	if (active_)
-		requestProcessed.emit(active_->camRequest_);
+	assert(headReq->drmRequest_.get() == request);
 
-	/* The queued request becomes active. */
-	active_ = std::move(queued_);
+	if (!headReq->expired_) {
+		requestProcessed.emit(headReq->camRequest_);
+		requests_.pop_front();
+	} else {
+		/* Remove candidates which are expired */
+		while (requests_.size() > 0) {
+			if (requests_.front()->expired_)
+				requests_.pop_front();
+			else
+				break;
+		}
 
-	/* Queue the pending request, if any. */
-	if (pending_) {
-		pending_->drmRequest_->commit(DRM::AtomicRequest::FlagAsync);
-		queued_ = std::move(pending_);
+		return;
 	}
+
+	if (requests_.size())
+		requests_.front()->drmRequest_->commit(DRM::AtomicRequest::FlagAsync);
 }
diff --git a/src/apps/cam/kms_sink.h b/src/apps/cam/kms_sink.h
index e2c618a1..a6b418aa 100644
--- a/src/apps/cam/kms_sink.h
+++ b/src/apps/cam/kms_sink.h
@@ -7,6 +7,7 @@
 
 #pragma once
 
+#include <deque>
 #include <list>
 #include <memory>
 #include <mutex>
@@ -41,12 +42,14 @@ private:
 	public:
 		Request(std::unique_ptr<DRM::AtomicRequest> drmRequest,
 			libcamera::Request *camRequest)
-			: drmRequest_(std::move(drmRequest)), camRequest_(camRequest)
+			: drmRequest_(std::move(drmRequest)), camRequest_(camRequest),
+			  expired_(false)
 		{
 		}
 
 		std::unique_ptr<DRM::AtomicRequest> drmRequest_;
 		libcamera::Request *camRequest_;
+		bool expired_;
 	};
 
 	int selectPipeline(const libcamera::PixelFormat &format);
@@ -76,8 +79,8 @@ private:
 
 	std::map<libcamera::FrameBuffer *, std::unique_ptr<DRM::FrameBuffer>> buffers_;
 
+	bool firstFrame_;
+
 	std::mutex lock_;
-	std::unique_ptr<Request> pending_;
-	std::unique_ptr<Request> queued_;
-	std::unique_ptr<Request> active_;
+	std::deque<std::unique_ptr<Request>> requests_;
 };
-- 
2.39.1