[platforms/drm] Use a shared pointer for gbm_surface

Summary:
The gbm_surface is owned by the EglGbmBackend, but it's not the only one
using it. The DrmSurfaceBuffer is also using it and needs it to destroy
the gbm_bo. Now this can become a problem in the following situation:

* a page flip is still pending
* the EglGbmBackend destroys the gbm_surface

-> when the page flip happens the DrmSurfaceBuffer will try to destroy
the gbm_bo and crash as the gbm_surface is no longer valid. This
situation can happen when switching screens or when switching compositing
backend (OpenGL 2 -> OpenGL 3).

To address this problem a class GbmSurface is added which wrapps the
gbm_surface pointer. The EglGbmBackend creates and holds a shared pointer
to the GbmSurface and passes that one to the DrmSurfaceBuffer. So when
cleaning up the gbm_surface only the shared pointer is reset and in case
the DrmSurfaceBuffer still needs it, it can access it without problems.

BUG: 385372
FIXED-IN: 5.11.0

Test Plan: Not yet

Reviewers: #kwin, #plasma, subdiff

Subscribers: plasma-devel, kwin

Tags: #kwin

Differential Revision: https://phabricator.kde.org/D8152

autotests/CMakeLists.txt

@@ -404,3 +404,10 @@ target_link_libraries(testXkb
 )
 add_test(kwin-testXkb testXkb)
 ecm_mark_as_test(testXkb)
+
+if(HAVE_GBM)
+    add_executable(testGbmSurface test_gbm_surface.cpp ../plugins/platforms/drm/gbm_surface.cpp)
+    target_link_libraries(testGbmSurface Qt5::Test)
+    add_test(kwin-testGbmSurface testGbmSurface)
+    ecm_mark_as_test(kwin-testGbmSurface)
+endif()

autotests/test_gbm_surface.cpp

@@ -0,0 +1,119 @@
+/********************************************************************
+ KWin - the KDE window manager
+ This file is part of the KDE project.
+
+Copyright (C) 2017 Martin Flöser <mgraesslin@kde.org>
+
+This program is free software; you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 2 of the License, or
+(at your option) any later version.
+
+This program is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*********************************************************************/
+#include "../plugins/platforms/drm/gbm_surface.h"
+#include <QtTest/QtTest>
+
+#include <gbm.h>
+
+// mocking
+
+struct gbm_device {
+    bool surfaceShouldFail = false;
+};
+
+struct gbm_surface {
+    uint32_t width;
+    uint32_t height;
+    uint32_t format;
+    uint32_t flags;
+};
+
+struct gbm_bo {
+};
+
+struct gbm_surface *gbm_surface_create(struct gbm_device *gbm, uint32_t width, uint32_t height, uint32_t format, uint32_t flags)
+{
+    if (gbm && gbm->surfaceShouldFail) {
+        return nullptr;
+    }
+    auto ret = new gbm_surface{width, height, format, flags};
+    return ret;
+}
+
+void gbm_surface_destroy(struct gbm_surface *surface)
+{
+    delete surface;
+}
+
+struct gbm_bo *gbm_surface_lock_front_buffer(struct gbm_surface *surface)
+{
+    Q_UNUSED(surface)
+    return new gbm_bo;
+}
+
+void gbm_surface_release_buffer(struct gbm_surface *surface, struct gbm_bo *bo)
+{
+    Q_UNUSED(surface)
+    delete bo;
+}
+
+using KWin::GbmSurface;
+
+class GbmSurfaceTest : public QObject
+{
+    Q_OBJECT
+private Q_SLOTS:
+    void testCreate();
+    void testCreateFailure();
+    void testBo();
+};
+
+void GbmSurfaceTest::testCreate()
+{
+    GbmSurface surface(nullptr, 2, 3, 4, 5);
+    gbm_surface *native = surface;
+    QVERIFY(surface);
+    QCOMPARE(native->width, 2u);
+    QCOMPARE(native->height, 3u);
+    QCOMPARE(native->format, 4u);
+    QCOMPARE(native->flags, 5u);
+}
+
+void GbmSurfaceTest::testCreateFailure()
+{
+    gbm_device dev{true};
+    GbmSurface surface(&dev, 2, 3, 4, 5);
+    QVERIFY(!surface);
+    gbm_surface *native = surface;
+    QVERIFY(!native);
+}
+
+void GbmSurfaceTest::testBo()
+{
+    GbmSurface surface(nullptr, 2, 3, 4, 5);
+    // release buffer on nullptr should not be a problem
+    surface.releaseBuffer(nullptr);
+    // now an actual buffer
+    auto bo = surface.lockFrontBuffer();
+    surface.releaseBuffer(bo);
+
+    // and a surface which fails
+    gbm_device dev{true};
+    GbmSurface surface2(&dev, 2, 3, 4, 5);
+    QVERIFY(!surface2.lockFrontBuffer());
+    auto bo2 = surface.lockFrontBuffer();
+    // this won't do anything
+    surface2.releaseBuffer(bo2);
+    // so we need to clean up properly
+    surface.releaseBuffer(bo2);
+}
+
+QTEST_GUILESS_MAIN(GbmSurfaceTest)
+#include "test_gbm_surface.moc"

