From ce160cf432fca6e3386de8060c88fb36f2f227cf Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Sun, 18 Jun 2023 17:12:57 +0100 Subject: [PATCH 01/19] 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 707d00e..f9e1765 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 807cf4e..739540d 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 6f3c290..8238ae8 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 fc6dd30..446fba8 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 580889488f37690d0a093596d070a7d321353feb Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Fri, 23 Jun 2023 00:17:35 +0100 Subject: [PATCH 02/19] 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 33a3d8b..dd3366d 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 f572a93..1b4df72 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 f9e1765..ac9da4c 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 8238ae8..75d9149 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 2bc180c2a74dc4c19b89df230e2b9c1df4b69d4b Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Wed, 5 Jul 2023 05:53:21 +0100 Subject: [PATCH 03/19] 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 ac9da4c..0009f11 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 75d9149..71178fe 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 67d0211..42d7bc2 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 1840ebb..54cf702 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 b5bd1a5..39bf5ab 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 3e8af9fb883e8786744c305bea8667f7a3272b3f Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Wed, 5 Jul 2023 05:54:44 +0100 Subject: [PATCH 04/19] 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 0009f11..889ca88 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 71178fe..2cfa08e 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 54cf702..89786e1 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 39bf5ab..60bcfef 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 b3c12b8724d6802a3ee4b7712ea1653980ad1a52 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Mon, 18 Dec 2023 03:04:59 +0000 Subject: [PATCH 05/19] 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 | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++ src/config.h | 7 ++++++ 2 files changed, 72 insertions(+) diff --git a/src/config.c b/src/config.c index ec39aa4..44bdafb 100644 --- a/src/config.c +++ b/src/config.c @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -615,6 +616,69 @@ 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", + setting, arg); + } + } else { + *value = 1; + } + 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 +881,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 31e6774..93bede8 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 40ca6d71467f8e7ad8605b96f8f11e87584f3709 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Sun, 9 Jul 2023 16:39:44 +0100 Subject: [PATCH 06/19] 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 | 30 ++-- src/config.h | 10 ++ src/meson.build | 3 +- src/picom.c | 454 ++++++++++++++++++++---------------------------- src/vblank.c | 227 ++++++++++++++++++++++++ src/vblank.h | 37 ++++ src/x.c | 24 ++- src/x.h | 8 +- 8 files changed, 495 insertions(+), 298 deletions(-) create mode 100644 src/vblank.c create mode 100644 src/vblank.h diff --git a/src/common.h b/src/common.h index 889ca88..e776fc2 100644 --- a/src/common.h +++ b/src/common.h @@ -156,8 +156,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 +223,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 +234,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 +247,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 93bede8..58bfda8 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 + PRESENT_VBLANK_SCHEDULER, + /// GLX_SGI_video_sync based vblank events + SGI_VIDEO_VSYNC_VBLANK_SCHEDULER, + /// 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 a608c57..51f9648 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 2cfa08e..57ee4f0 100644 --- a/src/picom.c +++ b/src/picom.c @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -64,6 +65,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 +144,150 @@ static inline struct managed_win *find_win_all(session_t *ps, const xcb_window_t return w; } +void collect_vblank_interval_statistics(struct vblank_event *e, void *ud) { + auto ps = (session_t *)ud; + assert(ps->frame_pacing); + assert(ps->vblank_scheduler); + + if (ps->last_msc == e->msc) { + // Already collected statistics for this vblank + return; + } + + // 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. + + if (ps->last_msc_instant != 0) { + auto frame_count = e->msc - ps->last_msc; + int 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; + double 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. + vblank_scheduler_schedule(ps->vblank_scheduler, + collect_vblank_interval_statistics, ud); + } +} +/// vblank callback scheduled by schedule_render. +/// +/// Check if previously queued render has finished, and record the time it took. +void 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); + vblank_scheduler_schedule(ps->vblank_scheduler, schedule_render_at_vblank, ud); + return; + } + + // 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); +} + /// 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 +337,37 @@ 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: " + 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 +1444,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 +1504,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 +1516,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 +1643,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 +1736,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 +1751,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 +1790,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 +1941,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 +2377,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 0000000..857f631 --- /dev/null +++ b/src/vblank.c @@ -0,0 +1,227 @@ +#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_callback { + vblank_callback_t fn; + void *user_data; +}; + +struct vblank_scheduler { + enum vblank_scheduler_type type; + xcb_window_t target_window; + struct x_connection *c; + size_t callback_capacity, callback_count; + struct vblank_callback *callbacks; + struct ev_loop *loop; +}; + +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; + bool vblank_event_requested; + xcb_present_event_t event_id; + xcb_special_event_t *event; +}; + +static void present_vblank_scheduler_schedule(struct present_vblank_scheduler *sched) { + assert(!sched->vblank_event_requested); + x_request_vblank_event(sched->base.c, sched->base.target_window, sched->last_msc + 1); + sched->vblank_event_requested = true; +} + +static void vblank_scheduler_schedule_internal(struct vblank_scheduler *self) { + switch (self->type) { + case PRESENT_VBLANK_SCHEDULER: + return present_vblank_scheduler_schedule((struct present_vblank_scheduler *)self); + case SGI_VIDEO_VSYNC_VBLANK_SCHEDULER: + case LAST_VBLANK_SCHEDULER: + default: assert(false); + } +} + +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; + for (size_t i = 0; i < count; i++) { + self->callbacks[i].fn(event, self->callbacks[i].user_data); + } + // remove the callbacks that we have called, keep the newly added ones. + memmove(self->callbacks, self->callbacks + count, + (self->callback_count - count) * sizeof(*self->callbacks)); + self->callback_count -= count; + if (self->callback_count) { + vblank_scheduler_schedule_internal(self); + } +} + +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->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 = PRESENT_VBLANK_SCHEDULER; + 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); +} + +void vblank_scheduler_free(struct vblank_scheduler *self) { + switch (self->type) { + case PRESENT_VBLANK_SCHEDULER: present_vblank_scheduler_deinit(self); break; + case SGI_VIDEO_VSYNC_VBLANK_SCHEDULER: break; + case LAST_VBLANK_SCHEDULER: + default: assert(false); + } + 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; + present_vblank_scheduler_init(self); + return 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_callback){ + .fn = vblank_callback, + .user_data = user_data, + }; + return true; +} + +/// 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 == PRESENT_VBLANK_SCHEDULER); + + if (cne->kind != XCB_PRESENT_COMPLETE_KIND_NOTIFY_MSC) { + return; + } + + assert(self->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 present_vblank_scheduler *self) { + xcb_present_generic_event_t *ev; + while ((ev = (void *)xcb_poll_for_special_event(self->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; +} +bool vblank_handle_x_events(struct vblank_scheduler *self) { + switch (self->type) { + case PRESENT_VBLANK_SCHEDULER: + return handle_present_events((struct present_vblank_scheduler *)self); + case SGI_VIDEO_VSYNC_VBLANK_SCHEDULER: return true; + case LAST_VBLANK_SCHEDULER: + default: assert(false); + } + return true; +} \ No newline at end of file diff --git a/src/vblank.h b/src/vblank.h new file mode 100644 index 0000000..3f5c7d1 --- /dev/null +++ b/src/vblank.h @@ -0,0 +1,37 @@ +#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; +}; + +typedef void (*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 89786e1..45abe31 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 60bcfef..df45b5c 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 f7b578dd54d5c424352ea5648a32e221d7d2838f Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Mon, 18 Dec 2023 04:34:25 +0000 Subject: [PATCH 07/19] 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 | 40 ++++++++++++++++++++++++++-------------- 3 files changed, 33 insertions(+), 16 deletions(-) diff --git a/src/config.c b/src/config.c index 44bdafb..316438e 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 58bfda8..652f891 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 57ee4f0..f24f18f 100644 --- a/src/picom.c +++ b/src/picom.c @@ -154,6 +154,12 @@ void collect_vblank_interval_statistics(struct vblank_event *e, void *ud) { return; } + if (!ps->o.debug_options.smart_frame_pacing) { + // We don't need to collect statistics if we are not doing smart frame + // pacing. + return; + } + // 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 @@ -212,13 +218,15 @@ void schedule_render_at_vblank(struct vblank_event *e, void *ud) { } // 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; @@ -230,8 +238,8 @@ void schedule_render_at_vblank(struct vblank_event *e, void *ud) { 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); + ", 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); @@ -310,6 +318,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) { @@ -337,11 +348,12 @@ void schedule_render(session_t *ps, bool triggered_by_vblank attr_unused) { delay_s, render_budget, frame_time, now_us, deadline); } - 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); + 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_queued = true; From 8384890d3a5afe5b97c8836b7a968dabb63c2150 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Mon, 18 Dec 2023 09:30:40 +0000 Subject: [PATCH 08/19] 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 | 146 ++++++++++++++++++++++++------------------ src/vblank.c | 2 + 3 files changed, 89 insertions(+), 67 deletions(-) diff --git a/src/backend/backend.c b/src/backend/backend.c index dd3366d..5120672 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 f24f18f..8e880ae 100644 --- a/src/picom.c +++ b/src/picom.c @@ -192,57 +192,52 @@ void collect_vblank_interval_statistics(struct vblank_event *e, void *ud) { collect_vblank_interval_statistics, ud); } } + +void schedule_render(session_t *ps, bool triggered_by_vblank); + /// vblank callback scheduled by schedule_render. /// /// Check if previously queued render has finished, and record the time it took. -void schedule_render_at_vblank(struct vblank_event *e, void *ud) { +void 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); - vblank_scheduler_schedule(ps->vblank_scheduler, schedule_render_at_vblank, ud); - return; + 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); + vblank_scheduler_schedule(ps->vblank_scheduler, + reschedule_render_at_vblank, ud); + return; + } + + // 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); } /// How many seconds into the future should we start rendering the next frame. @@ -252,7 +247,7 @@ void schedule_render_at_vblank(struct vblank_event *e, void *ud) { /// 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, @@ -262,14 +257,20 @@ void schedule_render_at_vblank(struct vblank_event *e, void *ud) { /// 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 @@ -297,6 +298,21 @@ void schedule_render_at_vblank(struct vblank_event *e, void *ud) { /// 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; @@ -349,37 +365,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); + } } /** diff --git a/src/vblank.c b/src/vblank.c index 857f631..449b629 100644 --- a/src/vblank.c +++ b/src/vblank.c @@ -40,6 +40,8 @@ struct present_vblank_scheduler { }; static void present_vblank_scheduler_schedule(struct present_vblank_scheduler *sched) { + log_verbose("Requesting vblank event for window 0x%08x, msc %" PRIu64, + sched->base.target_window, sched->last_msc + 1); assert(!sched->vblank_event_requested); x_request_vblank_event(sched->base.c, sched->base.target_window, sched->last_msc + 1); sched->vblank_event_requested = true; From c6b48d7cbcb820222c84f26d0d9b5f1d95298b90 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Mon, 18 Dec 2023 09:33:17 +0000 Subject: [PATCH 09/19] 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 45abe31..06c8b93 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 91667d7747fde7c62ea8391d693bf8444209f6df Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Mon, 18 Dec 2023 10:10:37 +0000 Subject: [PATCH 10/19] 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 449b629..4fbf526 100644 --- a/src/vblank.c +++ b/src/vblank.c @@ -18,11 +18,19 @@ struct vblank_callback { void *user_data; }; +#define VBLANK_WIND_DOWN 4 + struct vblank_scheduler { enum vblank_scheduler_type type; xcb_window_t target_window; struct x_connection *c; size_t callback_capacity, callback_count; + /// 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; struct vblank_callback *callbacks; struct ev_loop *loop; }; @@ -62,6 +70,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; + if (count == 0) { + self->wind_down--; + } else { + self->wind_down = VBLANK_WIND_DOWN; + } for (size_t i = 0; i < count; i++) { self->callbacks[i].fn(event, self->callbacks[i].user_data); } @@ -69,7 +82,7 @@ vblank_scheduler_invoke_callbacks(struct vblank_scheduler *self, struct vblank_e memmove(self->callbacks, self->callbacks + count, (self->callback_count - count) * sizeof(*self->callbacks)); self->callback_count -= count; - if (self->callback_count) { + if (self->callback_count || self->wind_down) { vblank_scheduler_schedule_internal(self); } } @@ -130,7 +143,7 @@ struct vblank_scheduler *vblank_scheduler_new(struct ev_loop *loop, struct x_con 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) { From 79bf7cd5c31be8d12ca706b2d1a1857ab74d415b Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Mon, 18 Dec 2023 11:00:35 +0000 Subject: [PATCH 11/19] vblank: use table of function pointers instead of switch Makes conditional compilation easier in the future. Signed-off-by: Yuxuan Shui --- src/vblank.c | 207 ++++++++++++++++++++++++++++----------------------- 1 file changed, 112 insertions(+), 95 deletions(-) diff --git a/src/vblank.c b/src/vblank.c index 4fbf526..31f7cc1 100644 --- a/src/vblank.c +++ b/src/vblank.c @@ -21,18 +21,19 @@ struct vblank_callback { #define VBLANK_WIND_DOWN 4 struct vblank_scheduler { - enum vblank_scheduler_type type; - xcb_window_t target_window; struct x_connection *c; size_t callback_capacity, callback_count; + struct vblank_callback *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; - struct vblank_callback *callbacks; - struct ev_loop *loop; + xcb_window_t target_window; + enum vblank_scheduler_type type; + bool vblank_event_requested; }; struct present_vblank_scheduler { @@ -42,49 +43,27 @@ struct present_vblank_scheduler { /// The timestamp for the end of last vblank. uint64_t last_ust; ev_timer callback_timer; - bool vblank_event_requested; xcb_present_event_t event_id; xcb_special_event_t *event; }; -static void present_vblank_scheduler_schedule(struct present_vblank_scheduler *sched) { - log_verbose("Requesting vblank event for window 0x%08x, msc %" PRIu64, - sched->base.target_window, sched->last_msc + 1); - assert(!sched->vblank_event_requested); - x_request_vblank_event(sched->base.c, sched->base.target_window, sched->last_msc + 1); - sched->vblank_event_requested = true; -} - -static void vblank_scheduler_schedule_internal(struct vblank_scheduler *self) { - switch (self->type) { - case PRESENT_VBLANK_SCHEDULER: - return present_vblank_scheduler_schedule((struct present_vblank_scheduler *)self); - case SGI_VIDEO_VSYNC_VBLANK_SCHEDULER: - case LAST_VBLANK_SCHEDULER: - default: assert(false); - } -} +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) { - // callbacks might be added during callback invocation, so we need to - // copy the callback_count. - size_t count = self->callback_count; - if (count == 0) { - self->wind_down--; - } else { - self->wind_down = VBLANK_WIND_DOWN; - } - for (size_t i = 0; i < count; i++) { - self->callbacks[i].fn(event, self->callbacks[i].user_data); - } - // remove the callbacks that we have called, keep the newly added ones. - memmove(self->callbacks, self->callbacks + count, - (self->callback_count - count) * sizeof(*self->callbacks)); - self->callback_count -= count; - if (self->callback_count || self->wind_down) { - vblank_scheduler_schedule_internal(self); - } +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) { @@ -93,7 +72,7 @@ static void present_vblank_callback(EV_P attr_unused, ev_timer *w, int attr_unus .msc = sched->last_msc, .ust = sched->last_ust, }; - sched->vblank_event_requested = false; + sched->base.vblank_event_requested = false; vblank_scheduler_invoke_callbacks(&sched->base, &event); } @@ -120,50 +99,6 @@ static void present_vblank_scheduler_deinit(struct vblank_scheduler *base) { xcb_unregister_for_special_event(base->c->c, self->event); } -void vblank_scheduler_free(struct vblank_scheduler *self) { - switch (self->type) { - case PRESENT_VBLANK_SCHEDULER: present_vblank_scheduler_deinit(self); break; - case SGI_VIDEO_VSYNC_VBLANK_SCHEDULER: break; - case LAST_VBLANK_SCHEDULER: - default: assert(false); - } - 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; - present_vblank_scheduler_init(self); - return self; -} - -bool vblank_scheduler_schedule(struct vblank_scheduler *self, - vblank_callback_t vblank_callback, void *user_data) { - if (self->callback_count == 0 && self->wind_down == 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_callback){ - .fn = vblank_callback, - .user_data = user_data, - }; - return true; -} - /// Handle PresentCompleteNotify events /// /// Schedule the registered callback to be called when the current vblank ends. @@ -175,7 +110,7 @@ static void handle_present_complete_notify(struct present_vblank_scheduler *self return; } - assert(self->vblank_event_requested); + 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 @@ -213,9 +148,10 @@ static void handle_present_complete_notify(struct present_vblank_scheduler *self ev_timer_start(self->base.loop, &self->callback_timer); } -static bool handle_present_events(struct present_vblank_scheduler *self) { +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(self->base.c->c, self->event))) { + 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. @@ -230,13 +166,94 @@ static bool handle_present_events(struct present_vblank_scheduler *self) { } return true; } + +static const struct vblank_scheduler_ops vblank_scheduler_ops[LAST_VBLANK_SCHEDULER] = { + [PRESENT_VBLANK_SCHEDULER] = + { + .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 && self->wind_down == 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_callback){ + .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; + if (count == 0) { + self->wind_down--; + } else { + self->wind_down = VBLANK_WIND_DOWN; + } + for (size_t i = 0; i < count; i++) { + self->callbacks[i].fn(event, self->callbacks[i].user_data); + } + // remove the callbacks that we have called, keep the newly added ones. + memmove(self->callbacks, self->callbacks + count, + (self->callback_count - count) * sizeof(*self->callbacks)); + self->callback_count -= count; + if (self->callback_count || self->wind_down) { + 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[PRESENT_VBLANK_SCHEDULER].init(self); + return self; +} + bool vblank_handle_x_events(struct vblank_scheduler *self) { - switch (self->type) { - case PRESENT_VBLANK_SCHEDULER: - return handle_present_events((struct present_vblank_scheduler *)self); - case SGI_VIDEO_VSYNC_VBLANK_SCHEDULER: return true; - case LAST_VBLANK_SCHEDULER: - default: assert(false); + 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 From 6986ba919a664cea5dc80b286c18ae2a59864705 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Mon, 18 Dec 2023 20:12:14 +0000 Subject: [PATCH 12/19] 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 | 68 +++++++++++++++++++++++++++++++++-------------------- 1 file changed, 42 insertions(+), 26 deletions(-) diff --git a/src/picom.c b/src/picom.c index 8e880ae..0de422c 100644 --- a/src/picom.c +++ b/src/picom.c @@ -144,6 +144,38 @@ static inline struct managed_win *find_win_all(session_t *ps, const xcb_window_t return w; } +void check_render_finish(struct vblank_event *e attr_unused, void *ud) { + auto ps = (session_t *)ud; + if (!ps->backend_busy) { + return; + } + + 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); + vblank_scheduler_schedule(ps->vblank_scheduler, check_render_finish, ud); + return; + } + + // 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; +} + void collect_vblank_interval_statistics(struct vblank_event *e, void *ud) { auto ps = (session_t *)ud; assert(ps->frame_pacing); @@ -207,34 +239,12 @@ void reschedule_render_at_vblank(struct vblank_event *e, void *ud) { 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); - vblank_scheduler_schedule(ps->vblank_scheduler, - reschedule_render_at_vblank, ud); - return; - } - - // 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; + vblank_scheduler_schedule(ps->vblank_scheduler, + reschedule_render_at_vblank, ud); + return; } schedule_render(ps, false); @@ -1807,6 +1817,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 72693b75504a0b3c46023ad51491e13b2a72507e Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Mon, 18 Dec 2023 20:32:22 +0000 Subject: [PATCH 13/19] 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 | 37 +++++++++++++++++-------------------- src/statistics.c | 13 +------------ src/statistics.h | 7 +------ 3 files changed, 19 insertions(+), 38 deletions(-) diff --git a/src/picom.c b/src/picom.c index 0de422c..2b24dec 100644 --- a/src/picom.c +++ b/src/picom.c @@ -334,20 +334,18 @@ 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 @@ -356,7 +354,16 @@ void schedule_render(session_t *ps, bool triggered_by_vblank attr_unused) { 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. + 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); @@ -399,17 +406,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 5e3d924..11c7466 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 42d7bc2..a111486 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 e7c00108d1a2c6b450a57d1393a7908e3d4ae8ba Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Mon, 18 Dec 2023 21:08:44 +0000 Subject: [PATCH 14/19] 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/config.h | 2 +- src/meson.build | 2 +- src/picom.c | 3 +- src/vblank.c | 280 +++++++++++++++++++++++++++++++++++++++++++++++- src/vblank.h | 4 +- 5 files changed, 282 insertions(+), 9 deletions(-) diff --git a/src/config.h b/src/config.h index 652f891..b414b9a 100644 --- a/src/config.h +++ b/src/config.h @@ -77,7 +77,7 @@ enum vblank_scheduler_type { /// X Present extension based vblank events PRESENT_VBLANK_SCHEDULER, /// GLX_SGI_video_sync based vblank events - SGI_VIDEO_VSYNC_VBLANK_SCHEDULER, + SGI_VIDEO_SYNC_VBLANK_SCHEDULER, /// An invalid scheduler, served as a scheduler count, and /// as a sentinel value. LAST_VBLANK_SCHEDULER, diff --git a/src/meson.build b/src/meson.build index 51f9648..a2f9c36 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 2b24dec..9ca6c96 100644 --- a/src/picom.c +++ b/src/picom.c @@ -1490,7 +1490,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), + PRESENT_VBLANK_SCHEDULER); if (!ps->vblank_scheduler) { return false; } diff --git a/src/vblank.c b/src/vblank.c index 31f7cc1..1717659 100644 --- a/src/vblank.c +++ b/src/vblank.c @@ -2,12 +2,25 @@ #include #include +#include #include +#include #include #include +#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 +61,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 +71,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 = SGI_VIDEO_SYNC_VBLANK_SCHEDULER; + 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 +420,22 @@ static bool handle_present_events(struct vblank_scheduler *base) { static const struct vblank_scheduler_ops vblank_scheduler_ops[LAST_VBLANK_SCHEDULER] = { [PRESENT_VBLANK_SCHEDULER] = { + .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 + [SGI_VIDEO_SYNC_VBLANK_SCHEDULER] = + { + .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) { @@ -239,13 +500,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[PRESENT_VBLANK_SCHEDULER].init(self); + init_fn(self); return self; } diff --git a/src/vblank.h b/src/vblank.h index 3f5c7d1..392b5ae 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. @@ -31,7 +32,8 @@ typedef void (*vblank_callback_t)(struct vblank_event *event, void *user_data); 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); +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 ffaa42a019f8aac6228c1c44d45c6912e00e5244 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Mon, 18 Dec 2023 21:14:29 +0000 Subject: [PATCH 15/19] core: add debug options to override the vblank scheduler Useful for debugging. Signed-off-by: Yuxuan Shui --- src/config.c | 12 ++++++++---- src/config.h | 2 ++ src/picom.c | 10 +++++++--- 3 files changed, 17 insertions(+), 7 deletions(-) diff --git a/src/config.c b/src/config.c index 316438e..2299039 100644 --- a/src/config.c +++ b/src/config.c @@ -623,11 +623,13 @@ struct debug_options_entry { size_t offset; }; +// clang-format off +static const char *vblank_scheduler_choices[] = { "present", "sgi_video_sync", NULL }; static const struct debug_options_entry debug_options_entries[] = { - "smart_frame_pacing", - NULL, - offsetof(struct debug_options, smart_frame_pacing), + {"smart_frame_pacing", NULL, offsetof(struct debug_options, smart_frame_pacing)}, + {"force_vblank_sched", vblank_scheduler_choices, 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, '='); @@ -667,7 +669,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 b414b9a..760088c 100644 --- a/src/config.h +++ b/src/config.h @@ -88,6 +88,8 @@ 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 9ca6c96..819a924 100644 --- a/src/picom.c +++ b/src/picom.c @@ -1489,9 +1489,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), - PRESENT_VBLANK_SCHEDULER); + enum vblank_scheduler_type scheduler_type = PRESENT_VBLANK_SCHEDULER; + 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 d7a2f8ade697272f5a7a8845b25855fd43f558d4 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Mon, 18 Dec 2023 21:30:07 +0000 Subject: [PATCH 16/19] core: add a few debug logs Signed-off-by: Yuxuan Shui --- src/picom.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/picom.c b/src/picom.c index 819a924..5a4735a 100644 --- a/src/picom.c +++ b/src/picom.c @@ -351,6 +351,7 @@ void schedule_render(session_t *ps, bool triggered_by_vblank attr_unused) { // 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; } @@ -359,6 +360,8 @@ void schedule_render(session_t *ps, bool triggered_by_vblank attr_unused) { // 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; } From 0b45b3415b0c2d504d88d2d40beb7e78435073d9 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Mon, 18 Dec 2023 22:00:51 +0000 Subject: [PATCH 17/19] driver: choose sgi_video_sync scheduler for NVIDIA Signed-off-by: Yuxuan Shui --- src/backend/driver.c | 7 +++++++ src/backend/driver.h | 3 +++ src/config.h | 8 ++++++++ src/picom.c | 13 ++++++++----- 4 files changed, 26 insertions(+), 5 deletions(-) diff --git a/src/backend/driver.c b/src/backend/driver.c index b909a62..e2690c4 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_INTEL) { + return SGI_VIDEO_SYNC_VBLANK_SCHEDULER; + } + return PRESENT_VBLANK_SCHEDULER; +} + 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 e4cc398..1b0877c 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/config.h b/src/config.h index 760088c..996df10 100644 --- a/src/config.h +++ b/src/config.h @@ -83,6 +83,14 @@ enum vblank_scheduler_type { LAST_VBLANK_SCHEDULER, }; +static inline const char *vblank_scheduler_type_str(enum vblank_scheduler_type type) { + switch (type) { + case PRESENT_VBLANK_SCHEDULER: return "present"; + case SGI_VIDEO_SYNC_VBLANK_SCHEDULER: return "sgi_video_sync"; + default: return "invalid"; + } +} + /// Internal, private options for debugging and development use. struct debug_options { /// Try to reduce frame latency by using vblank interval and render time diff --git a/src/picom.c b/src/picom.c index 5a4735a..12aff18 100644 --- a/src/picom.c +++ b/src/picom.c @@ -1485,6 +1485,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. @@ -1492,11 +1496,14 @@ 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 = PRESENT_VBLANK_SCHEDULER; + 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_type_str(scheduler_type)); ps->vblank_scheduler = vblank_scheduler_new( ps->loop, &ps->c, session_get_target_window(ps), scheduler_type); if (!ps->vblank_scheduler) { @@ -1515,10 +1522,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 fc62f6a4ed356fc5c7eaee776d6571ca9f93487d Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Mon, 18 Dec 2023 22:02:05 +0000 Subject: [PATCH 18/19] config: make naming consistent *_VBLANK_SCHEDULER -> VBLANK_SCHEDULER_* Signed-off-by: Yuxuan Shui --- src/backend/driver.c | 4 ++-- src/config.h | 8 ++++---- src/vblank.c | 10 +++++----- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/backend/driver.c b/src/backend/driver.c index e2690c4..3be7215 100644 --- a/src/backend/driver.c +++ b/src/backend/driver.c @@ -21,9 +21,9 @@ void apply_driver_workarounds(struct session *ps, enum driver driver) { enum vblank_scheduler_type choose_vblank_scheduler(enum driver driver) { if (driver & DRIVER_INTEL) { - return SGI_VIDEO_SYNC_VBLANK_SCHEDULER; + return VBLANK_SCHEDULER_SGI_VIDEO_SYNC; } - return PRESENT_VBLANK_SCHEDULER; + return VBLANK_SCHEDULER_PRESENT; } enum driver detect_driver(xcb_connection_t *c, backend_t *backend_data, xcb_window_t window) { diff --git a/src/config.h b/src/config.h index 996df10..ce25e55 100644 --- a/src/config.h +++ b/src/config.h @@ -75,9 +75,9 @@ typedef struct _c2_lptr c2_lptr_t; enum vblank_scheduler_type { /// X Present extension based vblank events - PRESENT_VBLANK_SCHEDULER, + VBLANK_SCHEDULER_PRESENT, /// GLX_SGI_video_sync based vblank events - SGI_VIDEO_SYNC_VBLANK_SCHEDULER, + VBLANK_SCHEDULER_SGI_VIDEO_SYNC, /// An invalid scheduler, served as a scheduler count, and /// as a sentinel value. LAST_VBLANK_SCHEDULER, @@ -85,8 +85,8 @@ enum vblank_scheduler_type { static inline const char *vblank_scheduler_type_str(enum vblank_scheduler_type type) { switch (type) { - case PRESENT_VBLANK_SCHEDULER: return "present"; - case SGI_VIDEO_SYNC_VBLANK_SCHEDULER: return "sgi_video_sync"; + case VBLANK_SCHEDULER_PRESENT: return "present"; + case VBLANK_SCHEDULER_SGI_VIDEO_SYNC: return "sgi_video_sync"; default: return "invalid"; } } diff --git a/src/vblank.c b/src/vblank.c index 1717659..cbfc18c 100644 --- a/src/vblank.c +++ b/src/vblank.c @@ -269,7 +269,7 @@ static void sgi_video_sync_scheduler_init(struct vblank_scheduler *base) { pthread_mutex_init(&args.start_mtx, NULL); pthread_cond_init(&args.start_cnd, NULL); - base->type = SGI_VIDEO_SYNC_VBLANK_SCHEDULER; + 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); @@ -328,7 +328,7 @@ static void present_vblank_callback(EV_P attr_unused, ev_timer *w, int attr_unus static void present_vblank_scheduler_init(struct vblank_scheduler *base) { auto self = (struct present_vblank_scheduler *)base; - base->type = PRESENT_VBLANK_SCHEDULER; + base->type = VBLANK_SCHEDULER_PRESENT; ev_timer_init(&self->callback_timer, present_vblank_callback, 0, 0); self->event_id = x_new_id(base->c); @@ -354,7 +354,7 @@ static void present_vblank_scheduler_deinit(struct vblank_scheduler *base) { /// 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 == PRESENT_VBLANK_SCHEDULER); + assert(self->base.type == VBLANK_SCHEDULER_PRESENT); if (cne->kind != XCB_PRESENT_COMPLETE_KIND_NOTIFY_MSC) { return; @@ -418,7 +418,7 @@ static bool handle_present_events(struct vblank_scheduler *base) { } static const struct vblank_scheduler_ops vblank_scheduler_ops[LAST_VBLANK_SCHEDULER] = { - [PRESENT_VBLANK_SCHEDULER] = + [VBLANK_SCHEDULER_PRESENT] = { .size = sizeof(struct present_vblank_scheduler), .init = present_vblank_scheduler_init, @@ -427,7 +427,7 @@ static const struct vblank_scheduler_ops vblank_scheduler_ops[LAST_VBLANK_SCHEDU .handle_x_events = handle_present_events, }, #ifdef CONFIG_OPENGL - [SGI_VIDEO_SYNC_VBLANK_SCHEDULER] = + [VBLANK_SCHEDULER_SGI_VIDEO_SYNC] = { .size = sizeof(struct sgi_video_sync_vblank_scheduler), .init = sgi_video_sync_scheduler_init, From ff691e62955313c66a34742806651b70f0bb6a8d Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Tue, 19 Dec 2023 09:53:57 +0000 Subject: [PATCH 19/19] 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 12aff18..dd3fdbd 100644 --- a/src/picom.c +++ b/src/picom.c @@ -2132,8 +2132,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