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

[Daniel Stone]

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);

> 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.

> 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.

Cheers,
Daniel