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

Fri Feb 17 14:22:13 UTC 2017

On Thu, 16 Feb 2017 14:53:39 +0000
Daniel Stone <daniel at fooishbar.org> wrote:

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

Ooh, I missed that. "On startup", indeed, when there is not just
missing the FB, but the CRTC might be off to begin with, hence vblank
timestamp query can fail. I never thought of that before.

You are completely right.

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

Agreed.

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

Yeah, though I more thought that if you are calling
weston_output_finish_frame() with FEEDBACK_INVALID, would there ever be
use for the (fake) timestamp?

But that's not important 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.

Yes, forgot the startup.

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

Nope, all seems good here. :-)

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