1197954 - Implement a hardware Vsync source for GLX

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Andrew Comminos [:acomminos]]

GLX has several extensions we can use to wait on buffer swaps, such as GLX_SGI_video_sync. We can use these to provide vsync timing information on Linux.

Comment 1

[Andrew Comminos [:acomminos]]

Attachment: Add support for GLX_SGI_video_sync.

Fairly straightforward, adds sync support to GLContextGLX.

Comment 2

[Andrew Comminos [:acomminos]]

Attachment: Add support for creating a GLContextGLX with a separate X11 display.

This adds a flag to context creation to use a separate display connection.

This is necessary to prevent contention on the Vsync thread.

Comment 3

[Andrew Comminos [:acomminos]]

Attachment: Implement Linux hardware vsync using GLX_SGI_video_sync.

This adds a vsync provider using SGI_video_sync, using glXWaitVideoSync to pull swap intervals.

Comment 4

[Andrew Comminos [:acomminos]]

Initial talos impressions: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=23a04f9a321c&newProject=try&newRevision=7d1233f5148f

Doesn't look to be affecting any results with the X11 basic compositor, hooray.

Comment 5

[Andrew Comminos [:acomminos]]

Attachment: Implement Linux hardware vsync using GLX_SGI_video_sync.

Updated to check result of setting up GLXDisplay and fallback appropriately.

Comment 6

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8652616 [details] [diff] [review]
Implement Linux hardware vsync using GLX_SGI_video_sync.

Review of attachment 8652616 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxPlatformGtk.cpp
@@ +593,5 @@
> +                 , mVsyncThread("GLXVsyncThread")
> +                 , mVsyncEnabledLock("GLXVsyncEnabledLock")
> +                 , mVsyncEnabled(false)
> +    {
> +      MOZ_RELEASE_ASSERT(mVsyncThread.Start(), "Failed to start GLX vsync thread");

Can we check this in setup instead? If it doesn't work correctly, we can just fall back to software.

@@ +597,5 @@
> +      MOZ_RELEASE_ASSERT(mVsyncThread.Start(), "Failed to start GLX vsync thread");
> +    }
> +
> +    // Sets up the display's GL context on a worker thread.
> +    // Returns true if setup was a success.

nit: add comment why you need this.

@@ +598,5 @@
> +    }
> +
> +    // Sets up the display's GL context on a worker thread.
> +    // Returns true if setup was a success.
> +    bool Setup()

assert which thread this happens on.

@@ +605,5 @@
> +      CancelableTask* vsyncSetup = NewRunnableMethod(this, &GLXDisplay::SetupGLContext);
> +      mVsyncThread.message_loop()->PostTask(FROM_HERE, vsyncSetup);
> +      // Wait until the setup has completed.
> +      lock.Wait();
> +      return mGLContext;

nit: return mGLContext == nullptr; Make this explicit.

