[PATCH weston v2 09/11] Switch to global output repaint timer

[Pekka Paalanen]

On Wed,  1 Mar 2017 11:34:08 +0000
Daniel Stone <daniels at collabora.com> wrote:

> In preparation for grouping output repaint together where possible,
> switch the per-output repaint timer, to a global timer which iterates
> across all outputs.
> 
> This is implemented by storing the absolute time for the next repaint
> for each output locally, and maintaining a global timer which iterates
> all of them, scheduling the repaint for the first available time.
> 
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> Cc: Mario Kleiner <mario.kleiner.de at gmail.com>
> Cc: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> ---
>  libweston/compositor.c | 113 ++++++++++++++++++++++++++++++++++++-------------
>  libweston/compositor.h |   6 ++-
>  2 files changed, 88 insertions(+), 31 deletions(-)
> 
> v2: Rebased on top of previous changes (repaint_status enum).
>     Suggestions from Pekka:
>     Keep absolute target repaint time as struct timespec, only using
>     int64_t nsec for relative comparison to current time, both in
>     maybe_repaint and finish_frame.
>     Remove unnecessary output_repaint_timer_arm from schedule_repaint
>     early-exit path.
>     Use explict bool rather than 0/~0 time to determine if any outputs
>     are eligible for repaint in the timer.
> 
> diff --git a/libweston/compositor.c b/libweston/compositor.c
> index 4aa30da..a64e695 100644
> --- a/libweston/compositor.c
> +++ b/libweston/compositor.c

> +static void
> +output_repaint_timer_arm(struct weston_compositor *compositor)
> +{
> +	struct weston_output *output;
> +	bool any_should_repaint = false;
> +	struct timespec now;
> +	int64_t msec_to_next;
> +
> +	weston_compositor_read_presentation_clock(compositor, &now);
> +
> +	wl_list_for_each(output, &compositor->output_list, link) {
> +		int64_t msec_to_this;
> +
> +		if (output->repaint_status != REPAINT_SCHEDULED)
> +			continue;
> +
> +		msec_to_this = timespec_sub_to_msec(&output->next_repaint,
> +						    &now);
> +		if (!any_should_repaint || msec_to_this < msec_to_next)
> +			msec_to_next = msec_to_this;
> +
> +		any_should_repaint = true;
> +	}
> +
> +	if (!any_should_repaint)
> +		return;
> +
> +	/* XXX: This can keep postponing forever, maybe? */
> +	if (msec_to_next < 1)
> +		msec_to_next = 1;

Hi,

I think there is no longer a risk of postponing indefinitely, because
output_repaint_timer_arm() is no longer called from
weston_output_schedule_repaint(). The other call sites are well
throttled.

I'm curious about the reason to have at least 1 ms delay, even if
weston_output_finish_frame() deemed that we should paint ASAP. Is it to
allow coalescing several outputs' finish_frame needing immediate
repaint into single call through output_repaint_timer_handler()?

Would be really nice to see some explanation in a comment here. Not
that it actually matters much, since we are talking about the time
instant to start painting, which is not that crucial to begin with.

Although, I suppose if one constructed a 16-head monster, each head
adding another 1 ms delay could ruin everything depending on how
finish_frame calls get timed.

That is the only comment I have for this patch, otherwise it is:
Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>

If there is just a change or removal of that comment, I can do that
while merging.

> +
> +	wl_event_source_timer_update(compositor->repaint_timer, msec_to_next);
> +}


Thanks,
pq