On Thu, 16 Feb 2017 14:53:39 +0000 Daniel Stone <[email protected]> wrote:
> Hi, > > On 16 February 2017 at 14:31, Pekka Paalanen <[email protected]> wrote: > > On Tue, 14 Feb 2017 13:18:05 +0000 > > Daniel Stone <[email protected]> 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 <[email protected]> > > 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
pgpmnyRXpdQXw.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