@@ +610,5 @@
> +    }
> +
> +    virtual void EnableVsync() override
> +    {
> +      MOZ_ASSERT(mGLContext, "GLContext not setup!");

assert main thread.

@@ +616,5 @@
> +      MonitorAutoLock lock(mVsyncEnabledLock);
> +      if (mVsyncEnabled)
> +        return;
> +
> +      mVsyncEnabled = true;

nit: set this to true after we post the task.

@@ +628,5 @@
> +      mVsyncEnabled = false;
> +    }
> +
> +    virtual bool IsVsyncEnabled() override
> +    {

Need a lock.

@@ +669,5 @@
> +          if (!mVsyncEnabled) return;
> +        }
> +
> +        unsigned int nextSync = syncCounter + 1;
> +        if (gl::sGLXLibrary.xWaitVideoSync(2, nextSync % 2,

nit: Please add a comment explaining this API and the wierdness.

@@ +692,5 @@
> +    bool mVsyncEnabled;
> +  };
> +private:
> +  virtual ~GLXVsyncSource()
> +  {

nit: assert which thread this is destroyed on.

@@ +708,5 @@
> +    if (!static_cast<GLXVsyncSource::GLXDisplay&>(display).Setup()) {
> +      NS_WARNING("Failed to setup GLContext, falling back to software vsync.");
> +      return gfxPlatform::CreateHardwareVsyncSource();
> +    }
> +    display.EnableVsync();

I think you can leave this disabled when this returns.

Comment 7

[Andrew Comminos [:acomminos]]

Attachment: Implement Linux hardware vsync using GLX_SGI_video_sync.

Updated to address review comments.

Comment 8

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8652625 [details] [diff] [review]
Implement Linux hardware vsync using GLX_SGI_video_sync.

Review of attachment 8652625 [details] [diff] [review]:
-----------------------------------------------------------------

Couple of changes. The biggest is to ensure that we only have 1 vsync loop running at a time.

::: gfx/thebes/gfxPlatformGtk.cpp
@@ +575,5 @@
> +{
> +public:
> +  GLXVsyncSource()
> +  {
> +    mGlobalDisplay = new GLXDisplay();

assert main thread.

@@ +580,5 @@
> +  }
> +
> +  virtual Display& GetGlobalDisplay() override
> +  {
> +    return *mGlobalDisplay;

assert main thread.

@@ +585,5 @@
> +  }
> +
> +  class GLXDisplay final : public VsyncSource::Display
> +  {
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(GLXDisplay)

Don't make this refcounted. Just make the object instantiate as a member of GlxVsyncSource.

@@ +610,5 @@
> +      mVsyncThread.message_loop()->PostTask(FROM_HERE, vsyncSetup);
> +      // Wait until the setup has completed.
> +      lock.Wait();
> +      return mGLContext != nullptr;
> +    }

nit: move SetupGLContext() here.

@@ +618,5 @@
> +      MOZ_ASSERT(NS_IsMainThread());
> +      MOZ_ASSERT(mGLContext, "GLContext not setup!");
> +
> +      MonitorAutoLock lock(mVsyncEnabledLock);
> +      if (mVsyncEnabled)

nit: add {}

@@ +629,5 @@
> +
> +    virtual void DisableVsync() override
> +    {
> +      MonitorAutoLock lock(mVsyncEnabledLock);
> +      mVsyncEnabled = false;

You probably have to keep a pointer to the vsyncStart task from EnableVsync. If We disable Vsync and somehow the vsyncStart hasn't started executing, you need to cancel the vsyncStart task in DisableVsync(). 

A thing that can happen is that vsync is quickly enabled / disabled. In those cases, you could hit an edge case where the vysnc start runs, quickly called DisableVsync(), and then quickly called EnableVsync().  The first posted task would enter the loop and run vsync. The second task would as well, so you'd have 2 vsync loops.

@@ +648,5 @@
> +    }
> +
> +    void SetupGLContext()
> +    {
> +        MonitorAutoLock lock(mSetupLock);

assert ! main thread. Add a comment that this is on the vsync thread.

@@ +652,5 @@
> +        MonitorAutoLock lock(mSetupLock);
> +        MOZ_ASSERT(!mGLContext, "GLContext already setup!");
> +
> +        // Create video sync timer on a separate Display to prevent locking the
> +        // main thread X display.

what happens if we use the main thread X display? If we use a separate X11 display, which physical monitor's vsync does this listen to?

@@ +658,5 @@
> +            gl::CreateContextFlags::USE_SEPARATE_X11_DISPLAY);
> +
> +        // Test that SGI_video_sync lets us get the counter.
> +        unsigned int syncCounter = 0;
> +        if (gl::sGLXLibrary.xGetVideoSync(&syncCounter) != 0)

nit: add {}

@@ +705,5 @@
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +  }
> +
> +  RefPtr<GLXDisplay> mGlobalDisplay;

nit: Make this a member instead of a RefPtr.

Comment 9

[Andrew Comminos [:acomminos]]

Attachment: Implement Linux hardware vsync using GLX_SGI_video_sync.

Updated, thanks.

> what happens if we use the main thread X display? If we use a separate X11 display, which physical monitor's vsync does this listen to?

If we use the main X display, depending on the GLX implementation we can either:

a) crash due to threading issues (mesa)
b) block a lot (NVIDIA)

