From 80847dd3fa115eb5cb834dd19a66f43d6d284ce4 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Sun, 30 Dec 2018 07:06:47 +0000 Subject: [PATCH] Refactor the XSync fence code Use a temporary fence everytime. Convert the Xlib XSync functions to use xcb_sync_*. Signed-off-by: Yuxuan Shui --- man/compton.1.asciidoc | 7 ++---- src/common.h | 54 ------------------------------------------ src/compton.c | 11 ++++----- src/config.h | 6 ++--- src/config_libconfig.c | 5 +++- src/meson.build | 2 +- src/options.c | 9 +++---- src/render.c | 19 +++++++++------ src/win.h | 4 ---- src/x.c | 51 +++++++++++++++++++++++++++++++++++++++ src/x.h | 3 +++ 11 files changed, 85 insertions(+), 86 deletions(-) diff --git a/man/compton.1.asciidoc b/man/compton.1.asciidoc index b32a5b5..17e51f3 100644 --- a/man/compton.1.asciidoc +++ b/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. diff --git a/src/common.h b/src/common.h index b344c72..8f56931 100644 --- a/src/common.h +++ b/src/common.h @@ -65,10 +65,6 @@ #include #include -#include -#include -#include - #include #include #include @@ -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 */ ///@{ diff --git a/src/compton.c b/src/compton.c index 40b75ff..8bbf844 100644 --- a/src/compton.c +++ b/src/compton.c @@ -12,7 +12,9 @@ #include #include #include +#include #include +#include #include #include #include @@ -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); diff --git a/src/config.h b/src/config.h index a20cd76..077208f 100644 --- a/src/config.h +++ b/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. diff --git a/src/config_libconfig.c b/src/config_libconfig.c index 3a6df4d..9ba9b9a 100644 --- a/src/config_libconfig.c +++ b/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); diff --git a/src/meson.build b/src/meson.build index 7478213..86cafee 100644 --- a/src/meson.build +++ b/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' ] diff --git a/src/options.c b/src/options.c index b2fb523..f510f3f 100644 --- a/src/options.c +++ b/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 diff --git a/src/render.c b/src/render.c index 5d3b355..8fdf735 100644 --- a/src/render.c +++ b/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 diff --git a/src/win.h b/src/win.h index 488ef0d..2d49f98 100644 --- a/src/win.h +++ b/src/win.h @@ -3,8 +3,6 @@ // Copyright (c) 2013 Richard Grenville #pragma once #include -#include -#include #include // 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. diff --git a/src/x.c b/src/x.c index 8c87de6..390b872 100644 --- a/src/x.c +++ b/src/x.c @@ -4,6 +4,7 @@ #include #include +#include #include #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); + } +} diff --git a/src/x.h b/src/x.h index 978a1de..03ba5ca 100644 --- a/src/x.h +++ b/src/x.h @@ -5,6 +5,7 @@ #include #include #include +#include #include #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);