backends/drm: remove the race condition in DrmAtomicCommit

There is a chance that DrmGpu::pageFlipHandler gets executed on the main
thread while drmModeAtomicCommit hasn't returned yet on the commit thread.
When this situation happens, the destructor of DrmAtomicCommit doesn't set
the buffers to be current for the relevant drm planes, and once it gets
deleted, the framebuffers get destroyed and the output turns off.
To prevent that from happening, this commit moves the relevant code to a
method that gets explicitly called from the pageflip handler.

src/backends/drm/drm_commit.cpp

@@ -28,6 +28,7 @@ DrmCommit::DrmCommit(DrmGpu *gpu)
 
 DrmCommit::~DrmCommit()
 {
+    Q_ASSERT(QThread::currentThread() == QApplication::instance()->thread());
 }
 
 DrmGpu *DrmCommit::gpu() const
@@ -35,22 +36,13 @@ DrmGpu *DrmCommit::gpu() const
     return m_gpu;
 }
 
-DrmAtomicCommit::DrmAtomicCommit(DrmGpu *gpu)
-    : DrmCommit(gpu)
+DrmAtomicCommit::DrmAtomicCommit(const QVector<DrmPipeline *> &pipelines)
+    : DrmCommit(pipelines.front()->gpu())
+    , m_pipelines(pipelines)
     , m_req(drmModeAtomicAlloc())
 {
 }
 
