[{"id":26923,"web_url":"https://patchwork.libcamera.org/comment/26923/","msgid":"<20230424012206.GQ21943@pendragon.ideasonboard.com>","date":"2023-04-24T01:22:06","subject":"Re: [libcamera-devel] [PATCH 3/3] apps: cam: kms_sink: Introduce a\n\trequests tracking queue","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Mon, Apr 24, 2023 at 02:09:31AM +0530, Umang Jain via libcamera-devel wrote:\n> Currently the queue depth tracking DRM completed requests is\n> effectively 2, via queued_ and pending_ class members in KMSSink.\n> This patch introduces a queue which can track more requests thus giving\n> a higher queue depth.\n> \n> The reason to introduce a higher queue depth is to avoid use-after-free\n> on KMSSink::stop() in cases where KMSSink class is frequently operated\n> under: start() -> configure() -> stop() cycles. As soon as the\n> KMSSink::stop() is called, it used to free the queued_ and pending_\n> requests, but a DRM request can still complete asynchronously (and\n> after the queued_ and pending_ are freed). This led to use-after-free\n> segfault in Device::pageFlipComplete() while emitting the\n> `requestComplete` signal on a (already freed) request.\n> \n> In the design introduced in this patch, the requests already in the queue\n> are marked as 'expired' and not freed in KMSSink::stop(). This prevents\n> the use-after-free segfault in Device::pageFlipComplete(). The expired\n> requests are dropped from the queue when new requests come into the\n> queue and gets completed in the KMSSink::requestComplete() handler.\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/apps/cam/kms_sink.cpp | 73 +++++++++++++++++++++------------------\n> src/apps/cam/kms_sink.h | 11 +++---\n> 2 files changed, 47 insertions(+), 37 deletions(-)\n> \n> diff --git a/src/apps/cam/kms_sink.cpp b/src/apps/cam/kms_sink.cpp\n> index 2aefec06..8305e6de 100644\n> --- a/src/apps/cam/kms_sink.cpp\n> +++ b/src/apps/cam/kms_sink.cpp\n> @@ -24,7 +24,8 @@\n> #include \"drm.h\"\n> \n> KMSSink::KMSSink(const std::string &connectorName)\n> -\t: connector_(nullptr), crtc_(nullptr), plane_(nullptr), mode_(nullptr)\n> +\t: connector_(nullptr), crtc_(nullptr), plane_(nullptr), mode_(nullptr),\n> +\t firstFrame_(false)\n> {\n> \tint ret = dev_.init();\n> \tif (ret < 0)\n> @@ -327,6 +328,8 @@ int KMSSink::start()\n> \n> \tdev_.requestComplete.connect(this, &KMSSink::requestComplete);\n> \n> +\tfirstFrame_ = true;\n> +\n> \treturn 0;\n> }\n> \n> @@ -334,6 +337,13 @@ int KMSSink::stop()\n> {\n> \tdev_.requestComplete.disconnect();\n> \n> +\t{\n> +\t\tstd::lock_guard lock(lock_);\n> +\t\t/* Expire all the DRM requests in the queue */\n> +\t\tfor (std::unique_ptr &req : requests_)\n> +\t\t\treq->expired_ = true;\n> +\t}\n> +\n> \t/* Display pipeline. */\n> \tDRM::AtomicRequest request(&dev_);\n> \n> @@ -352,9 +362,6 @@ int KMSSink::stop()\n> \t}\n> \n> \t/* Free all buffers. */\n> -\tpending_.reset();\n> -\tqueued_.reset();\n> -\tactive_.reset();\n> \tbuffers_.clear();\n> \n> \treturn FrameSink::stop();\n> @@ -450,13 +457,6 @@ bool KMSSink::setupComposition(DRM::FrameBuffer *drmBuffer)\n> \n> bool KMSSink::processRequest(libcamera::Request *camRequest)\n> {\n> -\t/*\n> -\t * Perform a very crude rate adaptation by simply dropping the request\n> -\t * if the display queue is full.\n> -\t */\n> -\tif (pending_)\n> -\t\treturn true;\n\nThe KMS API doesn't support queue depth larger than 1. This is why the\nKMSSink doesn't use a larger queue. I don't recall what happens if you\ntry to queue more commits than KMS can handle, whether the atomic commit\nioctl will fail, or if it will wait synchronously for the previous\ncommit to complete. In either case, that's not right. KMSSink needs to\nensure that it doesn't overflow the (small) KMS commit queue.\n\n> -\n> \tlibcamera::FrameBuffer *buffer = camRequest->buffers().begin()->second;\n> \tauto iter = buffers_.find(buffer);\n> \tif (iter == buffers_.end())\n> @@ -469,7 +469,7 @@ bool KMSSink::processRequest(libcamera::Request *camRequest)\n> \t\tstd::make_unique(&dev_);\n> \tdrmRequest->addProperty(plane_, \"FB_ID\", drmBuffer->id());\n> \n> -\tif (!active_ && !queued_) {\n> +\tif (firstFrame_) {\n> \t\t/* Enable the display pipeline on the first frame. */\n> \t\tif (!setupComposition(drmBuffer)) {\n> \t\t\tstd::cerr << \"Failed to setup composition\" << std::endl;\n> @@ -497,22 +497,22 @@ bool KMSSink::processRequest(libcamera::Request *camRequest)\n> \t\t\tdrmRequest->addProperty(plane_, \"COLOR_RANGE\", *colorRange_);\n> \n> \t\tflags |= DRM::AtomicRequest::FlagAllowModeset;\n> +\t\tfirstFrame_ = false;\n> \t}\n> \n> -\tpending_ = std::make_unique(std::move(drmRequest), camRequest);\n> +\tstd::unique_ptr pending =\n> +\t\tstd::make_unique(std::move(drmRequest), camRequest);\n> \n> \tstd::lock_guard lock(lock_);\n> \n> -\tif (!queued_) {\n> -\t\tint ret = pending_->drmRequest_->commit(flags);\n> -\t\tif (ret < 0) {\n> -\t\t\tstd::cerr\n> -\t\t\t\t<< \"Failed to commit atomic request: \"\n> -\t\t\t\t<< strerror(-ret) << std::endl;\n> -\t\t\t/* \\todo Implement error handling */\n> -\t\t}\n> -\n> -\t\tqueued_ = std::move(pending_);\n> +\tint ret = pending->drmRequest_->commit(flags);\n> +\tif (ret < 0) {\n> +\t\tstd::cerr\n> +\t\t\t<< \"Failed to commit atomic request: \"\n> +\t\t\t<< strerror(-ret) << std::endl;\n> +\t\t/* \\todo Implement error handling */\n> +\t} else {\n> +\t\trequests_.push_back(std::move(pending));\n> \t}\n> \n> \treturn false;\n> @@ -522,18 +522,25 @@ void KMSSink::requestComplete([[maybe_unused]] DRM::AtomicRequest *request)\n> {\n> \tstd::lock_guard lock(lock_);\n> \n> -\tassert(queued_ && queued_->drmRequest_.get() == request);\n> +\tstd::unique_ptr &headReq = requests_.front();\n> \n> -\t/* Complete the active request, if any. */\n> -\tif (active_)\n> -\t\trequestProcessed.emit(active_->camRequest_);\n> +\tassert(headReq->drmRequest_.get() == request);\n> \n> -\t/* The queued request becomes active. */\n> -\tactive_ = std::move(queued_);\n> +\tif (!headReq->expired_) {\n> +\t\trequestProcessed.emit(headReq->camRequest_);\n> +\t\trequests_.pop_front();\n> +\t} else {\n> +\t\t/* Remove candidates which are expired */\n> +\t\twhile (requests_.size() > 0) {\n> +\t\t\tif (requests_.front()->expired_)\n> +\t\t\t\trequests_.pop_front();\n> +\t\t\telse\n> +\t\t\t\tbreak;\n> +\t\t}\n> \n> -\t/* Queue the pending request, if any. */\n> -\tif (pending_) {\n> -\t\tpending_->drmRequest_->commit(DRM::AtomicRequest::FlagAsync);\n> -\t\tqueued_ = std::move(pending_);\n> +\t\treturn;\n> \t}\n> +\n> +\tif (requests_.size())\n> +\t\trequests_.front()->drmRequest_->commit(DRM::AtomicRequest::FlagAsync);\n> }\n> diff --git a/src/apps/cam/kms_sink.h b/src/apps/cam/kms_sink.h\n> index e2c618a1..a6b418aa 100644\n> --- a/src/apps/cam/kms_sink.h\n> +++ b/src/apps/cam/kms_sink.h\n> @@ -7,6 +7,7 @@\n> \n> #pragma once\n> \n> +#include \n> #include \n> #include \n> #include \n> @@ -41,12 +42,14 @@ private:\n> \tpublic:\n> \t\tRequest(std::unique_ptr drmRequest,\n> \t\t\tlibcamera::Request *camRequest)\n> -\t\t\t: drmRequest_(std::move(drmRequest)), camRequest_(camRequest)\n> +\t\t\t: drmRequest_(std::move(drmRequest)), camRequest_(camRequest),\n> +\t\t\t expired_(false)\n> \t\t{\n> \t\t}\n> \n> \t\tstd::unique_ptr drmRequest_;\n> \t\tlibcamera::Request *camRequest_;\n> +\t\tbool expired_;\n> \t};\n> \n> \tint selectPipeline(const libcamera::PixelFormat &format);\n> @@ -76,8 +79,8 @@ private:\n> \n> \tstd::map> buffers_;\n> \n> +\tbool firstFrame_;\n> +\n> \tstd::mutex lock_;\n> -\tstd::unique_ptr pending_;\n> -\tstd::unique_ptr queued_;\n> -\tstd::unique_ptr active_;\n> +\tstd::deque> requests_;\n> };","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6F709BE173\n\tfor ;\n\tMon, 24 Apr 2023 01:21:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CF5EF627CF;\n\tMon, 24 Apr 2023 03:21:54 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5BA45603A0\n\tfor ;\n\tMon, 24 Apr 2023 03:21:53 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(133-32-181-51.west.xps.vectant.ne.jp [133.32.181.51])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 02A16326;\n\tMon, 24 Apr 2023 03:21:42 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682299314;\n\tbh=r+6NIDgZBM5Kt2PnRYXdgWVXTi+7amWgyth3Hf66Lw8=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=HlmhghK8VNiSFMOsFB9ORjp1c/iJ9KXfuZv25ULpHyiq1xUflf3kPVqZGoOt60NjM\n\tLg5Kh+FIVGYX8t29vGTWFezvQ/EBFkzJYIDjZNLGh0Nqbx4dFJH+lB2Kq7do2D0Hmy\n\tKu/z6l4W4JHm2DSAm33ckhueEaani/0UiOnNXvvZZc2qcSBpg0+J4mA1QSXtuQU5As\n\tM0OuFuDvDjkWEu4ivZMx26KnTabHDJcvqAem+TYsNKZciPo1YBTYsRgn5w+JSvpzCL\n\tVePtD6/wwHHp3hfrmfE1lraSRYeppUnSdpRfXVqX5eRtxs0SzUwEZpuFEQL96q5ZBZ\n\tsx10xRNvJ3scQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1682299303;\n\tbh=r+6NIDgZBM5Kt2PnRYXdgWVXTi+7amWgyth3Hf66Lw8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uPjFkFCl04bGmLpE8XubCwHDMM+yqb+f9iFBTRQn3WVmkbSjWU+SnXJrg2QA0amx/\n\tnCAIkjPQydlui8VEQsV2eFMGwsmaw0YJQtX2vOe6RsYAwZBkoUyrUIfJ1pYFW5UWuw\n\t2xNfhesMofmkQtOxhtcUF4rztPfl7H4oStPRsuow="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"uPjFkFCl\"; dkim-atps=neutral","Date":"Mon, 24 Apr 2023 04:22:06 +0300","To":"Umang Jain ","Message-ID":"<20230424012206.GQ21943@pendragon.ideasonboard.com>","References":"<20230423203931.108022-1-umang.jain@ideasonboard.com>\n\t<20230423203931.108022-4-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230423203931.108022-4-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 3/3] apps: cam: kms_sink: Introduce a\n\trequests tracking queue","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]