From 753711970be25163479c8e1373e00656c17b8a9e Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Tue, 8 Oct 2024 19:14:09 +0100 Subject: [PATCH] core: make sure we don't sleep on events, again Once again we found ourselves going into sleep while there are X events in libxcb's buffer. This time it's because both vblank_handle_x_events and x_poll_for_events can read from the X connection. This is introduced as part of commit d617d0ba4f. Before that, we were only calling xcb_poll_for_queued_events after vblank_handle_x_events. By changing that to a xcb_poll_for_events (and later to x_poll_for_events, which calls xcb_poll_for_events), we could read additional vblank events into the buffer. These events will not be handled because vblank_handle_x_events has already be called, so we will be going into sleep with these events in the buffer. Since vblank events driver the frame render loop, this causes indefinitely long delay between frames. Related-to: #1345 #1330 Signed-off-by: Yuxuan Shui --- src/picom.c | 65 +++++++++++++++++++++++++++++++++------------------- src/vblank.c | 14 +++++++---- src/vblank.h | 8 ++++++- 3 files changed, 57 insertions(+), 30 deletions(-) diff --git a/src/picom.c b/src/picom.c index 2afb8be8cc..92c14e989f 100644 --- a/src/picom.c +++ b/src/picom.c @@ -1450,31 +1450,48 @@ static void unredirect(session_t *ps) { static void handle_x_events(struct session *ps) { bool wm_was_consistent = wm_is_consistent(ps->wm); - if (ps->vblank_scheduler) { - vblank_handle_x_events(ps->vblank_scheduler); - } + while (true) { + // 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, `xcb_flush`/`XFlush` may _read_ more events from the server + // (yes, this is ridiculous, I know). But since we are polling for events + // in a loop, this should be fine - if we read events here, they will be + // handled below; and if some requests is sent later in this loop, which + // means some events must have been received, we will loop once more and + // get here to flush them. + XFlush(ps->c.dpy); + xcb_flush(ps->c.c); + + // We have to check for vblank events (i.e. special xcb events) and normal + // events in a loop. This is because both `xcb_poll_for_event` and + // `xcb_poll_for_special_event` will read data from the X connection and + // put it in a buffer. So whichever one we call last, say + // `xcb_poll_for_special_event`, will read data into the buffer that might + // contain events that `xcb_poll_for_event` should handle, and vice versa. + // This causes us to go into sleep with events in the buffer. + // + // We have to keep calling both of them until neither of them return any + // events. + bool has_events = false; + if (ps->vblank_scheduler) { + has_events = vblank_handle_x_events(ps->vblank_scheduler) == + VBLANK_HANDLE_X_EVENTS_OK; + } + + xcb_generic_event_t *ev; + while ((ev = x_poll_for_event(&ps->c))) { + has_events = true; + ev_handle(ps, (xcb_generic_event_t *)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. - // - // 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); - - xcb_generic_event_t *ev; - while ((ev = x_poll_for_event(&ps->c))) { - ev_handle(ps, (xcb_generic_event_t *)ev); - free(ev); - }; + if (!has_events) { + break; + } + } int err = xcb_connection_has_error(ps->c.c); if (err) { log_fatal("X11 server connection broke (error %d)", err); diff --git a/src/vblank.c b/src/vblank.c index 057f7d4058..e63b0a51cb 100644 --- a/src/vblank.c +++ b/src/vblank.c @@ -69,7 +69,7 @@ struct vblank_scheduler_ops { bool (*init)(struct vblank_scheduler *self); void (*deinit)(struct vblank_scheduler *self); bool (*schedule)(struct vblank_scheduler *self); - bool (*handle_x_events)(struct vblank_scheduler *self); + enum vblank_handle_x_events_result (*handle_x_events)(struct vblank_scheduler *self); }; static void @@ -444,10 +444,14 @@ static void handle_present_complete_notify(struct present_vblank_scheduler *self ev_timer_start(self->base.loop, &self->callback_timer); } -static bool handle_present_events(struct vblank_scheduler *base) { +static enum vblank_handle_x_events_result +handle_present_events(struct vblank_scheduler *base) { auto self = (struct present_vblank_scheduler *)base; xcb_present_generic_event_t *ev; + enum vblank_handle_x_events_result result = VBLANK_HANDLE_X_EVENTS_NO_EVENTS; while ((ev = (void *)xcb_poll_for_special_event(base->c->c, self->event))) { + result = VBLANK_HANDLE_X_EVENTS_OK; + if (ev->event != self->event_id) { // This event doesn't have the right event context, it's not meant // for us. @@ -460,7 +464,7 @@ static bool handle_present_events(struct vblank_scheduler *base) { next: free(ev); } - return true; + return result; } static const struct vblank_scheduler_ops vblank_scheduler_ops[LAST_VBLANK_SCHEDULER] = { @@ -571,11 +575,11 @@ vblank_scheduler_new(struct ev_loop *loop, struct x_connection *c, xcb_window_t return self; } -bool vblank_handle_x_events(struct vblank_scheduler *self) { +enum vblank_handle_x_events_result vblank_handle_x_events(struct vblank_scheduler *self) { assert(self->type < LAST_VBLANK_SCHEDULER); auto fn = vblank_scheduler_ops[self->type].handle_x_events; if (fn != NULL) { return fn(self); } - return true; + return VBLANK_HANDLE_X_EVENTS_NO_EVENTS; } diff --git a/src/vblank.h b/src/vblank.h index a1916dc405..83bda69daa 100644 --- a/src/vblank.h +++ b/src/vblank.h @@ -29,6 +29,12 @@ enum vblank_callback_action { VBLANK_CALLBACK_DONE, }; +enum vblank_handle_x_events_result { + VBLANK_HANDLE_X_EVENTS_OK, + VBLANK_HANDLE_X_EVENTS_ERROR, + VBLANK_HANDLE_X_EVENTS_NO_EVENTS, +}; + typedef enum vblank_callback_action (*vblank_callback_t)(struct vblank_event *event, void *user_data); @@ -47,4 +53,4 @@ vblank_scheduler_new(struct ev_loop *loop, struct x_connection *c, xcb_window_t enum vblank_scheduler_type type, bool use_realtime_scheduling); void vblank_scheduler_free(struct vblank_scheduler *); -bool vblank_handle_x_events(struct vblank_scheduler *self); +enum vblank_handle_x_events_result vblank_handle_x_events(struct vblank_scheduler *self);