On Fri, 10 Mar 2017 14:52:58 +0000 Daniel Stone <[email protected]> wrote:
> Hi Pekka, > > On 10 March 2017 at 13:41, Pekka Paalanen > <[email protected]> wrote: > > On Wed, 1 Mar 2017 11:34:09 +0000 Daniel Stone <[email protected]> > > 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
pgpmvKFy9VytO.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
