From patchwork Sun Apr 23 20:39:31 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Umang Jain X-Patchwork-Id: 18546 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 5AC11C32A5 for ; Sun, 23 Apr 2023 20:39:47 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 9E16A627E1; Sun, 23 Apr 2023 22:39:46 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org; s=mail; t=1682282386; bh=yjvqSOgsailu/1FTDCgOq4WLw5lm2RyFZx68MNvNTNI=; h=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=Q92CgDSu4/3ujJU1L80udP7zTy59Y+pgSHfhDz7jDIXBoiYkBWn+BlAt9p5PBCVCE es1D8X3avCzR4WC0Z/EgKGeD//E/5oEXcMCf+tccLiotvcSVYnKZxtYKrjlBaS1i7p hviQwg6UHtMBDR0m5KyNxUPfYt57PK/J39QA5YQYu2sPHeTAwW0j/9ftmCxpCoYiWt uU+yLoc9uiv05Mej+zcSksrgrx5CFtI43Yjy++46PjbSgfoOoyWc9yRi0+TEPxhnaF NAjKyrDTci7Iu+Xfr5vwjEv6y2EbsvPfrz2TJ583SOAVvdH/qMPtnmKbRq16qO3ZEB vB/ZJMJ+gIVJw== Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id F2FE8627CF for ; Sun, 23 Apr 2023 22:39:42 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="mNrPUWy8"; dkim-atps=neutral Received: from umang.jainideasonboard.com (unknown [IPv6:2401:4900:1f3f:df01:2ad:735a:b54c:741d]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id ABE79DFD; Sun, 23 Apr 2023 22:39:32 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1682282373; bh=yjvqSOgsailu/1FTDCgOq4WLw5lm2RyFZx68MNvNTNI=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=mNrPUWy8GN0a/EFSjy6OMmKgcAX23M7eNNvJRXmU6iL0e+bn3EUaW3ttlqTZmB+wN YMy26ANoU1QnajFiDkQI6fFQcNSWOd/m8Z80x0FPJXpVSeDinxk2TmCW/zT+D/H0jZ wIGzUeXKNfeBvw4JEqLNS8w0fv+CfqZz7nAnEzXI= To: libcamera-devel@lists.libcamera.org Date: Mon, 24 Apr 2023 02:09:31 +0530 Message-Id: <20230423203931.108022-4-umang.jain@ideasonboard.com> X-Mailer: git-send-email 2.39.1 In-Reply-To: <20230423203931.108022-1-umang.jain@ideasonboard.com> References: <20230423203931.108022-1-umang.jain@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 3/3] apps: cam: kms_sink: Introduce a requests tracking queue X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Umang Jain via libcamera-devel From: Umang Jain Reply-To: Umang Jain Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" 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 --- 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 lock(lock_); + /* Expire all the DRM requests in the queue */ + for (std::unique_ptr &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(&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(std::move(drmRequest), camRequest); + std::unique_ptr pending = + std::make_unique(std::move(drmRequest), camRequest); std::lock_guard 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 lock(lock_); - assert(queued_ && queued_->drmRequest_.get() == request); + std::unique_ptr &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 #include #include #include @@ -41,12 +42,14 @@ private: public: Request(std::unique_ptr drmRequest, libcamera::Request *camRequest) - : drmRequest_(std::move(drmRequest)), camRequest_(camRequest) + : drmRequest_(std::move(drmRequest)), camRequest_(camRequest), + expired_(false) { } std::unique_ptr drmRequest_; libcamera::Request *camRequest_; + bool expired_; }; int selectPipeline(const libcamera::PixelFormat &format); @@ -76,8 +79,8 @@ private: std::map> buffers_; + bool firstFrame_; + std::mutex lock_; - std::unique_ptr pending_; - std::unique_ptr queued_; - std::unique_ptr active_; + std::deque> requests_; };