On Wed, 1 Mar 2017 11:34:09 +0000 Daniel Stone <[email protected]> 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 <[email protected]> > Cc: Pekka Paalanen <[email protected]> > Cc: Mario Kleiner <[email protected]> > --- > 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. ;-) _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
