Refactor the XSync fence code

Use a temporary fence everytime. Convert the Xlib XSync functions to use
xcb_sync_*.

Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

man/compton.1.asciidoc

@@ -235,7 +235,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. `--xrender-sync` and `--xrender-sync-fence` might be needed on some systems to avoid delay in changes of screen contents.
+* `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-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.
 --
 
@@ -251,11 +251,8 @@ 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`.
+	Use X Sync fence to sync clients' draw calls, to make sure all draw calls are finished before compton starts drawing. Needed on nvidia-drivers with GLX backend for some users.
 
 *--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

@@ -65,10 +65,6 @@
 #include <ctype.h>
 #include <sys/time.h>
 
-#include <X11/Xlib-xcb.h>
-#include <X11/Xlib.h>
-#include <X11/extensions/sync.h>
-
 #include <xcb/composite.h>
 #include <xcb/render.h>
 #include <xcb/damage.h>
@@ -461,7 +457,6 @@ 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.
   xcb_window_t reg_win;
 #ifdef CONFIG_OPENGL
@@ -970,16 +965,6 @@ 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 = XCB_NONE;
-}
-
 /**
  * Check if a rectangle includes the whole screen.
  */
@@ -1138,45 +1123,6 @@ 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 attr_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 {
-      log_error("Failed to create X Sync fence for %#010lx", d);
-    }
-    free_fence(ps, &tmp_fence);
-    if (*pfence)
-      XSyncResetFence(ps->dpy, *pfence);
-  }
-}
-
 /** @name DBus handling
  */
 ///@{

src/compton.c

@@ -12,7 +12,9 @@
 #include <ctype.h>
 #include <string.h>
 #include <X11/Xlib.h>
+#include <X11/Xlib-xcb.h>
 #include <X11/Xlibint.h>
+#include <X11/extensions/sync.h>
 #include <xcb/randr.h>
 #include <xcb/present.h>
 #include <xcb/damage.h>
@@ -174,7 +176,6 @@ 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
@@ -957,9 +958,9 @@ 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);
+  if (w->paint.pixmap && ps->o.xrender_sync_fence) {
+    x_fence_sync(ps, w->paint.pixmap);
+  }
 
   // Set focus out
   win_set_focused(ps, w, false);
@@ -2344,7 +2345,6 @@ redir_stop(session_t *ps) {
     // kept inaccessible somehow
     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);