plugins/platforms/drm/CMakeLists.txt

@@ -13,7 +13,7 @@ set(DRM_SOURCES
 )
 
 if(HAVE_GBM)
-    set(DRM_SOURCES ${DRM_SOURCES} egl_gbm_backend.cpp drm_buffer_gbm.cpp)
+    set(DRM_SOURCES ${DRM_SOURCES} egl_gbm_backend.cpp drm_buffer_gbm.cpp gbm_surface.cpp)
 endif()
 
 add_library(KWinWaylandDrmBackend MODULE ${DRM_SOURCES})

plugins/platforms/drm/drm_backend.cpp

@@ -714,7 +714,7 @@ DrmDumbBuffer *DrmBackend::createBuffer(const QSize &size)
 }
 
 #if HAVE_GBM
-DrmSurfaceBuffer *DrmBackend::createBuffer(gbm_surface *surface)
+DrmSurfaceBuffer *DrmBackend::createBuffer(const std::shared_ptr<GbmSurface> &surface)
 {
     DrmSurfaceBuffer *b = new DrmSurfaceBuffer(this, surface);
     return b;

plugins/platforms/drm/drm_backend.h

@@ -35,6 +35,8 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #include <QSize>
 #include <xf86drmMode.h>
 
+#include <memory>
+
 struct gbm_bo;
 struct gbm_device;
 struct gbm_surface;
@@ -60,6 +62,7 @@ class DrmOutput;
 class DrmPlane;
 class DrmCrtc;
 class DrmConnector;
+class GbmSurface;
 
 
 class KWIN_EXPORT DrmBackend : public Platform
@@ -79,7 +82,7 @@ public:
     void init() override;
     DrmDumbBuffer *createBuffer(const QSize &size);
 #if HAVE_GBM
-    DrmSurfaceBuffer *createBuffer(gbm_surface *surface);
+    DrmSurfaceBuffer *createBuffer(const std::shared_ptr<GbmSurface> &surface);
 #endif
     void present(DrmBuffer *buffer, DrmOutput *output);
 

plugins/platforms/drm/drm_buffer_gbm.cpp

@@ -20,6 +20,7 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
 *********************************************************************/
 #include "drm_backend.h"
 #include "drm_buffer_gbm.h"
+#include "gbm_surface.h"
 
 #include "logging.h"
 
@@ -34,11 +35,11 @@ namespace KWin
 {
 
 // DrmSurfaceBuffer
-DrmSurfaceBuffer::DrmSurfaceBuffer(DrmBackend *backend, gbm_surface *surface)
+DrmSurfaceBuffer::DrmSurfaceBuffer(DrmBackend *backend, const std::shared_ptr<GbmSurface> &surface)
     : DrmBuffer(backend)
     , m_surface(surface)
 {
-    m_bo = gbm_surface_lock_front_buffer(surface);
+    m_bo = m_surface->lockFrontBuffer();
     if (!m_bo) {
         qCWarning(KWIN_DRM) << "Locking front buffer failed";
         return;
@@ -60,10 +61,8 @@ DrmSurfaceBuffer::~DrmSurfaceBuffer()
 
 void DrmSurfaceBuffer::releaseGbm()
 {
-    if (m_bo) {
-        gbm_surface_release_buffer(m_surface, m_bo);
-        m_bo = nullptr;
-    }
+    m_surface->releaseBuffer(m_bo);
+    m_bo = nullptr;
 }
 
 }

plugins/platforms/drm/drm_buffer_gbm.h

@@ -23,18 +23,20 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 #include "drm_buffer.h"
 
+#include <memory>
+
 struct gbm_bo;
-struct gbm_surface;
 
 namespace KWin
 {
 
 class DrmBackend;
+class GbmSurface;
 
 class DrmSurfaceBuffer : public DrmBuffer
 {
 public:
-    DrmSurfaceBuffer(DrmBackend *backend, gbm_surface *surface);
+    DrmSurfaceBuffer(DrmBackend *backend, const std::shared_ptr<GbmSurface> &surface);
     ~DrmSurfaceBuffer();
 
     bool needsModeChange(DrmBuffer *b) const override {
@@ -51,7 +53,7 @@ public:
     void releaseGbm() override;
 
 private:
-    gbm_surface *m_surface = nullptr;
+    std::shared_ptr<GbmSurface> m_surface;
     gbm_bo *m_bo = nullptr;
 };
 

plugins/platforms/drm/egl_gbm_backend.cpp

@@ -22,6 +22,7 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #include "composite.h"
 #include "drm_backend.h"
 #include "drm_output.h"
+#include "gbm_surface.h"
 #include "logging.h"
 #include "options.h"
 #include "screens.h"
@@ -69,6 +70,7 @@ void EglGbmBackend::cleanupSurfaces()
     for (auto it = m_outputs.constBegin(); it != m_outputs.constEnd(); ++it) {
         cleanupOutput(*it);
     }
+    m_outputs.clear();
 }
 
 void EglGbmBackend::cleanupOutput(const Output &o)
@@ -78,9 +80,6 @@ void EglGbmBackend::cleanupOutput(const Output &o)
     if (o.eglSurface != EGL_NO_SURFACE) {
         eglDestroySurface(eglDisplay(), o.eglSurface);
     }
-    if (o.gbmSurface) {
-        gbm_surface_destroy(o.gbmSurface);
-    }
 }
 
 bool EglGbmBackend::initializeEgl()
@@ -157,16 +156,16 @@ void EglGbmBackend::createOutput(DrmOutput *drmOutput)
     o.output = drmOutput;
     auto size = drmOutput->pixelSize();
 
-    o.gbmSurface = gbm_surface_create(m_backend->gbmDevice(), size.width(), size.height(),
+    o.gbmSurface = std::make_shared<GbmSurface>(m_backend->gbmDevice(), size.width(), size.height(),
                                         GBM_FORMAT_XRGB8888, GBM_BO_USE_SCANOUT | GBM_BO_USE_RENDERING);
     if (!o.gbmSurface) {
         qCCritical(KWIN_DRM) << "Create gbm surface failed";
         return;
     }
-    o.eglSurface = eglCreatePlatformWindowSurfaceEXT(eglDisplay(), config(), (void *)o.gbmSurface, nullptr);
+    o.eglSurface = eglCreatePlatformWindowSurfaceEXT(eglDisplay(), config(), (void *)((gbm_surface*)o.gbmSurface.get()), nullptr);
     if (o.eglSurface == EGL_NO_SURFACE) {
         qCCritical(KWIN_DRM) << "Create Window Surface failed";
-        gbm_surface_destroy(o.gbmSurface);
+        o.gbmSurface.reset();
         return;
     }
     m_outputs << o;

plugins/platforms/drm/egl_gbm_backend.h

@@ -22,6 +22,8 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #include "abstract_egl_backend.h"
 #include "scene_opengl.h"
 
+#include <memory>
+
 struct gbm_surface;
 
 namespace KWin
@@ -29,6 +31,7 @@ namespace KWin
 class DrmBackend;
 class DrmBuffer;
 class DrmOutput;
+class GbmSurface;
 
 /**
  * @brief OpenGL Backend using Egl on a GBM surface.
@@ -60,7 +63,7 @@ private:
     struct Output {
         DrmOutput *output = nullptr;
         DrmBuffer *buffer = nullptr;
-        gbm_surface *gbmSurface = nullptr;
+        std::shared_ptr<GbmSurface> gbmSurface;
         EGLSurface eglSurface = EGL_NO_SURFACE;
         int bufferAge = 0;
         /**

plugins/platforms/drm/gbm_surface.cpp

@@ -0,0 +1,55 @@
+/********************************************************************
+ KWin - the KDE window manager
+ This file is part of the KDE project.
+
+Copyright (C) 2017 Martin Flöser <mgraesslin@kde.org>
+
+This program is free software; you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 2 of the License, or
+(at your option) any later version.
+
+This program is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*********************************************************************/
+#include "gbm_surface.h"
+
+#include <gbm.h>
+
+namespace KWin
+{
+
+GbmSurface::GbmSurface(gbm_device *gbm, uint32_t width, uint32_t height, uint32_t format, uint32_t flags)
+    : m_surface(gbm_surface_create(gbm, width, height, format, flags))
+{
+}
+
+GbmSurface::~GbmSurface()
+{
+    if (m_surface) {
+        gbm_surface_destroy(m_surface);
+    }
+}
+
+gbm_bo *GbmSurface::lockFrontBuffer()
+{
+    if (!m_surface) {
+        return nullptr;
+    }
+    return gbm_surface_lock_front_buffer(m_surface);
+}
+
+void GbmSurface::releaseBuffer(gbm_bo *bo)
+{
+    if (!bo || !m_surface) {
+        return;
+    }
+    gbm_surface_release_buffer(m_surface, bo);
+}
+
+}

plugins/platforms/drm/gbm_surface.h

@@ -0,0 +1,55 @@
+/********************************************************************
+ KWin - the KDE window manager
+ This file is part of the KDE project.
+
+Copyright (C) 2017 Martin Flöser <mgraesslin@kde.org>
+
+This program is free software; you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 2 of the License, or
+(at your option) any later version.
+
+This program is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*********************************************************************/
+#ifndef KWIN_DRM_GBM_SURFACE_H
+#define KWIN_DRM_GBM_SURFACE_H
+
+#include <stdint.h>
+
+struct gbm_bo;
+struct gbm_device;
+struct gbm_surface;
+
+namespace KWin
+{
+
+class GbmSurface
+{
+public:
+    explicit GbmSurface(gbm_device *gbm, uint32_t width, uint32_t height, uint32_t format, uint32_t flags);
+    ~GbmSurface();
+
+    gbm_bo *lockFrontBuffer();
+    void releaseBuffer(gbm_bo *bo);
+
+    operator bool() const {
+        return m_surface != nullptr;
+    }
+
+    operator gbm_surface*() const {
+        return m_surface;
+    }
+
+private:
+    gbm_surface *m_surface;
+};
+
+}
+
+#endif