From baeafb3a3b4ad3de3690929e68245b5cd54b9366 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Tue, 6 Feb 2024 08:56:37 +0000 Subject: [PATCH 1/4] event: tweak ev_reparent_notify Instead of change window attributes back and forth, calculate the evmask and set it just once. And also make sure the request is flushed. Signed-off-by: Yuxuan Shui --- src/event.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/event.c b/src/event.c index 28f67d8554..f59cf9e32f 100644 --- a/src/event.c +++ b/src/event.c @@ -1,6 +1,7 @@ // SPDX-License-Identifier: MPL-2.0 // Copyright (c) 2019, Yuxuan Shui +#include #include #include @@ -350,19 +351,14 @@ static inline void ev_reparent_notify(session_t *ps, xcb_reparent_notify_event_t } // Reset event mask in case something wrong happens - xcb_change_window_attributes( - ps->c.c, ev->window, XCB_CW_EVENT_MASK, - (const uint32_t[]){determine_evmask(ps, ev->window, WIN_EVMODE_UNKNOWN)}); + uint32_t evmask = determine_evmask(ps, ev->window, WIN_EVMODE_UNKNOWN); if (!wid_has_prop(ps, ev->window, ps->atoms->aWM_STATE)) { log_debug("Window %#010x doesn't have WM_STATE property, it is " "probably not a client window. But we will listen for " "property change in case it gains one.", ev->window); - xcb_change_window_attributes( - ps->c.c, ev->window, XCB_CW_EVENT_MASK, - (const uint32_t[]){determine_evmask(ps, ev->window, WIN_EVMODE_UNKNOWN) | - XCB_EVENT_MASK_PROPERTY_CHANGE}); + evmask |= XCB_EVENT_MASK_PROPERTY_CHANGE; } else { auto w_real_top = find_managed_window_or_parent(ps, ev->parent); if (w_real_top && w_real_top->state != WSTATE_UNMAPPED && @@ -386,6 +382,8 @@ static inline void ev_reparent_notify(session_t *ps, xcb_reparent_notify_event_t } } } + XCB_AWAIT_VOID(xcb_change_window_attributes, ps->c.c, ev->window, + XCB_CW_EVENT_MASK, (const uint32_t[]){evmask}); } } From bdc0943399584275c5bfcf01e84e4c38ecb2d515 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Tue, 6 Feb 2024 09:01:35 +0000 Subject: [PATCH 2/4] event: make sure ev_property_notify flushes its requests Signed-off-by: Yuxuan Shui --- src/event.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/event.c b/src/event.c index f59cf9e32f..5bfcbb03b4 100644 --- a/src/event.c +++ b/src/event.c @@ -473,9 +473,10 @@ static inline void ev_property_notify(session_t *ps, xcb_property_notify_event_t // Check whether it could be a client window if (!find_toplevel(ps, ev->window)) { // Reset event mask anyway - xcb_change_window_attributes(ps->c.c, ev->window, XCB_CW_EVENT_MASK, - (const uint32_t[]){determine_evmask( - ps, ev->window, WIN_EVMODE_UNKNOWN)}); + const uint32_t evmask = + determine_evmask(ps, ev->window, WIN_EVMODE_UNKNOWN); + XCB_AWAIT_VOID(xcb_change_window_attributes, ps->c.c, ev->window, + XCB_CW_EVENT_MASK, (const uint32_t[]){evmask}); auto w_top = find_managed_window_or_parent(ps, ev->window); // ev->window might have not been managed yet, in that case w_top @@ -490,8 +491,8 @@ static inline void ev_property_notify(session_t *ps, xcb_property_notify_event_t // If _NET_WM_WINDOW_TYPE changes... God knows why this would happen, but // there are always some stupid applications. (#144) if (ev->atom == ps->atoms->a_NET_WM_WINDOW_TYPE) { - struct managed_win *w = NULL; - if ((w = find_toplevel(ps, ev->window))) { + struct managed_win *w = find_toplevel(ps, ev->window); + if (w) { win_set_property_stale(w, ev->atom); } } From a5826b6fb0dc4888ce922df5d82151972ff4b247 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Tue, 6 Feb 2024 09:10:04 +0000 Subject: [PATCH 3/4] event: fix dumb bug in repair_win Basically we won't call xcb_damage_subtract if show_all_xerrors is set, which is very bad. Fixes that, and also make sure the damage subtract request is flushed in all branches. Fixes: 1307d9ec709c9fbbe99939d46ad04c57d5e4b501 Signed-off-by: Yuxuan Shui --- src/event.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/src/event.c b/src/event.c index 5bfcbb03b4..b3cb25a270 100644 --- a/src/event.c +++ b/src/event.c @@ -585,16 +585,28 @@ static inline void repair_win(session_t *ps, struct managed_win *w) { region_t parts; pixman_region32_init(&parts); + // If this is the first time this window is damaged, we would redraw the + // whole window, so we don't need to fetch the damage region. But we still need + // to make sure the X server receives the DamageSubtract request, hence the + // `xcb_request_check` here. + // Otherwise, we fetch the damage regions. That means we will receive a reply + // from the X server, which implies it has received our request. if (!w->ever_damaged) { - win_extents(w, &parts); - if (!ps->o.show_all_xerrors) { - set_ignore_cookie(&ps->c, xcb_damage_subtract(ps->c.c, w->damage, - XCB_NONE, XCB_NONE)); + auto e = xcb_request_check( + ps->c.c, xcb_damage_subtract(ps->c.c, w->damage, XCB_NONE, XCB_NONE)); + if (e) { + if (ps->o.show_all_xerrors) { + x_print_error(e->sequence, e->major_code, e->minor_code, + e->error_code); + } + free(e); } + win_extents(w, &parts); } else { + auto cookie = + xcb_damage_subtract(ps->c.c, w->damage, XCB_NONE, ps->damaged_region); if (!ps->o.show_all_xerrors) { - set_ignore_cookie(&ps->c, xcb_damage_subtract(ps->c.c, w->damage, XCB_NONE, - ps->damaged_region)); + set_ignore_cookie(&ps->c, cookie); } x_fetch_region(&ps->c, ps->damaged_region, &parts); pixman_region32_translate(&parts, w->g.x + w->g.border_width, From 75d0b7ba1e84e5057efaa1e61398f003a8e9e246 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Tue, 6 Feb 2024 09:14:05 +0000 Subject: [PATCH 4/4] core: don't flush X connection before go to sleep See the added comments for details. Fixes #1145 Fixes #1166 Fixes #1040? Signed-off-by: Yuxuan Shui --- src/event.c | 10 ++++++++-- src/picom.c | 32 ++++++++++++++++++++++++-------- 2 files changed, 32 insertions(+), 10 deletions(-) diff --git a/src/event.c b/src/event.c index b3cb25a270..a1b93fd476 100644 --- a/src/event.c +++ b/src/event.c @@ -48,8 +48,14 @@ /// When top half finished, we enter the render stage, where no server state should be /// queried. All rendering should be done with our internal knowledge of the server state. /// +/// P.S. There is another reason to avoid sending any request to the server as much as +/// possible. To make sure requests are sent, flushes are needed. And `xcb_flush`/`XFlush` +/// functions may read more events from the server into their queues. This is +/// undesirable, see the comments on `handle_queued_x_events` in picom.c for more details. -// TODO(yshui) the things described above +// TODO(yshui) the things described above. This is mostly done, maybe some of +// the functions here is still making unnecessary queries, we need +// to do some auditing to be sure. /** * Get a window's name from window ID. @@ -590,7 +596,7 @@ static inline void repair_win(session_t *ps, struct managed_win *w) { // to make sure the X server receives the DamageSubtract request, hence the // `xcb_request_check` here. // Otherwise, we fetch the damage regions. That means we will receive a reply - // from the X server, which implies it has received our request. + // from the X server, which implies it has received our DamageSubtract request. if (!w->ever_damaged) { auto e = xcb_request_check( ps->c.c, xcb_damage_subtract(ps->c.c, w->damage, XCB_NONE, XCB_NONE)); diff --git a/src/picom.c b/src/picom.c index cdacf75d0c..55c65d2ce2 100644 --- a/src/picom.c +++ b/src/picom.c @@ -1607,9 +1607,32 @@ static void unredirect(session_t *ps) { log_debug("Screen unredirected."); } -// Handle queued events before we go to sleep +/// Handle queued events before we go to sleep. +/// +/// This function is called by ev_prepare watcher, which is called just before +/// the event loop goes to sleep. X damage events are incremental, which means +/// if we don't handle the ones X server already sent us, we won't get new ones. +/// And if we don't get new ones, we won't render, i.e. we would freeze. libxcb +/// keeps an internal queue of events, so we have to be 100% sure no events are +/// left in that queue before we go to sleep. static void handle_queued_x_events(EV_P attr_unused, ev_prepare *w, int revents attr_unused) { session_t *ps = session_ptr(w, event_check); + // Flush because if we go into sleep when there is still requests in the + // outgoing buffer, they will not be sent for an indefinite amount of + // time. Use XFlush here too, we might still use some Xlib functions + // because OpenGL. + // + // Also note, after we have flushed here, we won't flush again in this + // function before going into sleep. This is because `xcb_flush`/`XFlush` + // may _read_ more events from the server (yes, this is ridiculous, I + // know). And we can't have that, see the comments above this function. + // + // This means if functions called ev_handle need to send some events, + // they need to carefully make sure those events are flushed, one way or + // another. + XFlush(ps->c.dpy); + xcb_flush(ps->c.c); + if (ps->vblank_scheduler) { vblank_handle_x_events(ps->vblank_scheduler); } @@ -1619,13 +1642,6 @@ static void handle_queued_x_events(EV_P attr_unused, ev_prepare *w, int revents ev_handle(ps, ev); free(ev); }; - // Flush because if we go into sleep when there is still - // requests in the outgoing buffer, they will not be sent - // for an indefinite amount of time. - // Use XFlush here too, we might still use some Xlib functions - // because OpenGL. - XFlush(ps->c.dpy); - xcb_flush(ps->c.c); int err = xcb_connection_has_error(ps->c.c); if (err) { log_fatal("X11 server connection broke (error %d)", err);