[PATCH weston 6/7] Switch to global output repaint hook

[Daniel Stone]

Hi,

On 16 February 2017 at 17:04, Pekka Paalanen
<pekka.paalanen at collabora.co.uk> wrote:
> For some background, here is first an explanation of how upstream
> Weston works currently.

I agree with the summary. Must document this somewhere more lasting. :)

Any review comments I haven't replied to I've just snipped for
brevity, and are fixed in the coming revision.

> On Tue, 14 Feb 2017 13:18:06 +0000 Daniel Stone <daniels at collabora.com> wrote:
>> +     if (first_time == 0xffffffff)
>> +             return;
>> +
>> +     /* Now we switch first_time into relative units for the timer. */
>> +     if (first_time <= now_ms + 1)
>> +             first_time = 1;
>> +     else
>> +             first_time -= now_ms;
>> +     wl_event_source_timer_update(compositor->repaint_timer, first_time);
>
> This makes a minimum delay of 1 ms. I worry that if
> output_repaint_timer_arm() (e.g. via weston_output_schedule_repaint()
> which probably gets called quite often) gets called too often, the
> timer will never expire.
>
> Maybe you need an idle callback instead when we need to repaint ASAP?
>
> However, see my note at weston_output_schedule_repaint() below.
>
> Maybe you should just call output_repaint_timer_handler() directly if
> the time has passed?

Mmm ... after removing the stray added timer_arm() call from
schedule_repaint(), I don't believe this is an issue. I can't remember
why I added it, and it doesn't make any sense to me.

That means that each output can, at worst, reset the timer to be 1ms
in the future (where it would've been immediate), exactly once per
repaint call. After the output does so, it can no longer do so again,
as it will hit the short-circuit return in schedule_repaint due to
repaint_scheduled already being true. I guess we could have a bool
weston_compositor->timer_already_armed_for_1ms_in_the_future, but that
seems overkill, especially for the truly baroque set of circumstances
you'd need in order to trigger the cascade, and for it to be of any
real effect.

>> @@ -2396,12 +2455,11 @@ weston_output_finish_frame(struct weston_output *output,
>>                                                 presented_flags);
>>
>>       output->frame_time = timespec_to_msec(stamp);
>
> Oh boy... I noticed this only now. Overflow here too, and it has been
> upstream for who knows how long. Luckily frame_time is not used for
> anything important, just as the frame callback argument, internal
> animation timings, and the video recorder...

Changing things like the video recorder and animation timings to use a
real clock would definitely be nice; the callback argument is pretty
unfixable though as it's uint32_t ... I don't really see what issues
the overflow brings though, as the frame timer is explicitly allowed
to overflow.

Nevertheless, I'm now using struct timespec throughout to store frame
timings, with occasional deltas in int64_t nsec. If anyone ever has
outputs whose vblank intervals are spaced 584 years apart, the
overflow is not going to be their most pressing issue.

>> @@ -2548,8 +2603,10 @@ weston_output_schedule_repaint(struct weston_output *output)
>>
>>       loop = wl_display_get_event_loop(compositor->wl_display);
>>       output->repaint_needed = 1;
>> -     if (output->repaint_scheduled)
>> +     if (output->repaint_scheduled) {
>> +             output_repaint_timer_arm(compositor);
>
> Tbqh, I don't understand why call this here?

Me neither, and it would've caused the schedule_repaint issue you
pointed out above to be far worse.

Cheers,
Daniel