Revert "Remove xrender-sync and xrender-sync-fence"

This reverts commit 50e2259.

Temporarily revert the removal until we have more information about this whole
thing.

Turns off a couple of drivers don't work properly without the sync fence,
including intel, llvmpipe and NVIDIA.

Although sync fence is needed, from the information I have gathered (looking
at old bug reports, protocol specifications, look at other compositors' code),
compton's usage of it is not proper. So we need to rewrite it in the future,
after we get more information from driver developers.

Signed-off-by: Yuxuan Shui <[email protected]>

compton.sample.conf

@@ -77,6 +77,8 @@ invert-color-include = [ ];
 # glx-no-rebind-pixmap = true;
 glx-swap-method = "undefined";
 # glx-use-gpushader4 = true;
+# xrender-sync = true;
+# xrender-sync-fence = true;
 
 # Window type settings
 wintypes:

man/compton.1.asciidoc

@@ -242,7 +242,7 @@ May also be one of the predefined kernels: `3x3box` (default), `5x5box`, `7x7box
 +
 --
 * `xrender` backend performs all rendering operations with X Render extension. It is what `xcompmgr` uses, and is generally a safe fallback when you encounter rendering artifacts or instability.
-* `glx` (OpenGL) backend performs all rendering operations with OpenGL. It is more friendly to some VSync methods, and has significantly superior performance on color inversion (`--invert-color-include`) or blur (`--blur-background`). It requires proper OpenGL 2.0 support from your driver and hardware. You may wish to look at the GLX performance optimization options below.
+* `glx` (OpenGL) backend performs all rendering operations with OpenGL. It is more friendly to some VSync methods, and has significantly superior performance on color inversion (`--invert-color-include`) or blur (`--blur-background`). It requires proper OpenGL 2.0 support from your driver and hardware. You may wish to look at the GLX performance optimization options below. `--xrender-sync` and `--xrender-sync-fence` might be needed on some systems to avoid delay in changes of screen contents.
 * `xr_glx_hybrid` backend renders the updated screen contents with X Render and presents it on the screen with GLX. It attempts to address the rendering issues some users encountered with GLX backend and enables the better VSync of GLX backends. `--vsync-use-glfinish` might fix some rendering issues with this backend.
 --
 
@@ -258,6 +258,12 @@ May also be one of the predefined kernels: `3x3box` (default), `5x5box`, `7x7box
 *--glx-use-gpushader4*::
 	GLX backend: Use 'GL_EXT_gpu_shader4' for some optimization on blur GLSL code. My tests on GTX 670 show no noticeable effect.
 
+*--xrender-sync*::
+	Attempt to synchronize client applications' draw calls with `XSync()`, used on GLX backend to ensure up-to-date window content is painted.
+
+*--xrender-sync-fence*::
+	Additionally use X Sync fence to sync clients' draw calls. Needed on nvidia-drivers with GLX backend for some users. May be disabled at compile time with `NO_XSYNC=1`.
+
 *--glx-fshader-win* 'SHADER'::
 	GLX backend: Use specified GLSL fragment shader for rendering window contents. See `compton-default-fshader-win.glsl` and `compton-fake-transparency-fshader-win.glsl` in the source tree for examples.
 

src/common.h

@@ -49,6 +49,10 @@
 // #define CONFIG_OPENGL 1
 // Whether to enable DBus support with libdbus.
 // #define CONFIG_DBUS 1
+// Whether to enable X Sync support.
+// #define CONFIG_XSYNC 1
+// Whether to enable GLX Sync support.
+// #define CONFIG_GLX_XSYNC 1
 
 #ifndef COMPTON_VERSION
 #define COMPTON_VERSION "unknown"
@@ -73,6 +77,7 @@
 
 #include <X11/Xlib-xcb.h>
 #include <X11/Xlib.h>
+#include <X11/extensions/sync.h>
 
 #include <xcb/composite.h>
 #include <xcb/render.h>
@@ -310,7 +315,7 @@ typedef int (*f_SwapIntervalMESA) (unsigned int interval);
 typedef void (*f_BindTexImageEXT) (Display *display, GLXDrawable drawable, int buffer, const int *attrib_list);
 typedef void (*f_ReleaseTexImageEXT) (Display *display, GLXDrawable drawable, int buffer);
 
-#ifdef CONFIG_GLX_SYNC
+#ifdef CONFIG_OPENGL
 // Looks like duplicate typedef of the same type is safe?
 typedef int64_t GLint64;
 typedef uint64_t GLuint64;
@@ -449,6 +454,11 @@ typedef struct options_t {
   char *display_repr;
   /// The backend in use.
   enum backend backend;
+  /// Whether to sync X drawing to avoid certain delay issues with
+  /// GLX backend.
+  bool xrender_sync;
+  /// Whether to sync X drawing with X Sync fence.
+  bool xrender_sync_fence;
   /// Whether to avoid using stencil buffer under GLX backend. Might be
   /// unsafe.
   bool glx_no_stencil;
@@ -646,7 +656,6 @@ typedef struct {
   f_BindTexImageEXT glXBindTexImageProc;
   /// Pointer to glXReleaseTexImageEXT function.
   f_ReleaseTexImageEXT glXReleaseTexImageProc;
-#ifdef CONFIG_GLX_SYNC
   /// Pointer to the glFenceSync() function.
   f_FenceSync glFenceSyncProc;
   /// Pointer to the glIsSync() function.
@@ -659,7 +668,6 @@ typedef struct {
   f_WaitSync glWaitSyncProc;
   /// Pointer to the glImportSyncEXT() function.
   f_ImportSyncEXT glImportSyncEXT;
-#endif
 #ifdef DEBUG_GLX_MARK
   /// Pointer to StringMarkerGREMEDY function.
   f_StringMarkerGREMEDY glStringMarkerGREMEDY;
@@ -721,6 +729,7 @@ typedef struct session {
   xcb_render_picture_t tgt_picture;
   /// Temporary buffer to paint to before sending to display.
   paint_t tgt_buffer;
+  XSyncFence tgt_buffer_fence;
   /// Window ID of the window we register as a symbol.
   Window reg_win;
 #ifdef CONFIG_OPENGL
@@ -885,6 +894,12 @@ typedef struct session {
   /// Number of Xinerama screens.
   int xinerama_nscrs;
 #endif
+  /// Whether X Sync extension exists.
+  bool xsync_exists;
+  /// Event base number for X Sync extension.
+  int xsync_event;
+  /// Error base number for X Sync extension.
+  int xsync_error;
   /// Whether X Render convolution filter exists.
   bool xrfilter_convolution_exists;
 
@@ -1463,6 +1478,16 @@ free_all_damage_last(session_t *ps) {
     pixman_region32_clear(&ps->all_damage_last[i]);
 }
 
+/**
+ * Free a XSync fence.
+ */
+static inline void
+free_fence(session_t *ps, XSyncFence *pfence) {
+  if (*pfence)
+    XSyncDestroyFence(ps->dpy, *pfence);
+  *pfence = None;
+}
+
 /**
  * Check if a rectangle includes the whole screen.
  */
@@ -1613,10 +1638,8 @@ vsync_deinit(session_t *ps);
  */
 ///@{
 
-#ifdef CONFIG_GLX_SYNC
 void
 xr_glx_sync(session_t *ps, Drawable d, XSyncFence *pfence);
-#endif
 
 /**
  * Free a GLX texture.
@@ -1701,6 +1724,48 @@ glx_mark_frame(session_t *ps) {
 
 ///@}
 
+/**
+ * Synchronizes a X Render drawable to ensure all pending painting requests
+ * are completed.
+ */
+static inline void
+xr_sync(session_t *ps, Drawable d, XSyncFence *pfence) {
+  if (!ps->o.xrender_sync)
+    return;
+
+  x_sync(ps->c);
+  if (ps->o.xrender_sync_fence && ps->xsync_exists) {
+    // TODO: If everybody just follows the rules stated in X Sync prototype,
+    // we need only one fence per screen, but let's stay a bit cautious right
+    // now
+    XSyncFence tmp_fence = None;
+    if (!pfence)
+      pfence = &tmp_fence;
+    assert(pfence);
+    if (!*pfence)
+      *pfence = XSyncCreateFence(ps->dpy, d, False);
+    if (*pfence) {
+      Bool __attribute__((unused)) triggered = False;
+      /* if (XSyncQueryFence(ps->dpy, *pfence, &triggered) && triggered)
+        XSyncResetFence(ps->dpy, *pfence); */
+      // The fence may fail to be created (e.g. because of died drawable)
+      assert(!XSyncQueryFence(ps->dpy, *pfence, &triggered) || !triggered);
+      XSyncTriggerFence(ps->dpy, *pfence);
+      XSyncAwaitFence(ps->dpy, pfence, 1);
+      assert(!XSyncQueryFence(ps->dpy, *pfence, &triggered) || triggered);
+    }
+    else {
+      printf_errf("(%#010lx): Failed to create X Sync fence.", d);
+    }
+    free_fence(ps, &tmp_fence);
+    if (*pfence)
+      XSyncResetFence(ps->dpy, *pfence);
+  }
+#ifdef OPENGL
+  xr_glx_sync(ps, d, pfence);
+#endif
+}
+
 /** @name DBus handling
  */
 ///@{

src/compton.c

@@ -248,6 +248,7 @@ static inline void
 free_win_res(session_t *ps, win *w) {
   free_win_res_glx(ps, w);
   free_paint(ps, &w->paint);
+  free_fence(ps, &w->fence);
   pixman_region32_fini(&w->bounding_shape);
   free_paint(ps, &w->shadow_paint);
   // BadDamage may be thrown if the window is destroyed
@@ -886,6 +887,9 @@ win_build_shadow(session_t *ps, win *w, double opacity) {
   assert(!w->shadow_paint.pict);
   w->shadow_paint.pict = shadow_picture_argb;
 
+  // Sync it once and only once
+  xr_sync(ps, w->shadow_paint.pixmap, NULL);
+
   xcb_free_gc(ps->c, gc);
   xcb_image_destroy(shadow_image);
   xcb_free_pixmap(ps->c, shadow_pixmap);
@@ -1627,6 +1631,8 @@ paint_one(session_t *ps, win *w, const region_t *reg_paint) {
     w->paint.pixmap = xcb_generate_id(ps->c);
     set_ignore_cookie(ps,
         xcb_composite_name_window_pixmap(ps->c, w->id, w->paint.pixmap));
+    if (w->paint.pixmap)
+      free_fence(ps, &w->fence);
   }
 
   Drawable draw = w->paint.pixmap;
@@ -1643,6 +1649,9 @@ paint_one(session_t *ps, win *w, const region_t *reg_paint) {
         draw, XCB_RENDER_CP_SUBWINDOW_MODE, &pa);
   }
 
+  if (IsViewable == w->a.map_state)
+    xr_sync(ps, draw, &w->fence);
+
   // GLX: Build texture
   // Let glx_bind_pixmap() determine pixmap size, because if the user
   // is resizing windows, the width and height we get may not be up-to-date,
@@ -2023,9 +2032,12 @@ paint_all(session_t *ps, region_t *region, const region_t *region_real, win * co
         glFlush();
       glXWaitX();
       assert(ps->tgt_buffer.pixmap);
+      xr_sync(ps, ps->tgt_buffer.pixmap, &ps->tgt_buffer_fence);
       paint_bind_tex(ps, &ps->tgt_buffer,
           ps->root_width, ps->root_height, ps->depth,
           !ps->o.glx_no_rebind_pixmap);
+      // See #163
+      xr_sync(ps, ps->tgt_buffer.pixmap, &ps->tgt_buffer_fence);
       if (ps->o.vsync_use_glfinish)
         glFinish();
       else
@@ -2259,6 +2271,11 @@ unmap_win(session_t *ps, win **_w) {
 
   if (w->destroyed) return;
 
+  // One last synchronization
+  if (w->paint.pixmap)
+    xr_sync(ps, w->paint.pixmap, &w->fence);
+  free_fence(ps, &w->fence);
+
   // Set focus out
   win_set_focused(ps, w, false);
 
@@ -2723,6 +2740,14 @@ ev_name(session_t *ps, xcb_generic_event_t *ev) {
   if (ps->shape_exists && ev->response_type == ps->shape_event)
     return "ShapeNotify";
 
+  if (ps->xsync_exists) {
+    int o = ev->response_type - ps->xsync_event;
+    switch (o) {
+      CASESTRRET(XSyncCounterNotify);
+      CASESTRRET(XSyncAlarmNotify);
+    }
+  }
+
   sprintf(buf, "Event %d", ev->response_type);
 
   return buf;
@@ -3570,7 +3595,6 @@ usage(int ret) {
     "--backend backend\n"
     "  Choose backend. Possible choices are xrender, glx, and\n"
     "  xr_glx_hybrid" WARNING ".\n"
-#undef WARNING
     "\n"
     "--glx-no-stencil\n"
     "  GLX backend: Avoid using stencil buffer. Might cause issues\n"
@@ -3595,6 +3619,16 @@ usage(int ret) {
     "  GLX backend: Use GL_EXT_gpu_shader4 for some optimization on blur\n"
     "  GLSL code. My tests on GTX 670 show no noticeable effect.\n"
     "\n"
+    "--xrender-sync\n"
+    "  Attempt to synchronize client applications' draw calls with XSync(),\n"
+    "  used on GLX backend to ensure up-to-date window content is painted.\n"
+#undef WARNING
+#define WARNING
+    "\n"
+    "--xrender-sync-fence\n"
+    "  Additionally use X Sync fence to sync clients' draw calls. Needed\n"
+    "  on nvidia-drivers with GLX backend for some users." WARNING "\n"
+    "\n"
     "--glx-fshader-win shader\n"
     "  GLX backend: Use specified GLSL fragment shader for rendering window\n"
     "  contents.\n"
@@ -3612,7 +3646,6 @@ usage(int ret) {
     "--dbus\n"
     "  Enable remote control via D-Bus. See the D-BUS API section in the\n"
     "  man page for more details." WARNING "\n"
-#undef WARNING
     "\n"
     "--benchmark cycles\n"
     "  Benchmark mode. Repeatedly paint until reaching the specified cycles.\n"
@@ -3626,6 +3659,7 @@ usage(int ret) {
     ;
   FILE *f = (ret ? stderr: stdout);
   fputs(usage_text, f);
+#undef WARNING
 #undef WARNING_DISABLED
 
   exit(ret);
@@ -4154,12 +4188,8 @@ get_cfg(session_t *ps, int argc, char *const *argv, bool first_pass) {
         ps->o.write_pid_path = mstrcpy(optarg);
         break;
       P_CASEBOOL(311, vsync_use_glfinish);
-      case 312:
-        printf_errf("(): --xrender-sync %s", deprecation_message);
-        break;
-      case 313:
-        printf_errf("(): --xrender-sync-fence %s", deprecation_message);
-        break;
+      P_CASEBOOL(312, xrender_sync);
+      P_CASEBOOL(313, xrender_sync_fence);
       P_CASEBOOL(315, no_fading_destroyed_argb);
       P_CASEBOOL(316, force_win_blend);
       case 317:
@@ -4216,6 +4246,9 @@ get_cfg(session_t *ps, int argc, char *const *argv, bool first_pass) {
   if (ps->o.blur_background_frame)
     ps->o.blur_background = true;
 
+  if (ps->o.xrender_sync_fence)
+    ps->o.xrender_sync = true;
+
   // Other variables determined by options
 
   // Determine whether we need to track focus changes
@@ -4789,8 +4822,10 @@ redir_stop(session_t *ps) {
     // Destroy all Pictures as they expire once windows are unredirected
     // If we don't destroy them here, looks like the resources are just
     // kept inaccessible somehow
-    for (win *w = ps->list; w; w = w->next)
+    for (win *w = ps->list; w; w = w->next) {
       free_paint(ps, &w->paint);
+      free_fence(ps, &w->fence);
+    }
 
     xcb_composite_unredirect_subwindows(ps->c, ps->root, XCB_COMPOSITE_REDIRECT_MANUAL);
     // Unmap overlay window
@@ -5301,6 +5336,20 @@ session_init(session_t *ps_old, int argc, char **argv) {
     ps->present_exists = true;
   }
 
+  // Query X Sync
+  if (XSyncQueryExtension(ps->dpy, &ps->xsync_event, &ps->xsync_error)) {
+    // TODO: Fencing may require version >= 3.0?
+    int major_version_return = 0, minor_version_return = 0;
+    if (XSyncInitialize(ps->dpy, &major_version_return, &minor_version_return))
+      ps->xsync_exists = true;
+  }
+
+  if (!ps->xsync_exists && ps->o.xrender_sync_fence) {
+    printf_errf("(): X Sync extension not found. No X Sync fence sync is "
+        "possible.");
+    exit(1);
+  }
+
   // Query X RandR
   if ((ps->o.sw_opti && !ps->o.refresh_rate) || ps->o.xinerama_shadow_crop) {
     if (!ps->randr_exists) {
@@ -5614,6 +5663,7 @@ session_destroy(session_t *ps) {
     ps->tgt_picture = None;
   else
     free_picture(ps, &ps->tgt_picture);
+  free_fence(ps, &ps->tgt_buffer_fence);
 
   free_picture(ps, &ps->root_picture);
   free_paint(ps, &ps->tgt_buffer);

src/config_libconfig.c

@@ -353,6 +353,10 @@ parse_config(session_t *ps, struct options_tmp *pcfgtmp) {
     exit(1);
   // --glx-use-gpushader4
   lcfg_lookup_bool(&cfg, "glx-use-gpushader4", &ps->o.glx_use_gpushader4);
+  // --xrender-sync
+  lcfg_lookup_bool(&cfg, "xrender-sync", &ps->o.xrender_sync);
+  // --xrender-sync-fence
+  lcfg_lookup_bool(&cfg, "xrender-sync-fence", &ps->o.xrender_sync_fence);
 
   if (lcfg_lookup_bool(&cfg, "clear-shadow", &bval))
     printf_errf("(): \"clear-shadow\" is removed as an option, and is always"
@@ -367,10 +371,6 @@ parse_config(session_t *ps, struct options_tmp *pcfgtmp) {
     printf_errf("(): \"glx-use-copysubbuffermesa\" %s", deprecation_message);
   if (lcfg_lookup_bool(&cfg, "glx-copy-from-front", &bval) && bval)
     printf_errf("(): \"glx-copy-from-front\" %s", deprecation_message);
-  if (lcfg_lookup_bool(&cfg, "xrender-sync", &bval) && bval)
-    printf_errf("(): \"xrender-sync\" %s", deprecation_message);
-  if (lcfg_lookup_bool(&cfg, "xrender-sync-fence", &bval) && bval)
-    printf_errf("(): \"xrender-sync-fence\" %s", deprecation_message);
 
   // Wintype settings