@@ -3151,7 +3151,6 @@ session_destroy(session_t *ps) {
     ps->tgt_picture = XCB_NONE;
   else
     free_picture(ps->c, &ps->tgt_picture);
-  free_fence(ps, &ps->tgt_buffer_fence);
 
   free_picture(ps->c, &ps->root_picture);
   free_paint(ps, &ps->tgt_buffer);

src/config.h

@@ -72,10 +72,8 @@ typedef struct options_t {
 	char *write_pid_path;
 	/// 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.
+	/// Whether to sync X drawing with X Sync fence to avoid certain delay
+	/// issues with GLX backend.
 	bool xrender_sync_fence;
 	/// Whether to avoid using stencil buffer under GLX backend. Might be
 	/// unsafe.

src/config_libconfig.c

@@ -370,7 +370,10 @@ char *parse_config_libconfig(options_t *opt, const char *config_file, bool *shad
   // --glx-use-gpushader4
   lcfg_lookup_bool(&cfg, "glx-use-gpushader4", &opt->glx_use_gpushader4);
   // --xrender-sync
-  lcfg_lookup_bool(&cfg, "xrender-sync", &opt->xrender_sync);
+  if (config_lookup_bool(&cfg, "xrender-sync", &ival) && ival) {
+    log_warn("Please use xrender-sync-fence instead of xrender-sync.");
+    opt->xrender_sync_fence = true;
+  }
   // --xrender-sync-fence
   lcfg_lookup_bool(&cfg, "xrender-sync-fence", &opt->xrender_sync_fence);
 

src/meson.build

@@ -14,7 +14,7 @@ cflags = []
 
 required_package = [
 	'x11', 'x11-xcb', 'xcb-renderutil',
-	'xcb-render', 'xcb-damage', 'xcb-randr',
+	'xcb-render', 'xcb-damage', 'xcb-randr', 'xcb-sync',
 	'xcb-composite', 'xcb-shape', 'xcb-image',
 	'xcb-xfixes', 'xcb-present', 'xext', 'pixman-1'
 ]

src/options.c

@@ -743,7 +743,11 @@ void get_cfg(options_t *opt, int argc, char *const *argv, bool shadow_enable,
 			opt->write_pid_path = strdup(optarg);
 			break;
 		P_CASEBOOL(311, vsync_use_glfinish);
-		P_CASEBOOL(312, xrender_sync);
+		case 312:
+			// --xrender-sync
+			log_warn("Please use --xrender-sync-fence instead of --xrender-sync");
+			opt->xrender_sync_fence = true;
+ 			break;
 		P_CASEBOOL(313, xrender_sync_fence);
 		P_CASEBOOL(315, no_fading_destroyed_argb);
 		P_CASEBOOL(316, force_win_blend);
@@ -799,9 +803,6 @@ void get_cfg(options_t *opt, int argc, char *const *argv, bool shadow_enable,
 	if (opt->blur_background_frame)
 		opt->blur_background = true;
 
-	if (opt->xrender_sync_fence)
-		opt->xrender_sync = true;
-
 	// Other variables determined by options
 
 	// Determine whether we need to track focus changes

src/render.c

@@ -165,8 +165,6 @@ void 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;
@@ -183,8 +181,9 @@ void paint_one(session_t *ps, win *w, const region_t *reg_paint) {
 		    ps, w->pictfmt, draw, XCB_RENDER_CP_SUBWINDOW_MODE, &pa);
 	}
 
-	if (IsViewable == w->a.map_state)
-		xr_sync(ps, draw, &w->fence);
+	if (IsViewable == w->a.map_state && ps->o.xrender_sync_fence) {
+		x_fence_sync(ps, draw);
+	}
 
 	// GLX: Build texture
 	// Let glx_bind_pixmap() determine pixmap size, because if the user
@@ -607,7 +606,9 @@ static bool win_build_shadow(session_t *ps, win *w, double opacity) {
 	w->shadow_paint.pict = shadow_picture_argb;
 
 	// Sync it once and only once
-	xr_sync(ps, w->shadow_paint.pixmap, NULL);
+	if (ps->o.xrender_sync_fence) {
+		x_fence_sync(ps, w->shadow_paint.pixmap);
+	}
 
 	xcb_free_gc(ps->c, gc);
 	xcb_image_destroy(shadow_image);
@@ -1026,11 +1027,15 @@ void paint_all(session_t *ps, region_t *region, const region_t *region_real, win
 			glFlush();
 		glXWaitX();
 		assert(ps->tgt_buffer.pixmap);
-		xr_sync(ps, ps->tgt_buffer.pixmap, &ps->tgt_buffer_fence);
+		if (ps->o.xrender_sync_fence) {
+			x_fence_sync(ps, ps->tgt_buffer.pixmap);
+		}
 		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.xrender_sync_fence) {
+			x_fence_sync(ps, ps->tgt_buffer.pixmap);
+		}
 		if (ps->o.vsync_use_glfinish)
 			glFinish();
 		else

src/win.h

@@ -3,8 +3,6 @@
 // Copyright (c) 2013 Richard Grenville <pyxlcy@gmail.com>
 #pragma once
 #include <stdbool.h>
-#include <X11/Xlib.h>
-#include <X11/extensions/sync.h>
 #include <xcb/damage.h>
 
 // FIXME shouldn't need this
@@ -98,8 +96,6 @@ struct win {
   winmode_t mode;
   /// Whether the window has been damaged at least once.
   bool ever_damaged;
-  /// X Sync fence of drawable.
-  XSyncFence fence;
   /// Whether the window was damaged after last paint.
   bool pixmap_damaged;
   /// Damage of the window.

src/x.c

@@ -4,6 +4,7 @@
 
 #include <xcb/xcb_renderutil.h>
 #include <xcb/xfixes.h>
+#include <xcb/sync.h>
 #include <pixman.h>
 
 #include "compiler.h"
@@ -283,6 +284,12 @@ void x_clear_picture_clip_region(session_t *ps, xcb_render_picture_t pict) {
   return;
 }
 
+enum {
+  XSyncBadCounter = 0,
+  XSyncBadAlarm = 1,
+  XSyncBadFence = 2,
+};
+
 /**
  * X11 error handler function.
  *
@@ -446,3 +453,47 @@ bool x_atom_is_background_prop(session_t *ps, xcb_atom_t atom) {
   }
   return false;
 }
+
+/**
+ * Free a XSync fence.
+ */
+static inline void
+x_free_fence(session_t *ps, xcb_sync_fence_t *pfence) {
+  if (*pfence) {
+    xcb_sync_destroy_fence(ps->c, *pfence);
+  }
+  *pfence = XCB_NONE;
+}
+
+/**
+ * Synchronizes a X Render drawable to ensure all pending painting requests
+ * are completed.
+ */
+void x_fence_sync(session_t *ps, xcb_drawable_t d) {
+  x_sync(ps->c);
+  if (ps->xsync_exists) {
+    // TODO(richardgv): 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
+    xcb_sync_fence_t tmp_fence = xcb_generate_id(ps->c);
+    xcb_generic_error_t *e =
+      xcb_request_check(ps->c,
+                        xcb_sync_create_fence(ps->c, d, tmp_fence, 0));
+    if (e) {
+      log_error("Failed to create a XSync fence for %#010x", d);
+      free(e);
+      return;
+    }
+
+    e = xcb_request_check(ps->c, xcb_sync_trigger_fence(ps->c, tmp_fence));
+    if (e) {
+      log_error("Failed to trigger the fence");
+      free(e);
+      x_free_fence(ps, &tmp_fence);
+      return;
+    }
+
+    xcb_sync_await_fence(ps->c, 1, &tmp_fence);
+    x_free_fence(ps, &tmp_fence);
+  }
+}

src/x.h

@@ -5,6 +5,7 @@
 #include <stdbool.h>
 #include <xcb/xcb.h>
 #include <xcb/render.h>
+#include <xcb/sync.h>
 #include <xcb/xcb_renderutil.h>
 
 #include "region.h"
@@ -167,3 +168,5 @@ xcb_pixmap_t x_get_root_back_pixmap(session_t *ps);
 /// Return true if the atom refers to a property name that is used for the
 /// root window background pixmap
 bool x_atom_is_background_prop(session_t *ps, xcb_atom_t atom);
+
+void x_fence_sync(session_t *, xcb_sync_fence_t);