Neither are preferable. An X11 display doesn't really correspond to a monitor, so nothing effectively changes with regards to what gets synced.

In this case we synchronize with the default CRTC (at least on NVIDIA).

Comment 10

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8652563 [details] [diff] [review]
Add support for creating a GLContextGLX with a separate X11 display.

Review of attachment 8652563 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/GLContextProviderGLX.cpp
@@ +1187,5 @@
>          return nullptr;
>      }
>  
> +    bool createDisplay = static_cast<bool>(flags & CreateContextFlags::USE_SEPARATE_X11_DISPLAY);
> +    Display *display;

Display* display;

Comment 11

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8652938 [details] [diff] [review]
Implement Linux hardware vsync using GLX_SGI_video_sync.

Review of attachment 8652938 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxPlatformGtk.cpp
@@ +698,5 @@
> +        // until the parity of the counter value changes.
> +        unsigned int nextSync = syncCounter + 1;
> +        if (gl::sGLXLibrary.xWaitVideoSync(2, nextSync % 2,
> +                                           &syncCounter) == 0) {
> +          if (syncCounter == (nextSync - 1)) {

can syncCounter ever be less than nextVsync - 1? Do we ever get really bad values?

Comment 12

[Mason Chang [Inactive] [:mchang]]

Will this land anytime soon?

Comment 13

[Andrew Comminos [:acomminos]]

> Will this land anytime soon?

Currently this patch retriggers bug 1189132- even if the test infrastructure is an unstable condition, I'd rather not regress obtaining talos results with GL layers, especially considering that half of the benefit of this patch comes from improved glXSwapBuffers timing for GLX implementations that block until VSync. Maybe we'll have to block on bug 1198290 or a real fix.

Comment 14

[Andrew Comminos [:acomminos]]

Attachment: Extract GLXFBConfig selection code for X11 windows.

This moves the GLXFBConfig matching code for windows into its own function so that we can use it to get the root window GLXFBConfig in the vsync source.

Comment 15

[Andrew Comminos [:acomminos]]

Attachment: Add support for GLX_SGI_video_sync to GLContextProviderGLX. Carry r=jgilbert

Un-bitrotted this one.

Comment 16

[Andrew Comminos [:acomminos]]

Attachment: Implement Linux hardware vsync using GLX_SGI_video_sync. Carry r=mchang

Updated to listen to vsync events on a GLX context hooked up to the root window instead of a pixmap; this is what many compositors (e.g. compton) do for their SGI_video_sync implementation of vsync.

Sending r? to Lee for X11 changes, the vsync strategy hasn't changed.

Comment 17

[Andrew Comminos [:acomminos]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dbd2cf12871a

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f4d6b01a806
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6e83c1c3494
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1e04f19b4a9

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/72356b90a5a5

Comment 20

[Andrew Comminos [:acomminos]]

Looks like this is causing hangs with talos tests similar to what we had when using the GL compositor with GLX (sigh). Reverting the last commit for now while I figure this out.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c6642a759ee

Comment 22

[Andrew Comminos [:acomminos]]

Attachment: Implement Linux hardware vsync using GLX_SGI_video_sync. Carry r=mchang

Updated to destroy the GLContext on the correct thread, close the X11 display, and fix a race condition. Thanks!

Comment 23

[Andrew Comminos [:acomminos]]

With bug 1273286 fixed, we should be good to land this now. If there are still a few machines that are left hanging on talos jobs, the issue should be resolved after a restart.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f20674c7792
Add support for GLX_SGI_video_sync to GLContextProviderGLX. r=jgilbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3930a21b6ed
Implement Linux hardware vsync using GLX_SGI_video_sync. r=mchang,lsalzman

Comment 25

[Mason Chang [Inactive] [:mchang]]

\o/

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4f20674c7792
https://hg.mozilla.org/mozilla-central/rev/b3930a21b6ed