[PATCH weston v2 2/2] compositor: add repaint delay timer

[Derek Foreman]

On 18/03/15 08:27 AM, Pekka Paalanen wrote:
> From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> 
> 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 <pekka.paalanen at collabora.co.uk>
> ---
>  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)

What's the msec > 1000 for?  When doing fake presentation clock later I
think this will be a problem.  I guess at that point I can make the "too
long" test conditional on test mode being off...

Under normal circumstances maybe we should log (perhaps just the first
time) the > 1000 case, as it seems to indicate something is rather wrong?

This all looks pretty good to me though,
Reviewed-By: Derek Foreman <derekf at osg.samsung.com>

> +		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;
>  };
>