From ac05dd01c84ab2d0b51273131bcc04c258bc4f9d Mon Sep 17 00:00:00 2001 From: Roman Gilg Date: Thu, 9 Jan 2020 18:06:14 +0100 Subject: [PATCH] Revert "[platforms/x11] Never block on retrace, always present after paint" This reverts commit 8d137290317a25aeff9b2abcc0914395e1903aa5. See: https://mail.kde.org/pipermail/kwin/2020-January/002999.html --- composite.cpp | 67 +++++++++++++++++-- composite.h | 2 - platformsupport/scenes/opengl/backend.cpp | 3 +- platformsupport/scenes/opengl/backend.h | 24 +++++++ .../platforms/x11/common/eglonxbackend.cpp | 14 +++- .../platforms/x11/standalone/glxbackend.cpp | 21 +++++- plugins/scenes/opengl/scene_opengl.cpp | 5 ++ plugins/scenes/opengl/scene_opengl.h | 1 + scene.cpp | 5 ++ scene.h | 1 + workspace.cpp | 5 ++ 11 files changed, 136 insertions(+), 12 deletions(-) diff --git a/composite.cpp b/composite.cpp index 2fbf3e269c..51fb0c667f 100644 --- a/composite.cpp +++ b/composite.cpp @@ -779,7 +779,62 @@ void Compositor::setCompositeTimer() return; } - uint waitTime = 1000 / refreshRate(); + uint waitTime = 1; + + if (m_scene->blocksForRetrace()) { + + // TODO: make vBlankTime dynamic?! + // It's required because glXWaitVideoSync will *likely* block a full frame if one enters + // a retrace pass which can last a variable amount of time, depending on the actual screen + // Now, my ooold 19" CRT can do such retrace so that 2ms are entirely sufficient, + // while another ooold 15" TFT requires about 6ms + + qint64 padding = m_timeSinceLastVBlank; + if (padding > fpsInterval) { + // We're at low repaints or spent more time in painting than the user wanted to wait + // for that frame. Align to next vblank: + padding = vBlankInterval - (padding % vBlankInterval); + } else { + // Align to the next maxFps tick: + // "remaining time of the first vsync" + "time for the other vsyncs of the frame" + padding = ((vBlankInterval - padding % vBlankInterval) + + (fpsInterval / vBlankInterval - 1) * vBlankInterval); + } + + if (padding < options->vBlankTime()) { + // We'll likely miss this frame so we add one: + waitTime = nanoToMilli(padding + vBlankInterval - options->vBlankTime()); + } else { + waitTime = nanoToMilli(padding - options->vBlankTime()); + } + } + else { // w/o blocking vsync we just jump to the next demanded tick + if (fpsInterval > m_timeSinceLastVBlank) { + waitTime = nanoToMilli(fpsInterval - m_timeSinceLastVBlank); + if (!waitTime) { + // Will ensure we don't block out the eventloop - the system's just not faster ... + waitTime = 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 + // queues frames + // now here's the mean part: if we take that as "OMG, we're late - next frame ASAP", + // there'll immediately be 2 frames in the pipe, swapping will block, we think we're + // late ... ewww + // so instead we pad to the clock again and add 2ms safety to ensure the pipe is really + // free + // NOTICE: obviously m_timeSinceLastVBlank can be too big because we're too slow as well + // So if this code was enabled, we'd needlessly half the framerate once more (15 instead of 30) + waitTime = nanoToMilli(vBlankInterval - (m_timeSinceLastVBlank - fpsInterval)%vBlankInterval) + 2; + }*/ + else { + // "0" would be sufficient here, but the compositor isn't the WMs only task. + waitTime = 1; + } + } // Force 4fps minimum: compositeTimer.start(qMin(waitTime, 250u), this); } @@ -808,11 +863,6 @@ void WaylandCompositor::start() return; } - // TODO: This is problematic on Wayland. We should get the highest refresh rate - // and not the one of the first output. Also on hotplug reevaluate. - // On X11 do it also like this? - m_refreshRate = KWin::currentRefreshRate(); - if (Workspace::self()) { startupWithWorkspace(); } else { @@ -823,7 +873,10 @@ void WaylandCompositor::start() int WaylandCompositor::refreshRate() const { - return m_refreshRate; + // TODO: This makes no sense on Wayland. First step would be to atleast + // set the refresh rate to the highest available one. Second step + // would be to not use a uniform value at all but per screen. + return KWin::currentRefreshRate(); } X11Compositor::X11Compositor(QObject *parent) diff --git a/composite.h b/composite.h index 1c90df79c8..84783c8316 100644 --- a/composite.h +++ b/composite.h @@ -187,8 +187,6 @@ protected: private: explicit WaylandCompositor(QObject *parent); - - int m_refreshRate; }; class KWIN_EXPORT X11Compositor : public Compositor diff --git a/platformsupport/scenes/opengl/backend.cpp b/platformsupport/scenes/opengl/backend.cpp index 611e2244e3..18fc7e61b6 100644 --- a/platformsupport/scenes/opengl/backend.cpp +++ b/platformsupport/scenes/opengl/backend.cpp @@ -30,7 +30,8 @@ namespace KWin { OpenGLBackend::OpenGLBackend() - : m_directRendering(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 51a5e7da53..6e405b663f 100644 --- a/platformsupport/scenes/opengl/backend.h +++ b/platformsupport/scenes/opengl/backend.h @@ -124,6 +124,16 @@ public: bool isFailed() const { return m_failed; } + /** + * @brief Whether VSync blocks execution until the screen is in the retrace + * + * Case for waitVideoSync and non triple buffering buffer swaps (triple buffering support + * has been removed). + * + */ + bool blocksForRetrace() const { + return m_blocksForRetrace; + } /** * @brief Whether the backend uses direct rendering. * @@ -192,6 +202,16 @@ protected: * @param reason The reason why the initialization failed. */ void setFailed(const QString &reason); + /** + * @brief Sets whether the VSync iplementation blocks + * + * Should be called by the concrete subclass once it is determined how VSync works. + * If the subclass does not call this method, the backend defaults to @c false. + * @param enabled @c true if VSync blocks, @c false otherwise. + */ + void setBlocksForRetrace(bool enabled) { + m_blocksForRetrace = enabled; + } /** * @brief Sets whether the OpenGL context is direct. * @@ -244,6 +264,10 @@ protected: } private: + /** + * @brief Whether present() will block execution until the next vertical retrace @c false. + */ + bool m_blocksForRetrace; /** * @brief Whether direct rendering is used, defaults to @c false. */ diff --git a/plugins/platforms/x11/common/eglonxbackend.cpp b/plugins/platforms/x11/common/eglonxbackend.cpp index 38fb41c2fd..f4b8fe8cdb 100644 --- a/plugins/platforms/x11/common/eglonxbackend.cpp +++ b/plugins/platforms/x11/common/eglonxbackend.cpp @@ -121,6 +121,7 @@ void EglOnXBackend::init() } } + setBlocksForRetrace(true); if (surfaceHasSubPost) { qCDebug(KWIN_CORE) << "EGL implementation and surface support eglPostSubBufferNV, let's use it"; @@ -354,6 +355,8 @@ QRegion EglOnXBackend::prepareRenderingFrame() { QRegion repaint; + present(); + if (supportsBufferAge()) repaint = accumulatedDamageHistory(m_bufferAge); @@ -383,7 +386,16 @@ void EglOnXBackend::endRenderingFrame(const QRegion &renderedRegion, const QRegi } setLastDamage(renderedRegion); - present(); + + if (!blocksForRetrace()) { + // This also sets lastDamage to empty which prevents the frame from + // being posted again when prepareRenderingFrame() is called. + present(); + } else { + // Make sure that the GPU begins processing the command stream + // now and not the next time prepareRenderingFrame() is called. + glFlush(); + } if (m_overlayWindow && overlayWindow()->window()) // show the window only after the first pass, overlayWindow()->show(); // since that pass may take long diff --git a/plugins/platforms/x11/standalone/glxbackend.cpp b/plugins/platforms/x11/standalone/glxbackend.cpp index 3897525554..453f345ac9 100644 --- a/plugins/platforms/x11/standalone/glxbackend.cpp +++ b/plugins/platforms/x11/standalone/glxbackend.cpp @@ -115,6 +115,11 @@ GlxBackend::GlxBackend(Display *display) , m_bufferAge(0) , m_x11Display(display) { + // Ensures calls to glXSwapBuffers will always block until the next + // retrace when using the proprietary NVIDIA driver. This must be + // set before libGL.so is loaded. + setenv("__GL_MaxFramesAllowed", "1", true); + // Force initialization of GLX integration in the Qt's xcb backend // to make it call XESetWireToEvent callbacks, which is required // by Mesa when using DRI2. @@ -224,6 +229,8 @@ void GlxBackend::init() setSupportsBufferAge(true); } + setBlocksForRetrace(true); + if (m_haveEXTSwapControl) { glXSwapIntervalEXT(display(), glxWindow, 1); } else if (m_haveMESASwapControl) { @@ -719,10 +726,13 @@ QRegion GlxBackend::prepareRenderingFrame() { QRegion repaint; + present(); + if (supportsBufferAge()) repaint = accumulatedDamageHistory(m_bufferAge); startRenderTimer(); + glXWaitX(); return repaint; } @@ -747,7 +757,16 @@ void GlxBackend::endRenderingFrame(const QRegion &renderedRegion, const QRegion } setLastDamage(renderedRegion); - present(); + + if (!blocksForRetrace()) { + // This also sets lastDamage to empty which prevents the frame from + // being posted again when prepareRenderingFrame() is called. + present(); + } else { + // Make sure that the GPU begins processing the command stream + // now and not the next time prepareRenderingFrame() is called. + glFlush(); + } if (overlayWindow()->window()) // show the window only after the first pass, overlayWindow()->show(); // since that pass may take long diff --git a/plugins/scenes/opengl/scene_opengl.cpp b/plugins/scenes/opengl/scene_opengl.cpp index 7599f0ac4d..4f7478ea59 100644 --- a/plugins/scenes/opengl/scene_opengl.cpp +++ b/plugins/scenes/opengl/scene_opengl.cpp @@ -492,6 +492,11 @@ OverlayWindow *SceneOpenGL::overlayWindow() const return m_backend->overlayWindow(); } +bool SceneOpenGL::blocksForRetrace() const +{ + return m_backend->blocksForRetrace(); +} + void SceneOpenGL::idle() { m_backend->idle(); diff --git a/plugins/scenes/opengl/scene_opengl.h b/plugins/scenes/opengl/scene_opengl.h index 9c80f4d1ba..1b5be8bbe4 100644 --- a/plugins/scenes/opengl/scene_opengl.h +++ b/plugins/scenes/opengl/scene_opengl.h @@ -52,6 +52,7 @@ public: void screenGeometryChanged(const QSize &size) override; OverlayWindow *overlayWindow() const override; bool usesOverlayWindow() const override; + bool blocksForRetrace() const override; bool makeOpenGLContextCurrent() override; void doneOpenGLContextCurrent() override; Decoration::Renderer *createDecorationRenderer(Decoration::DecoratedClientImpl *impl) override; diff --git a/scene.cpp b/scene.cpp index 8594df83c5..a1115dc280 100644 --- a/scene.cpp +++ b/scene.cpp @@ -621,6 +621,11 @@ void Scene::extendPaintRegion(QRegion ®ion, bool opaqueFullscreen) Q_UNUSED(opaqueFullscreen); } +bool Scene::blocksForRetrace() const +{ + return false; +} + void Scene::screenGeometryChanged(const QSize &size) { if (!overlayWindow()) { diff --git a/scene.h b/scene.h index 7e7aee5847..82e0379fb4 100644 --- a/scene.h +++ b/scene.h @@ -145,6 +145,7 @@ public: enum ImageFilterType { ImageFilterFast, ImageFilterGood }; // there's nothing to paint (adjust time_diff later) virtual void idle(); + virtual bool blocksForRetrace() const; virtual OverlayWindow* overlayWindow() const = 0; virtual bool makeOpenGLContextCurrent(); diff --git a/workspace.cpp b/workspace.cpp index eec37dcaaa..f434be5fa3 100644 --- a/workspace.cpp +++ b/workspace.cpp @@ -1609,6 +1609,11 @@ QString Workspace::supportInformation() const } support.append(QStringLiteral("OpenGL 2 Shaders are used\n")); + support.append(QStringLiteral("Painting blocks for vertical retrace: ")); + if (m_compositor->scene()->blocksForRetrace()) + support.append(QStringLiteral(" yes\n")); + else + support.append(QStringLiteral(" no\n")); break; } case XRenderCompositing: