[PATCH weston v2 10/11] [RFC] Account for very large repaint window misses

[Pekka Paalanen]

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

> At the bottom of weston_output_finish_frame(), code exists to account
> for flips which have missed the repaint window, by shifting them to lock
> on to the next repaint window rather than repainting immediately.
> 
> This code only accounted for flips which missed their target by one
> repaint window. If they miss by multiples of the repaint window, adjust
> them until the next repaint timestamp is in the future.
> 
> I was unable to find from discussion of the original development whether
> or not this is desirable; I can see an argument both ways. Hence, RFC
> tag.
> 
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> Cc: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> Cc: Mario Kleiner <mario.kleiner.de at gmail.com>
> ---
>  libweston/compositor.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> v2: New in this revision. Unsure if necessary or not.
> 
> diff --git a/libweston/compositor.c b/libweston/compositor.c
> index a64e695..511f85a 100644
> --- a/libweston/compositor.c
> +++ b/libweston/compositor.c
> @@ -2485,9 +2485,14 @@ weston_output_finish_frame(struct weston_output *output,
>  	 * the deadline given by repaint_msec? In that case we delay until
>  	 * the deadline of the next frame, to give clients a more predictable
>  	 * timing of the repaint cycle to lock on. */
> -	if (presented_flags == WP_PRESENTATION_FEEDBACK_INVALID && msec_rel < 0)
> -		timespec_add_nsec(&output->next_repaint, &output->next_repaint,
> -				  refresh_nsec);
> +	if (presented_flags == WP_PRESENTATION_FEEDBACK_INVALID &&
> +	    msec_rel < 0) {
> +		while (timespec_sub_to_nsec(&output->next_repaint, &now) < 0) {
> +			timespec_add_nsec(&output->next_repaint,
> +					  &output->next_repaint,
> +					  refresh_nsec);
> +		}
> +	}
>  
>  out:
>  	output->repaint_status = REPAINT_SCHEDULED;

Hi,

the very point is to postpone the repaint to a future time so that
clients would not be racing each other any more than they do when the
repaint loop is continuously active. In that sense this is a correct
change. It just seems a bit awkward to use a loop instead computing the
rounded number of frame periods by division.

However, what strikes me as odd here is that this is done only when
coming from start_repaint_loop(). How could start_repaint_loop() cause
weston_output_finish_frame() to be called with a timestamp that is
older than the most recent vblank?

Did you see a case where it happened?

I think it would only happen with a) broken driver reporting bad
timestamps with pageflip events (we already verify timestamps from the
vblank query and fall back to pageflip otherwise), or b) something
stalled weston for too long.

a) would be a bug we want to log. b) might be a bug we want to log, or
just high system load in general which we wouldn't want to log, really.
Unfortunately there is no way to tell.

There is the original reason we want to ensure the target time is in
the future, but there are no reasons when doing so would be bad.
Therefore I think this is a good idea.

I'd like to get rid of the loop if it doesn't make things too
messy, even when the "insanity check" already ensures we don't
loop too long.

Failed insanity check makes the target time 'now'. Should that also be
subject to postponing for a period? I think it would be more
consistent. That is, make the comparison <= 0.

What were the arguments you thought to make this an undesireable patch?
I couldn't come up with any.


Thanks,
pq

PS. Hmm, or maybe there could be a flag "your system is overloaded"
that weston-desktop-shell could show on the panel to indicate that the
user is not getting the intended performance... in case the user
doesn't notice the reduced or jittery framerates. ;-)