From b3a19f9e5b779bb42276a2210813e6b4dd322c8c Mon Sep 17 00:00:00 2001 From: Roman Gilg Date: Tue, 27 Aug 2019 22:23:17 +0200 Subject: [PATCH] Remove vsync detection and configurability Summary: Selecting not to vsync does not make sense for an X11 compositor. In the end we want clients to be able to present async if they want to but the compositor is supposed to send swaps with vsync to the XServer in order to not generate tearing artifacts. There was also a detection logic which did some questionable things in case vsync was not available. I don't think this is necessary at all since we can just always run a timer to present with or without vsync. Test Plan: kwin_x11 tested on i915. Reviewers: #kwin, zzag Subscribers: zzag, kwin Tags: #kwin Maniphest Tasks: T11071 Differential Revision: https://phabricator.kde.org/D23511 --- composite.cpp | 18 +++++----- kcmkwin/kwincompositing/compositing.cpp | 24 ++++++------- kcmkwin/kwincompositing/compositing.ui | 5 --- options.cpp | 4 +-- options.h | 2 +- platformsupport/scenes/opengl/backend.cpp | 3 +- platformsupport/scenes/opengl/backend.h | 24 ------------- plugins/platforms/drm/egl_gbm_backend.cpp | 1 - plugins/platforms/drm/egl_stream_backend.cpp | 1 - .../hwcomposer/egl_hwcomposer_backend.cpp | 1 - .../platforms/x11/common/eglonxbackend.cpp | 22 +++++------- .../platforms/x11/standalone/glxbackend.cpp | 36 +++++-------------- plugins/platforms/x11/standalone/glxbackend.h | 2 -- plugins/scenes/opengl/scene_opengl.cpp | 5 --- plugins/scenes/opengl/scene_opengl.h | 1 - scene.cpp | 5 --- scene.h | 1 - 17 files changed, 39 insertions(+), 116 deletions(-) diff --git a/composite.cpp b/composite.cpp index a95382fb6e..234a214319 100644 --- a/composite.cpp +++ b/composite.cpp @@ -334,14 +334,12 @@ void Compositor::startupWithWorkspace() setupX11Support(); fpsInterval = options->maxFpsInterval(); - if (m_scene->syncsToVBlank()) { - // If we do vsync, set the fps to the next multiple of the vblank rate. - vBlankInterval = milliToNano(1000) / currentRefreshRate(); - fpsInterval = qMax((fpsInterval / vBlankInterval) * vBlankInterval, vBlankInterval); - } else { - // No vsync - DO NOT set "0", would cause div-by-zero segfaults. - vBlankInterval = milliToNano(1); - } + const auto rate = currentRefreshRate(); + Q_ASSERT(rate != 0); // There is a fallback in options.cpp, so why check at all? + + // If we do vsync, set the fps to the next multiple of the vblank rate. + vBlankInterval = milliToNano(1000) / currentRefreshRate(); + fpsInterval = qMax((fpsInterval / vBlankInterval) * vBlankInterval, vBlankInterval); // Sets also the 'effects' pointer. kwinApp()->platform()->createEffectsHandler(this, m_scene); @@ -719,7 +717,7 @@ void Compositor::performCompositing() // is called the next time. If there would be nothing pending, it will not restart the timer and // scheduleRepaint() would restart it again somewhen later, called from functions that // would again add something pending. - if (m_bufferSwapPending && m_scene->syncsToVBlank()) { + if (m_bufferSwapPending) { m_composeAtSwapCompletion = true; } else { scheduleRepaint(); @@ -818,7 +816,7 @@ void Compositor::setCompositeTimer() waitTime = 1; } } - /* else if (m_scene->syncsToVBlank() && m_timeSinceLastVBlank - fpsInterval < (vBlankInterval<<1)) { + /* else if (m_timeSinceLastVBlank - fpsInterval < (vBlankInterval<<1)) { // NOTICE - "for later" ------------------------------------------------------------------ // It can happen that we push two frames within one refresh cycle. // Swapping will then block even with triple buffering when the GPU does not discard but diff --git a/kcmkwin/kwincompositing/compositing.cpp b/kcmkwin/kwincompositing/compositing.cpp index b43f7cea20..eb63d977a9 100644 --- a/kcmkwin/kwincompositing/compositing.cpp +++ b/kcmkwin/kwincompositing/compositing.cpp @@ -81,16 +81,14 @@ void Compositing::reset() auto swapStrategy = [&kwinConfig]() { const QString glSwapStrategyValue = kwinConfig.readEntry("GLPreferBufferSwap", "a"); - if (glSwapStrategyValue == "n") { + if (glSwapStrategyValue == "a") { return 0; - } else if (glSwapStrategyValue == "a") { - return 1; } else if (glSwapStrategyValue == "e") { - return 2; + return 1; } else if (glSwapStrategyValue == "p") { - return 3; + return 2; } else if (glSwapStrategyValue == "c") { - return 4; + return 3; } return 0; }; @@ -286,15 +284,13 @@ void Compositing::save() } auto swapStrategy = [this] { switch (glSwapStrategy()) { - case 0: - return QStringLiteral("n"); - case 2: - return QStringLiteral("e"); - case 3: - return QStringLiteral("p"); - case 4: - return QStringLiteral("c"); case 1: + return QStringLiteral("e"); + case 2: + return QStringLiteral("p"); + case 3: + return QStringLiteral("c"); + case 0: default: return QStringLiteral("a"); } diff --git a/kcmkwin/kwincompositing/compositing.ui b/kcmkwin/kwincompositing/compositing.ui index ec30fc32fb..45e82fe70b 100644 --- a/kcmkwin/kwincompositing/compositing.ui +++ b/kcmkwin/kwincompositing/compositing.ui @@ -165,11 +165,6 @@ Alternatively, you might want to use the XRender backend instead. - - - Never - - Automatic diff --git a/options.cpp b/options.cpp index fc727855fc..07534d7c75 100644 --- a/options.cpp +++ b/options.cpp @@ -918,12 +918,12 @@ void Options::reloadCompositingSettings(bool force) } setGLCoreProfile(config.readEntry("GLCore", Options::defaultGLCoreProfile())); - char c = 0; + char c = 'a'; const QString s = config.readEntry("GLPreferBufferSwap", QString(Options::defaultGlPreferBufferSwap())); if (!s.isEmpty()) c = s.at(0).toLatin1(); if (c != 'a' && c != 'c' && c != 'p' && c != 'e') - c = 0; + c = 'a'; setGlPreferBufferSwap(c); m_xrenderSmoothScale = config.readEntry("XRenderSmoothScale", false); diff --git a/options.h b/options.h index ee31cfea90..8f28f1df84 100644 --- a/options.h +++ b/options.h @@ -570,7 +570,7 @@ public: return m_glPlatformInterface; } - enum GlSwapStrategy { NoSwapEncourage = 0, CopyFrontBuffer = 'c', PaintFullScreen = 'p', ExtendDamage = 'e', AutoSwapStrategy = 'a' }; + enum GlSwapStrategy { CopyFrontBuffer = 'c', PaintFullScreen = 'p', ExtendDamage = 'e', AutoSwapStrategy = 'a' }; GlSwapStrategy glPreferBufferSwap() const { return m_glPreferBufferSwap; } diff --git a/platformsupport/scenes/opengl/backend.cpp b/platformsupport/scenes/opengl/backend.cpp index 45b6fbd30a..18fc7e61b6 100644 --- a/platformsupport/scenes/opengl/backend.cpp +++ b/platformsupport/scenes/opengl/backend.cpp @@ -30,8 +30,7 @@ namespace KWin { OpenGLBackend::OpenGLBackend() - : m_syncsToVBlank(false) - , m_blocksForRetrace(false) + : m_blocksForRetrace(false) , m_directRendering(false) , m_haveBufferAge(false) , m_failed(false) diff --git a/platformsupport/scenes/opengl/backend.h b/platformsupport/scenes/opengl/backend.h index 2879f68e6e..6e405b663f 100644 --- a/platformsupport/scenes/opengl/backend.h +++ b/platformsupport/scenes/opengl/backend.h @@ -124,16 +124,6 @@ public: bool isFailed() const { return m_failed; } - /** - * @brief Whether the Backend provides VSync. - * - * Currently only the GLX backend can provide VSync. - * - * @return bool @c true if VSync support is available, @c false otherwise - */ - bool syncsToVBlank() const { - return m_syncsToVBlank; - } /** * @brief Whether VSync blocks execution until the screen is in the retrace * @@ -212,16 +202,6 @@ protected: * @param reason The reason why the initialization failed. */ void setFailed(const QString &reason); - /** - * @brief Sets whether the backend provides VSync. - * - * Should be called by the concrete subclass once it is determined whether VSync is supported. - * If the subclass does not call this method, the backend defaults to @c false. - * @param enabled @c true if VSync support available, @c false otherwise. - */ - void setSyncsToVBlank(bool enabled) { - m_syncsToVBlank = enabled; - } /** * @brief Sets whether the VSync iplementation blocks * @@ -284,10 +264,6 @@ protected: } private: - /** - * @brief Whether VSync is available and used, defaults to @c false. - */ - bool m_syncsToVBlank; /** * @brief Whether present() will block execution until the next vertical retrace @c false. */ diff --git a/plugins/platforms/drm/egl_gbm_backend.cpp b/plugins/platforms/drm/egl_gbm_backend.cpp index 48ec495b24..c63635d53c 100644 --- a/plugins/platforms/drm/egl_gbm_backend.cpp +++ b/plugins/platforms/drm/egl_gbm_backend.cpp @@ -42,7 +42,6 @@ EglGbmBackend::EglGbmBackend(DrmBackend *b) { // Egl is always direct rendering setIsDirectRendering(true); - setSyncsToVBlank(true); connect(m_backend, &DrmBackend::outputAdded, this, &EglGbmBackend::createOutput); connect(m_backend, &DrmBackend::outputRemoved, this, [this] (DrmOutput *output) { diff --git a/plugins/platforms/drm/egl_stream_backend.cpp b/plugins/platforms/drm/egl_stream_backend.cpp index 374ab42bbb..ce5741d959 100644 --- a/plugins/platforms/drm/egl_stream_backend.cpp +++ b/plugins/platforms/drm/egl_stream_backend.cpp @@ -86,7 +86,6 @@ EglStreamBackend::EglStreamBackend(DrmBackend *b) : AbstractEglBackend(), m_backend(b) { setIsDirectRendering(true); - setSyncsToVBlank(true); connect(m_backend, &DrmBackend::outputAdded, this, &EglStreamBackend::createOutput); connect(m_backend, &DrmBackend::outputRemoved, this, [this] (DrmOutput *output) { diff --git a/plugins/platforms/hwcomposer/egl_hwcomposer_backend.cpp b/plugins/platforms/hwcomposer/egl_hwcomposer_backend.cpp index 5207545bdb..9d354268c8 100644 --- a/plugins/platforms/hwcomposer/egl_hwcomposer_backend.cpp +++ b/plugins/platforms/hwcomposer/egl_hwcomposer_backend.cpp @@ -30,7 +30,6 @@ EglHwcomposerBackend::EglHwcomposerBackend(HwcomposerBackend *backend) { // EGL is always direct rendering setIsDirectRendering(true); - setSyncsToVBlank(true); setBlocksForRetrace(true); } diff --git a/plugins/platforms/x11/common/eglonxbackend.cpp b/plugins/platforms/x11/common/eglonxbackend.cpp index baa9c06ac2..f4b8fe8cdb 100644 --- a/plugins/platforms/x11/common/eglonxbackend.cpp +++ b/plugins/platforms/x11/common/eglonxbackend.cpp @@ -121,27 +121,21 @@ void EglOnXBackend::init() } } - setSyncsToVBlank(false); setBlocksForRetrace(true); if (surfaceHasSubPost) { qCDebug(KWIN_CORE) << "EGL implementation and surface support eglPostSubBufferNV, let's use it"; - if (options->glPreferBufferSwap() != Options::NoSwapEncourage) { - // check if swap interval 1 is supported - EGLint val; - eglGetConfigAttrib(eglDisplay(), config(), EGL_MAX_SWAP_INTERVAL, &val); - if (val >= 1) { - if (eglSwapInterval(eglDisplay(), 1)) { - qCDebug(KWIN_CORE) << "Enabled v-sync"; - setSyncsToVBlank(true); - } - } else { - qCWarning(KWIN_CORE) << "Cannot enable v-sync as max. swap interval is" << val; + // check if swap interval 1 is supported + EGLint val; + eglGetConfigAttrib(eglDisplay(), config(), EGL_MAX_SWAP_INTERVAL, &val); + if (val >= 1) { + if (eglSwapInterval(eglDisplay(), 1)) { + qCDebug(KWIN_CORE) << "Enabled v-sync"; } } else { - // disable v-sync - eglSwapInterval(eglDisplay(), 0); + qCWarning(KWIN_CORE) << "Cannot enable v-sync as max. swap interval is" << val; } + } else { /* In the GLX backend, we fall back to using glCopyPixels if we have no extension providing support for partial screen updates. * However, that does not work in EGL - glCopyPixels with glDrawBuffer(GL_FRONT); does nothing. diff --git a/plugins/platforms/x11/standalone/glxbackend.cpp b/plugins/platforms/x11/standalone/glxbackend.cpp index 09507c9117..fc1f94c588 100644 --- a/plugins/platforms/x11/standalone/glxbackend.cpp +++ b/plugins/platforms/x11/standalone/glxbackend.cpp @@ -113,7 +113,6 @@ GlxBackend::GlxBackend(Display *display) , glxWindow(None) , ctx(nullptr) , m_bufferAge(0) - , haveSwapInterval(false) , m_x11Display(display) { // Ensures calls to glXSwapBuffers will always block until the next @@ -221,8 +220,6 @@ void GlxBackend::init() glXSelectEvent(display(), glxWindow, GLX_BUFFER_SWAP_COMPLETE_INTEL_MASK); } - haveSwapInterval = m_haveMESASwapControl || m_haveEXTSwapControl; - setSupportsBufferAge(false); if (hasExtension(QByteArrayLiteral("GLX_EXT_buffer_age"))) { @@ -232,20 +229,16 @@ void GlxBackend::init() setSupportsBufferAge(true); } - setSyncsToVBlank(false); setBlocksForRetrace(true); - const bool wantSync = options->glPreferBufferSwap() != Options::NoSwapEncourage; - if (wantSync && glXIsDirect(display(), ctx)) { - if (haveSwapInterval) { // glXSwapInterval is preferred being more reliable - setSwapInterval(1); - setSyncsToVBlank(true); - } else { - qCWarning(KWIN_X11STANDALONE) << "NO VSYNC! glSwapInterval is not supported"; - } + + if (m_haveEXTSwapControl) { + glXSwapIntervalEXT(display(), glxWindow, 1); + } else if (m_haveMESASwapControl) { + glXSwapIntervalMESA(1); } else { - // disable v-sync (if possible) - setSwapInterval(0); + qCWarning(KWIN_X11STANDALONE) << "NO VSYNC! glSwapInterval is not supported"; } + if (glPlatform->isVirtualBox()) { // VirtualBox does not support glxQueryDrawable // this should actually be in kwinglutils_funcs, but QueryDrawable seems not to be provided by an extension @@ -672,14 +665,6 @@ FBConfigInfo *GlxBackend::infoForVisual(xcb_visualid_t visual) return info; } -void GlxBackend::setSwapInterval(int interval) -{ - if (m_haveEXTSwapControl) - glXSwapIntervalEXT(display(), glxWindow, interval); - else if (m_haveMESASwapControl) - glXSwapIntervalMESA(interval); -} - void GlxBackend::present() { if (lastDamage().isEmpty()) @@ -693,11 +678,8 @@ void GlxBackend::present() if (m_haveINTELSwapEvent) Compositor::self()->aboutToSwapBuffers(); - if (haveSwapInterval) { - glXSwapBuffers(display(), glxWindow); - } else { - glXSwapBuffers(display(), glxWindow); - } + glXSwapBuffers(display(), glxWindow); + if (supportsBufferAge()) { glXQueryDrawable(display(), glxWindow, GLX_BACK_BUFFER_AGE_EXT, (GLuint *) &m_bufferAge); } diff --git a/plugins/platforms/x11/standalone/glxbackend.h b/plugins/platforms/x11/standalone/glxbackend.h index ec55ac49b1..d72dda8658 100644 --- a/plugins/platforms/x11/standalone/glxbackend.h +++ b/plugins/platforms/x11/standalone/glxbackend.h @@ -89,7 +89,6 @@ private: bool initRenderingContext(); bool initFbConfig(); void initVisualDepthHashTable(); - void setSwapInterval(int interval); Display *display() const { return m_x11Display; } @@ -113,7 +112,6 @@ private: bool m_haveMESASwapControl = false; bool m_haveEXTSwapControl = false; bool m_haveINTELSwapEvent = false; - bool haveSwapInterval = false; Display *m_x11Display; friend class GlxTexture; }; diff --git a/plugins/scenes/opengl/scene_opengl.cpp b/plugins/scenes/opengl/scene_opengl.cpp index d566a8bd2f..0945cbd38c 100644 --- a/plugins/scenes/opengl/scene_opengl.cpp +++ b/plugins/scenes/opengl/scene_opengl.cpp @@ -525,11 +525,6 @@ OverlayWindow *SceneOpenGL::overlayWindow() const return m_backend->overlayWindow(); } -bool SceneOpenGL::syncsToVBlank() const -{ - return m_backend->syncsToVBlank(); -} - bool SceneOpenGL::blocksForRetrace() const { return m_backend->blocksForRetrace(); diff --git a/plugins/scenes/opengl/scene_opengl.h b/plugins/scenes/opengl/scene_opengl.h index af4309bc59..b9978c9645 100644 --- a/plugins/scenes/opengl/scene_opengl.h +++ b/plugins/scenes/opengl/scene_opengl.h @@ -54,7 +54,6 @@ public: OverlayWindow *overlayWindow() const override; bool usesOverlayWindow() const override; bool blocksForRetrace() const override; - bool syncsToVBlank() const override; bool makeOpenGLContextCurrent() override; void doneOpenGLContextCurrent() override; Decoration::Renderer *createDecorationRenderer(Decoration::DecoratedClientImpl *impl) override; diff --git a/scene.cpp b/scene.cpp index ce186a4d02..fdc135c7bc 100644 --- a/scene.cpp +++ b/scene.cpp @@ -632,11 +632,6 @@ bool Scene::blocksForRetrace() const return false; } -bool Scene::syncsToVBlank() const -{ - return false; -} - void Scene::screenGeometryChanged(const QSize &size) { if (!overlayWindow()) { diff --git a/scene.h b/scene.h index 561901fcab..d547d1228b 100644 --- a/scene.h +++ b/scene.h @@ -146,7 +146,6 @@ public: // there's nothing to paint (adjust time_diff later) virtual void idle(); virtual bool blocksForRetrace() const; - virtual bool syncsToVBlank() const; virtual OverlayWindow* overlayWindow() const = 0; virtual bool makeOpenGLContextCurrent();