From 8d98b7d6392aa2327f1c96b725c5774cfeefba53 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Sun, 18 Jun 2023 17:12:57 +0100 Subject: [PATCH 01/18] core: don't call schedule_render too early I mistakenly assumed that PresentCompleteNotify event signifies the end of a vblank (or the start of scanout). But actually this event can in theory in sent at any point during a vblank, with its timestamp pointing to when the end of vblank is. (that's why we often find the timestamp to be in the future). Add a delay so schedule_render is actually called at the end of vblank, so it doesn't mistakenly think the render is too slow to complete. Signed-off-by: Yuxuan Shui --- src/common.h | 2 ++ src/event.c | 3 ++- src/picom.c | 37 +++++++++++++++++++++++++++---------- src/utils.h | 22 ++++++++++++++++------ 4 files changed, 47 insertions(+), 17 deletions(-) diff --git a/src/common.h b/src/common.h index 707d00e7..f9e1765e 100644 --- a/src/common.h +++ b/src/common.h @@ -147,6 +147,8 @@ typedef struct session { ev_timer fade_timer; /// Use an ev_timer callback for drawing ev_timer draw_timer; + /// Timer for the end of each vblanks. Used for calling schedule_render. + ev_timer vblank_timer; /// Called every time we have timeouts or new data on socket, /// so we can be sure if xcb read from X socket at anytime during event /// handling, we will not left any event unhandled in the queue diff --git a/src/event.c b/src/event.c index 807cf4ed..739540dc 100644 --- a/src/event.c +++ b/src/event.c @@ -716,7 +716,8 @@ void ev_handle(session_t *ps, xcb_generic_event_t *ev) { // XXX redraw needs to be more fine grained queue_redraw(ps); - // the events sent from SendEvent will be ignored + // We intentionally ignore events sent via SendEvent. Those events has the 8th bit + // of response_type set, meaning they will match none of the cases below. switch (ev->response_type) { case FocusIn: ev_focus_in(ps, (xcb_focus_in_event_t *)ev); break; case FocusOut: ev_focus_out(ps, (xcb_focus_out_event_t *)ev); break; diff --git a/src/picom.c b/src/picom.c index 6f3c2908..8238ae8f 100644 --- a/src/picom.c +++ b/src/picom.c @@ -1515,13 +1515,8 @@ handle_present_complete_notify(session_t *ps, xcb_present_complete_notify_event_ struct timespec now; clock_gettime(CLOCK_MONOTONIC, &now); - uint64_t now_usec = (uint64_t)(now.tv_sec * 1000000 + now.tv_nsec / 1000); - uint64_t drift; - if (cne->ust > now_usec) { - drift = cne->ust - now_usec; - } else { - drift = now_usec - cne->ust; - } + auto now_us = (int64_t)(now.tv_sec * 1000000L + now.tv_nsec / 1000); + auto drift = iabs((int64_t)cne->ust - now_us); if (ps->last_msc_instant != 0) { auto frame_count = cne->msc - ps->last_msc; @@ -1537,14 +1532,28 @@ handle_present_complete_notify(session_t *ps, xcb_present_complete_notify_event_ // align with the monotonic clock. If not, disable frame pacing because we // can't schedule frames reliably. log_error("Temporal anomaly detected, frame pacing disabled. (Are we " - "running inside a time namespace?), %" PRIu64 " %" PRIu64, - now_usec, ps->last_msc_instant); + "running inside a time namespace?), %" PRIi64 " %" PRIu64, + now_us, ps->last_msc_instant); ps->frame_pacing = false; } ps->last_msc_instant = cne->ust; ps->last_msc = cne->msc; if (ps->redraw_needed) { - schedule_render(ps, true); + if (now_us > (int64_t)cne->ust) { + schedule_render(ps, true); + } else { + // Wait until the end of the current vblank to call + // schedule_render. If we call schedule_render too early, it can + // mistakenly think the render missed the vblank, and doesn't + // schedule render for the next vblank, causing frame drops. + log_trace("The end of this vblank is %" PRIi64 " us into the " + "future", + (int64_t)cne->ust - now_us); + assert(!ev_is_active(&ps->vblank_timer)); + ev_timer_set(&ps->vblank_timer, + ((double)cne->ust - (double)now_us) / 1000000.0, 0); + ev_timer_start(ps->loop, &ps->vblank_timer); + } } } @@ -1823,6 +1832,13 @@ static void draw_callback(EV_P_ ev_timer *w, int revents) { } } +static void schedule_render_callback(EV_P_ ev_timer *w, int revents attr_unused) { + session_t *ps = session_ptr(w, vblank_timer); + ev_timer_stop(EV_A_ w); + + schedule_render(ps, true); +} + static void x_event_callback(EV_P attr_unused, ev_io *w, int revents attr_unused) { session_t *ps = (session_t *)w; xcb_generic_event_t *ev = xcb_poll_for_event(ps->c.c); @@ -2419,6 +2435,7 @@ static session_t *session_init(int argc, char **argv, Display *dpy, ev_io_start(ps->loop, &ps->xiow); ev_init(&ps->unredir_timer, tmout_unredir_callback); ev_init(&ps->draw_timer, draw_callback); + ev_init(&ps->vblank_timer, schedule_render_callback); ev_init(&ps->fade_timer, fade_timer_callback); diff --git a/src/utils.h b/src/utils.h index fc6dd306..446fba8b 100644 --- a/src/utils.h +++ b/src/utils.h @@ -125,14 +125,22 @@ safe_isnan(double a) { * @param max maximum value * @return normalized value */ -static inline int attr_const normalize_i_range(int i, int min, int max) { - if (i > max) +static inline int attr_const attr_unused normalize_i_range(int i, int min, int max) { + if (i > max) { return max; - if (i < min) + } + if (i < min) { return min; + } return i; } +/// Generic integer abs() +#define iabs(val) \ + ({ \ + __auto_type __tmp = (val); \ + __tmp > 0 ? __tmp : -__tmp; \ + }) #define min2(a, b) ((a) > (b) ? (b) : (a)) #define max2(a, b) ((a) > (b) ? (a) : (b)) #define min3(a, b, c) min2(a, min2(b, c)) @@ -149,10 +157,12 @@ static inline int attr_const normalize_i_range(int i, int min, int max) { * @return normalized value */ static inline double attr_const normalize_d_range(double d, double min, double max) { - if (d > max) + if (d > max) { return max; - if (d < min) + } + if (d < min) { return min; + } return d; } @@ -162,7 +172,7 @@ static inline double attr_const normalize_d_range(double d, double min, double m * @param d double value to normalize * @return normalized value */ -static inline double attr_const normalize_d(double d) { +static inline double attr_const attr_unused normalize_d(double d) { return normalize_d_range(d, 0.0, 1.0); } From 827380e1e348893ef568b647bd49ee9f7d61e577 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Fri, 23 Jun 2023 00:17:35 +0100 Subject: [PATCH 02/18] core: simplify the pacing logic a little bit Make it simpler to stop requesting PresentCompleteNotify when there is nothing to render. Related: #1079 Signed-off-by: Yuxuan Shui --- src/backend/backend.c | 14 ++- src/backend/backend.h | 6 +- src/common.h | 25 +++-- src/picom.c | 232 +++++++++++++++++++++++------------------- 4 files changed, 162 insertions(+), 115 deletions(-) diff --git a/src/backend/backend.c b/src/backend/backend.c index 33a3d8bc..dd3366d6 100644 --- a/src/backend/backend.c +++ b/src/backend/backend.c @@ -82,13 +82,17 @@ void handle_device_reset(session_t *ps) { } /// paint all windows -void paint_all_new(session_t *ps, struct managed_win *t) { +/// +/// Returns if any render command is issued. IOW if nothing on the screen has changed, +/// this function will return false. +bool paint_all_new(session_t *ps, struct managed_win *const t) { struct timespec now = get_time_timespec(); auto paint_all_start_us = (uint64_t)now.tv_sec * 1000000UL + (uint64_t)now.tv_nsec / 1000; if (ps->backend_data->ops->device_status && ps->backend_data->ops->device_status(ps->backend_data) != DEVICE_STATUS_NORMAL) { - return handle_device_reset(ps); + handle_device_reset(ps); + return false; } if (ps->o.xrender_sync_fence) { if (ps->xsync_exists && !x_fence_sync(&ps->c, ps->sync_fence)) { @@ -114,7 +118,7 @@ void paint_all_new(session_t *ps, struct managed_win *t) { if (!pixman_region32_not_empty(®_damage)) { pixman_region32_fini(®_damage); - return; + return false; } #ifdef DEBUG_REPAINT @@ -199,7 +203,6 @@ void paint_all_new(session_t *ps, struct managed_win *t) { ps->last_schedule_delay = after_damage_us - ps->next_render; } } - ps->did_render = true; if (ps->backend_data->ops->prepare) { ps->backend_data->ops->prepare(ps->backend_data, ®_paint); @@ -219,7 +222,7 @@ void paint_all_new(session_t *ps, struct managed_win *t) { // on top of that window. This is used to reduce the number of pixels painted. // // Whether this is beneficial is to be determined XXX - for (auto w = t; w; w = w->prev_trans) { + for (struct managed_win *w = t; w; w = w->prev_trans) { pixman_region32_subtract(®_visible, &ps->screen_reg, w->reg_ignore); assert(!(w->flags & WIN_FLAGS_IMAGE_ERROR)); assert(!(w->flags & WIN_FLAGS_PIXMAP_STALE)); @@ -541,6 +544,7 @@ void paint_all_new(session_t *ps, struct managed_win *t) { for (win *w = t; w; w = w->prev_trans) log_trace(" %#010lx", w->id); #endif + return true; } // vim: set noet sw=8 ts=8 : diff --git a/src/backend/backend.h b/src/backend/backend.h index f572a935..1b4df729 100644 --- a/src/backend/backend.h +++ b/src/backend/backend.h @@ -366,4 +366,8 @@ struct backend_operations { extern struct backend_operations *backend_list[]; -void paint_all_new(session_t *ps, struct managed_win *const t) attr_nonnull(1); +/// paint all windows +/// +/// Returns if any render command is issued. IOW if nothing on the screen has changed, +/// this function will return false. +bool paint_all_new(session_t *ps, struct managed_win *t) attr_nonnull(1); diff --git a/src/common.h b/src/common.h index f9e1765e..ac9da4c7 100644 --- a/src/common.h +++ b/src/common.h @@ -134,6 +134,17 @@ struct shader_info { UT_hash_handle hh; }; +enum render_progress { + /// Render is finished and presented to the screen. + RENDER_IDLE = 0, + /// Rendering is queued, but not started yet. + RENDER_QUEUED, + /// Backend has been called, render commands have been issued. + RENDER_STARTED, + /// Backend reported render commands have been finished. (not actually used). + RENDER_FINISHED, +}; + /// Structure containing all necessary data for a session. typedef struct session { // === Event handlers === @@ -223,17 +234,11 @@ typedef struct session { uint64_t last_msc_instant; /// The last MSC number uint64_t last_msc; - /// When the currently rendered frame will be displayed. - /// 0 means there is no pending frame. - uint64_t target_msc; /// The delay between when the last frame was scheduled to be rendered, and when /// the render actually started. uint64_t last_schedule_delay; /// When do we want our next frame to start rendering. uint64_t next_render; - /// Did we actually render the last frame. Sometimes redraw will be scheduled only - /// to find out nothing has changed. In which case this will be set to false. - bool did_render; /// Whether we can perform frame pacing. bool frame_pacing; @@ -247,7 +252,13 @@ typedef struct session { options_t o; /// Whether we have hit unredirection timeout. bool tmout_unredir_hit; - /// Whether we need to redraw the screen + /// Rendering is currently in progress. This means we are in any stage of + /// rendering a frame. The render could be queued but not yet started, or it could + /// have finished but not yet presented. + enum render_progress render_in_progress; + /// Whether there are changes pending for the next render. A render is currently + /// in progress, otherwise we would have started a new render instead of setting + /// this flag. bool redraw_needed; /// Cache a xfixes region so we don't need to allocate it every time. diff --git a/src/picom.c b/src/picom.c index 8238ae8f..75d91493 100644 --- a/src/picom.c +++ b/src/picom.c @@ -167,19 +167,31 @@ static inline struct managed_win *find_win_all(session_t *ps, const xcb_window_t /// /// Renders are scheduled like this: /// -/// 1. queue_redraw() registers the intention to render. redraw_needed is set to true to -/// indicate what is on screen needs to be updated. +/// 1. queue_redraw() queues a new render by calling schedule_render, if there is no +/// render currently scheduled. i.e. render_in_progress == RENDER_IDLE. /// 2. then, we need to figure out the best time to start rendering. first, we need to -/// know when the next frame will be displayed on screen. we have this information from -/// the Present extension: we know when was the last frame displayed, and we know the -/// refresh rate. so we can calculate the next frame's display time. if our render time -/// estimation shows we could miss that target, we push the target back one frame. -/// 3. if there is already render completed for that target frame, or there is a render -/// currently underway, we don't do anything, and wait for the next Present Complete -/// Notify event to try to schedule again. -/// 4. otherwise, we schedule a render for that target frame. we use past statistics about -/// how long our renders took to figure out when to start rendering. we start rendering -/// at the latest point of time possible to still hit the target frame. +/// know when the current vblank will end. we have this information from the Present +/// extension: we know when was the end of last vblank, and we know the refresh rate. +/// so we can calculate the end of the current vblank. if our render time estimation +/// shows we could miss that target, we push the target back an integer number of +/// frames. and we calculate the end of the target vblank similarly. +/// 3. We schedule a render for that target. we use past statistics about how long our +/// renders took to figure out when to start rendering. we start rendering as late as +/// possible, but not too late that we miss the target vblank. render_in_progress is +/// set to RENDER_QUEUED. +/// 4. draw_callback() is called at the schedule time. Backend APIs are called to issue +/// render commands. render_in_progress is set to RENDER_STARTED. +/// 5. PresentCompleteNotify is received, which gives us the actual time when the current +/// vblank will end/ended. We schedule a call to handle_end_of_vblank at the +/// appropriate time. +/// 6. in handle_end_of_vblank, we check the backend to see if the render has finished. if +/// not, render_in_progress is unchanged; otherwise, render_in_progress is set to +/// RENDER_IDLE, and the next frame can be scheduled. +/// +/// This is what happens when frame_pacing is true. Otherwise render_in_progress is +/// either QUEUED or IDLE, and queue_redraw will always schedule a render to be started +/// immediately. PresentCompleteNotify will not be received, and handle_end_of_vblank will +/// not be called. /// /// The `triggered_by_timer` parameter is used to indicate whether this function is /// triggered by a steady timer, i.e. we are rendering for each vblank. The other case is @@ -188,80 +200,34 @@ static inline struct managed_win *find_win_all(session_t *ps, const xcb_window_t /// when the schedule is triggered by a steady timer, schedule_render will be called at a /// predictable offset into each vblank. -void schedule_render(session_t *ps, bool triggered_by_vblank) { +void schedule_render(session_t *ps, bool triggered_by_vblank attr_unused) { + // By default, we want to schedule render immediately, later in this function we + // might adjust that and move the render later, based on render timing statistics. double delay_s = 0; - ps->next_render = 0; + unsigned int divisor = 0; + struct timespec now; + clock_gettime(CLOCK_MONOTONIC, &now); + auto now_us = (uint64_t)now.tv_sec * 1000000 + (uint64_t)now.tv_nsec / 1000; + + ps->next_render = now_us; + if (!ps->frame_pacing || !ps->redirected) { - // Not doing frame pacing, schedule a render immediately, if not already - // scheduled. - // If not redirected, we schedule immediately to have a chance to - // redirect. We won't have frame or render timing information anyway. + // If not doing frame pacing, schedule a render immediately unless it's + // already scheduled; if not redirected, we schedule immediately to have a + // chance to redirect. We won't have frame or render timing information + // anyway. if (!ev_is_active(&ps->draw_timer)) { - // We don't know the msc, so we set it to 1, because 0 is a - // special value - ps->target_msc = 1; goto schedule; } return; } - struct timespec render_time; - bool completed = - ps->backend_data->ops->last_render_time(ps->backend_data, &render_time); - if (!completed || ev_is_active(&ps->draw_timer)) { - // There is already a render underway (either just scheduled, or is - // rendered but awaiting completion), don't schedule another one. - if (ps->target_msc <= ps->last_msc) { - log_debug("Target frame %ld is in the past, but we are still " - "rendering", - ps->target_msc); - // We missed our target, push it back one frame - ps->target_msc = ps->last_msc + 1; - } - log_trace("Still rendering for target frame %ld, not scheduling another " - "render", - ps->target_msc); - return; - } - if (ps->target_msc > ps->last_msc) { - // Render for the target frame is completed, but is yet to be displayed. - // Don't schedule another render. - log_trace("Target frame %ld is in the future, and we have already " - "rendered, last msc: %d", - ps->target_msc, (int)ps->last_msc); - return; - } - struct timespec now; - clock_gettime(CLOCK_MONOTONIC, &now); - auto now_us = (uint64_t)now.tv_sec * 1000000 + (uint64_t)now.tv_nsec / 1000; - if (triggered_by_vblank) { - log_trace("vblank schedule delay: %ld us", now_us - ps->last_msc_instant); - } - - int render_time_us = - (int)(render_time.tv_sec * 1000000L + render_time.tv_nsec / 1000L); - if (ps->target_msc == ps->last_msc) { - // The frame has just been displayed, record its render time; - if (ps->did_render) { - log_trace("Last render call took: %d (gpu) + %d (cpu) us, " - "last_msc: %" PRIu64, - render_time_us, (int)ps->last_schedule_delay, ps->last_msc); - render_statistics_add_render_time_sample( - &ps->render_stats, render_time_us + (int)ps->last_schedule_delay); - } - ps->target_msc = 0; - ps->did_render = false; - ps->last_schedule_delay = 0; - } - - unsigned int divisor = 0; auto render_budget = render_statistics_get_budget(&ps->render_stats, &divisor); auto frame_time = render_statistics_get_vblank_time(&ps->render_stats); if (frame_time == 0) { // We don't have enough data for render time estimates, maybe there's // no frame rendered yet, or the backend doesn't support render timing // information, schedule render immediately. - ps->target_msc = ps->last_msc + 1; goto schedule; } @@ -271,14 +237,11 @@ void schedule_render(session_t *ps, bool triggered_by_vblank) { available = (unsigned int)(deadline - now_us); } - ps->target_msc = ps->last_msc + divisor; if (available > render_budget) { delay_s = (double)(available - render_budget) / 1000000.0; ps->next_render = deadline - render_budget; - } else { - delay_s = 0; - ps->next_render = now_us; } + if (delay_s > 1) { log_warn("Delay too long: %f s, render_budget: %d us, frame_time: " "%" PRIu32 " us, now_us: %" PRIu64 " us, next_msc: %" PRIu64 " u" @@ -288,16 +251,57 @@ void schedule_render(session_t *ps, bool triggered_by_vblank) { log_trace("Delay: %.6lf s, last_msc: %" PRIu64 ", render_budget: %d, frame_time: " "%" PRIu32 ", now_us: %" PRIu64 ", next_msc: %" PRIu64 ", " - "target_msc: %" PRIu64 ", divisor: %d", + "divisor: %d", delay_s, ps->last_msc_instant, render_budget, frame_time, now_us, - deadline, ps->target_msc, divisor); + deadline, divisor); schedule: + ps->render_in_progress = RENDER_QUEUED; + ps->redraw_needed = false; assert(!ev_is_active(&ps->draw_timer)); ev_timer_set(&ps->draw_timer, delay_s, 0); ev_timer_start(ps->loop, &ps->draw_timer); } +/// Called after a vblank has ended +/// +/// Check if previously queued render has finished, and record the time it took. +void handle_end_of_vblank(session_t *ps) { + if (ps->render_in_progress != RENDER_STARTED) { + // We didn't start rendering for this vblank, nothing to do + return; + } + + // We shouldn't have scheduled a render if the previous render hasn't been + // presented yet. + assert(!ev_is_active(&ps->draw_timer)); + + struct timespec render_time; + bool completed = + ps->backend_data->ops->last_render_time(ps->backend_data, &render_time); + if (!completed) { + // Render hasn't completed yet, keep render_in_progress as RENDER_STARTED + log_debug("Last render did not complete during vblank, msc: %" PRIu64, + ps->last_msc); + return; + } + + int render_time_us = + (int)(render_time.tv_sec * 1000000L + render_time.tv_nsec / 1000L); + // The frame has been finished and presented, record its render time. + log_trace("Last render call took: %d (gpu) + %d (cpu) us, " + "last_msc: %" PRIu64, + render_time_us, (int)ps->last_schedule_delay, ps->last_msc); + render_statistics_add_render_time_sample( + &ps->render_stats, render_time_us + (int)ps->last_schedule_delay); + ps->last_schedule_delay = 0; + ps->render_in_progress = RENDER_IDLE; + + if (ps->redraw_needed) { + schedule_render(ps, true); + } +} + void queue_redraw(session_t *ps) { if (ps->screen_is_off) { // The screen is off, if there is a draw queued for the next frame (i.e. @@ -314,10 +318,14 @@ void queue_redraw(session_t *ps) { // Whether we have already rendered for the current frame. // If frame pacing is not enabled, pretend this is false. // If --benchmark is used, redraw is always queued - if (!ps->redraw_needed && !ps->o.benchmark) { + if (ps->render_in_progress == RENDER_IDLE && !ps->o.benchmark) { schedule_render(ps, false); + } else if (ps->render_in_progress > RENDER_QUEUED) { + // render_in_progress > RENDER_QUEUED means we have already issued the + // render commands, so a new render must be scheduled to reflect new + // changes. Otherwise the queued render will include1 the new changes. + ps->redraw_needed = true; } - ps->redraw_needed = true; } /** @@ -1423,7 +1431,6 @@ static bool redirect_start(session_t *ps) { ps->last_msc_instant = 0; ps->last_msc = 0; ps->last_schedule_delay = 0; - ps->target_msc = 0; render_statistics_reset(&ps->render_stats); } else if (ps->frame_pacing) { log_error("Present extension is not supported, frame pacing disabled."); @@ -1487,6 +1494,10 @@ static void unredirect(session_t *ps) { log_debug("Screen unredirected."); } +/// Handle PresentCompleteNotify events +/// +/// Record the MSC value and their timestamps, and schedule handle_end_of_vblank() at the +/// correct time. static void handle_present_complete_notify(session_t *ps, xcb_present_complete_notify_event_t *cne) { if (cne->kind != XCB_PRESENT_COMPLETE_KIND_NOTIFY_MSC) { @@ -1538,22 +1549,24 @@ handle_present_complete_notify(session_t *ps, xcb_present_complete_notify_event_ } ps->last_msc_instant = cne->ust; ps->last_msc = cne->msc; - if (ps->redraw_needed) { - if (now_us > (int64_t)cne->ust) { - schedule_render(ps, true); - } else { - // Wait until the end of the current vblank to call - // schedule_render. If we call schedule_render too early, it can - // mistakenly think the render missed the vblank, and doesn't - // schedule render for the next vblank, causing frame drops. - log_trace("The end of this vblank is %" PRIi64 " us into the " - "future", - (int64_t)cne->ust - now_us); - assert(!ev_is_active(&ps->vblank_timer)); - ev_timer_set(&ps->vblank_timer, - ((double)cne->ust - (double)now_us) / 1000000.0, 0); - ev_timer_start(ps->loop, &ps->vblank_timer); - } + // Note we can't update ps->render_in_progress here because of this present + // complete notify, as we don't know if the render finished before the end of + // vblank or not. We schedule a call to handle_end_of_vblank() to figure out if we + // are still rendering, and update ps->render_in_progress accordingly. + if (now_us > (int64_t)cne->ust) { + handle_end_of_vblank(ps); + } else { + // Wait until the end of the current vblank to call + // handle_end_of_vblank. If we call it too early, it can + // mistakenly think the render missed the vblank, and doesn't + // schedule render for the next vblank, causing frame drops. + log_trace("The end of this vblank is %" PRIi64 " us into the " + "future", + (int64_t)cne->ust - now_us); + assert(!ev_is_active(&ps->vblank_timer)); + ev_timer_set(&ps->vblank_timer, + ((double)cne->ust - (double)now_us) / 1000000.0, 0); + ev_timer_start(ps->loop, &ps->vblank_timer); } } @@ -1788,13 +1801,14 @@ static void draw_callback_impl(EV_P_ session_t *ps, int revents attr_unused) { log_trace("paint_preprocess took: %" PRIi64 " us", after_preprocess_us - after_handle_pending_updates_us); - // If the screen is unredirected, free all_damage to stop painting + // If the screen is unredirected, we don't render anything. + bool did_render = false; if (ps->redirected && ps->o.stoppaint_force != ON) { static int paint = 0; log_trace("Render start, frame %d", paint); if (!ps->o.legacy_backends) { - paint_all_new(ps, bottom); + did_render = paint_all_new(ps, bottom); } else { paint_all(ps, bottom); } @@ -1807,6 +1821,20 @@ static void draw_callback_impl(EV_P_ session_t *ps, int revents attr_unused) { } } + if (ps->frame_pacing && did_render) { + ps->render_in_progress = RENDER_STARTED; + } else { + // With frame pacing, we set render_in_progress to RENDER_IDLE after the + // end of vblank. Without frame pacing, we won't be receiving vblank + // events, so we set render_in_progress to RENDER_IDLE here, right after + // we issue the render commands. + // The other case is if we decided there is no change to render, in that + // case no render command is issued, so we also set render_in_progress to + // RENDER_IDLE. + ps->render_in_progress = RENDER_IDLE; + } + ps->next_render = 0; + if (!fade_running) { ps->fade_time = 0L; } @@ -1832,11 +1860,11 @@ static void draw_callback(EV_P_ ev_timer *w, int revents) { } } -static void schedule_render_callback(EV_P_ ev_timer *w, int revents attr_unused) { +static void vblank_callback(EV_P_ ev_timer *w, int revents attr_unused) { session_t *ps = session_ptr(w, vblank_timer); ev_timer_stop(EV_A_ w); - schedule_render(ps, true); + handle_end_of_vblank(ps); } static void x_event_callback(EV_P attr_unused, ev_io *w, int revents attr_unused) { @@ -2435,7 +2463,7 @@ static session_t *session_init(int argc, char **argv, Display *dpy, ev_io_start(ps->loop, &ps->xiow); ev_init(&ps->unredir_timer, tmout_unredir_callback); ev_init(&ps->draw_timer, draw_callback); - ev_init(&ps->vblank_timer, schedule_render_callback); + ev_init(&ps->vblank_timer, vblank_callback); ev_init(&ps->fade_timer, fade_timer_callback); From 2e1c4e51b3b06c579040a03711a37a762b372de5 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Wed, 5 Jul 2023 05:53:21 +0100 Subject: [PATCH 03/18] core: don't request vblank events when we are not rendering Previously everytime we receive a vblank event, we always request a new one. This made the logic somewhat simpler. But this generated many useless vblank events, and wasted power. We only need vblank events for two things: 1. after we rendered a frame, we need to know when it has been displayed on the screen. 2. estimating the refresh rate. This commit makes sure we only request vblank events when it's actually needed. Fixes #1079 Signed-off-by: Yuxuan Shui --- src/common.h | 3 ++ src/picom.c | 109 ++++++++++++++++++++++++++++++++--------------- src/statistics.h | 2 + src/x.c | 12 ++++++ src/x.h | 3 ++ 5 files changed, 94 insertions(+), 35 deletions(-) diff --git a/src/common.h b/src/common.h index ac9da4c7..0009f112 100644 --- a/src/common.h +++ b/src/common.h @@ -227,6 +227,9 @@ typedef struct session { bool first_frame; /// Whether screen has been turned off bool screen_is_off; + /// We asked X server to send us a event for the end of a vblank, and we haven't + /// received one yet. + bool vblank_event_requested; /// Event context for X Present extension. uint32_t present_event_id; xcb_special_event_t *present_event; diff --git a/src/picom.c b/src/picom.c index 75d91493..71178fe9 100644 --- a/src/picom.c +++ b/src/picom.c @@ -258,6 +258,9 @@ void schedule_render(session_t *ps, bool triggered_by_vblank attr_unused) { schedule: ps->render_in_progress = RENDER_QUEUED; ps->redraw_needed = false; + + x_request_vblank_event(ps, ps->last_msc + 1); + assert(!ev_is_active(&ps->draw_timer)); ev_timer_set(&ps->draw_timer, delay_s, 0); ev_timer_start(ps->loop, &ps->draw_timer); @@ -267,20 +270,43 @@ schedule: /// /// Check if previously queued render has finished, and record the time it took. void handle_end_of_vblank(session_t *ps) { - if (ps->render_in_progress != RENDER_STARTED) { - // We didn't start rendering for this vblank, nothing to do + if (ps->render_in_progress == RENDER_IDLE) { + // We didn't start rendering for this vblank, no render time to record. + // But if we don't have a vblank estimate, we will ask for one more vblank + // event, so we can collect more data and get an estimate sooner. + if (render_statistics_get_vblank_time(&ps->render_stats) == 0) { + x_request_vblank_event(ps, ps->last_msc + 1); + } return; } - // We shouldn't have scheduled a render if the previous render hasn't been - // presented yet. - assert(!ev_is_active(&ps->draw_timer)); - + // render_in_progress is either RENDER_STARTED or RENDER_QUEUED struct timespec render_time; - bool completed = - ps->backend_data->ops->last_render_time(ps->backend_data, &render_time); + bool completed; + if (ps->render_in_progress == RENDER_STARTED) { + completed = + ps->backend_data->ops->last_render_time(ps->backend_data, &render_time); + } else { + completed = false; + } + + // Do we want to be notified when the next vblank comes? First, if frame_pacing is + // disabled, we don't need vblank events; or if the screen is off, we cannot + // request vblank events. Otherwise, we need vblank events in these cases: + // 1) if we know we need to redraw for the next vblank. + // 2) previous render hasn't completed yet, so it will be presented during the + // next vblank. we need to ask for an event for that. + // 3) if we don't have enough data for a vblank interval estimate, see above. + bool need_vblank_events = + ps->frame_pacing && (ps->redraw_needed || !completed || + render_statistics_get_vblank_time(&ps->render_stats) == 0); + + if (need_vblank_events) { + x_request_vblank_event(ps, ps->last_msc + 1); + } + if (!completed) { - // Render hasn't completed yet, keep render_in_progress as RENDER_STARTED + // Render hasn't completed yet, keep render_in_progress unchanged. log_debug("Last render did not complete during vblank, msc: %" PRIu64, ps->last_msc); return; @@ -1504,23 +1530,21 @@ handle_present_complete_notify(session_t *ps, xcb_present_complete_notify_event_ return; } - bool event_is_invalid = false; - if (ps->frame_pacing) { - auto next_msc = cne->msc + 1; - if (cne->msc <= ps->last_msc || cne->ust == 0) { - // X sometimes sends duplicate/bogus MSC events, don't - // use the msc value. Also ignore these events. - // - // See: - // https://gitlab.freedesktop.org/xorg/xserver/-/issues/1418 - next_msc = ps->last_msc + 1; - event_is_invalid = true; - } - auto cookie = xcb_present_notify_msc( - ps->c.c, session_get_target_window(ps), 0, next_msc, 0, 0); - set_cant_fail_cookie(&ps->c, cookie); - } + assert(ps->frame_pacing); + assert(ps->vblank_event_requested); + ps->vblank_event_requested = false; + + // X sometimes sends duplicate/bogus MSC events, when screen has just been turned + // off. Don't use the msc value in these events. We treat this as not receiving a + // vblank event at all, and try to get a new one. + // + // See: + // https://gitlab.freedesktop.org/xorg/xserver/-/issues/1418 + bool event_is_invalid = cne->msc <= ps->last_msc || cne->ust == 0; if (event_is_invalid) { + log_debug("Invalid PresentCompleteNotify event, %" PRIu64 " %" PRIu64, + cne->msc, cne->ust); + x_request_vblank_event(ps, ps->last_msc + 1); return; } @@ -1529,23 +1553,38 @@ handle_present_complete_notify(session_t *ps, xcb_present_complete_notify_event_ auto now_us = (int64_t)(now.tv_sec * 1000000L + now.tv_nsec / 1000); auto drift = iabs((int64_t)cne->ust - now_us); - if (ps->last_msc_instant != 0) { - auto frame_count = cne->msc - ps->last_msc; - int frame_time = (int)((cne->ust - ps->last_msc_instant) / frame_count); - render_statistics_add_vblank_time_sample(&ps->render_stats, frame_time); - log_trace("Frame count %lu, frame time: %d us, rolling average: %u us, " - "msc: %" PRIu64 ", offset: %d us", - frame_count, frame_time, - render_statistics_get_vblank_time(&ps->render_stats), cne->ust, - (int)drift); - } else if (drift > 1000000 && ps->frame_pacing) { + if (ps->last_msc_instant == 0 && drift > 1000000) { // This is the first MSC event we receive, let's check if the timestamps // align with the monotonic clock. If not, disable frame pacing because we // can't schedule frames reliably. log_error("Temporal anomaly detected, frame pacing disabled. (Are we " "running inside a time namespace?), %" PRIi64 " %" PRIu64, now_us, ps->last_msc_instant); + if (ps->render_in_progress == RENDER_STARTED) { + // When frame_pacing is off, render_in_progress can't be + // RENDER_STARTED. See the comment on schedule_render(). + ps->render_in_progress = RENDER_IDLE; + } ps->frame_pacing = false; + return; + } + + if (ps->last_msc_instant != 0) { + auto frame_count = cne->msc - ps->last_msc; + int frame_time = (int)((cne->ust - ps->last_msc_instant) / frame_count); + if (frame_count == 1 && !ps->screen_is_off) { + render_statistics_add_vblank_time_sample(&ps->render_stats, frame_time); + log_trace("Frame count %lu, frame time: %d us, rolling average: " + "%u us, " + "msc: %" PRIu64 ", offset: %d us", + frame_count, frame_time, + render_statistics_get_vblank_time(&ps->render_stats), + cne->ust, (int)drift); + } else { + log_trace("Frame count %lu, frame time: %d us, msc: %" PRIu64 + ", offset: %d us, not adding sample.", + frame_count, frame_time, cne->ust, (int)drift); + } } ps->last_msc_instant = cne->ust; ps->last_msc = cne->msc; diff --git a/src/statistics.h b/src/statistics.h index 67d02119..42d7bc2d 100644 --- a/src/statistics.h +++ b/src/statistics.h @@ -30,4 +30,6 @@ void render_statistics_add_render_time_sample(struct render_statistics *rs, int unsigned int render_statistics_get_budget(struct render_statistics *rs, unsigned int *divisor); +/// Return the measured vblank interval in microseconds. Returns 0 if not enough +/// samples have been collected yet. unsigned int render_statistics_get_vblank_time(struct render_statistics *rs); diff --git a/src/x.c b/src/x.c index 1840ebb1..54cf702e 100644 --- a/src/x.c +++ b/src/x.c @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -777,6 +778,17 @@ err: return false; } +void x_request_vblank_event(session_t *ps, uint64_t msc) { + if (ps->vblank_event_requested) { + return; + } + + auto cookie = + xcb_present_notify_msc(ps->c.c, session_get_target_window(ps), 0, msc, 0, 0); + set_cant_fail_cookie(&ps->c, cookie); + ps->vblank_event_requested = true; +} + /** * Convert a struct conv to a X picture convolution filter, normalizing the kernel * in the process. Allow the caller to specify the element at the center of the kernel, diff --git a/src/x.h b/src/x.h index b5bd1a59..39bf5ab0 100644 --- a/src/x.h +++ b/src/x.h @@ -419,3 +419,6 @@ void x_update_monitors(struct x_connection *, struct x_monitors *); void x_free_monitor_info(struct x_monitors *); uint32_t attr_deprecated xcb_generate_id(xcb_connection_t *c); + +/// Ask X server to send us a notification for the next end of vblank. +void x_request_vblank_event(session_t *ps, uint64_t msc); From 23eb3d5f52b808f38386dc5848819a7c55f2a294 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Wed, 5 Jul 2023 05:54:44 +0100 Subject: [PATCH 04/18] core: don't unredir when display is turned off We unredirect because we receive bad vblank events, and also vblank events at a different interval compared to when the screen is on. But it is enough to just not record the vblank interval statistics when the screen is off. Although, unredirecting when display is off can also fix the problem where use-damage causes the screen to flicker when the display is turned off then back on. So we need something else for that. Signed-off-by: Yuxuan Shui --- src/common.h | 2 -- src/picom.c | 63 +++++----------------------------------------------- src/x.c | 20 +++++++++++++++++ src/x.h | 3 +++ 4 files changed, 28 insertions(+), 60 deletions(-) diff --git a/src/common.h b/src/common.h index 0009f112..889ca883 100644 --- a/src/common.h +++ b/src/common.h @@ -150,8 +150,6 @@ typedef struct session { // === Event handlers === /// ev_io for X connection ev_io xiow; - /// Timer for checking DPMS power level - ev_timer dpms_check_timer; /// Timeout for delayed unredirection. ev_timer unredir_timer; /// Timer for fading diff --git a/src/picom.c b/src/picom.c index 71178fe9..2cfa08ec 100644 --- a/src/picom.c +++ b/src/picom.c @@ -122,27 +122,6 @@ static inline int64_t get_time_ms(void) { return (int64_t)tp.tv_sec * 1000 + (int64_t)tp.tv_nsec / 1000000; } -static inline bool dpms_screen_is_off(xcb_dpms_info_reply_t *info) { - // state is a bool indicating whether dpms is enabled - return info->state && (info->power_level != XCB_DPMS_DPMS_MODE_ON); -} - -void check_dpms_status(EV_P attr_unused, ev_timer *w, int revents attr_unused) { - auto ps = session_ptr(w, dpms_check_timer); - auto r = xcb_dpms_info_reply(ps->c.c, xcb_dpms_info(ps->c.c), NULL); - if (!r) { - log_fatal("Failed to query DPMS status."); - abort(); - } - auto now_screen_is_off = dpms_screen_is_off(r); - if (ps->screen_is_off != now_screen_is_off) { - log_debug("Screen is now %s", now_screen_is_off ? "off" : "on"); - ps->screen_is_off = now_screen_is_off; - queue_redraw(ps); - } - free(r); -} - /** * Find matched window. * @@ -329,18 +308,6 @@ void handle_end_of_vblank(session_t *ps) { } void queue_redraw(session_t *ps) { - if (ps->screen_is_off) { - // The screen is off, if there is a draw queued for the next frame (i.e. - // ps->redraw_needed == true), it won't be triggered until the screen is - // on again, because the abnormal Present events we will receive from the - // X server when the screen is off. Yet we need the draw_callback to be - // called as soon as possible so the screen can be unredirected. - // So here we unconditionally start the draw timer. - ev_timer_stop(ps->loop, &ps->draw_timer); - ev_timer_set(&ps->draw_timer, 0, 0); - ev_timer_start(ps->loop, &ps->draw_timer); - return; - } // Whether we have already rendered for the current frame. // If frame pacing is not enabled, pretend this is false. // If --benchmark is used, redraw is always queued @@ -1047,19 +1014,6 @@ static bool paint_preprocess(session_t *ps, bool *fade_running, bool *animation, // If there's no window to paint, and the screen isn't redirected, // don't redirect it. unredir_possible = true; - } else if (ps->screen_is_off) { - // Screen is off, unredirect - // We do this unconditionally disregarding "unredir_if_possible" - // because it's important for correctness, because we need to - // workaround problems X server has around screen off. - // - // Known problems: - // 1. Sometimes OpenGL front buffer can lose content, and if we - // are doing partial updates (i.e. use-damage = true), the - // result will be wrong. - // 2. For frame pacing, X server sends bogus - // PresentCompleteNotify events when screen is off. - unredir_possible = true; } if (unredir_possible) { if (ps->redirected) { @@ -1569,6 +1523,8 @@ handle_present_complete_notify(session_t *ps, xcb_present_complete_notify_event_ return; } + x_check_dpms_status(ps); + if (ps->last_msc_instant != 0) { auto frame_count = cne->msc - ps->last_msc; int frame_time = (int)((cne->ust - ps->last_msc_instant) / frame_count); @@ -2192,17 +2148,9 @@ static session_t *session_init(int argc, char **argv, Display *dpy, ext_info = xcb_get_extension_data(ps->c.c, &xcb_dpms_id); ps->dpms_exists = ext_info && ext_info->present; - if (ps->dpms_exists) { - auto r = xcb_dpms_info_reply(ps->c.c, xcb_dpms_info(ps->c.c), NULL); - if (!r) { - log_fatal("Failed to query DPMS info"); - goto err; - } - ps->screen_is_off = dpms_screen_is_off(r); - // Check screen status every half second - ev_timer_init(&ps->dpms_check_timer, check_dpms_status, 0, 0.5); - ev_timer_start(ps->loop, &ps->dpms_check_timer); - free(r); + if (!ps->dpms_exists) { + log_fatal("No DPMS extension"); + exit(1); } // Parse configuration file @@ -2811,7 +2759,6 @@ static void session_destroy(session_t *ps) { // Stop libev event handlers ev_timer_stop(ps->loop, &ps->unredir_timer); ev_timer_stop(ps->loop, &ps->fade_timer); - ev_timer_stop(ps->loop, &ps->dpms_check_timer); ev_timer_stop(ps->loop, &ps->draw_timer); ev_prepare_stop(ps->loop, &ps->event_check); ev_signal_stop(ps->loop, &ps->usr1_signal); diff --git a/src/x.c b/src/x.c index 54cf702e..89786e18 100644 --- a/src/x.c +++ b/src/x.c @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -789,6 +790,25 @@ void x_request_vblank_event(session_t *ps, uint64_t msc) { ps->vblank_event_requested = true; } +static inline bool dpms_screen_is_off(xcb_dpms_info_reply_t *info) { + // state is a bool indicating whether dpms is enabled + return info->state && (info->power_level != XCB_DPMS_DPMS_MODE_ON); +} + +void x_check_dpms_status(session_t *ps) { + auto r = xcb_dpms_info_reply(ps->c.c, xcb_dpms_info(ps->c.c), NULL); + if (!r) { + log_fatal("Failed to query DPMS status."); + abort(); + } + auto now_screen_is_off = dpms_screen_is_off(r); + if (ps->screen_is_off != now_screen_is_off) { + log_debug("Screen is now %s", now_screen_is_off ? "off" : "on"); + ps->screen_is_off = now_screen_is_off; + } + free(r); +} + /** * Convert a struct conv to a X picture convolution filter, normalizing the kernel * in the process. Allow the caller to specify the element at the center of the kernel, diff --git a/src/x.h b/src/x.h index 39bf5ab0..60bcfef4 100644 --- a/src/x.h +++ b/src/x.h @@ -422,3 +422,6 @@ uint32_t attr_deprecated xcb_generate_id(xcb_connection_t *c); /// Ask X server to send us a notification for the next end of vblank. void x_request_vblank_event(session_t *ps, uint64_t msc); + +/// Update ps->screen_is_off to reflect the current DPMS state. +void x_check_dpms_status(session_t *ps); From 147561a313687ccf62fe10fea9b6cebcec8990b2 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Mon, 18 Dec 2023 03:04:59 +0000 Subject: [PATCH 05/18] config: add a debug environment variable This is where we keep temporary, short living, private debug options. Adding and removing command line and config file options are troublesome, and we don't want people adding these to their config files. Signed-off-by: Yuxuan Shui --- src/config.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++ src/config.h | 7 ++++++ 2 files changed, 75 insertions(+) diff --git a/src/config.c b/src/config.c index ec39aa43..efbf279f 100644 --- a/src/config.c +++ b/src/config.c @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -615,6 +616,72 @@ char *locate_auxiliary_file(const char *scope, const char *path, const char *inc return ret; } +struct debug_options_entry { + const char *name; + const char **choices; + size_t offset; +}; + +static const struct debug_options_entry debug_options_entries[] = { + +}; + +void parse_debug_option_single(char *setting, struct debug_options *debug_options) { + char *equal = strchr(setting, '='); + size_t name_len = equal ? (size_t)(equal - setting) : strlen(setting); + for (size_t i = 0; i < ARR_SIZE(debug_options_entries); i++) { + if (strncmp(setting, debug_options_entries[i].name, name_len) != 0) { + continue; + } + if (debug_options_entries[i].name[name_len] != '\0') { + continue; + } + auto value = (int *)((void *)debug_options + debug_options_entries[i].offset); + if (equal) { + const char *const arg = equal + 1; + if (debug_options_entries[i].choices != NULL) { + for (size_t j = 0; debug_options_entries[i].choices[j]; j++) { + if (strcmp(arg, debug_options_entries[i].choices[j]) == + 0) { + *value = (int)j; + return; + } + } + } + if (!parse_int(arg, value)) { + log_error("Invalid value for debug option %s: %s, it " + "will be ignored.", + debug_options_entries[i].name, arg); + } + } else if (debug_options_entries[i].choices == NULL) { + *value = 1; + } else { + log_error( + "Missing value for debug option %s, it will be ignored.", setting); + } + return; + } + log_error("Invalid debug option: %s", setting); +} + +/// Parse debug options from environment variable `PICOM_DEBUG`. +void parse_debug_options(struct debug_options *debug_options) { + const char *debug = getenv("PICOM_DEBUG"); + const struct debug_options default_debug_options = {}; + + *debug_options = default_debug_options; + if (!debug) { + return; + } + + scoped_charp debug_copy = strdup(debug); + char *tmp, *needle = strtok_r(debug_copy, ";", &tmp); + while (needle) { + parse_debug_option_single(needle, debug_options); + needle = strtok_r(NULL, ";", &tmp); + } +} + /** * Parse a list of window shader rules. */ @@ -817,5 +884,6 @@ char *parse_config(options_t *opt, const char *config_file, bool *shadow_enable, (void)hasneg; (void)winopt_mask; #endif + parse_debug_options(&opt->debug_options); return ret; } diff --git a/src/config.h b/src/config.h index 31e6774c..93bede84 100644 --- a/src/config.h +++ b/src/config.h @@ -73,6 +73,11 @@ enum blur_method { typedef struct _c2_lptr c2_lptr_t; +/// Internal, private options for debugging and development use. +struct debug_options { + +}; + /// Structure representing all options. typedef struct options { // === Debugging === @@ -262,6 +267,8 @@ typedef struct options { c2_lptr_t *transparent_clipping_blacklist; bool dithered_present; + + struct debug_options debug_options; } options_t; extern const char *const BACKEND_STRS[NUM_BKEND + 1]; From 1430d281161fcaff7bd1807f861c34c7a287a100 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Sun, 9 Jul 2023 16:39:44 +0100 Subject: [PATCH 06/18] core: refactor frame pacing Factored out vblank event generation, add abstraction over how vblank events are generated. The goal is so we can request vblank events in different ways based on the driver we are running on. Tried to simplify the frame scheduling logic, we will see if I succeeded or not. Also, the logic to exclude vblank events for vblank interval estimation when the screen off is dropped. It's too hard to get right, we need to find something robust. Signed-off-by: Yuxuan Shui --- src/common.h | 41 ++--- src/config.h | 10 + src/meson.build | 3 +- src/picom.c | 472 +++++++++++++++++++++--------------------------- src/vblank.c | 257 ++++++++++++++++++++++++++ src/vblank.h | 45 +++++ src/x.c | 24 +-- src/x.h | 8 +- 8 files changed, 547 insertions(+), 313 deletions(-) create mode 100644 src/vblank.c create mode 100644 src/vblank.h diff --git a/src/common.h b/src/common.h index 889ca883..52967e60 100644 --- a/src/common.h +++ b/src/common.h @@ -134,17 +134,6 @@ struct shader_info { UT_hash_handle hh; }; -enum render_progress { - /// Render is finished and presented to the screen. - RENDER_IDLE = 0, - /// Rendering is queued, but not started yet. - RENDER_QUEUED, - /// Backend has been called, render commands have been issued. - RENDER_STARTED, - /// Backend reported render commands have been finished. (not actually used). - RENDER_FINISHED, -}; - /// Structure containing all necessary data for a session. typedef struct session { // === Event handlers === @@ -156,8 +145,6 @@ typedef struct session { ev_timer fade_timer; /// Use an ev_timer callback for drawing ev_timer draw_timer; - /// Timer for the end of each vblanks. Used for calling schedule_render. - ev_timer vblank_timer; /// Called every time we have timeouts or new data on socket, /// so we can be sure if xcb read from X socket at anytime during event /// handling, we will not left any event unhandled in the queue @@ -225,12 +212,6 @@ typedef struct session { bool first_frame; /// Whether screen has been turned off bool screen_is_off; - /// We asked X server to send us a event for the end of a vblank, and we haven't - /// received one yet. - bool vblank_event_requested; - /// Event context for X Present extension. - uint32_t present_event_id; - xcb_special_event_t *present_event; /// When last MSC event happened, in useconds. uint64_t last_msc_instant; /// The last MSC number @@ -242,6 +223,8 @@ typedef struct session { uint64_t next_render; /// Whether we can perform frame pacing. bool frame_pacing; + /// Vblank event scheduler + struct vblank_scheduler *vblank_scheduler; /// Render statistics struct render_statistics render_stats; @@ -253,14 +236,18 @@ typedef struct session { options_t o; /// Whether we have hit unredirection timeout. bool tmout_unredir_hit; - /// Rendering is currently in progress. This means we are in any stage of - /// rendering a frame. The render could be queued but not yet started, or it could - /// have finished but not yet presented. - enum render_progress render_in_progress; - /// Whether there are changes pending for the next render. A render is currently - /// in progress, otherwise we would have started a new render instead of setting - /// this flag. - bool redraw_needed; + /// If the backend is busy. This means two things: + /// Either the backend is currently rendering a frame, or a frame has been + /// rendered but has yet to be presented. In either case, we should not start + /// another render right now. As if we start issuing rendering commands now, we + /// will have to wait for either the the current render to finish, or the current + /// back buffer to be become available again. In either case, we will be wasting + /// time. + bool backend_busy; + /// Whether a render is queued. This generally means there are pending updates + /// to the screen that's neither included in the current render, nor on the + /// screen. + bool render_queued; /// Cache a xfixes region so we don't need to allocate it every time. /// A workaround for yshui/picom#301 diff --git a/src/config.h b/src/config.h index 93bede84..bdf384ac 100644 --- a/src/config.h +++ b/src/config.h @@ -73,6 +73,16 @@ enum blur_method { typedef struct _c2_lptr c2_lptr_t; +enum vblank_scheduler_type { + /// X Present extension based vblank events + VBLANK_SCHEDULER_PRESENT, + /// GLX_SGI_video_sync based vblank events + VBLANK_SCHEDULER_SGI_VIDEO_SYNC, + /// An invalid scheduler, served as a scheduler count, and + /// as a sentinel value. + LAST_VBLANK_SCHEDULER, +}; + /// Internal, private options for debugging and development use. struct debug_options { diff --git a/src/meson.build b/src/meson.build index a608c575..51f96488 100644 --- a/src/meson.build +++ b/src/meson.build @@ -9,7 +9,8 @@ base_deps = [ srcs = [ files('picom.c', 'win.c', 'c2.c', 'x.c', 'config.c', 'vsync.c', 'utils.c', 'diagnostic.c', 'string_utils.c', 'render.c', 'kernel.c', 'log.c', - 'options.c', 'event.c', 'cache.c', 'atom.c', 'file_watch.c', 'statistics.c') ] + 'options.c', 'event.c', 'cache.c', 'atom.c', 'file_watch.c', 'statistics.c', + 'vblank.c') ] picom_inc = include_directories('.') cflags = [] diff --git a/src/picom.c b/src/picom.c index 2cfa08ec..4c75329d 100644 --- a/src/picom.c +++ b/src/picom.c @@ -16,11 +16,13 @@ #include #include #include +#include #include #include #include #include #include +#include #include #include #include @@ -64,6 +66,7 @@ #include "options.h" #include "statistics.h" #include "uthash_extra.h" +#include "vblank.h" /// Get session_t pointer from a pointer to a member of session_t #define session_ptr(ptr, member) \ @@ -142,43 +145,158 @@ static inline struct managed_win *find_win_all(session_t *ps, const xcb_window_t return w; } +enum vblank_callback_action +collect_vblank_interval_statistics(struct vblank_event *e, void *ud) { + auto ps = (session_t *)ud; + double vblank_interval = NAN; + assert(ps->frame_pacing); + assert(ps->vblank_scheduler); + + // TODO(yshui): this naive method of estimating vblank interval does not handle + // the variable refresh rate case very well. This includes the case + // of a VRR enabled monitor; or a monitor that's turned off, in which + // case the vblank events might slow down or stop all together. + // I tried using DPMS to detect monitor power state, and stop adding + // samples when the monitor is off, but I had a hard time to get it + // working reliably, there are just too many corner cases. + + // Don't add sample again if we already collected statistics for this vblank + if (ps->last_msc < e->msc) { + if (ps->last_msc_instant != 0) { + auto frame_count = e->msc - ps->last_msc; + auto frame_time = + (int)((e->ust - ps->last_msc_instant) / frame_count); + if (frame_count == 1) { + render_statistics_add_vblank_time_sample( + &ps->render_stats, frame_time); + log_trace("Frame count %lu, frame time: %d us, ust: " + "%" PRIu64 "", + frame_count, frame_time, e->ust); + } else { + log_trace("Frame count %lu, frame time: %d us, msc: " + "%" PRIu64 ", not adding sample.", + frame_count, frame_time, e->ust); + } + } + ps->last_msc_instant = e->ust; + ps->last_msc = e->msc; + } else if (ps->last_msc > e->msc) { + log_warn("PresentCompleteNotify msc is going backwards, last_msc: " + "%" PRIu64 ", current msc: %" PRIu64, + ps->last_msc, e->msc); + } + + vblank_interval = render_statistics_get_vblank_time(&ps->render_stats); + log_trace("Vblank interval estimate: %f us", vblank_interval); + if (vblank_interval == 0) { + // We don't have enough data for vblank interval estimate, schedule + // another vblank event. + return VBLANK_CALLBACK_AGAIN; + } + return VBLANK_CALLBACK_DONE; +} +/// vblank callback scheduled by schedule_render. +/// +/// Check if previously queued render has finished, and record the time it took. +enum vblank_callback_action schedule_render_at_vblank(struct vblank_event *e, void *ud) { + auto ps = (session_t *)ud; + assert(ps->frame_pacing); + assert(ps->backend_busy); + assert(ps->render_queued); + assert(ps->vblank_scheduler); + + collect_vblank_interval_statistics(e, ud); + + struct timespec render_time; + bool completed = + ps->backend_data->ops->last_render_time(ps->backend_data, &render_time); + if (!completed) { + // Render hasn't completed yet, we can't start another render. + // Check again at the next vblank. + log_debug("Last render did not complete during vblank, msc: " + "%" PRIu64, + ps->last_msc); + return VBLANK_CALLBACK_AGAIN; + } + + // The frame has been finished and presented, record its render time. + int render_time_us = + (int)(render_time.tv_sec * 1000000L + render_time.tv_nsec / 1000L); + render_statistics_add_render_time_sample( + &ps->render_stats, render_time_us + (int)ps->last_schedule_delay); + log_verbose("Last render call took: %d (gpu) + %d (cpu) us, " + "last_msc: %" PRIu64, + render_time_us, (int)ps->last_schedule_delay, ps->last_msc); + ps->last_schedule_delay = 0; + ps->backend_busy = false; + + struct timespec now; + clock_gettime(CLOCK_MONOTONIC, &now); + auto now_us = (uint64_t)now.tv_sec * 1000000 + (uint64_t)now.tv_nsec / 1000; + double delay_s = 0; + if (ps->next_render > now_us) { + delay_s = (double)(ps->next_render - now_us) / 1000000.0; + } + log_verbose("Prepare to start rendering: delay: %f s, next_render: %" PRIu64 + ", now_us: %" PRIu64, + delay_s, ps->next_render, now_us); + assert(!ev_is_active(&ps->draw_timer)); + ev_timer_set(&ps->draw_timer, delay_s, 0); + ev_timer_start(ps->loop, &ps->draw_timer); + return VBLANK_CALLBACK_DONE; +} + /// How many seconds into the future should we start rendering the next frame. /// /// Renders are scheduled like this: /// -/// 1. queue_redraw() queues a new render by calling schedule_render, if there is no -/// render currently scheduled. i.e. render_in_progress == RENDER_IDLE. -/// 2. then, we need to figure out the best time to start rendering. first, we need to -/// know when the current vblank will end. we have this information from the Present -/// extension: we know when was the end of last vblank, and we know the refresh rate. -/// so we can calculate the end of the current vblank. if our render time estimation -/// shows we could miss that target, we push the target back an integer number of -/// frames. and we calculate the end of the target vblank similarly. -/// 3. We schedule a render for that target. we use past statistics about how long our -/// renders took to figure out when to start rendering. we start rendering as late as -/// possible, but not too late that we miss the target vblank. render_in_progress is -/// set to RENDER_QUEUED. -/// 4. draw_callback() is called at the schedule time. Backend APIs are called to issue -/// render commands. render_in_progress is set to RENDER_STARTED. -/// 5. PresentCompleteNotify is received, which gives us the actual time when the current -/// vblank will end/ended. We schedule a call to handle_end_of_vblank at the -/// appropriate time. -/// 6. in handle_end_of_vblank, we check the backend to see if the render has finished. if -/// not, render_in_progress is unchanged; otherwise, render_in_progress is set to -/// RENDER_IDLE, and the next frame can be scheduled. +/// 1. queue_redraw() queues a new render by calling schedule_render, if there +/// is no render currently scheduled. i.e. render_queued == false. +/// 2. then, we need to figure out the best time to start rendering. we need to +/// at least know when the current vblank will start, as we can't start render +/// before the current rendered frame is diplayed on screen. we have this +/// information from the vblank scheduler, it will notify us when that happens. +/// we might also want to delay the rendering even further to reduce latency, +/// this is discussed below, in FUTURE WORKS. +/// 3. we schedule a render for that target point in time. +/// 4. draw_callback() is called at the schedule time (i.e. when scheduled +/// vblank event is delivered). Backend APIs are called to issue render +/// commands. render_queued is set to false, and backend_busy is set to true. /// -/// This is what happens when frame_pacing is true. Otherwise render_in_progress is -/// either QUEUED or IDLE, and queue_redraw will always schedule a render to be started -/// immediately. PresentCompleteNotify will not be received, and handle_end_of_vblank will -/// not be called. +/// There is a small caveat in step 2. As a vblank event being delivered +/// doesn't necessarily mean the frame has been displayed on screen. If a frame +/// takes too long to render, it might miss the current vblank, and will be +/// displayed on screen during one of the subsequent vblanks. So in +/// schedule_render_at_vblank, we ask the backend to see if it has finished +/// rendering. if not, render_queued is unchanged, and another vblank is +/// scheduled; otherwise, draw_callback_impl will be scheduled to be call at +/// an appropriate time. /// -/// The `triggered_by_timer` parameter is used to indicate whether this function is -/// triggered by a steady timer, i.e. we are rendering for each vblank. The other case is -/// when we stop rendering for a while because there is no changes on screen, then -/// something changed and schedule_render is triggered by a DamageNotify. The idea is that -/// when the schedule is triggered by a steady timer, schedule_render will be called at a -/// predictable offset into each vblank. - +/// All of the above is what happens when frame_pacing is true. Otherwise +/// render_in_progress is either QUEUED or IDLE, and queue_redraw will always +/// schedule a render to be started immediately. PresentCompleteNotify will not +/// be received, and handle_end_of_vblank will not be called. +/// +/// The `triggered_by_timer` parameter is used to indicate whether this function +/// is triggered by a steady timer, i.e. we are rendering for each vblank. The +/// other case is when we stop rendering for a while because there is no changes +/// on screen, then something changed and schedule_render is triggered by a +/// DamageNotify. The idea is that when the schedule is triggered by a steady +/// timer, schedule_render will be called at a predictable offset into each +/// vblank. +/// +/// # FUTURE WORKS +/// +/// As discussed in step 2 above, we might want to delay the rendering even +/// further. If we know the time it takes to render a frame, and the interval +/// between vblanks, we can try to schedule the render to start at a point in +/// time that's closer to the next vblank. We should be able to get this +/// information by doing statistics on the render time of previous frames, which +/// is available from the backends; and the interval between vblank events, +/// which is available from the vblank scheduler. +/// +/// The code that does this is already implemented below, but disabled by +/// default. There are several problems with it, see bug #1072. void schedule_render(session_t *ps, bool triggered_by_vblank attr_unused) { // By default, we want to schedule render immediately, later in this function we // might adjust that and move the render later, based on render timing statistics. @@ -228,97 +346,38 @@ void schedule_render(session_t *ps, bool triggered_by_vblank attr_unused) { delay_s, render_budget, frame_time, now_us, deadline); } - log_trace("Delay: %.6lf s, last_msc: %" PRIu64 ", render_budget: %d, frame_time: " - "%" PRIu32 ", now_us: %" PRIu64 ", next_msc: %" PRIu64 ", " - "divisor: %d", - delay_s, ps->last_msc_instant, render_budget, frame_time, now_us, - deadline, divisor); + log_verbose("Delay: %.6lf s, last_msc: %" PRIu64 ", render_budget: %d, " + "frame_time: " + "%" PRIu32 ", now_us: %" PRIu64 ", next_msc: %" PRIu64 ", " + "divisor: %d", + delay_s, ps->last_msc_instant, render_budget, frame_time, now_us, + deadline, divisor); schedule: - ps->render_in_progress = RENDER_QUEUED; - ps->redraw_needed = false; - - x_request_vblank_event(ps, ps->last_msc + 1); - - assert(!ev_is_active(&ps->draw_timer)); - ev_timer_set(&ps->draw_timer, delay_s, 0); - ev_timer_start(ps->loop, &ps->draw_timer); -} - -/// Called after a vblank has ended -/// -/// Check if previously queued render has finished, and record the time it took. -void handle_end_of_vblank(session_t *ps) { - if (ps->render_in_progress == RENDER_IDLE) { - // We didn't start rendering for this vblank, no render time to record. - // But if we don't have a vblank estimate, we will ask for one more vblank - // event, so we can collect more data and get an estimate sooner. - if (render_statistics_get_vblank_time(&ps->render_stats) == 0) { - x_request_vblank_event(ps, ps->last_msc + 1); - } - return; - } - - // render_in_progress is either RENDER_STARTED or RENDER_QUEUED - struct timespec render_time; - bool completed; - if (ps->render_in_progress == RENDER_STARTED) { - completed = - ps->backend_data->ops->last_render_time(ps->backend_data, &render_time); + ps->render_queued = true; + // If the backend is not busy, we just need to schedule the render at the + // specified time; otherwise we need to wait for vblank events. + if (!ps->backend_busy) { + assert(!ev_is_active(&ps->draw_timer)); + ev_timer_set(&ps->draw_timer, delay_s, 0); + ev_timer_start(ps->loop, &ps->draw_timer); } else { - completed = false; - } - - // Do we want to be notified when the next vblank comes? First, if frame_pacing is - // disabled, we don't need vblank events; or if the screen is off, we cannot - // request vblank events. Otherwise, we need vblank events in these cases: - // 1) if we know we need to redraw for the next vblank. - // 2) previous render hasn't completed yet, so it will be presented during the - // next vblank. we need to ask for an event for that. - // 3) if we don't have enough data for a vblank interval estimate, see above. - bool need_vblank_events = - ps->frame_pacing && (ps->redraw_needed || !completed || - render_statistics_get_vblank_time(&ps->render_stats) == 0); - - if (need_vblank_events) { - x_request_vblank_event(ps, ps->last_msc + 1); - } - - if (!completed) { - // Render hasn't completed yet, keep render_in_progress unchanged. - log_debug("Last render did not complete during vblank, msc: %" PRIu64, - ps->last_msc); - return; - } - - int render_time_us = - (int)(render_time.tv_sec * 1000000L + render_time.tv_nsec / 1000L); - // The frame has been finished and presented, record its render time. - log_trace("Last render call took: %d (gpu) + %d (cpu) us, " - "last_msc: %" PRIu64, - render_time_us, (int)ps->last_schedule_delay, ps->last_msc); - render_statistics_add_render_time_sample( - &ps->render_stats, render_time_us + (int)ps->last_schedule_delay); - ps->last_schedule_delay = 0; - ps->render_in_progress = RENDER_IDLE; - - if (ps->redraw_needed) { - schedule_render(ps, true); + // We should never set backend_busy to true unless frame_pacing is + // enabled. + assert(ps->vblank_scheduler); + if (!vblank_scheduler_schedule(ps->vblank_scheduler, + schedule_render_at_vblank, ps)) { + // TODO(yshui): handle error here + abort(); + } } } void queue_redraw(session_t *ps) { - // Whether we have already rendered for the current frame. - // If frame pacing is not enabled, pretend this is false. - // If --benchmark is used, redraw is always queued - if (ps->render_in_progress == RENDER_IDLE && !ps->o.benchmark) { - schedule_render(ps, false); - } else if (ps->render_in_progress > RENDER_QUEUED) { - // render_in_progress > RENDER_QUEUED means we have already issued the - // render commands, so a new render must be scheduled to reflect new - // changes. Otherwise the queued render will include1 the new changes. - ps->redraw_needed = true; + if (ps->render_queued) { + return; } + schedule_render(ps, false); } /** @@ -1395,23 +1454,19 @@ static bool redirect_start(session_t *ps) { } if (ps->present_exists && ps->frame_pacing) { - ps->present_event_id = x_new_id(&ps->c); - auto select_input = xcb_present_select_input( - ps->c.c, ps->present_event_id, session_get_target_window(ps), - XCB_PRESENT_EVENT_MASK_COMPLETE_NOTIFY); - auto notify_msc = xcb_present_notify_msc( - ps->c.c, session_get_target_window(ps), 0, 0, 1, 0); - set_cant_fail_cookie(&ps->c, select_input); - set_cant_fail_cookie(&ps->c, notify_msc); - ps->present_event = xcb_register_for_special_xge( - ps->c.c, &xcb_present_id, ps->present_event_id, NULL); - // Initialize rendering and frame timing statistics, and frame pacing // states. ps->last_msc_instant = 0; ps->last_msc = 0; ps->last_schedule_delay = 0; render_statistics_reset(&ps->render_stats); + ps->vblank_scheduler = + vblank_scheduler_new(ps->loop, &ps->c, session_get_target_window(ps)); + if (!ps->vblank_scheduler) { + return false; + } + vblank_scheduler_schedule(ps->vblank_scheduler, + collect_vblank_interval_statistics, ps); } else if (ps->frame_pacing) { log_error("Present extension is not supported, frame pacing disabled."); ps->frame_pacing = false; @@ -1459,12 +1514,9 @@ static void unredirect(session_t *ps) { free(ps->damage_ring); ps->damage_ring = ps->damage = NULL; - if (ps->present_event_id) { - xcb_present_select_input(ps->c.c, ps->present_event_id, - session_get_target_window(ps), 0); - ps->present_event_id = XCB_NONE; - xcb_unregister_for_special_event(ps->c.c, ps->present_event); - ps->present_event = NULL; + if (ps->vblank_scheduler) { + vblank_scheduler_free(ps->vblank_scheduler); + ps->vblank_scheduler = NULL; } // Must call XSync() here @@ -1474,122 +1526,12 @@ static void unredirect(session_t *ps) { log_debug("Screen unredirected."); } -/// Handle PresentCompleteNotify events -/// -/// Record the MSC value and their timestamps, and schedule handle_end_of_vblank() at the -/// correct time. -static void -handle_present_complete_notify(session_t *ps, xcb_present_complete_notify_event_t *cne) { - if (cne->kind != XCB_PRESENT_COMPLETE_KIND_NOTIFY_MSC) { - return; - } - - assert(ps->frame_pacing); - assert(ps->vblank_event_requested); - ps->vblank_event_requested = false; - - // X sometimes sends duplicate/bogus MSC events, when screen has just been turned - // off. Don't use the msc value in these events. We treat this as not receiving a - // vblank event at all, and try to get a new one. - // - // See: - // https://gitlab.freedesktop.org/xorg/xserver/-/issues/1418 - bool event_is_invalid = cne->msc <= ps->last_msc || cne->ust == 0; - if (event_is_invalid) { - log_debug("Invalid PresentCompleteNotify event, %" PRIu64 " %" PRIu64, - cne->msc, cne->ust); - x_request_vblank_event(ps, ps->last_msc + 1); - return; - } - - struct timespec now; - clock_gettime(CLOCK_MONOTONIC, &now); - auto now_us = (int64_t)(now.tv_sec * 1000000L + now.tv_nsec / 1000); - auto drift = iabs((int64_t)cne->ust - now_us); - - if (ps->last_msc_instant == 0 && drift > 1000000) { - // This is the first MSC event we receive, let's check if the timestamps - // align with the monotonic clock. If not, disable frame pacing because we - // can't schedule frames reliably. - log_error("Temporal anomaly detected, frame pacing disabled. (Are we " - "running inside a time namespace?), %" PRIi64 " %" PRIu64, - now_us, ps->last_msc_instant); - if (ps->render_in_progress == RENDER_STARTED) { - // When frame_pacing is off, render_in_progress can't be - // RENDER_STARTED. See the comment on schedule_render(). - ps->render_in_progress = RENDER_IDLE; - } - ps->frame_pacing = false; - return; - } - - x_check_dpms_status(ps); - - if (ps->last_msc_instant != 0) { - auto frame_count = cne->msc - ps->last_msc; - int frame_time = (int)((cne->ust - ps->last_msc_instant) / frame_count); - if (frame_count == 1 && !ps->screen_is_off) { - render_statistics_add_vblank_time_sample(&ps->render_stats, frame_time); - log_trace("Frame count %lu, frame time: %d us, rolling average: " - "%u us, " - "msc: %" PRIu64 ", offset: %d us", - frame_count, frame_time, - render_statistics_get_vblank_time(&ps->render_stats), - cne->ust, (int)drift); - } else { - log_trace("Frame count %lu, frame time: %d us, msc: %" PRIu64 - ", offset: %d us, not adding sample.", - frame_count, frame_time, cne->ust, (int)drift); - } - } - ps->last_msc_instant = cne->ust; - ps->last_msc = cne->msc; - // Note we can't update ps->render_in_progress here because of this present - // complete notify, as we don't know if the render finished before the end of - // vblank or not. We schedule a call to handle_end_of_vblank() to figure out if we - // are still rendering, and update ps->render_in_progress accordingly. - if (now_us > (int64_t)cne->ust) { - handle_end_of_vblank(ps); - } else { - // Wait until the end of the current vblank to call - // handle_end_of_vblank. If we call it too early, it can - // mistakenly think the render missed the vblank, and doesn't - // schedule render for the next vblank, causing frame drops. - log_trace("The end of this vblank is %" PRIi64 " us into the " - "future", - (int64_t)cne->ust - now_us); - assert(!ev_is_active(&ps->vblank_timer)); - ev_timer_set(&ps->vblank_timer, - ((double)cne->ust - (double)now_us) / 1000000.0, 0); - ev_timer_start(ps->loop, &ps->vblank_timer); - } -} - -static void handle_present_events(session_t *ps) { - if (!ps->present_event) { - // Screen not redirected - return; - } - xcb_present_generic_event_t *ev; - while ((ev = (void *)xcb_poll_for_special_event(ps->c.c, ps->present_event))) { - if (ev->event != ps->present_event_id) { - // This event doesn't have the right event context, it's not meant - // for us. - goto next; - } - - // We only subscribed to the complete notify event. - assert(ev->evtype == XCB_PRESENT_EVENT_COMPLETE_NOTIFY); - handle_present_complete_notify(ps, (void *)ev); - next: - free(ev); - } -} - // Handle queued events 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); - handle_present_events(ps); + if (ps->vblank_scheduler) { + vblank_handle_x_events(ps->vblank_scheduler); + } xcb_generic_event_t *ev; while ((ev = xcb_poll_for_queued_event(ps->c.c))) { @@ -1711,6 +1653,9 @@ static void handle_pending_updates(EV_P_ struct session *ps) { } static void draw_callback_impl(EV_P_ session_t *ps, int revents attr_unused) { + assert(!ps->backend_busy); + assert(ps->render_queued); + struct timespec now; int64_t draw_callback_enter_us; clock_gettime(CLOCK_MONOTONIC, &now); @@ -1801,13 +1746,13 @@ static void draw_callback_impl(EV_P_ session_t *ps, int revents attr_unused) { if (ps->redirected && ps->o.stoppaint_force != ON) { static int paint = 0; - log_trace("Render start, frame %d", paint); + log_verbose("Render start, frame %d", paint); if (!ps->o.legacy_backends) { did_render = paint_all_new(ps, bottom); } else { paint_all(ps, bottom); } - log_trace("Render end"); + log_verbose("Render end"); ps->first_frame = false; paint++; @@ -1816,30 +1761,30 @@ static void draw_callback_impl(EV_P_ session_t *ps, int revents attr_unused) { } } - if (ps->frame_pacing && did_render) { - ps->render_in_progress = RENDER_STARTED; - } else { - // With frame pacing, we set render_in_progress to RENDER_IDLE after the - // end of vblank. Without frame pacing, we won't be receiving vblank - // events, so we set render_in_progress to RENDER_IDLE here, right after - // we issue the render commands. - // The other case is if we decided there is no change to render, in that - // case no render command is issued, so we also set render_in_progress to - // RENDER_IDLE. - ps->render_in_progress = RENDER_IDLE; - } + // With frame pacing, we set backend_busy to true after the end of + // vblank. Without frame pacing, we won't be receiving vblank events, so + // we set backend_busy to false here, right after we issue the render + // commands. + // The other case is if we decided there is no change to render, in that + // case no render command is issued, so we also set backend_busy to + // false. + ps->backend_busy = (ps->frame_pacing && did_render); ps->next_render = 0; if (!fade_running) { ps->fade_time = 0L; } + ps->render_queued = false; + // TODO(yshui) Investigate how big the X critical section needs to be. There are // suggestions that rendering should be in the critical section as well. // Queue redraw if animation is running. This should be picked up by next present // event. - ps->redraw_needed = animation; + if (animation) { + queue_redraw(ps); + } } static void draw_callback(EV_P_ ev_timer *w, int revents) { @@ -1855,13 +1800,6 @@ static void draw_callback(EV_P_ ev_timer *w, int revents) { } } -static void vblank_callback(EV_P_ ev_timer *w, int revents attr_unused) { - session_t *ps = session_ptr(w, vblank_timer); - ev_timer_stop(EV_A_ w); - - handle_end_of_vblank(ps); -} - static void x_event_callback(EV_P attr_unused, ev_io *w, int revents attr_unused) { session_t *ps = (session_t *)w; xcb_generic_event_t *ev = xcb_poll_for_event(ps->c.c); @@ -2013,7 +1951,6 @@ static session_t *session_init(int argc, char **argv, Display *dpy, .randr_exists = 0, .randr_event = 0, .randr_error = 0, - .present_event_id = XCB_NONE, .glx_exists = false, .glx_event = 0, .glx_error = 0, @@ -2450,7 +2387,6 @@ static session_t *session_init(int argc, char **argv, Display *dpy, ev_io_start(ps->loop, &ps->xiow); ev_init(&ps->unredir_timer, tmout_unredir_callback); ev_init(&ps->draw_timer, draw_callback); - ev_init(&ps->vblank_timer, vblank_callback); ev_init(&ps->fade_timer, fade_timer_callback); diff --git a/src/vblank.c b/src/vblank.c new file mode 100644 index 00000000..74420ace --- /dev/null +++ b/src/vblank.c @@ -0,0 +1,257 @@ +#include + +#include +#include +#include +#include +#include + +#include "compiler.h" +#include "config.h" +#include "list.h" // for container_of +#include "log.h" +#include "vblank.h" +#include "x.h" + +struct vblank_closure { + vblank_callback_t fn; + void *user_data; +}; + +struct vblank_scheduler { + struct x_connection *c; + size_t callback_capacity, callback_count; + struct vblank_closure *callbacks; + struct ev_loop *loop; + xcb_window_t target_window; + enum vblank_scheduler_type type; + bool vblank_event_requested; +}; + +struct present_vblank_scheduler { + struct vblank_scheduler base; + + uint64_t last_msc; + /// The timestamp for the end of last vblank. + uint64_t last_ust; + ev_timer callback_timer; + xcb_present_event_t event_id; + xcb_special_event_t *event; +}; + +struct vblank_scheduler_ops { + void (*init)(struct vblank_scheduler *self); + void (*deinit)(struct vblank_scheduler *self); + void (*schedule)(struct vblank_scheduler *self); + bool (*handle_x_events)(struct vblank_scheduler *self); +}; + +static void +vblank_scheduler_invoke_callbacks(struct vblank_scheduler *self, struct vblank_event *event); + +static void present_vblank_scheduler_schedule(struct vblank_scheduler *base) { + auto self = (struct present_vblank_scheduler *)base; + log_verbose("Requesting vblank event for window 0x%08x, msc %" PRIu64, + base->target_window, self->last_msc + 1); + assert(!base->vblank_event_requested); + x_request_vblank_event(base->c, base->target_window, self->last_msc + 1); + base->vblank_event_requested = true; +} + +static void present_vblank_callback(EV_P attr_unused, ev_timer *w, int attr_unused revents) { + auto sched = container_of(w, struct present_vblank_scheduler, callback_timer); + auto event = (struct vblank_event){ + .msc = sched->last_msc, + .ust = sched->last_ust, + }; + sched->base.vblank_event_requested = false; + vblank_scheduler_invoke_callbacks(&sched->base, &event); +} + +static void present_vblank_scheduler_init(struct vblank_scheduler *base) { + auto self = (struct present_vblank_scheduler *)base; + base->type = VBLANK_SCHEDULER_PRESENT; + ev_timer_init(&self->callback_timer, present_vblank_callback, 0, 0); + + self->event_id = x_new_id(base->c); + auto select_input = + xcb_present_select_input(base->c->c, self->event_id, base->target_window, + XCB_PRESENT_EVENT_MASK_COMPLETE_NOTIFY); + set_cant_fail_cookie(base->c, select_input); + self->event = + xcb_register_for_special_xge(base->c->c, &xcb_present_id, self->event_id, NULL); +} + +static void present_vblank_scheduler_deinit(struct vblank_scheduler *base) { + auto self = (struct present_vblank_scheduler *)base; + ev_timer_stop(base->loop, &self->callback_timer); + auto select_input = + xcb_present_select_input(base->c->c, self->event_id, base->target_window, 0); + set_cant_fail_cookie(base->c, select_input); + xcb_unregister_for_special_event(base->c->c, self->event); +} + +/// Handle PresentCompleteNotify events +/// +/// Schedule the registered callback to be called when the current vblank ends. +static void handle_present_complete_notify(struct present_vblank_scheduler *self, + xcb_present_complete_notify_event_t *cne) { + assert(self->base.type == VBLANK_SCHEDULER_PRESENT); + + if (cne->kind != XCB_PRESENT_COMPLETE_KIND_NOTIFY_MSC) { + return; + } + + assert(self->base.vblank_event_requested); + + // X sometimes sends duplicate/bogus MSC events, when screen has just been turned + // off. Don't use the msc value in these events. We treat this as not receiving a + // vblank event at all, and try to get a new one. + // + // See: + // https://gitlab.freedesktop.org/xorg/xserver/-/issues/1418 + bool event_is_invalid = cne->msc <= self->last_msc || cne->ust == 0; + if (event_is_invalid) { + log_debug("Invalid PresentCompleteNotify event, %" PRIu64 " %" PRIu64, + cne->msc, cne->ust); + x_request_vblank_event(self->base.c, cne->window, self->last_msc + 1); + return; + } + + self->last_ust = cne->ust; + self->last_msc = cne->msc; + + struct timespec now; + clock_gettime(CLOCK_MONOTONIC, &now); + auto now_us = (unsigned long)(now.tv_sec * 1000000L + now.tv_nsec / 1000); + double delay_sec = 0.0; + if (now_us < cne->ust) { + log_trace("The end of this vblank is %lu us into the " + "future", + cne->ust - now_us); + delay_sec = (double)(cne->ust - now_us) / 1000000.0; + } + // Wait until the end of the current vblank to invoke callbacks. If we + // call it too early, it can mistakenly think the render missed the + // vblank, and doesn't schedule render for the next vblank, causing frame + // drops. + assert(!ev_is_active(&self->callback_timer)); + ev_timer_set(&self->callback_timer, delay_sec, 0); + ev_timer_start(self->base.loop, &self->callback_timer); +} + +static bool handle_present_events(struct vblank_scheduler *base) { + auto self = (struct present_vblank_scheduler *)base; + xcb_present_generic_event_t *ev; + while ((ev = (void *)xcb_poll_for_special_event(base->c->c, self->event))) { + if (ev->event != self->event_id) { + // This event doesn't have the right event context, it's not meant + // for us. + goto next; + } + + // We only subscribed to the complete notify event. + assert(ev->evtype == XCB_PRESENT_EVENT_COMPLETE_NOTIFY); + handle_present_complete_notify(self, (void *)ev); + next: + free(ev); + } + return true; +} + +static const struct vblank_scheduler_ops vblank_scheduler_ops[LAST_VBLANK_SCHEDULER] = { + [VBLANK_SCHEDULER_PRESENT] = + { + .init = present_vblank_scheduler_init, + .deinit = present_vblank_scheduler_deinit, + .schedule = present_vblank_scheduler_schedule, + .handle_x_events = handle_present_events, + }, +}; + +static void vblank_scheduler_schedule_internal(struct vblank_scheduler *self) { + assert(self->type < LAST_VBLANK_SCHEDULER); + auto fn = vblank_scheduler_ops[self->type].schedule; + assert(fn != NULL); + fn(self); +} + +bool vblank_scheduler_schedule(struct vblank_scheduler *self, + vblank_callback_t vblank_callback, void *user_data) { + if (self->callback_count == 0) { + vblank_scheduler_schedule_internal(self); + } + if (self->callback_count == self->callback_capacity) { + size_t new_capacity = + self->callback_capacity ? self->callback_capacity * 2 : 1; + void *new_buffer = + realloc(self->callbacks, new_capacity * sizeof(*self->callbacks)); + if (!new_buffer) { + return false; + } + self->callbacks = new_buffer; + self->callback_capacity = new_capacity; + } + self->callbacks[self->callback_count++] = (struct vblank_closure){ + .fn = vblank_callback, + .user_data = user_data, + }; + return true; +} + +static void +vblank_scheduler_invoke_callbacks(struct vblank_scheduler *self, struct vblank_event *event) { + // callbacks might be added during callback invocation, so we need to + // copy the callback_count. + size_t count = self->callback_count, write_head = 0; + for (size_t i = 0; i < count; i++) { + auto action = self->callbacks[i].fn(event, self->callbacks[i].user_data); + switch (action) { + case VBLANK_CALLBACK_AGAIN: + if (i != write_head) { + self->callbacks[write_head] = self->callbacks[i]; + } + write_head++; + case VBLANK_CALLBACK_DONE: + default: // nothing to do + break; + } + } + memset(self->callbacks + write_head, 0, + (count - write_head) * sizeof(*self->callbacks)); + assert(count == self->callback_count && "callbacks should not be added when " + "callbacks are being invoked."); + self->callback_count = write_head; + if (self->callback_count) { + vblank_scheduler_schedule_internal(self); + } +} + +void vblank_scheduler_free(struct vblank_scheduler *self) { + assert(self->type < LAST_VBLANK_SCHEDULER); + auto fn = vblank_scheduler_ops[self->type].deinit; + if (fn != NULL) { + fn(self); + } + free(self->callbacks); + free(self); +} + +struct vblank_scheduler *vblank_scheduler_new(struct ev_loop *loop, struct x_connection *c, + xcb_window_t target_window) { + struct vblank_scheduler *self = calloc(1, sizeof(struct present_vblank_scheduler)); + self->target_window = target_window; + self->c = c; + self->loop = loop; + vblank_scheduler_ops[VBLANK_SCHEDULER_PRESENT].init(self); + return self; +} + +bool 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; +} \ No newline at end of file diff --git a/src/vblank.h b/src/vblank.h new file mode 100644 index 00000000..a5073456 --- /dev/null +++ b/src/vblank.h @@ -0,0 +1,45 @@ +#pragma once + +#include +#include +#include +#include + +#include +#include + +#include "x.h" + +/// An object that schedule vblank events. +struct vblank_scheduler; + +struct vblank_event { + uint64_t msc; + uint64_t ust; +}; + +enum vblank_callback_action { + /// The callback should be called again in the next vblank. + VBLANK_CALLBACK_AGAIN, + /// The callback is done and should not be called again. + VBLANK_CALLBACK_DONE, +}; + +typedef enum vblank_callback_action (*vblank_callback_t)(struct vblank_event *event, + void *user_data); + +/// Schedule a vblank event. +/// +/// Schedule for `cb` to be called when the current vblank ends. If this is called +/// from a callback function for the current vblank, the newly scheduled callback +/// will be called in the next vblank. +/// +/// Returns whether the scheduling is successful. Scheduling can fail if there +/// is not enough memory. +bool vblank_scheduler_schedule(struct vblank_scheduler *self, vblank_callback_t cb, + void *user_data); +struct vblank_scheduler *vblank_scheduler_new(struct ev_loop *loop, struct x_connection *c, + xcb_window_t target_window); +void vblank_scheduler_free(struct vblank_scheduler *); + +bool vblank_handle_x_events(struct vblank_scheduler *self); diff --git a/src/x.c b/src/x.c index 89786e18..45abe311 100644 --- a/src/x.c +++ b/src/x.c @@ -779,15 +779,10 @@ err: return false; } -void x_request_vblank_event(session_t *ps, uint64_t msc) { - if (ps->vblank_event_requested) { - return; - } - +void x_request_vblank_event(struct x_connection *c, xcb_window_t window, uint64_t msc) { auto cookie = - xcb_present_notify_msc(ps->c.c, session_get_target_window(ps), 0, msc, 0, 0); - set_cant_fail_cookie(&ps->c, cookie); - ps->vblank_event_requested = true; + xcb_present_notify_msc(c->c, window, 0, msc, 0, 0); + set_cant_fail_cookie(c, cookie); } static inline bool dpms_screen_is_off(xcb_dpms_info_reply_t *info) { @@ -795,18 +790,19 @@ static inline bool dpms_screen_is_off(xcb_dpms_info_reply_t *info) { return info->state && (info->power_level != XCB_DPMS_DPMS_MODE_ON); } -void x_check_dpms_status(session_t *ps) { - auto r = xcb_dpms_info_reply(ps->c.c, xcb_dpms_info(ps->c.c), NULL); +bool x_check_dpms_status(struct x_connection *c, bool *screen_is_off) { + auto r = xcb_dpms_info_reply(c->c, xcb_dpms_info(c->c), NULL); if (!r) { - log_fatal("Failed to query DPMS status."); - abort(); + log_error("Failed to query DPMS status."); + return false; } auto now_screen_is_off = dpms_screen_is_off(r); - if (ps->screen_is_off != now_screen_is_off) { + if (*screen_is_off != now_screen_is_off) { log_debug("Screen is now %s", now_screen_is_off ? "off" : "on"); - ps->screen_is_off = now_screen_is_off; + *screen_is_off = now_screen_is_off; } free(r); + return true; } /** diff --git a/src/x.h b/src/x.h index 60bcfef4..df45b5cc 100644 --- a/src/x.h +++ b/src/x.h @@ -421,7 +421,9 @@ void x_free_monitor_info(struct x_monitors *); uint32_t attr_deprecated xcb_generate_id(xcb_connection_t *c); /// Ask X server to send us a notification for the next end of vblank. -void x_request_vblank_event(session_t *ps, uint64_t msc); +void x_request_vblank_event(struct x_connection *c, xcb_window_t window, uint64_t msc); -/// Update ps->screen_is_off to reflect the current DPMS state. -void x_check_dpms_status(session_t *ps); +/// Update screen_is_off to reflect the current DPMS state. +/// +/// Returns true if the DPMS state was successfully queried, false otherwise. +bool x_check_dpms_status(struct x_connection *c, bool *screen_is_off); From 4b57e3aa4c1c1c4c48bf36ce01f1fc33130c07c1 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Tue, 19 Dec 2023 23:07:32 +0000 Subject: [PATCH 07/18] config: add debug options to enable timing based pacing Disable timing estimation based pacing by default, as it might not work well across drivers, and might have subtle bugs. You can try setting `PICOM_DEBUG=smart_frame_pacing` if you want to try it out. Signed-off-by: Yuxuan Shui --- src/config.c | 5 ++++- src/config.h | 4 +++- src/picom.c | 25 ++++++++++++++++++------- 3 files changed, 25 insertions(+), 9 deletions(-) diff --git a/src/config.c b/src/config.c index efbf279f..8c1ca91d 100644 --- a/src/config.c +++ b/src/config.c @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -623,7 +624,9 @@ struct debug_options_entry { }; static const struct debug_options_entry debug_options_entries[] = { - + "smart_frame_pacing", + NULL, + offsetof(struct debug_options, smart_frame_pacing), }; void parse_debug_option_single(char *setting, struct debug_options *debug_options) { diff --git a/src/config.h b/src/config.h index bdf384ac..ac3b6ad2 100644 --- a/src/config.h +++ b/src/config.h @@ -85,7 +85,9 @@ enum vblank_scheduler_type { /// Internal, private options for debugging and development use. struct debug_options { - + /// Try to reduce frame latency by using vblank interval and render time + /// estimates. Right now it's not working well across drivers. + int smart_frame_pacing; }; /// Structure representing all options. diff --git a/src/picom.c b/src/picom.c index 4c75329d..b7bb14c6 100644 --- a/src/picom.c +++ b/src/picom.c @@ -152,6 +152,12 @@ collect_vblank_interval_statistics(struct vblank_event *e, void *ud) { assert(ps->frame_pacing); assert(ps->vblank_scheduler); + if (!ps->o.debug_options.smart_frame_pacing) { + // We don't need to collect statistics if we are not doing smart frame + // pacing. + return VBLANK_CALLBACK_DONE; + } + // TODO(yshui): this naive method of estimating vblank interval does not handle // the variable refresh rate case very well. This includes the case // of a VRR enabled monitor; or a monitor that's turned off, in which @@ -220,13 +226,15 @@ enum vblank_callback_action schedule_render_at_vblank(struct vblank_event *e, vo } // The frame has been finished and presented, record its render time. - int render_time_us = - (int)(render_time.tv_sec * 1000000L + render_time.tv_nsec / 1000L); - render_statistics_add_render_time_sample( - &ps->render_stats, render_time_us + (int)ps->last_schedule_delay); - log_verbose("Last render call took: %d (gpu) + %d (cpu) us, " - "last_msc: %" PRIu64, - render_time_us, (int)ps->last_schedule_delay, ps->last_msc); + if (ps->o.debug_options.smart_frame_pacing) { + int render_time_us = + (int)(render_time.tv_sec * 1000000L + render_time.tv_nsec / 1000L); + render_statistics_add_render_time_sample( + &ps->render_stats, render_time_us + (int)ps->last_schedule_delay); + log_verbose("Last render call took: %d (gpu) + %d (cpu) us, " + "last_msc: %" PRIu64, + render_time_us, (int)ps->last_schedule_delay, ps->last_msc); + } ps->last_schedule_delay = 0; ps->backend_busy = false; @@ -319,6 +327,9 @@ void schedule_render(session_t *ps, bool triggered_by_vblank attr_unused) { return; } + // if ps->o.debug_options.smart_frame_pacing is false, we won't have any render + // time or vblank interval estimates, so we would naturally fallback to schedule + // render immediately. auto render_budget = render_statistics_get_budget(&ps->render_stats, &divisor); auto frame_time = render_statistics_get_vblank_time(&ps->render_stats); if (frame_time == 0) { From 3838b45e2c44cd6c976c4b0b5089b777269e9d2b Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Tue, 19 Dec 2023 23:14:15 +0000 Subject: [PATCH 08/18] core: simplify pacing logic a bit more Also, vblank event callback should call schedule_render to queue renders instead of starting the draw timer directly, so that the CPU time calculation will be correct. Signed-off-by: Yuxuan Shui --- src/backend/backend.c | 8 +-- src/picom.c | 147 ++++++++++++++++++++++++------------------ 2 files changed, 87 insertions(+), 68 deletions(-) diff --git a/src/backend/backend.c b/src/backend/backend.c index dd3366d6..5120672b 100644 --- a/src/backend/backend.c +++ b/src/backend/backend.c @@ -194,10 +194,10 @@ bool paint_all_new(session_t *ps, struct managed_win *const t) { auto after_damage_us = (uint64_t)now.tv_sec * 1000000UL + (uint64_t)now.tv_nsec / 1000; log_trace("Getting damage took %" PRIu64 " us", after_damage_us - after_sync_fence_us); if (ps->next_render > 0) { - log_trace("Render schedule deviation: %ld us (%s) %" PRIu64 " %ld", - labs((long)after_damage_us - (long)ps->next_render), - after_damage_us < ps->next_render ? "early" : "late", - after_damage_us, ps->next_render); + log_verbose("Render schedule deviation: %ld us (%s) %" PRIu64 " %ld", + labs((long)after_damage_us - (long)ps->next_render), + after_damage_us < ps->next_render ? "early" : "late", + after_damage_us, ps->next_render); ps->last_schedule_delay = 0; if (after_damage_us > ps->next_render) { ps->last_schedule_delay = after_damage_us - ps->next_render; diff --git a/src/picom.c b/src/picom.c index b7bb14c6..9664c825 100644 --- a/src/picom.c +++ b/src/picom.c @@ -201,56 +201,50 @@ collect_vblank_interval_statistics(struct vblank_event *e, void *ud) { } return VBLANK_CALLBACK_DONE; } -/// vblank callback scheduled by schedule_render. + +void schedule_render(session_t *ps, bool triggered_by_vblank); + +/// vblank callback scheduled by schedule_render, when a render is ongoing. /// -/// Check if previously queued render has finished, and record the time it took. -enum vblank_callback_action schedule_render_at_vblank(struct vblank_event *e, void *ud) { +/// Check if previously queued render has finished, and reschedule render if it has. +enum vblank_callback_action reschedule_render_at_vblank(struct vblank_event *e, void *ud) { auto ps = (session_t *)ud; assert(ps->frame_pacing); - assert(ps->backend_busy); assert(ps->render_queued); assert(ps->vblank_scheduler); + log_verbose("Rescheduling render at vblank, msc: %" PRIu64, e->msc); + collect_vblank_interval_statistics(e, ud); - struct timespec render_time; - bool completed = - ps->backend_data->ops->last_render_time(ps->backend_data, &render_time); - if (!completed) { - // Render hasn't completed yet, we can't start another render. - // Check again at the next vblank. - log_debug("Last render did not complete during vblank, msc: " - "%" PRIu64, - ps->last_msc); - return VBLANK_CALLBACK_AGAIN; + if (ps->backend_busy) { + struct timespec render_time; + bool completed = + ps->backend_data->ops->last_render_time(ps->backend_data, &render_time); + if (!completed) { + // Render hasn't completed yet, we can't start another render. + // Check again at the next vblank. + log_debug("Last render did not complete during vblank, msc: " + "%" PRIu64, + ps->last_msc); + return VBLANK_CALLBACK_AGAIN; + } + + // The frame has been finished and presented, record its render time. + if (ps->o.debug_options.smart_frame_pacing) { + int render_time_us = (int)(render_time.tv_sec * 1000000L + + render_time.tv_nsec / 1000L); + render_statistics_add_render_time_sample( + &ps->render_stats, render_time_us + (int)ps->last_schedule_delay); + log_verbose("Last render call took: %d (gpu) + %d (cpu) us, " + "last_msc: %" PRIu64, + render_time_us, (int)ps->last_schedule_delay, + ps->last_msc); + } + ps->backend_busy = false; } - // The frame has been finished and presented, record its render time. - if (ps->o.debug_options.smart_frame_pacing) { - int render_time_us = - (int)(render_time.tv_sec * 1000000L + render_time.tv_nsec / 1000L); - render_statistics_add_render_time_sample( - &ps->render_stats, render_time_us + (int)ps->last_schedule_delay); - log_verbose("Last render call took: %d (gpu) + %d (cpu) us, " - "last_msc: %" PRIu64, - render_time_us, (int)ps->last_schedule_delay, ps->last_msc); - } - ps->last_schedule_delay = 0; - ps->backend_busy = false; - - struct timespec now; - clock_gettime(CLOCK_MONOTONIC, &now); - auto now_us = (uint64_t)now.tv_sec * 1000000 + (uint64_t)now.tv_nsec / 1000; - double delay_s = 0; - if (ps->next_render > now_us) { - delay_s = (double)(ps->next_render - now_us) / 1000000.0; - } - log_verbose("Prepare to start rendering: delay: %f s, next_render: %" PRIu64 - ", now_us: %" PRIu64, - delay_s, ps->next_render, now_us); - assert(!ev_is_active(&ps->draw_timer)); - ev_timer_set(&ps->draw_timer, delay_s, 0); - ev_timer_start(ps->loop, &ps->draw_timer); + schedule_render(ps, false); return VBLANK_CALLBACK_DONE; } @@ -261,7 +255,7 @@ enum vblank_callback_action schedule_render_at_vblank(struct vblank_event *e, vo /// 1. queue_redraw() queues a new render by calling schedule_render, if there /// is no render currently scheduled. i.e. render_queued == false. /// 2. then, we need to figure out the best time to start rendering. we need to -/// at least know when the current vblank will start, as we can't start render +/// at least know when the next vblank will start, as we can't start render /// before the current rendered frame is diplayed on screen. we have this /// information from the vblank scheduler, it will notify us when that happens. /// we might also want to delay the rendering even further to reduce latency, @@ -271,14 +265,20 @@ enum vblank_callback_action schedule_render_at_vblank(struct vblank_event *e, vo /// vblank event is delivered). Backend APIs are called to issue render /// commands. render_queued is set to false, and backend_busy is set to true. /// -/// There is a small caveat in step 2. As a vblank event being delivered +/// There are some considerations in step 2: +/// +/// First of all, a vblank event being delivered /// doesn't necessarily mean the frame has been displayed on screen. If a frame /// takes too long to render, it might miss the current vblank, and will be /// displayed on screen during one of the subsequent vblanks. So in /// schedule_render_at_vblank, we ask the backend to see if it has finished /// rendering. if not, render_queued is unchanged, and another vblank is /// scheduled; otherwise, draw_callback_impl will be scheduled to be call at -/// an appropriate time. +/// an appropriate time. Second, we might not have rendered for the previous vblank, +/// in which case the last vblank event we received could be many frames in the past, +/// so we can't make scheduling decisions based on that. So we always schedule +/// a vblank event when render is queued, and make scheduling decisions when the +/// event is delivered. /// /// All of the above is what happens when frame_pacing is true. Otherwise /// render_in_progress is either QUEUED or IDLE, and queue_redraw will always @@ -306,6 +306,21 @@ enum vblank_callback_action schedule_render_at_vblank(struct vblank_event *e, vo /// The code that does this is already implemented below, but disabled by /// default. There are several problems with it, see bug #1072. void schedule_render(session_t *ps, bool triggered_by_vblank attr_unused) { + // If the backend is busy, we will try again at the next vblank. + if (ps->backend_busy) { + // We should never have set backend_busy to true unless frame_pacing is + // enabled. + assert(ps->vblank_scheduler); + assert(ps->frame_pacing); + log_verbose("Backend busy, will reschedule render at next vblank."); + if (!vblank_scheduler_schedule(ps->vblank_scheduler, + reschedule_render_at_vblank, ps)) { + // TODO(yshui): handle error here + abort(); + } + return; + } + // By default, we want to schedule render immediately, later in this function we // might adjust that and move the render later, based on render timing statistics. double delay_s = 0; @@ -358,37 +373,41 @@ void schedule_render(session_t *ps, bool triggered_by_vblank attr_unused) { } log_verbose("Delay: %.6lf s, last_msc: %" PRIu64 ", render_budget: %d, " - "frame_time: " - "%" PRIu32 ", now_us: %" PRIu64 ", next_msc: %" PRIu64 ", " - "divisor: %d", + "frame_time: %" PRIu32 ", now_us: %" PRIu64 ", next_render: %" PRIu64 + ", next_msc: %" PRIu64 ", divisor: " + "%d", delay_s, ps->last_msc_instant, render_budget, frame_time, now_us, - deadline, divisor); + ps->next_render, deadline, divisor); schedule: - ps->render_queued = true; // If the backend is not busy, we just need to schedule the render at the - // specified time; otherwise we need to wait for vblank events. - if (!ps->backend_busy) { - assert(!ev_is_active(&ps->draw_timer)); - ev_timer_set(&ps->draw_timer, delay_s, 0); - ev_timer_start(ps->loop, &ps->draw_timer); - } else { - // We should never set backend_busy to true unless frame_pacing is - // enabled. - assert(ps->vblank_scheduler); - if (!vblank_scheduler_schedule(ps->vblank_scheduler, - schedule_render_at_vblank, ps)) { - // TODO(yshui): handle error here - abort(); - } - } + // specified time; otherwise we need to wait for the next vblank event and + // reschedule. + ps->last_schedule_delay = 0; + assert(!ev_is_active(&ps->draw_timer)); + ev_timer_set(&ps->draw_timer, delay_s, 0); + ev_timer_start(ps->loop, &ps->draw_timer); } void queue_redraw(session_t *ps) { + log_verbose("Queue redraw, render_queued: %d, backend_busy: %d", + ps->render_queued, ps->backend_busy); + if (ps->render_queued) { return; } - schedule_render(ps, false); + ps->render_queued = true; + if (ps->o.debug_options.smart_frame_pacing && ps->vblank_scheduler) { + // Make we schedule_render call is synced with vblank events. + // See the comment on schedule_render for more details. + if (!vblank_scheduler_schedule(ps->vblank_scheduler, + reschedule_render_at_vblank, ps)) { + // TODO(yshui): handle error here + abort(); + } + } else { + schedule_render(ps, false); + } } /** From 25d16b03527b2a7da9b23848380bb0593e3472d9 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Mon, 18 Dec 2023 09:33:17 +0000 Subject: [PATCH 09/18] x: fix x_request_vblank_event present_notify_msc with divisor == 0 has undocumented special meaning, it means async present notify, which means we could receive MSC notifications from the past. Signed-off-by: Yuxuan Shui --- src/x.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/x.c b/src/x.c index 45abe311..06c8b939 100644 --- a/src/x.c +++ b/src/x.c @@ -780,8 +780,7 @@ err: } void x_request_vblank_event(struct x_connection *c, xcb_window_t window, uint64_t msc) { - auto cookie = - xcb_present_notify_msc(c->c, window, 0, msc, 0, 0); + auto cookie = xcb_present_notify_msc(c->c, window, 0, msc, 1, 0); set_cant_fail_cookie(c, cookie); } From c42e6cef0a0f5c6999917d6f36fd549610575d33 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Tue, 19 Dec 2023 23:17:09 +0000 Subject: [PATCH 10/18] vblank: winding down vblank events instead of stopping immediately I noticed sometimes full frame rate video is rendered at half frame rate sometimes. That is because the damage notify is sent very close to vblank, and since we request vblank events when we get the damage, we miss the vblank event immediately after, despite the damage happening before the vblank. request next ...... next next damage vblank vblank vblank | | | ...... | v v v v ---------------------->>>>>>--------- `request vblank` is triggered by `damage`, but because it's too close to `next vblank`, that vblank is missed despite we requested before it happening, and we only get `next next vblank`. The result is we will drop every other frame. The solution in this commit is that we will keep requesting vblank events, right after we received a vblank event, even when nobody is asking for them. We would do that for a set number of vblanks before stopping (currently 4). Signed-off-by: Yuxuan Shui --- src/vblank.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/vblank.c b/src/vblank.c index 74420ace..da729111 100644 --- a/src/vblank.c +++ b/src/vblank.c @@ -18,11 +18,19 @@ struct vblank_closure { void *user_data; }; +#define VBLANK_WIND_DOWN 4 + struct vblank_scheduler { struct x_connection *c; size_t callback_capacity, callback_count; struct vblank_closure *callbacks; struct ev_loop *loop; + /// Request extra vblank events even when no callbacks are scheduled. + /// This is because when callbacks are scheduled too close to a vblank, + /// we might send PresentNotifyMsc request too late and miss the vblank event. + /// So we request extra vblank events right after the last vblank event + /// to make sure this doesn't happen. + unsigned int wind_down; xcb_window_t target_window; enum vblank_scheduler_type type; bool vblank_event_requested; @@ -178,7 +186,7 @@ static void vblank_scheduler_schedule_internal(struct vblank_scheduler *self) { bool vblank_scheduler_schedule(struct vblank_scheduler *self, vblank_callback_t vblank_callback, void *user_data) { - if (self->callback_count == 0) { + if (self->callback_count == 0 && self->wind_down == 0) { vblank_scheduler_schedule_internal(self); } if (self->callback_count == self->callback_capacity) { @@ -204,6 +212,11 @@ vblank_scheduler_invoke_callbacks(struct vblank_scheduler *self, struct vblank_e // callbacks might be added during callback invocation, so we need to // copy the callback_count. size_t count = self->callback_count, write_head = 0; + if (count == 0) { + self->wind_down--; + } else { + self->wind_down = VBLANK_WIND_DOWN; + } for (size_t i = 0; i < count; i++) { auto action = self->callbacks[i].fn(event, self->callbacks[i].user_data); switch (action) { @@ -222,7 +235,7 @@ vblank_scheduler_invoke_callbacks(struct vblank_scheduler *self, struct vblank_e assert(count == self->callback_count && "callbacks should not be added when " "callbacks are being invoked."); self->callback_count = write_head; - if (self->callback_count) { + if (self->callback_count || self->wind_down) { vblank_scheduler_schedule_internal(self); } } From dd83f550e1af2aaf28638ad7b68b3511ff8169b5 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Tue, 19 Dec 2023 23:21:07 +0000 Subject: [PATCH 11/18] core: always check if render is finished at next vblank Right now we rely on `reschedule_render_at_vblank` being scheduled for the render finish check. Which means we will only know if a frame has finished rendering the next time we queue a render. And we only check at vblank boundary, which means when a render is queued we have to wait for the next vblank, when had we checked earlier, we will be able to start rendering immediately Signed-off-by: Yuxuan Shui --- src/picom.c | 64 +++++++++++++++++++++++++++++++++-------------------- 1 file changed, 40 insertions(+), 24 deletions(-) diff --git a/src/picom.c b/src/picom.c index 9664c825..8b53551d 100644 --- a/src/picom.c +++ b/src/picom.c @@ -145,6 +145,38 @@ static inline struct managed_win *find_win_all(session_t *ps, const xcb_window_t return w; } +enum vblank_callback_action check_render_finish(struct vblank_event *e attr_unused, void *ud) { + auto ps = (session_t *)ud; + if (!ps->backend_busy) { + return VBLANK_CALLBACK_DONE; + } + + struct timespec render_time; + bool completed = + ps->backend_data->ops->last_render_time(ps->backend_data, &render_time); + if (!completed) { + // Render hasn't completed yet, we can't start another render. + // Check again at the next vblank. + log_debug("Last render did not complete during vblank, msc: " + "%" PRIu64, + ps->last_msc); + return VBLANK_CALLBACK_AGAIN; + } + + // The frame has been finished and presented, record its render time. + if (ps->o.debug_options.smart_frame_pacing) { + int render_time_us = + (int)(render_time.tv_sec * 1000000L + render_time.tv_nsec / 1000L); + render_statistics_add_render_time_sample( + &ps->render_stats, render_time_us + (int)ps->last_schedule_delay); + log_verbose("Last render call took: %d (gpu) + %d (cpu) us, " + "last_msc: %" PRIu64, + render_time_us, (int)ps->last_schedule_delay, ps->last_msc); + } + ps->backend_busy = false; + return VBLANK_CALLBACK_DONE; +} + enum vblank_callback_action collect_vblank_interval_statistics(struct vblank_event *e, void *ud) { auto ps = (session_t *)ud; @@ -216,32 +248,10 @@ enum vblank_callback_action reschedule_render_at_vblank(struct vblank_event *e, log_verbose("Rescheduling render at vblank, msc: %" PRIu64, e->msc); collect_vblank_interval_statistics(e, ud); + check_render_finish(e, ud); if (ps->backend_busy) { - struct timespec render_time; - bool completed = - ps->backend_data->ops->last_render_time(ps->backend_data, &render_time); - if (!completed) { - // Render hasn't completed yet, we can't start another render. - // Check again at the next vblank. - log_debug("Last render did not complete during vblank, msc: " - "%" PRIu64, - ps->last_msc); - return VBLANK_CALLBACK_AGAIN; - } - - // The frame has been finished and presented, record its render time. - if (ps->o.debug_options.smart_frame_pacing) { - int render_time_us = (int)(render_time.tv_sec * 1000000L + - render_time.tv_nsec / 1000L); - render_statistics_add_render_time_sample( - &ps->render_stats, render_time_us + (int)ps->last_schedule_delay); - log_verbose("Last render call took: %d (gpu) + %d (cpu) us, " - "last_msc: %" PRIu64, - render_time_us, (int)ps->last_schedule_delay, - ps->last_msc); - } - ps->backend_busy = false; + return VBLANK_CALLBACK_AGAIN; } schedule_render(ps, false); @@ -1815,6 +1825,12 @@ static void draw_callback_impl(EV_P_ session_t *ps, int revents attr_unused) { if (animation) { queue_redraw(ps); } + if (ps->vblank_scheduler) { + // Even if we might not want to render during next vblank, we want to keep + // `backend_busy` up to date, so when the next render comes, we can + // immediately know if we can render. + vblank_scheduler_schedule(ps->vblank_scheduler, check_render_finish, ps); + } } static void draw_callback(EV_P_ ev_timer *w, int revents) { From db6ed75b605090dd708f5c143e210fea9f782c35 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Mon, 18 Dec 2023 20:32:22 +0000 Subject: [PATCH 12/18] core: don't always delay schedule_render to vblank It's kind of dumb anyway. If we get damage event right after a vblank event, we would waste the whole vblank. Instead improve the frame scheduling logic to target the right vblank interval. This only affects smart_frame_pacing anyway. Signed-off-by: Yuxuan Shui --- src/picom.c | 40 ++++++++++++++++++++-------------------- src/statistics.c | 13 +------------ src/statistics.h | 7 +------ 3 files changed, 22 insertions(+), 38 deletions(-) diff --git a/src/picom.c b/src/picom.c index 8b53551d..c2e4251a 100644 --- a/src/picom.c +++ b/src/picom.c @@ -342,29 +342,39 @@ void schedule_render(session_t *ps, bool triggered_by_vblank attr_unused) { ps->next_render = now_us; if (!ps->frame_pacing || !ps->redirected) { - // If not doing frame pacing, schedule a render immediately unless it's - // already scheduled; if not redirected, we schedule immediately to have a - // chance to redirect. We won't have frame or render timing information + // If not doing frame pacing, schedule a render immediately; if + // not redirected, we schedule immediately to have a chance to + // redirect. We won't have frame or render timing information // anyway. - if (!ev_is_active(&ps->draw_timer)) { - goto schedule; - } - return; + assert(!ev_is_active(&ps->draw_timer)); + goto schedule; } // if ps->o.debug_options.smart_frame_pacing is false, we won't have any render // time or vblank interval estimates, so we would naturally fallback to schedule // render immediately. - auto render_budget = render_statistics_get_budget(&ps->render_stats, &divisor); + auto render_budget = render_statistics_get_budget(&ps->render_stats); auto frame_time = render_statistics_get_vblank_time(&ps->render_stats); if (frame_time == 0) { // We don't have enough data for render time estimates, maybe there's // no frame rendered yet, or the backend doesn't support render timing // information, schedule render immediately. + log_verbose("Not enough data for render time estimates."); goto schedule; } - auto const deadline = ps->last_msc_instant + (unsigned long)divisor * frame_time; + if (render_budget >= frame_time) { + // If the estimated render time is already longer than the estimated + // vblank interval, there is no way we can make it. Instead of always + // dropping frames, we try desperately to catch up and schedule a + // render immediately. + log_verbose("Render budget: %u us >= frame time: %" PRIu32 " us", + render_budget, frame_time); + goto schedule; + } + + auto target_frame = (now_us + render_budget - ps->last_msc_instant) / frame_time + 1; + auto const deadline = ps->last_msc_instant + target_frame * frame_time; unsigned int available = 0; if (deadline > now_us) { available = (unsigned int)(deadline - now_us); @@ -407,17 +417,7 @@ void queue_redraw(session_t *ps) { return; } ps->render_queued = true; - if (ps->o.debug_options.smart_frame_pacing && ps->vblank_scheduler) { - // Make we schedule_render call is synced with vblank events. - // See the comment on schedule_render for more details. - if (!vblank_scheduler_schedule(ps->vblank_scheduler, - reschedule_render_at_vblank, ps)) { - // TODO(yshui): handle error here - abort(); - } - } else { - schedule_render(ps, false); - } + schedule_render(ps, false); } /** diff --git a/src/statistics.c b/src/statistics.c index 5e3d9249..11c7466e 100644 --- a/src/statistics.c +++ b/src/statistics.c @@ -55,26 +55,15 @@ void render_statistics_add_render_time_sample(struct render_statistics *rs, int /// A `divisor` is also returned, indicating the target framerate. The divisor is /// the number of vblanks we should wait between each frame. A divisor of 1 means /// full framerate, 2 means half framerate, etc. -unsigned int -render_statistics_get_budget(struct render_statistics *rs, unsigned int *divisor) { +unsigned int render_statistics_get_budget(struct render_statistics *rs) { if (rs->render_times.nelem < rs->render_times.window_size) { // No valid render time estimates yet. Assume maximum budget. - *divisor = 1; return UINT_MAX; } // N-th percentile of render times, see render_statistics_init for N. auto render_time_percentile = rolling_quantile_estimate(&rs->render_time_quantile, &rs->render_times); - auto vblank_time_us = render_statistics_get_vblank_time(rs); - if (vblank_time_us == 0) { - // We don't have a good estimate of the vblank time yet, so we - // assume we can finish in one vblank. - *divisor = 1; - } else { - *divisor = - (unsigned int)(render_time_percentile / rs->vblank_time_us.mean + 1); - } return (unsigned int)render_time_percentile; } diff --git a/src/statistics.h b/src/statistics.h index 42d7bc2d..a1114861 100644 --- a/src/statistics.h +++ b/src/statistics.h @@ -23,12 +23,7 @@ void render_statistics_add_vblank_time_sample(struct render_statistics *rs, int void render_statistics_add_render_time_sample(struct render_statistics *rs, int time_us); /// How much time budget we should give to the backend for rendering, in microseconds. -/// -/// A `divisor` is also returned, indicating the target framerate. The divisor is -/// the number of vblanks we should wait between each frame. A divisor of 1 means -/// full framerate, 2 means half framerate, etc. -unsigned int -render_statistics_get_budget(struct render_statistics *rs, unsigned int *divisor); +unsigned int render_statistics_get_budget(struct render_statistics *rs); /// Return the measured vblank interval in microseconds. Returns 0 if not enough /// samples have been collected yet. From db9b808bfbfc0ff4382d9934aeb8a062dbeaa6dd Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Tue, 19 Dec 2023 23:21:53 +0000 Subject: [PATCH 13/18] vblank: add GLX_SGI_video_sync based scheduler Present extension based scheduler doesn't work well on NVIDIA drivers. GLX_SGI_video_sync is less accurate, but is rumoured to work. See [kwin's usage](https://invent.kde.org/plasma/kwin/-/blob/master/src/ backends/x11/standalone/x11_standalone_sgivideosyncvsyncmonitor.cpp) Signed-off-by: Yuxuan Shui --- src/meson.build | 2 +- src/picom.c | 3 +- src/vblank.c | 281 +++++++++++++++++++++++++++++++++++++++++++++++- src/vblank.h | 6 +- 4 files changed, 283 insertions(+), 9 deletions(-) diff --git a/src/meson.build b/src/meson.build index 51f96488..a2f9c36c 100644 --- a/src/meson.build +++ b/src/meson.build @@ -59,7 +59,7 @@ endif if get_option('opengl') cflags += ['-DCONFIG_OPENGL', '-DGL_GLEXT_PROTOTYPES'] - deps += [dependency('gl', required: true), dependency('egl', required: true)] + deps += [dependency('gl', required: true), dependency('egl', required: true), dependency('threads', required:true)] srcs += [ 'opengl.c' ] endif diff --git a/src/picom.c b/src/picom.c index c2e4251a..e5ac8336 100644 --- a/src/picom.c +++ b/src/picom.c @@ -1501,7 +1501,8 @@ static bool redirect_start(session_t *ps) { ps->last_schedule_delay = 0; render_statistics_reset(&ps->render_stats); ps->vblank_scheduler = - vblank_scheduler_new(ps->loop, &ps->c, session_get_target_window(ps)); + vblank_scheduler_new(ps->loop, &ps->c, session_get_target_window(ps), + VBLANK_SCHEDULER_PRESENT); if (!ps->vblank_scheduler) { return false; } diff --git a/src/vblank.c b/src/vblank.c index da729111..ff83a9a7 100644 --- a/src/vblank.c +++ b/src/vblank.c @@ -2,12 +2,26 @@ #include #include +#include #include +#include #include #include +#include "config.h" + +#ifdef CONFIG_OPENGL +// Enable sgi_video_sync_vblank_scheduler +#include +#include +#include +#include +#include +#include + +#include "backend/gl/glx.h" +#endif #include "compiler.h" -#include "config.h" #include "list.h" // for container_of #include "log.h" #include "vblank.h" @@ -48,6 +62,7 @@ struct present_vblank_scheduler { }; struct vblank_scheduler_ops { + size_t size; void (*init)(struct vblank_scheduler *self); void (*deinit)(struct vblank_scheduler *self); void (*schedule)(struct vblank_scheduler *self); @@ -57,6 +72,242 @@ struct vblank_scheduler_ops { static void vblank_scheduler_invoke_callbacks(struct vblank_scheduler *self, struct vblank_event *event); +#ifdef CONFIG_OPENGL +struct sgi_video_sync_vblank_scheduler { + struct vblank_scheduler base; + + // Since glXWaitVideoSyncSGI blocks, we need to run it in a separate thread. + // ... and all the thread shenanigans that come with it. + _Atomic unsigned int last_msc; + _Atomic uint64_t last_ust; + ev_async notify; + pthread_t sync_thread; + bool running, error; + + /// Protects `running`, `error` and `base.vblank_event_requested` + pthread_mutex_t vblank_requested_mtx; + pthread_cond_t vblank_requested_cnd; +}; + +struct sgi_video_sync_thread_args { + struct sgi_video_sync_vblank_scheduler *self; + int start_status; + pthread_mutex_t start_mtx; + pthread_cond_t start_cnd; +}; + +static bool check_sgi_video_sync_extension(Display *dpy, int screen) { + const char *glx_ext = glXQueryExtensionsString(dpy, screen); + const char *needle = "GLX_SGI_video_sync"; + char *found = strstr(glx_ext, needle); + if (!found) { + return false; + } + if (found != glx_ext && found[-1] != ' ') { + return false; + } + if (found[strlen(needle)] != ' ' && found[strlen(needle)] != '\0') { + return false; + } + + glXWaitVideoSyncSGI = (PFNGLXWAITVIDEOSYNCSGIPROC)(void *)glXGetProcAddress( + (const GLubyte *)"glXWaitVideoSyncSGI"); + if (!glXWaitVideoSyncSGI) { + return false; + } + return true; +} + +static void *sgi_video_sync_thread(void *data) { + auto args = (struct sgi_video_sync_thread_args *)data; + auto self = args->self; + Display *dpy = XOpenDisplay(NULL); + int error_code = 0; + if (!dpy) { + error_code = 1; + goto start_failed; + } + Window root = DefaultRootWindow(dpy), dummy = None; + int screen = DefaultScreen(dpy); + int ncfg = 0; + GLXFBConfig *cfg_ = glXChooseFBConfig( + dpy, screen, + (int[]){GLX_RENDER_TYPE, GLX_RGBA_BIT, GLX_DRAWABLE_TYPE, GLX_WINDOW_BIT, 0}, + &ncfg); + GLXContext ctx = NULL; + GLXDrawable drawable = None; + + if (!cfg_) { + error_code = 2; + goto start_failed; + } + GLXFBConfig cfg = cfg_[0]; + XFree(cfg_); + + XVisualInfo *vi = glXGetVisualFromFBConfig(dpy, cfg); + if (!vi) { + error_code = 3; + goto start_failed; + } + + Visual *visual = vi->visual; + const int depth = vi->depth; + XFree(vi); + + Colormap colormap = XCreateColormap(dpy, root, visual, AllocNone); + XSetWindowAttributes attributes; + attributes.colormap = colormap; + + dummy = XCreateWindow(dpy, root, 0, 0, 1, 1, 0, depth, InputOutput, visual, + CWColormap, &attributes); + XFreeColormap(dpy, colormap); + if (dummy == None) { + error_code = 4; + goto start_failed; + } + + drawable = glXCreateWindow(dpy, cfg, dummy, NULL); + if (drawable == None) { + error_code = 5; + goto start_failed; + } + + ctx = glXCreateNewContext(dpy, cfg, GLX_RGBA_TYPE, 0, true); + if (ctx == NULL) { + error_code = 6; + goto start_failed; + } + + if (!glXMakeContextCurrent(dpy, drawable, drawable, ctx)) { + error_code = 7; + goto start_failed; + } + + if (!check_sgi_video_sync_extension(dpy, screen)) { + error_code = 8; + goto start_failed; + } + + pthread_mutex_lock(&args->start_mtx); + args->start_status = 0; + pthread_cond_signal(&args->start_cnd); + pthread_mutex_unlock(&args->start_mtx); + + pthread_mutex_lock(&self->vblank_requested_mtx); + while (self->running) { + if (!self->base.vblank_event_requested) { + pthread_cond_wait(&self->vblank_requested_cnd, + &self->vblank_requested_mtx); + continue; + } + pthread_mutex_unlock(&self->vblank_requested_mtx); + + unsigned int last_msc; + glXWaitVideoSyncSGI(1, 0, &last_msc); + + struct timespec now; + clock_gettime(CLOCK_MONOTONIC, &now); + atomic_store(&self->last_msc, last_msc); + atomic_store(&self->last_ust, + (uint64_t)(now.tv_sec * 1000000 + now.tv_nsec / 1000)); + ev_async_send(self->base.loop, &self->notify); + pthread_mutex_lock(&self->vblank_requested_mtx); + } + pthread_mutex_unlock(&self->vblank_requested_mtx); + goto cleanup; + +start_failed: + pthread_mutex_lock(&args->start_mtx); + args->start_status = error_code; + pthread_cond_signal(&args->start_cnd); + pthread_mutex_unlock(&args->start_mtx); + +cleanup: + if (dpy) { + glXMakeCurrent(dpy, None, NULL); + if (ctx) { + glXDestroyContext(dpy, ctx); + } + if (drawable) { + glXDestroyWindow(dpy, drawable); + } + if (dummy) { + XDestroyWindow(dpy, dummy); + } + XCloseDisplay(dpy); + } + return NULL; +} + +static void sgi_video_sync_scheduler_schedule(struct vblank_scheduler *base) { + auto self = (struct sgi_video_sync_vblank_scheduler *)base; + log_verbose("Requesting vblank event for msc %d", self->last_msc + 1); + pthread_mutex_lock(&self->vblank_requested_mtx); + assert(!base->vblank_event_requested); + base->vblank_event_requested = true; + pthread_cond_signal(&self->vblank_requested_cnd); + pthread_mutex_unlock(&self->vblank_requested_mtx); +} + +static void +sgi_video_sync_scheduler_callback(EV_P attr_unused, ev_async *w, int attr_unused revents) { + auto sched = container_of(w, struct sgi_video_sync_vblank_scheduler, notify); + auto event = (struct vblank_event){ + .msc = atomic_load(&sched->last_msc), + .ust = atomic_load(&sched->last_ust), + }; + sched->base.vblank_event_requested = false; + log_verbose("Received vblank event for msc %lu", event.msc); + vblank_scheduler_invoke_callbacks(&sched->base, &event); +} + +static void sgi_video_sync_scheduler_init(struct vblank_scheduler *base) { + auto self = (struct sgi_video_sync_vblank_scheduler *)base; + auto args = (struct sgi_video_sync_thread_args){ + .self = self, + .start_status = -1, + }; + pthread_mutex_init(&args.start_mtx, NULL); + pthread_cond_init(&args.start_cnd, NULL); + + base->type = VBLANK_SCHEDULER_SGI_VIDEO_SYNC; + ev_async_init(&self->notify, sgi_video_sync_scheduler_callback); + ev_async_start(base->loop, &self->notify); + pthread_mutex_init(&self->vblank_requested_mtx, NULL); + pthread_cond_init(&self->vblank_requested_cnd, NULL); + + self->running = true; + pthread_create(&self->sync_thread, NULL, sgi_video_sync_thread, &args); + + pthread_mutex_lock(&args.start_mtx); + while (args.start_status == -1) { + pthread_cond_wait(&args.start_cnd, &args.start_mtx); + } + if (args.start_status != 0) { + log_fatal("Failed to start sgi_video_sync_thread, error code: %d", + args.start_status); + abort(); + } + pthread_mutex_destroy(&args.start_mtx); + pthread_cond_destroy(&args.start_cnd); + log_info("Started sgi_video_sync_thread"); +} + +static void sgi_video_sync_scheduler_deinit(struct vblank_scheduler *base) { + auto self = (struct sgi_video_sync_vblank_scheduler *)base; + ev_async_stop(base->loop, &self->notify); + pthread_mutex_lock(&self->vblank_requested_mtx); + self->running = false; + pthread_cond_signal(&self->vblank_requested_cnd); + pthread_mutex_unlock(&self->vblank_requested_mtx); + + pthread_join(self->sync_thread, NULL); + + pthread_mutex_destroy(&self->vblank_requested_mtx); + pthread_cond_destroy(&self->vblank_requested_cnd); +} +#endif + static void present_vblank_scheduler_schedule(struct vblank_scheduler *base) { auto self = (struct present_vblank_scheduler *)base; log_verbose("Requesting vblank event for window 0x%08x, msc %" PRIu64, @@ -170,11 +421,22 @@ static bool handle_present_events(struct vblank_scheduler *base) { static const struct vblank_scheduler_ops vblank_scheduler_ops[LAST_VBLANK_SCHEDULER] = { [VBLANK_SCHEDULER_PRESENT] = { + .size = sizeof(struct present_vblank_scheduler), .init = present_vblank_scheduler_init, .deinit = present_vblank_scheduler_deinit, .schedule = present_vblank_scheduler_schedule, .handle_x_events = handle_present_events, }, +#ifdef CONFIG_OPENGL + [VBLANK_SCHEDULER_SGI_VIDEO_SYNC] = + { + .size = sizeof(struct sgi_video_sync_vblank_scheduler), + .init = sgi_video_sync_scheduler_init, + .deinit = sgi_video_sync_scheduler_deinit, + .schedule = sgi_video_sync_scheduler_schedule, + .handle_x_events = NULL, + }, +#endif }; static void vblank_scheduler_schedule_internal(struct vblank_scheduler *self) { @@ -250,13 +512,22 @@ void vblank_scheduler_free(struct vblank_scheduler *self) { free(self); } -struct vblank_scheduler *vblank_scheduler_new(struct ev_loop *loop, struct x_connection *c, - xcb_window_t target_window) { - struct vblank_scheduler *self = calloc(1, sizeof(struct present_vblank_scheduler)); +struct vblank_scheduler * +vblank_scheduler_new(struct ev_loop *loop, struct x_connection *c, + xcb_window_t target_window, enum vblank_scheduler_type type) { + size_t object_size = vblank_scheduler_ops[type].size; + auto init_fn = vblank_scheduler_ops[type].init; + if (!object_size || !init_fn) { + log_error("Unsupported or invalid vblank scheduler type: %d", type); + return NULL; + } + + assert(object_size >= sizeof(struct vblank_scheduler)); + struct vblank_scheduler *self = calloc(1, object_size); self->target_window = target_window; self->c = c; self->loop = loop; - vblank_scheduler_ops[VBLANK_SCHEDULER_PRESENT].init(self); + init_fn(self); return self; } diff --git a/src/vblank.h b/src/vblank.h index a5073456..8050f487 100644 --- a/src/vblank.h +++ b/src/vblank.h @@ -8,6 +8,7 @@ #include #include +#include "config.h" #include "x.h" /// An object that schedule vblank events. @@ -38,8 +39,9 @@ typedef enum vblank_callback_action (*vblank_callback_t)(struct vblank_event *ev /// is not enough memory. bool vblank_scheduler_schedule(struct vblank_scheduler *self, vblank_callback_t cb, void *user_data); -struct vblank_scheduler *vblank_scheduler_new(struct ev_loop *loop, struct x_connection *c, - xcb_window_t target_window); +struct vblank_scheduler * +vblank_scheduler_new(struct ev_loop *loop, struct x_connection *c, + xcb_window_t target_window, enum vblank_scheduler_type type); void vblank_scheduler_free(struct vblank_scheduler *); bool vblank_handle_x_events(struct vblank_scheduler *self); From b582d2989e4901606421f633277884e39ab3fc7b Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Tue, 19 Dec 2023 10:29:31 +0000 Subject: [PATCH 14/18] core: add debug options to override the vblank scheduler Useful for debugging. Signed-off-by: Yuxuan Shui --- src/config.c | 18 +++++++++++++----- src/config.h | 4 ++++ src/picom.c | 10 +++++++--- 3 files changed, 24 insertions(+), 8 deletions(-) diff --git a/src/config.c b/src/config.c index 8c1ca91d..7dc28146 100644 --- a/src/config.c +++ b/src/config.c @@ -623,11 +623,17 @@ struct debug_options_entry { size_t offset; }; -static const struct debug_options_entry debug_options_entries[] = { - "smart_frame_pacing", - NULL, - offsetof(struct debug_options, smart_frame_pacing), +// clang-format off +const char *vblank_scheduler_str[] = { + [VBLANK_SCHEDULER_PRESENT] = "present", + [VBLANK_SCHEDULER_SGI_VIDEO_SYNC] = "sgi_video_sync", + [LAST_VBLANK_SCHEDULER] = NULL }; +static const struct debug_options_entry debug_options_entries[] = { + {"smart_frame_pacing", NULL, offsetof(struct debug_options, smart_frame_pacing)}, + {"force_vblank_sched", vblank_scheduler_str, offsetof(struct debug_options, force_vblank_scheduler)}, +}; +// clang-format on void parse_debug_option_single(char *setting, struct debug_options *debug_options) { char *equal = strchr(setting, '='); @@ -670,7 +676,9 @@ void parse_debug_option_single(char *setting, struct debug_options *debug_option /// Parse debug options from environment variable `PICOM_DEBUG`. void parse_debug_options(struct debug_options *debug_options) { const char *debug = getenv("PICOM_DEBUG"); - const struct debug_options default_debug_options = {}; + const struct debug_options default_debug_options = { + .force_vblank_scheduler = LAST_VBLANK_SCHEDULER, + }; *debug_options = default_debug_options; if (!debug) { diff --git a/src/config.h b/src/config.h index ac3b6ad2..d91dbb45 100644 --- a/src/config.h +++ b/src/config.h @@ -83,11 +83,15 @@ enum vblank_scheduler_type { LAST_VBLANK_SCHEDULER, }; +extern const char *vblank_scheduler_str[]; + /// Internal, private options for debugging and development use. struct debug_options { /// Try to reduce frame latency by using vblank interval and render time /// estimates. Right now it's not working well across drivers. int smart_frame_pacing; + /// Override the vblank scheduler chosen by the compositor. + int force_vblank_scheduler; }; /// Structure representing all options. diff --git a/src/picom.c b/src/picom.c index e5ac8336..cc1bc426 100644 --- a/src/picom.c +++ b/src/picom.c @@ -1500,9 +1500,13 @@ static bool redirect_start(session_t *ps) { ps->last_msc = 0; ps->last_schedule_delay = 0; render_statistics_reset(&ps->render_stats); - ps->vblank_scheduler = - vblank_scheduler_new(ps->loop, &ps->c, session_get_target_window(ps), - VBLANK_SCHEDULER_PRESENT); + enum vblank_scheduler_type scheduler_type = VBLANK_SCHEDULER_PRESENT; + if (ps->o.debug_options.force_vblank_scheduler != LAST_VBLANK_SCHEDULER) { + scheduler_type = + (enum vblank_scheduler_type)ps->o.debug_options.force_vblank_scheduler; + } + ps->vblank_scheduler = vblank_scheduler_new( + ps->loop, &ps->c, session_get_target_window(ps), scheduler_type); if (!ps->vblank_scheduler) { return false; } From a28e221b838daea112a55118d082d7097459f154 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Tue, 19 Dec 2023 10:29:59 +0000 Subject: [PATCH 15/18] driver: choose sgi_video_sync scheduler for NVIDIA Signed-off-by: Yuxuan Shui --- src/backend/driver.c | 7 +++++++ src/backend/driver.h | 3 +++ src/picom.c | 12 +++++++----- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/src/backend/driver.c b/src/backend/driver.c index b909a622..f17743a4 100644 --- a/src/backend/driver.c +++ b/src/backend/driver.c @@ -19,6 +19,13 @@ void apply_driver_workarounds(struct session *ps, enum driver driver) { } } +enum vblank_scheduler_type choose_vblank_scheduler(enum driver driver) { + if (driver & DRIVER_NVIDIA) { + return VBLANK_SCHEDULER_SGI_VIDEO_SYNC; + } + return VBLANK_SCHEDULER_PRESENT; +} + enum driver detect_driver(xcb_connection_t *c, backend_t *backend_data, xcb_window_t window) { enum driver ret = 0; // First we try doing backend agnostic detection using RANDR diff --git a/src/backend/driver.h b/src/backend/driver.h index e4cc3981..1b0877c1 100644 --- a/src/backend/driver.h +++ b/src/backend/driver.h @@ -7,6 +7,7 @@ #include #include +#include "config.h" #include "utils.h" struct session; @@ -41,6 +42,8 @@ enum driver detect_driver(xcb_connection_t *, struct backend_base *, xcb_window_ /// Apply driver specified global workarounds. It's safe to call this multiple times. void apply_driver_workarounds(struct session *ps, enum driver); +/// Choose a vblank scheduler based on the driver. +enum vblank_scheduler_type choose_vblank_scheduler(enum driver driver); // Print driver names to stdout, for diagnostics static inline void print_drivers(enum driver drivers) { diff --git a/src/picom.c b/src/picom.c index cc1bc426..700320c4 100644 --- a/src/picom.c +++ b/src/picom.c @@ -1493,6 +1493,10 @@ static bool redirect_start(session_t *ps) { ps->frame_pacing = false; } + // Re-detect driver since we now have a backend + ps->drivers = detect_driver(ps->c.c, ps->backend_data, ps->c.screen_info->root); + apply_driver_workarounds(ps, ps->drivers); + if (ps->present_exists && ps->frame_pacing) { // Initialize rendering and frame timing statistics, and frame pacing // states. @@ -1500,11 +1504,13 @@ static bool redirect_start(session_t *ps) { ps->last_msc = 0; ps->last_schedule_delay = 0; render_statistics_reset(&ps->render_stats); - enum vblank_scheduler_type scheduler_type = VBLANK_SCHEDULER_PRESENT; + enum vblank_scheduler_type scheduler_type = + choose_vblank_scheduler(ps->drivers); if (ps->o.debug_options.force_vblank_scheduler != LAST_VBLANK_SCHEDULER) { scheduler_type = (enum vblank_scheduler_type)ps->o.debug_options.force_vblank_scheduler; } + log_info("Using vblank scheduler: %s.", vblank_scheduler_str[scheduler_type]); ps->vblank_scheduler = vblank_scheduler_new( ps->loop, &ps->c, session_get_target_window(ps), scheduler_type); if (!ps->vblank_scheduler) { @@ -1523,10 +1529,6 @@ static bool redirect_start(session_t *ps) { ps->redirected = true; ps->first_frame = true; - // Re-detect driver since we now have a backend - ps->drivers = detect_driver(ps->c.c, ps->backend_data, ps->c.screen_info->root); - apply_driver_workarounds(ps, ps->drivers); - root_damaged(ps); // Repaint the whole screen From 6e0bad0034bc07d868f57003f6c567cc307f4ba2 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Tue, 19 Dec 2023 09:53:57 +0000 Subject: [PATCH 16/18] core: make missing dpms extension non-fatal We are not using it for anything at the moment, and it is breaking CI. Signed-off-by: Yuxuan Shui --- src/picom.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/picom.c b/src/picom.c index 700320c4..4bce4e47 100644 --- a/src/picom.c +++ b/src/picom.c @@ -2139,8 +2139,7 @@ static session_t *session_init(int argc, char **argv, Display *dpy, ext_info = xcb_get_extension_data(ps->c.c, &xcb_dpms_id); ps->dpms_exists = ext_info && ext_info->present; if (!ps->dpms_exists) { - log_fatal("No DPMS extension"); - exit(1); + log_warn("No DPMS extension"); } // Parse configuration file From 359d004b992a7eff6a85afa93713747fade20aff Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Tue, 19 Dec 2023 11:07:55 +0000 Subject: [PATCH 17/18] core: disable frame pacing when vsync is disabled Signed-off-by: Yuxuan Shui --- src/picom.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/picom.c b/src/picom.c index 4bce4e47..5435cecd 100644 --- a/src/picom.c +++ b/src/picom.c @@ -1484,7 +1484,7 @@ static bool redirect_start(session_t *ps) { pixman_region32_init(&ps->damage_ring[i]); } - ps->frame_pacing = !ps->o.no_frame_pacing; + ps->frame_pacing = !ps->o.no_frame_pacing && ps->o.vsync; if ((ps->o.legacy_backends || ps->o.benchmark || !ps->backend_data->ops->last_render_time) && ps->frame_pacing) { // Disable frame pacing if we are using a legacy backend or if we are in From d8f303761bbefa6209792bdd8a286c5eda0b1ef6 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Sun, 14 Jan 2024 15:41:10 +0000 Subject: [PATCH 18/18] core: reset msc counter if it went backwards Otherwise we might be repeatedly hitting this condition and spam the warning. Signed-off-by: Yuxuan Shui --- src/picom.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/picom.c b/src/picom.c index 5435cecd..044a9eb1 100644 --- a/src/picom.c +++ b/src/picom.c @@ -222,6 +222,8 @@ collect_vblank_interval_statistics(struct vblank_event *e, void *ud) { log_warn("PresentCompleteNotify msc is going backwards, last_msc: " "%" PRIu64 ", current msc: %" PRIu64, ps->last_msc, e->msc); + ps->last_msc_instant = 0; + ps->last_msc = 0; } vblank_interval = render_statistics_get_vblank_time(&ps->render_stats);