-DrmAtomicCommit::~DrmAtomicCommit()
-{
-    Q_ASSERT(QThread::currentThread() == QApplication::instance()->thread());
-    if (m_commitSuccessful) {
-        for (const auto &[plane, buffer] : m_buffers) {
-            plane->setCurrentBuffer(buffer);
-        }
-    }
-}
-
 void DrmAtomicCommit::addProperty(const DrmProperty &prop, uint64_t value)
 {
     drmModeAtomicAddProperty(m_req.get(), prop.drmObject()->id(), prop.propId(), value);
@@ -70,24 +62,22 @@ void DrmAtomicCommit::addBuffer(DrmPlane *plane, const std::shared_ptr<DrmFrameb
 
 bool DrmAtomicCommit::test()
 {
-    return drmModeAtomicCommit(m_gpu->fd(), m_req.get(), DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_NONBLOCK, this) == 0;
+    return drmModeAtomicCommit(gpu()->fd(), m_req.get(), DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_NONBLOCK, this) == 0;
 }
 
 bool DrmAtomicCommit::testAllowModeset()
 {
-    return drmModeAtomicCommit(m_gpu->fd(), m_req.get(), DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, this) == 0;
+    return drmModeAtomicCommit(gpu()->fd(), m_req.get(), DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, this) == 0;
 }
 
 bool DrmAtomicCommit::commit()
 {
-    m_commitSuccessful = (drmModeAtomicCommit(m_gpu->fd(), m_req.get(), DRM_MODE_ATOMIC_NONBLOCK | DRM_MODE_PAGE_FLIP_EVENT, this) == 0);
-    return m_commitSuccessful;
+    return drmModeAtomicCommit(gpu()->fd(), m_req.get(), DRM_MODE_ATOMIC_NONBLOCK | DRM_MODE_PAGE_FLIP_EVENT, this) == 0;
 }
 
 bool DrmAtomicCommit::commitModeset()
 {
-    m_commitSuccessful = (drmModeAtomicCommit(m_gpu->fd(), m_req.get(), DRM_MODE_ATOMIC_ALLOW_MODESET, this) == 0);
-    return m_commitSuccessful;
+    return drmModeAtomicCommit(gpu()->fd(), m_req.get(), DRM_MODE_ATOMIC_ALLOW_MODESET, this) == 0;
 }
 
 drmModeAtomicReq *DrmAtomicCommit::req() const
@@ -95,32 +85,44 @@ drmModeAtomicReq *DrmAtomicCommit::req() const
     return m_req.get();
 }
 
-DrmLegacyCommit::DrmLegacyCommit(DrmCrtc *crtc, const std::shared_ptr<DrmFramebuffer> &buffer)
-    : DrmCommit(crtc->gpu())
-    , m_crtc(crtc)
-    , m_buffer(buffer)
-{
-}
-
-DrmLegacyCommit::~DrmLegacyCommit()
+void DrmAtomicCommit::pageFlipped(std::chrono::nanoseconds timestamp) const
 {
     Q_ASSERT(QThread::currentThread() == QApplication::instance()->thread());
-    if (m_success) {
-        m_crtc->setCurrent(m_buffer);
+    for (const auto &[plane, buffer] : m_buffers) {
+        plane->setCurrentBuffer(buffer);
     }
+    for (const auto &pipeline : m_pipelines) {
+        pipeline->pageFlipped(timestamp);
+    }
+}
+
+DrmLegacyCommit::DrmLegacyCommit(DrmPipeline *pipeline, const std::shared_ptr<DrmFramebuffer> &buffer)
+    : DrmCommit(pipeline->gpu())
+    , m_pipeline(pipeline)
+    , m_buffer(buffer)
+{
 }
 
 bool DrmLegacyCommit::doModeset(DrmConnector *connector, DrmConnectorMode *mode)
 {
     uint32_t connectorId = connector->id();
-    m_success = (drmModeSetCrtc(gpu()->fd(), m_crtc->id(), m_buffer->framebufferId(), 0, 0, &connectorId, 1, mode->nativeMode()) == 0);
-    return m_success;
+    if (drmModeSetCrtc(gpu()->fd(), m_pipeline->crtc()->id(), m_buffer->framebufferId(), 0, 0, &connectorId, 1, mode->nativeMode()) == 0) {
+        pageFlipped(std::chrono::steady_clock::now().time_since_epoch());
+        return true;
+    } else {
+        return false;
+    }
 }
 
 bool DrmLegacyCommit::doPageflip(uint32_t flags)
 {
-    m_success = (drmModePageFlip(gpu()->fd(), m_crtc->id(), m_buffer->framebufferId(), flags, this) == 0);
-    return m_success;
+    return drmModePageFlip(gpu()->fd(), m_pipeline->crtc()->id(), m_buffer->framebufferId(), flags, this) == 0;
 }
 
+void DrmLegacyCommit::pageFlipped(std::chrono::nanoseconds timestamp) const
+{
+    Q_ASSERT(QThread::currentThread() == QApplication::instance()->thread());
+    m_pipeline->crtc()->setCurrent(m_buffer);
+    m_pipeline->pageFlipped(timestamp);
+}
 }

src/backends/drm/drm_commit.h

@@ -12,6 +12,7 @@
 #include <xf86drmMode.h>
 
 #include <QHash>
+#include <chrono>
 #include <unordered_map>
 
 #include "drm_pointer.h"
@@ -28,6 +29,7 @@ class DrmFramebuffer;
 class DrmGpu;
 class DrmPlane;
 class DrmProperty;
+class DrmPipeline;
 
 class DrmCommit
 {
@@ -35,6 +37,7 @@ public:
     virtual ~DrmCommit();
 
     DrmGpu *gpu() const;
+    virtual void pageFlipped(std::chrono::nanoseconds timestamp) const = 0;
 
 protected:
     DrmCommit(DrmGpu *gpu);
@@ -45,8 +48,7 @@ protected:
 class DrmAtomicCommit : public DrmCommit
 {
 public:
-    DrmAtomicCommit(DrmGpu *gpu);
-    ~DrmAtomicCommit() override;
+    DrmAtomicCommit(const QVector<DrmPipeline *> &pipelines);
 
     void addProperty(const DrmProperty &prop, uint64_t value);
     template<typename T>
@@ -62,28 +64,29 @@ public:
     bool commit();
     bool commitModeset();
 
+    void pageFlipped(std::chrono::nanoseconds timestamp) const override;
+
     drmModeAtomicReq *req() const;
 
 private:
+    const QVector<DrmPipeline *> m_pipelines;
     DrmUniquePtr<drmModeAtomicReq> m_req;
     QHash<const DrmProperty *, std::shared_ptr<DrmBlob>> m_blobs;
     std::unordered_map<DrmPlane *, std::shared_ptr<DrmFramebuffer>> m_buffers;
-    bool m_commitSuccessful = false;
 };
 
 class DrmLegacyCommit : public DrmCommit
 {
 public:
-    DrmLegacyCommit(DrmCrtc *crtc, const std::shared_ptr<DrmFramebuffer> &buffer);
-    ~DrmLegacyCommit() override;
+    DrmLegacyCommit(DrmPipeline *pipeline, const std::shared_ptr<DrmFramebuffer> &buffer);
 
     bool doModeset(DrmConnector *connector, DrmConnectorMode *mode);
     bool doPageflip(uint32_t flags);
+    void pageFlipped(std::chrono::nanoseconds timestamp) const override;
 
 private:
-    DrmCrtc *const m_crtc;
+    DrmPipeline *const m_pipeline;
     const std::shared_ptr<DrmFramebuffer> m_buffer;
-    bool m_success = false;
 };
 
 }

src/backends/drm/drm_gpu.cpp

@@ -555,15 +555,7 @@ void DrmGpu::pageFlipHandler(int fd, unsigned int sequence, unsigned int sec, un
                 sec, usec, qPrintable(gpu->devNode()));
         timestamp = std::chrono::steady_clock::now().time_since_epoch();
     }
-    const auto pipelines = gpu->pipelines();
-    auto it = std::find_if(pipelines.begin(), pipelines.end(), [crtc_id](const auto &pipeline) {
-        return pipeline->currentCrtc() && pipeline->currentCrtc()->id() == crtc_id;
-    });
-    if (it == pipelines.end()) {
-        qCWarning(KWIN_DRM, "received invalid page flip event for crtc %u", crtc_id);
-    } else {
-        (*it)->pageFlipped(timestamp);
-    }
+    commit->pageFlipped(timestamp);
     delete commit;
 }
 

