From 75d0b7ba1e84e5057efaa1e61398f003a8e9e246 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Tue, 6 Feb 2024 09:14:05 +0000 Subject: [PATCH] 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 b3cb25a2..a1b93fd4 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 cdacf75d..55c65d2c 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);