| Message ID | 20230423203931.108022-4-umang.jain@ideasonboard.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, Thank you for the patch. On Mon, Apr 24, 2023 at 02:09:31AM +0530, Umang Jain via libcamera-devel wrote: > 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@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; The KMS API doesn't support queue depth larger than 1. This is why the KMSSink doesn't use a larger queue. I don't recall what happens if you try to queue more commits than KMS can handle, whether the atomic commit ioctl will fail, or if it will wait synchronously for the previous commit to complete. In either case, that's not right. KMSSink needs to ensure that it doesn't overflow the (small) KMS commit queue. > - > 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_; > };
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_; };
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@ideasonboard.com> --- src/apps/cam/kms_sink.cpp | 73 +++++++++++++++++++++------------------ src/apps/cam/kms_sink.h | 11 +++--- 2 files changed, 47 insertions(+), 37 deletions(-)