src/backends/drm/drm_pipeline.cpp

@@ -107,7 +107,7 @@ DrmPipeline::Error DrmPipeline::commitPipelines(const QVector<DrmPipeline *> &pi
 
 DrmPipeline::Error DrmPipeline::commitPipelinesAtomic(const QVector<DrmPipeline *> &pipelines, CommitMode mode, const QVector<DrmObject *> &unusedObjects)
 {
-    auto commit = std::make_unique<DrmAtomicCommit>(pipelines.front()->gpu());
+    auto commit = std::make_unique<DrmAtomicCommit>(pipelines);
     for (const auto &pipeline : pipelines) {
         if (pipeline->activePending()) {
             if (!pipeline->m_pending.layer->checkTestBuffer()) {
@@ -145,12 +145,13 @@ DrmPipeline::Error DrmPipeline::commitPipelinesAtomic(const QVector<DrmPipeline
         // The kernel fails commits with DRM_MODE_PAGE_FLIP_EVENT when a crtc is disabled in the commit
         // and already was disabled before, to work around some quirks in old userspace.
         // Instead of using DRM_MODE_PAGE_FLIP_EVENT | DRM_MODE_ATOMIC_NONBLOCK, do the modeset in a blocking
-        // fashion without page flip events
+        // fashion without page flip events and trigger the pageflip notification directly
         if (!commit->commitModeset()) {
             qCCritical(KWIN_DRM) << "Atomic modeset commit failed!" << strerror(errno);
             return errnoToError();
         }
-        std::for_each(pipelines.begin(), pipelines.end(), std::mem_fn(&DrmPipeline::atomicModesetSuccessful));
+        std::for_each(pipelines.begin(), pipelines.end(), std::mem_fn(&DrmPipeline::atomicCommitSuccessful));
+        commit->pageFlipped(std::chrono::steady_clock::now().time_since_epoch());
         return Error::None;
     }
     case CommitMode::Test: {
@@ -346,21 +347,10 @@ DrmPipeline::Error DrmPipeline::errnoToError()
 void DrmPipeline::atomicCommitSuccessful()
 {
     m_pending.needsModeset = false;
-    if (activePending()) {
-        m_pageflipPending = true;
-    }
+    m_pageflipPending = true;
     m_current = m_pending;
 }
 
-void DrmPipeline::atomicModesetSuccessful()
-{
-    atomicCommitSuccessful();
-    m_pending.needsModeset = false;
-    if (activePending()) {
-        pageFlipped(std::chrono::steady_clock::now().time_since_epoch());
-    }
-}
-
 bool DrmPipeline::setCursor(const QPoint &hotspot)
 {
     bool result;
@@ -420,7 +410,7 @@ DrmGpu *DrmPipeline::gpu() const
 void DrmPipeline::pageFlipped(std::chrono::nanoseconds timestamp)
 {
     m_pageflipPending = false;
-    if (m_output) {
+    if (m_output && activePending()) {
         m_output->pageFlipped(timestamp);
     }
 }

src/backends/drm/drm_pipeline.h

@@ -157,7 +157,6 @@ private:
 
     // atomic modesetting only
     void atomicCommitSuccessful();
-    void atomicModesetSuccessful();
     bool prepareAtomicModeset(DrmAtomicCommit *commit);
     bool prepareAtomicPresentation(DrmAtomicCommit *commit);
     void prepareAtomicDisable(DrmAtomicCommit *commit);

src/backends/drm/drm_pipeline_legacy.cpp

@@ -36,7 +36,7 @@ DrmPipeline::Error DrmPipeline::presentLegacy()
     if (m_pending.syncMode == RenderLoopPrivate::SyncMode::Async || m_pending.syncMode == RenderLoopPrivate::SyncMode::AdaptiveAsync) {
         flags |= DRM_MODE_PAGE_FLIP_ASYNC;
     }
-    auto commit = std::make_unique<DrmLegacyCommit>(m_pending.crtc, buffer);
+    auto commit = std::make_unique<DrmLegacyCommit>(this, buffer);
     if (!commit->doPageflip(flags)) {
         qCWarning(KWIN_DRM) << "Page flip failed:" << strerror(errno);
         return errnoToError();
@@ -57,7 +57,7 @@ DrmPipeline::Error DrmPipeline::legacyModeset()
     if (!m_pending.layer->checkTestBuffer()) {
         return Error::TestBufferFailed;
     }
-    auto commit = std::make_unique<DrmLegacyCommit>(m_pending.crtc, m_pending.layer->currentBuffer());
+    auto commit = std::make_unique<DrmLegacyCommit>(this, m_pending.layer->currentBuffer());
     if (!commit->doModeset(m_connector, m_pending.mode.get())) {
         qCWarning(KWIN_DRM) << "Modeset failed!" << strerror(errno);
         return errnoToError();