[PATCH weston 5/7] Don't delay initial output paint

[Daniel Stone]

Hi,

On 16 February 2017 at 14:31, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Tue, 14 Feb 2017 13:18:05 +0000
> Daniel Stone <daniels at collabora.com> wrote:
>> On startup, we cannot lock on to the repaint timer because it is unknown
>> to us. We deal with this by claiming that the moment of entry into the
>> repaint loop is the moment a frame returned, causing finish_frame to
>> delay our initial repaint to (refresh_time - repaint_delay), typically
>> around 9ms of utterly wasted time.
>
> I wonder. The reason the repaint_delay exists is that clients would not
> race to who gets to submit its frame first and kick everyone else to
> the following refresh cycle. That's a concern when weston is in
> continuous update mode.
>
> This patch is for repainting from idle instead. Do we still have the
> same concern? I suppose one can say it makes no difference, because it's
> not the compositor triggering client actions with e.g. frame callbacks,
> clients wake up at arbitrary times.

It's not repainting from idle, but rather repainting (make that
'painting': we have no framebuffer) from dead. I'll try to clarify the
commit message beyond the 'On startup ...' introduction. If we can't
query any times at all, then we schedule our _initial_ paint for
now+7ms. That is purely random: we don't have a repaint cycle to lock
on to, because the clocks may not even be running. Even if they were,
we have no idea what they are.

I was about to say 'this doesn't gain us anything', except if all
clients do submit and trigger repaint immediately, there's a 7ms grace
period where we could potentially group repaints together. I don't
think that's something we should be relying on though: desktop-shell
has the desktop_ready request for this exact purpose. If anything else
is relying on the arbitrary 7ms delay to group the initial repaint
together, then someone (to be clear: not me) should fix that.

>> Add an explicit stamp == NULL, to determine that we are just beginning
>> our repaint loop, that the timings are in fact totally invalid, and that
>> it would be beneficial to repaint the output immediately.
>
> This is probably the more important justifaction for you, to get rid of
> fake timestamps?

It's not ...

>> --- a/libweston/compositor-drm.c
>> +++ b/libweston/compositor-drm.c
>> @@ -862,8 +862,7 @@ drm_output_start_repaint_loop(struct weston_output *output_base)
>>
>>  finish_frame:
>>       /* if we cannot page-flip, immediately finish frame */
>> -     weston_compositor_read_presentation_clock(output_base->compositor, &ts);
>> -     weston_output_finish_frame(output_base, &ts,
>> +     weston_output_finish_frame(output_base, NULL,
>>                                  WP_PRESENTATION_FEEDBACK_INVALID);
>
> This is a pattern used by practically every backend. Shouldn't they all
> be fixed alike?

fbdev is pretty much beyond hope, so I don't propose to fix that; I'd
far sooner delete it. RDP copied fbdev, so doesn't discern between the
start-from-dead and start-from-idle cases; I can't even test that, so
don't propose to fix that either. The Wayland backend attaches a
transparent buffer to the surface, so it can lock on to the repaint
loop; it could certainly have this path removed to use the same
fallback path, but that's more invasive than I'd hoped for right now.
The X11 backend is like fbdev and RDP, except it repaints every 10ms
instead of 16; I could guess why, but don't actually know.

Anyway, in summary, a good cleanup task for later, but nothing I want
to touch right now.

> OTOH, for DRM-backend this is the fallback of the fallback path, so
> hardly important.

It never gets hit on regular usage, but it does get hit on startup.
For actually-atomic modesetting (cf. cover letter), we need to ensure
that all the repaints land together. Having a real genuine 'as soon as
possible, because I don't know times' is thus pretty important there.

>> diff --git a/libweston/compositor.c b/libweston/compositor.c
>> index 9eab0e298..ed8ef90fd 100644
>> --- a/libweston/compositor.c
>> +++ b/libweston/compositor.c
>> @@ -2383,6 +2383,12 @@ weston_output_finish_frame(struct weston_output *output,
>>       TL_POINT("core_repaint_finished", TLP_OUTPUT(output),
>>                TLP_VBLANK(stamp), TLP_END);
>
> I suppose there could be an
> assert(stamp || (presented_flags & WP_PRESENTATION_FEEDBACK_INVALID))?

Good point; will add.

>> @@ -2415,6 +2421,7 @@ weston_output_finish_frame(struct weston_output *output,
>>       if (presented_flags == WP_PRESENTATION_FEEDBACK_INVALID && msec_rel < 0)
>>               msec_rel += refresh_nsec / 1000000;
>>
>> +out:
>>       if (msec_rel < 1)
>>               output_repaint_timer_handler(output);
>>       else
>
> Right. Feel free to ignore my thinking-out-loud, and slap a:
> Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>

I will, thanks. :) In the meantime, it's not exactly going to go in
before the release, so if you want to follow up on anything, we've
still got time. Thanks for the review!

Cheers,
Daniel