From 10f2b11fda3cd3d92d8333babbeb4e8ca2dbea16 Mon Sep 17 00:00:00 2001 From: Ray Strode Date: Wed, 27 May 2020 14:53:10 -0400 Subject: [PATCH 1/3] x11: Add back support for the damage extension commit 14bf58ec5dfdf19e3ca603b977626608dafc729b dropped support for using the DAMAGE extension since there was no code that needed it. We're going to need it again, however, to address an NVidia vendor driver issue. This commit does the plumbing to add it back. --- config.h.meson | 3 +++ gdk/x11/gdkdisplay-x11.c | 8 ++++++++ gdk/x11/gdkdisplay-x11.h | 6 ++++++ gdk/x11/meson.build | 1 + meson.build | 5 +++++ 5 files changed, 23 insertions(+) diff --git a/config.h.meson b/config.h.meson index ee5ddbf7b3..d8d5228693 100644 --- a/config.h.meson +++ b/config.h.meson @@ -123,6 +123,9 @@ /* Have the Xcursor library */ #mesondefine HAVE_XCURSOR +/* Have the XDAMAGE X extension */ +#mesondefine HAVE_XDAMAGE + /* Have the XFIXES X extension */ #mesondefine HAVE_XFIXES diff --git a/gdk/x11/gdkdisplay-x11.c b/gdk/x11/gdkdisplay-x11.c index 700ffb2d28..6acf25c37d 100644 --- a/gdk/x11/gdkdisplay-x11.c +++ b/gdk/x11/gdkdisplay-x11.c @@ -1603,6 +1603,14 @@ gdk_x11_display_open (const gchar *display_name) } #endif +#ifdef HAVE_XDAMAGE + display_x11->have_damage = FALSE; + if (XDamageQueryExtension (display_x11->xdisplay, + &display_x11->damage_event_base, + &display_x11->damage_error_base)) + display_x11->have_damage = TRUE; +#endif + display->clipboard = gdk_x11_clipboard_new (display, "CLIPBOARD"); display->primary_clipboard = gdk_x11_clipboard_new (display, "PRIMARY"); diff --git a/gdk/x11/gdkdisplay-x11.h b/gdk/x11/gdkdisplay-x11.h index 272646b65f..38ddda5d60 100644 --- a/gdk/x11/gdkdisplay-x11.h +++ b/gdk/x11/gdkdisplay-x11.h @@ -149,6 +149,12 @@ struct _GdkX11Display guint has_glx_multisample : 1; guint has_glx_visual_rating : 1; guint has_glx_create_es2_context : 1; + +#ifdef HAVE_XDAMAGE + gint damage_event_base; + gint damage_error_base; + guint have_damage; +#endif }; struct _GdkX11DisplayClass diff --git a/gdk/x11/meson.build b/gdk/x11/meson.build index c3f2c54b73..f846450c3b 100644 --- a/gdk/x11/meson.build +++ b/gdk/x11/meson.build @@ -65,6 +65,7 @@ gdk_x11_deps = [ xext_dep, x11_dep, xcursor_dep, + xdamage_dep, xfixes_dep, xcomposite_dep, xrandr_dep, diff --git a/meson.build b/meson.build index 9295163603..a8a3c7cdbe 100644 --- a/meson.build +++ b/meson.build @@ -501,6 +501,7 @@ if x11_enabled xi_dep = dependency('xi') xext_dep = dependency('xext') xcursor_dep = dependency('xcursor', required: false) + xdamage_dep = dependency('xdamage', required: false) xfixes_dep = dependency('xfixes', required: false) xcomposite_dep = dependency('xcomposite', required: false) fontconfig_dep = dependency('fontconfig') @@ -513,6 +514,9 @@ if x11_enabled if xcursor_dep.found() x11_pkgs += ['xcursor'] endif + if xdamage_dep.found() + x11_pkgs += ['xdamage'] + endif if xfixes_dep.found() x11_pkgs += ['xfixes'] endif @@ -523,6 +527,7 @@ if x11_enabled atk_pkgs += ['atk-bridge-2.0'] cdata.set('HAVE_XCURSOR', xcursor_dep.found()) + cdata.set('HAVE_XDAMAGE', xdamage_dep.found()) cdata.set('HAVE_XCOMPOSITE', xcomposite_dep.found()) cdata.set('HAVE_XFIXES', xfixes_dep.found()) From f8770b78ea87751407f94e39282eeca3ba04ca09 Mon Sep 17 00:00:00 2001 From: Ray Strode Date: Tue, 2 Jun 2020 16:29:03 -0400 Subject: [PATCH 2/3] x11: Factor out some of frame sync code into subroutines This commit moves some of the end frame sync counter handling code to subroutines. It's a minor readability win, but the main motivation is to make it easier in a subsequent commit to defer updating the sync counter until a more appropriate time. --- gdk/x11/gdksurface-x11.c | 39 +++++++++++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 6 deletions(-) diff --git a/gdk/x11/gdksurface-x11.c b/gdk/x11/gdksurface-x11.c index 41d9d4d91d..8ac6d0f651 100644 --- a/gdk/x11/gdksurface-x11.c +++ b/gdk/x11/gdksurface-x11.c @@ -360,6 +360,37 @@ gdk_x11_surface_begin_frame (GdkSurface *surface, } } +static gboolean +should_sync_frame_drawing (GdkSurface *surface) +{ + GdkX11Surface *impl = GDK_X11_SURFACE (surface); + + /* disabled client side */ + if (!impl->frame_sync_enabled) + return FALSE; + + /* disabled compositor side */ + if (!gdk_x11_screen_supports_net_wm_hint (GDK_SURFACE_SCREEN (surface), + g_intern_static_string ("_NET_WM_FRAME_DRAWN"))) + return FALSE; + + return TRUE; +} + +static void +sync_counter_for_end_frame (GdkSurface *surface) +{ + GdkX11Surface *impl = GDK_X11_SURFACE (surface); + + g_assert (!impl->toplevel->in_frame); + g_assert (impl->toplevel->extended_update_counter != None); + g_assert ((impl->toplevel->current_counter_value % 2) == 0); + + set_sync_counter (GDK_SURFACE_XDISPLAY (surface), + impl->toplevel->extended_update_counter, + impl->toplevel->current_counter_value); +} + static void gdk_x11_surface_end_frame (GdkSurface *surface) { @@ -405,13 +436,9 @@ gdk_x11_surface_end_frame (GdkSurface *surface) else impl->toplevel->current_counter_value += 1; - set_sync_counter(GDK_SURFACE_XDISPLAY (surface), - impl->toplevel->extended_update_counter, - impl->toplevel->current_counter_value); + sync_counter_for_end_frame (surface); - if (impl->frame_sync_enabled && - gdk_x11_screen_supports_net_wm_hint (GDK_SURFACE_SCREEN (surface), - g_intern_static_string ("_NET_WM_FRAME_DRAWN"))) + if (should_sync_frame_drawing (surface)) { impl->toplevel->frame_pending = TRUE; gdk_surface_freeze_updates (surface); From 972134abe48a4c9c7b6ad41b0723f30f4e7ae16b Mon Sep 17 00:00:00 2001 From: Ray Strode Date: Wed, 27 May 2020 15:12:35 -0400 Subject: [PATCH 3/3] x11: Defer _NET_WM_FRAME_DRAWN update until frame usable by compositor With the vendor provided Nvidia driver there is a small window of time after drawing to a GL surface before the updates to that surface can be used by the compositor. Drawing is already coordinated with the compositor through the frame synchronization protocol detailed here: https://fishsoup.net/misc/wm-spec-synchronization.html Unfortunately, at the moment, GdkX11Surface tells the compositor the frame is ready immediately after drawing to the surface, not later, when it's consumable by the compositor. This commit defers announcing the frame as ready until it's consumable by the compositor. It does this by listening for the X server to announce damage events associated with the frame drawing. It tries to find the right damage event by waiting until fence placed at buffer swap time signals. --- gdk/x11/gdkdisplay-x11.h | 1 + gdk/x11/gdkglcontext-x11.c | 128 +++++++++++++++++++++++++++++++++++++ gdk/x11/gdkglcontext-x11.h | 9 +++ gdk/x11/gdkprivate-x11.h | 5 ++ gdk/x11/gdksurface-x11.c | 42 +++++++++++- gdk/x11/gdksurface-x11.h | 3 + 6 files changed, 187 insertions(+), 1 deletion(-) diff --git a/gdk/x11/gdkdisplay-x11.h b/gdk/x11/gdkdisplay-x11.h index 38ddda5d60..39e9219d30 100644 --- a/gdk/x11/gdkdisplay-x11.h +++ b/gdk/x11/gdkdisplay-x11.h @@ -149,6 +149,7 @@ struct _GdkX11Display guint has_glx_multisample : 1; guint has_glx_visual_rating : 1; guint has_glx_create_es2_context : 1; + guint has_async_glx_swap_buffers : 1; #ifdef HAVE_XDAMAGE gint damage_event_base; diff --git a/gdk/x11/gdkglcontext-x11.c b/gdk/x11/gdkglcontext-x11.c index 95d8e66c66..72c775885b 100644 --- a/gdk/x11/gdkglcontext-x11.c +++ b/gdk/x11/gdkglcontext-x11.c @@ -183,6 +183,22 @@ gdk_x11_gl_context_end_frame (GdkDrawContext *draw_context, gdk_x11_surface_pre_damage (surface); +#ifdef HAVE_XDAMAGE + if (context_x11->xdamage != 0) + { + g_assert (context_x11->frame_fence == 0); + + context_x11->frame_fence = glFenceSync (GL_SYNC_GPU_COMMANDS_COMPLETE, 0); + + /* We consider the frame still getting painted until the GL operation is + * finished, and the window gets damage reported from the X server. + * It's only at this point the compositor can be sure it has full + * access to the new updates. + */ + _gdk_x11_surface_set_frame_still_painting (surface, TRUE); + } +#endif + glXSwapBuffers (dpy, drawable); if (context_x11->do_frame_sync && info != NULL && display_x11->has_glx_video_sync) @@ -565,6 +581,81 @@ create_legacy_context (GdkDisplay *display, return res; } +#ifdef HAVE_XDAMAGE +static gboolean +on_gl_surface_xevent (GdkGLContext *context, + XEvent *xevent, + GdkX11Display *display_x11) +{ + GdkX11GLContext *context_x11 = GDK_X11_GL_CONTEXT (context); + GdkSurface *surface = gdk_gl_context_get_surface (context); + XDamageNotifyEvent *damage_xevent; + + if (!context_x11->is_attached) + return FALSE; + + if (xevent->type != (display_x11->damage_event_base + XDamageNotify)) + return FALSE; + + damage_xevent = (XDamageNotifyEvent *) xevent; + + if (damage_xevent->damage != context_x11->xdamage) + return FALSE; + + if (context_x11->frame_fence) + { + GLenum wait_result; + + wait_result = glClientWaitSync (context_x11->frame_fence, 0, 0); + + switch (wait_result) + { + /* We assume that if the fence has been signaled, that this damage + * event is the damage event that was triggered by the GL drawing + * associated with the fence. That's, technically, not necessarly + * always true. The X server could have generated damage for + * an unrelated event (say the size of the window changing), at + * just the right moment such that we're picking it up instead. + * + * We're choosing not to handle this edge case, but if it does ever + * happen in the wild, it could lead to slight underdrawing by + * the compositor for one frame. In the future, if we find out + * this edge case is noticeable, we can compensate by copying the + * painted region from gdk_x11_gl_context_end_frame and subtracting + * damaged areas from the copy as they come in. Once the copied + * region goes empty, we know that there won't be any underdraw, + * and can mark painting has finished. It's not worth the added + * complexity and resource usage to do this bookkeeping, however, + * unless the problem is practically visible. + */ + case GL_ALREADY_SIGNALED: + case GL_CONDITION_SATISFIED: + case GL_WAIT_FAILED: + if (wait_result == GL_WAIT_FAILED) + g_warning ("failed to wait on GL fence associated with last swap buffers call"); + glDeleteSync (context_x11->frame_fence); + context_x11->frame_fence = 0; + _gdk_x11_surface_set_frame_still_painting (surface, FALSE); + break; + + /* We assume that if the fence hasn't been signaled, that this + * damage event is not the damage event that was triggered by the + * GL drawing associated with the fence. That's only true for + * the Nvidia vendor driver. When using open source drivers, damage + * is emitted immediately on swap buffers, before the fence ever + * has a chance to signal. + */ + case GL_TIMEOUT_EXPIRED: + break; + default: + g_assert_not_reached (); + } + } + + return FALSE; +} +#endif + static gboolean gdk_x11_gl_context_realize (GdkGLContext *context, GError **error) @@ -737,6 +828,24 @@ gdk_x11_gl_context_realize (GdkGLContext *context, context_x11->attached_drawable = info->glx_drawable ? info->glx_drawable : gdk_x11_surface_get_xid (surface); context_x11->unattached_drawable = info->dummy_glx ? info->dummy_glx : info->dummy_xwin; +#ifdef HAVE_XDAMAGE + if (display_x11->have_damage && display_x11->has_async_glx_swap_buffers) + { + gdk_x11_display_error_trap_push (display); + context_x11->xdamage = XDamageCreate (dpy, + gdk_x11_surface_get_xid (surface), + XDamageReportRawRectangles); + if (gdk_x11_display_error_trap_pop (display)) + context_x11->xdamage = 0; + else + g_signal_connect_object (G_OBJECT (display), + "xevent", + G_CALLBACK (on_gl_surface_xevent), + context, + G_CONNECT_SWAPPED); + } +#endif + context_x11->is_direct = glXIsDirect (dpy, context_x11->glx_context); GDK_DISPLAY_NOTE (display, OPENGL, @@ -766,6 +875,10 @@ gdk_x11_gl_context_dispose (GObject *gobject) context_x11->glx_context = NULL; } +#ifdef HAVE_XDAMAGE + context_x11->xdamage = 0; +#endif + G_OBJECT_CLASS (gdk_x11_gl_context_parent_class)->dispose (gobject); } @@ -841,6 +954,21 @@ gdk_x11_screen_init_gl (GdkX11Screen *screen) display_x11->has_glx_visual_rating = epoxy_has_glx_extension (dpy, screen_num, "GLX_EXT_visual_rating"); + if (g_strcmp0 (glXGetClientString (dpy, GLX_VENDOR), "NVIDIA Corporation") == 0) + { + /* With the mesa based drivers, we can safely assume the compositor can + * access the updated surface texture immediately after glXSwapBuffers is + * run, because the kernel ensures there is an implicit synchronization + * operation upon texture access. This is not true with the Nvidia vendor + * driver. There is a window of time after glXSwapBuffers before other + * processes can see the updated drawing. We need to take special care, + * in that case, to defer telling the compositor our latest frame is + * ready until after the GPU has completed all issued commands related + * to the frame, and that the X server says the frame has been drawn. + */ + display_x11->has_async_glx_swap_buffers = TRUE; + } + GDK_DISPLAY_NOTE (display, OPENGL, g_message ("GLX version %d.%d found\n" " - Vendor: %s\n" diff --git a/gdk/x11/gdkglcontext-x11.h b/gdk/x11/gdkglcontext-x11.h index 89d1131fcc..b17ed2956e 100644 --- a/gdk/x11/gdkglcontext-x11.h +++ b/gdk/x11/gdkglcontext-x11.h @@ -24,6 +24,10 @@ #include #include +#ifdef HAVE_XDAMAGE +#include +#endif + #include #include @@ -44,6 +48,11 @@ struct _GdkX11GLContext GLXDrawable attached_drawable; GLXDrawable unattached_drawable; +#ifdef HAVE_XDAMAGE + GLsync frame_fence; + Damage xdamage; +#endif + guint is_attached : 1; guint is_direct : 1; guint do_frame_sync : 1; diff --git a/gdk/x11/gdkprivate-x11.h b/gdk/x11/gdkprivate-x11.h index 7b8fbc7711..23540383cf 100644 --- a/gdk/x11/gdkprivate-x11.h +++ b/gdk/x11/gdkprivate-x11.h @@ -104,6 +104,11 @@ void _gdk_x11_surface_grab_check_unmap (GdkSurface *window, gulong serial); void _gdk_x11_surface_grab_check_destroy (GdkSurface *window); +#ifdef HAVE_XDAMAGE +void _gdk_x11_surface_set_frame_still_painting (GdkSurface *surface, + gboolean painting); +#endif + gboolean _gdk_x11_display_is_root_window (GdkDisplay *display, Window xroot_window); diff --git a/gdk/x11/gdksurface-x11.c b/gdk/x11/gdksurface-x11.c index 8ac6d0f651..0bae7a5e03 100644 --- a/gdk/x11/gdksurface-x11.c +++ b/gdk/x11/gdksurface-x11.c @@ -391,6 +391,46 @@ sync_counter_for_end_frame (GdkSurface *surface) impl->toplevel->current_counter_value); } +static void +maybe_sync_counter_for_end_frame (GdkSurface *surface) +{ + GdkX11Surface *impl = GDK_X11_SURFACE (surface); + gboolean frame_sync_negotiated = should_sync_frame_drawing (surface); + gboolean frame_done_painting = !impl->toplevel->frame_pending; + +#ifdef HAVE_XDAMAGE + frame_done_painting = !impl->toplevel->frame_still_painting && frame_sync_negotiated; +#endif + + if (!impl->toplevel->frame_pending) + { + if (!frame_sync_negotiated || frame_done_painting) + sync_counter_for_end_frame (surface); + } + else + { + if (frame_done_painting) + sync_counter_for_end_frame (surface); + } +} + +#ifdef HAVE_XDAMAGE +void +_gdk_x11_surface_set_frame_still_painting (GdkSurface *surface, + gboolean painting) +{ + GdkX11Surface *impl = GDK_X11_SURFACE (surface); + + if (impl->toplevel->frame_still_painting == painting) + return; + + impl->toplevel->frame_still_painting = painting; + + if (!impl->toplevel->frame_still_painting) + maybe_sync_counter_for_end_frame (surface); +} +#endif + static void gdk_x11_surface_end_frame (GdkSurface *surface) { @@ -436,7 +476,7 @@ gdk_x11_surface_end_frame (GdkSurface *surface) else impl->toplevel->current_counter_value += 1; - sync_counter_for_end_frame (surface); + maybe_sync_counter_for_end_frame (surface); if (should_sync_frame_drawing (surface)) { diff --git a/gdk/x11/gdksurface-x11.h b/gdk/x11/gdksurface-x11.h index 8d8e255a36..5715c2f21d 100644 --- a/gdk/x11/gdksurface-x11.h +++ b/gdk/x11/gdksurface-x11.h @@ -124,6 +124,9 @@ struct _GdkToplevelX11 guint in_frame : 1; + /* If we're waiting for damage from the X server after painting a frame */ + guint frame_still_painting : 1; + /* If we're expecting a response from the compositor after painting a frame */ guint frame_pending : 1;