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

[Pekka Paalanen]

On Fri, 10 Mar 2017 14:52:58 +0000
Daniel Stone <daniel at fooishbar.org> wrote:

> Hi Pekka,
> 
> On 10 March 2017 at 13:41, Pekka Paalanen
> <pekka.paalanen at collabora.co.uk> wrote:
> > On Wed,  1 Mar 2017 11:34:09 +0000 Daniel Stone <daniels at collabora.com> wrote:  
> >>        * 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;  
> >
> > 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.  
> 
> Sure, not that difficult, just a little ugly:
> int n_periods = !!(msec_rel % (refresh_nsec * NSEC_TO_MSEC));
> n_periods += msec_rel / (refresh_nsec * NSEC_TO_MSEC);

Maybe I'll send my own proposal for this, if I can come up with
something nicer.

> > 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?  
> 
> No idea: if you look at the diff, the original code did the exact same
> thing, but only adjusted by a maximum of one period. I agree with you
> that it would probably seem better to postpone in all cases, giving
> sheer predictability (always respecting the repaint window exactly)
> for speed. But I'd like some more input before changing this, and I'd
> like to change it in a separate patch to this.

Sure!

> > Did you see a case where it happened?  
> 
> No, it's just something that had been bugging me ever since I started
> looking into repaint. Found purely by inspection.
> 
> > 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.  
> 
> As long as the above looks reasonable, then sure; happy to expand the
> !! to a ternary or something as well.
> 
> > 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.  
> 
> Er, unsure. Probably?
> 
> > What were the arguments you thought to make this an undesireable patch?
> > I couldn't come up with any.  
> 
> None per se. I flagged it as such because a) it isn't required to
> achieve the actual goal of the patchset, b) given the somewhat
> intricate nature of the code in question, I wasn't sure if it was
> deliberate or not, and c) I only noticed it by inspection and have
> never observed it myself. But it does indeed seem to be correct, so
> I'll take that as a positive comment, and maybe wait to see what Mario
> says before we think about merging it.

Yeah, let's leave this brewing for a bit.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170313/3afcb651/attachment.sig>