On Wed, 18 Mar 2015 14:45:34 -0700 Bryce Harrington <[email protected]> wrote:
> On Wed, Mar 18, 2015 at 03:27:22PM +0200, Pekka Paalanen wrote: > > From: Pekka Paalanen <[email protected]> > > > > This timer delays the output_repaint towards the end of the refresh > > period, reducing the time from repaint to present. > > > > The length of the repaint window can be set in weston.ini. > > > > The call to weston_output_schedule_repaint_reset() is delayed by one > > more period. If we exit the continuous repaint loop (set > > output->repaint_scheduled to false) in finish_frame, we may call > > start_repaint_loop() unnecessarily. The problem case was actually > > observed with two outputs on the DRM backend at 60 Hz, and 7 ms > > repaint-window. During a window move, one output was constantly falling > > off the continuous repaint loop and introducing additional one frame > > latency, leading to jerky window motion. This code now avoids the > > problem. > > > > Changes in v2: > > > > - Rename repaint_delay_timer to repaint_timer and > > output_repaint_delay_handler to output_repaint_timer_handler. > > > > - When computing the delay, take the current time into account. The timer > > uses a relative timeout, so we have to subtract any time already gone. > > > > Note, that 'gone' may also be negative. DRM has a habit of predicting > > the page flip timestamp so it may be still in the future when we get the > > completion event. > > > > - Do also a sanity check 'msec > 1000'. In the unlikely case that > > something fails to provide a good timestamp, never delay for more than > > one second. > > > > Signed-off-by: Pekka Paalanen <[email protected]> > > Integer 32 math with numbers up in the billions kind of worries me about > out-of-range errors, but the math all appears to work out. Might be > nice to have tests for timespec_sub and timespec_to_nsec anyway just to > be sure, but the code itself looks ok to land, so: > > Reviewed-by: Bryce Harrington <[email protected]> Yeah, I lifted timespec_sub() from wesgr. There I have much more rigorous testing of timestamp validity. tv_nsec is of type 'long' which may be 32-bit (tv_sec *might* be 32-bit too, we have no guarantees). For valid timestamps, tv_nsec is always in the range [0, 999999999] so the math is safe for valid timestamps, and produces valid timestamps. I've been thinking of putting all timespec helpers in a new shared file with tests and exact documentation, but there isn't enough of them yet. Thanks, pq > > --- > > man/weston.ini.man | 10 +++++++ > > src/compositor.c | 83 > > ++++++++++++++++++++++++++++++++++++++++++++++-------- > > src/compositor.h | 2 ++ > > 3 files changed, 84 insertions(+), 11 deletions(-) > > > > diff --git a/man/weston.ini.man b/man/weston.ini.man > > index 7a353c9..d7df0e4 100644 > > --- a/man/weston.ini.man > > +++ b/man/weston.ini.man > > @@ -137,6 +137,16 @@ directory are: > > .fi > > .RE > > .TP 7 > > +.BI "repaint-window=" N > > +Set the approximate length of the repaint window in milliseconds. The > > repaint > > +window is used to control and reduce the output latency for clients. If the > > +window is longer than the output refresh period, the repaint will be done > > +immediately when the previous repaint finishes, not processing client > > requests > > +in between. If the repaint window is too short, the compositor may miss the > > +target vertical blank, increasing output latency. The default value is 7 > > +milliseconds. The allowed range is from -10 to 1000 milliseconds. Using a > > +negative value will force the compositor to always miss the target vblank. > > +.TP 7 > > .BI "gbm-format="format > > sets the GBM format used for the framebuffer for the GBM backend. Can be > > .B xrgb8888, > > diff --git a/src/compositor.c b/src/compositor.c > > index e060be1..3f6aa7d 100644 > > --- a/src/compositor.c > > +++ b/src/compositor.c > > @@ -62,6 +62,28 @@ > > #include "git-version.h" > > #include "version.h" > > > > +#define DEFAULT_REPAINT_WINDOW 7 /* milliseconds */ > > + > > +#define NSEC_PER_SEC 1000000000 > > + > > +static void > > +timespec_sub(struct timespec *r, > > + const struct timespec *a, const struct timespec *b) > > +{ > > + r->tv_sec = a->tv_sec - b->tv_sec; > > + r->tv_nsec = a->tv_nsec - b->tv_nsec; > > + if (r->tv_nsec < 0) { > > + r->tv_sec--; > > + r->tv_nsec += NSEC_PER_SEC; > > + } > > +} > > + > > +static int64_t > > +timespec_to_nsec(const struct timespec *a) > > +{ > > + return (int64_t)a->tv_sec * NSEC_PER_SEC + a->tv_nsec; > > +} > > + > > static struct wl_list child_process_list; > > static struct weston_compositor *segv_compositor; > > > > @@ -2346,19 +2368,38 @@ weston_output_schedule_repaint_reset(struct > > weston_output *output) > > weston_compositor_read_input, compositor); > > } > > > > +static int > > +output_repaint_timer_handler(void *data) > > +{ > > + struct weston_output *output = data; > > + struct weston_compositor *compositor = output->compositor; > > + > > + if (output->repaint_needed && > > + compositor->state != WESTON_COMPOSITOR_SLEEPING && > > + compositor->state != WESTON_COMPOSITOR_OFFSCREEN && > > + weston_output_repaint(output) == 0) > > + return 0; > > + > > + weston_output_schedule_repaint_reset(output); > > + > > + return 0; > > +} > > + > > WL_EXPORT void > > weston_output_finish_frame(struct weston_output *output, > > const struct timespec *stamp, > > uint32_t presented_flags) > > { > > struct weston_compositor *compositor = output->compositor; > > - int r; > > - uint32_t refresh_nsec; > > + int32_t refresh_nsec; > > + struct timespec now; > > + struct timespec gone; > > + int msec; > > > > TL_POINT("core_repaint_finished", TLP_OUTPUT(output), > > TLP_VBLANK(stamp), TLP_END); > > > > - refresh_nsec = 1000000000000UL / output->current_mode->refresh; > > + refresh_nsec = 1000000000000LL / output->current_mode->refresh; > > weston_presentation_feedback_present_list(&output->feedback_list, > > output, refresh_nsec, stamp, > > output->msc, > > @@ -2366,15 +2407,16 @@ weston_output_finish_frame(struct weston_output > > *output, > > > > output->frame_time = stamp->tv_sec * 1000 + stamp->tv_nsec / 1000000; > > > > - if (output->repaint_needed && > > - compositor->state != WESTON_COMPOSITOR_SLEEPING && > > - compositor->state != WESTON_COMPOSITOR_OFFSCREEN) { > > - r = weston_output_repaint(output); > > - if (!r) > > - return; > > - } > > + weston_compositor_read_presentation_clock(compositor, &now); > > + timespec_sub(&gone, &now, stamp); > > + msec = (refresh_nsec - timespec_to_nsec(&gone)) / 1000000; /* floor */ > > + msec -= compositor->repaint_msec; > > > > - weston_output_schedule_repaint_reset(output); > > + /* Also sanity check. */ > > + if (msec < 1 || msec > 1000) > > + output_repaint_timer_handler(output); > > + else > > + wl_event_source_timer_update(output->repaint_timer, msec); > > } > > > > static void > > @@ -3873,6 +3915,8 @@ weston_output_destroy(struct weston_output *output) > > > > output->destroying = 1; > > > > + wl_event_source_remove(output->repaint_timer); > > + > > weston_presentation_feedback_discard_list(&output->feedback_list); > > > > weston_compositor_remove_output(output->compositor, output); > > @@ -4033,6 +4077,8 @@ weston_output_init(struct weston_output *output, > > struct weston_compositor *c, > > int x, int y, int mm_width, int mm_height, uint32_t > > transform, > > int32_t scale) > > { > > + struct wl_event_loop *loop; > > + > > output->compositor = c; > > output->x = x; > > output->y = y; > > @@ -4053,6 +4099,10 @@ weston_output_init(struct weston_output *output, > > struct weston_compositor *c, > > wl_list_init(&output->resource_list); > > wl_list_init(&output->feedback_list); > > > > + loop = wl_display_get_event_loop(c->wl_display); > > + output->repaint_timer = wl_event_loop_add_timer(loop, > > + output_repaint_timer_handler, output); > > + > > output->id = ffs(~output->compositor->output_id_pool) - 1; > > output->compositor->output_id_pool |= 1 << output->id; > > > > @@ -4517,6 +4567,17 @@ weston_compositor_init(struct weston_compositor *ec, > > weston_compositor_add_debug_binding(ec, KEY_T, > > timeline_key_binding_handler, ec); > > > > + s = weston_config_get_section(ec->config, "core", NULL, NULL); > > + weston_config_section_get_int(s, "repaint-window", &ec->repaint_msec, > > + DEFAULT_REPAINT_WINDOW); > > + if (ec->repaint_msec < -10 || ec->repaint_msec > 1000) { > > + weston_log("Invalid repaint_window value in config: %d\n", > > + ec->repaint_msec); > > + ec->repaint_msec = DEFAULT_REPAINT_WINDOW; > > + } > > + weston_log("Output repaint window is %d ms maximum.\n", > > + ec->repaint_msec); > > + > > weston_compositor_schedule_repaint(ec); > > > > return 0; > > diff --git a/src/compositor.h b/src/compositor.h > > index 2e6ef9d..a451ba3 100644 > > --- a/src/compositor.h > > +++ b/src/compositor.h > > @@ -196,6 +196,7 @@ struct weston_output { > > pixman_region32_t previous_damage; > > int repaint_needed; > > int repaint_scheduled; > > + struct wl_event_source *repaint_timer; > > struct weston_output_zoom zoom; > > int dirty; > > struct wl_signal frame_signal; > > @@ -683,6 +684,7 @@ struct weston_compositor { > > int32_t kb_repeat_delay; > > > > clockid_t presentation_clock; > > + int32_t repaint_msec; > > > > int exit_code; > > }; > > -- > > 2.0.5 > > > > _______________________________________________ > > wayland-devel mailing list > > [email protected] > > http://lists.freedesktop.org/mailman/listinfo/wayland-devel _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
