[PATCH weston] Make backends always specify output repaint time
Kristian Høgsberg
hoegsberg at gmail.com
Mon Apr 8 14:11:27 PDT 2013
On Sun, Apr 07, 2013 at 05:24:51PM +0300, Pekka Paalanen wrote:
> On Fri, 5 Apr 2013 23:07:11 +0200
> Jonas Ådahl <jadahl at gmail.com> wrote:
>
> > Most backends relies on gettimeofday(2) for output repaint timestamps
> > but this is not a requirement. Before this patch repaints coming from
> > idle_repaint() always used gettimeofday(2) for timestamps. For backends
> > not using that time source this could cause large jumps between
> > timestamps.
> >
> > To fix this, timestamps needs to always come from the backend. This
> > means that the backend needs to always be responsible of starting the
> > repaint loop in case that the repaint cannot start immediately.
>
> Good idea.
>
> > The drm backend implementation is from the patch by Ander Conselvan de
> > Oliveira found here:
> > http://lists.freedesktop.org/archives/wayland-devel/2013-February/007393.html
> >
> > Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> > ---
> >
> > Hi,
> >
> > This is a follow up from reviews of Ander's patch[0] from February.
> >
> > What is done since that patch is to implement the "start_repaint_loop"
> > for the current backends found in weston. All but the wayland backend
> > implementation are pretty straight forward.
> >
> > [0] http://lists.freedesktop.org/archives/wayland-devel/2013-February/007393.html
> >
> > Jonas
> >
> > src/compositor-drm.c | 34 ++++++++++++++--
> > src/compositor-fbdev.c | 18 ++++++---
> > src/compositor-headless.c | 12 ++++--
> > src/compositor-rdp.c | 19 +++++----
> > src/compositor-rpi.c | 27 ++++++++++---
> > src/compositor-wayland.c | 97 +++++++++++++++++++++++++++++++++++++++++++++
> > src/compositor-x11.c | 20 +++++++---
> > src/compositor.c | 2 +-
> > src/compositor.h | 1 +
> > 9 files changed, 199 insertions(+), 31 deletions(-)
> >
> > diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> > index 6e0a126..49f197e 100644
> > --- a/src/compositor-drm.c
> > +++ b/src/compositor-drm.c
> > @@ -623,6 +623,26 @@ drm_output_repaint(struct weston_output *output_base,
> > }
> >
> > static void
> > +drm_output_start_repaint_loop(struct weston_output *output_base)
> > +{
> > + struct drm_output *output = (struct drm_output *) output_base;
> > + struct drm_compositor *compositor = (struct drm_compositor *)
> > + output_base->compositor;
> > + uint32_t fb_id;
> > +
> > + if (output->current)
> > + fb_id = output->current->fb_id;
> > + else
> > + fb_id = output->original_crtc->buffer_id;
> > +
> > + if (drmModePageFlip(compositor->drm.fd, output->crtc_id, fb_id,
> > + DRM_MODE_PAGE_FLIP_EVENT, output) < 0) {
> > + weston_log("queueing pageflip failed: %m\n");
>
> Btw, does this cause one refresh cycle (scanout frame) delay in
> starting the repaint loop?
Previously, we repaint in an idle handler once somebody queues a
repaint. This typically means immediately, and as such the next page
flip will present the updated contents. With this patch, we now queue
a pageflip, then render and pageflip. The result is going to be
visible the next frame, so yes it does. However, this is now
consistent with how we render in steady-state rendering mode, that is,
what we do when the repaint loop is already running.
The way the repaint loop works now maximizes the window available to
client to submit new content at the cost of making the pipeline one
frame deeper. By repainting the next frame right after pageflip to
the previous frame, we give clients the remainder of the frame to
submit new content for the frame after next. The downside is that
when apps submit new content, the next frame is already baked and
sitting in the pipeline.
What we do now during steady state is this:
--------V--------------V--------------V--------------V-------> (time)
^evdev event arrives
^compositor sends frame event, input events
^client repaints, commits
^compositor repaints, sends frame event, pageflips
^content becomes visible
<----------------------------------> 2-3 frames lag
and with Jonas' change the cold-start scenario is this:
--------V--------------V--------------V--------------V------->
^client repaints, commits
^compositor calls start_frame
^compositor repaints, sends frame event, pageflips
^content becomes visible
While we add a frame of lag to this case, we do make it more
consistent with the steady state case.
Another problem that Pekka brought up is that the timestamp isn't
actually for the frame that presents the committed content, it's the
frame just before (or the dummy timestamp in the coldstart case).
If we want to change that, we should change both cases. We'll have to
pull the repaint forward so it happens before the pageflip. This is a
can of worms, but removing a frame of lag could be well worth it.
Certainly for touchscreens, which are pretty unforgiving in exposing
lag between input events and UI updates.
So here is what it could look like:
------------V----------------------V----------------------V-------->
^evdev event arrives
^compositor repaints prev frame, sends frame event and input events
^client repaints, commits
^compositor repaints, schedules pageflip
<---------------> repaint delay
^content becomes visible
<--------------------------------> 1-2 frames lag
The can of worms mentioned above is that many of these events now
happen at some hand-wavey, unspecified points in time between the
vblank events. Sending out the frame events is what triggers clients
rendering their next frames. We still want to do that immediately
after the compositor has rendered its frame, to make sure clients can
start rendering as soon as possible. We also send out queued up input
events at this time, so clients receive the frame event and input
events for that frame together. The clients repaint and commit in
response to this and the big question then is when to start the
compositor repaint. We can't start until the vblank immediately
following the previous repaint has happened, it has to happen after
that. This is "repaint delay" in the timeline above. As we delay the
repaint relative to the vblank, we reduce the lag, but increase the
risk of repainting too late and missing a frame. What we have now, is
equivalente of delaying the repaint a frame.
The problem that Pekka brought up of not sending the right timestamp
is still not really fixable with the above structure. When the
compositor repaints and sends out frame events it still doesn't know
when the frame is going to be visible. We could send out frame events
when the pageflip happens, but then we've used up precious time in the
repaint cycle. Or we could send out the frame event and then a new
"this is the timestamp when your content was actually presented", but
I don't really think it helps clients at that point. They need to
start rendering the next frame as soon as possible, but we simply
don't know the timestamp at that time. When we do know the timestamp,
it's too late.
Regardless, I think we do want to introduce the repaint_delay setting,
but it's just one of those annoying "tweakables". I don't think
there's a good universal value we can use.
Kristian
> > + return;
> > + }
> > +}
> > +
> > +static void
> > vblank_handler(int fd, unsigned int frame, unsigned int sec, unsigned int usec,
> > void *data)
> > {
> > @@ -649,11 +669,16 @@ page_flip_handler(int fd, unsigned int frame,
> > struct drm_output *output = (struct drm_output *) data;
> > uint32_t msecs;
> >
> > - output->page_flip_pending = 0;
> > + /* We don't set page_flip_pending on start_repaint_loop, in that case
> > + * we just want to page flip to the current buffer to get an accurate
> > + * timestamp */
> > + if (output->page_flip_pending) {
> > + drm_output_release_fb(output, output->current);
> > + output->current = output->next;
> > + output->next = NULL;
> > + }
> >
> > - drm_output_release_fb(output, output->current);
> > - output->current = output->next;
> > - output->next = NULL;
> > + output->page_flip_pending = 0;
> >
> > if (!output->vblank_pending) {
> > msecs = sec * 1000 + usec / 1000;
> > @@ -1618,6 +1643,7 @@ create_output_for_connector(struct drm_compositor *ec,
> > wl_list_insert(ec->base.output_list.prev, &output->base.link);
> >
> > output->base.origin = output->base.current;
> > + output->base.start_repaint_loop = drm_output_start_repaint_loop;
> > output->base.repaint = drm_output_repaint;
> > output->base.destroy = drm_output_destroy;
> > output->base.assign_planes = drm_assign_planes;
>
>
>
> > diff --git a/src/compositor-rpi.c b/src/compositor-rpi.c
> > index cae6f7b..43fc14a 100644
> > --- a/src/compositor-rpi.c
> > +++ b/src/compositor-rpi.c
> > @@ -612,19 +612,26 @@ rpi_element_update(struct rpi_element *element,
> > return 0;
> > }
> >
> > +static uint64_t
> > +rpi_get_current_time(void)
> > +{
> > + struct timeval tv;
> > +
> > + /* manufacture flip completion timestamp */
>
> See below.
>
> > + /* XXX: use CLOCK_MONOTONIC instead? */
> > + gettimeofday(&tv, NULL);
> > + return (uint64_t)tv.tv_sec * 1000 + tv.tv_usec / 1000;
> > +}
> > +
> > static void
> > rpi_flippipe_update_complete(DISPMANX_UPDATE_HANDLE_T update, void *data)
> > {
> > /* This function runs in a different thread. */
> > struct rpi_flippipe *flippipe = data;
> > - struct timeval tv;
> > uint64_t time;
> > ssize_t ret;
> >
> > - /* manufacture flip completion timestamp */
>
> This "manufacture" comment should stay here, not be moved.
> Otherwise ok.
>
> > - /* XXX: use CLOCK_MONOTONIC instead? */
> > - gettimeofday(&tv, NULL);
> > - time = (uint64_t)tv.tv_sec * 1000 + tv.tv_usec / 1000;
> > + time = rpi_get_current_time();
> >
> > ret = write(flippipe->writefd, &time, sizeof time);
> > if (ret != sizeof time)
> > @@ -885,6 +892,15 @@ rpi_output_destroy_old_elements(struct rpi_output *output)
> > }
> >
> > static void
> > +rpi_output_start_repaint_loop(struct weston_output *output)
> > +{
> > + uint64_t time;
> > +
> > + time = rpi_get_current_time();
> > + weston_output_finish_frame(output, time);
> > +}
> > +
> > +static void
> > rpi_output_repaint(struct weston_output *base, pixman_region32_t *damage)
> > {
> > struct rpi_output *output = to_rpi_output(base);
> > @@ -1029,6 +1045,7 @@ rpi_output_create(struct rpi_compositor *compositor)
> > output->egl_window.width = modeinfo.width;
> > output->egl_window.height = modeinfo.height;
> >
> > + output->base.start_repaint_loop = rpi_output_start_repaint_loop;
> > output->base.repaint = rpi_output_repaint;
> > output->base.destroy = rpi_output_destroy;
> > if (compositor->max_planes > 0)
>
> The rpi bits look good, apart from that one comment.
>
>
> > diff --git a/src/compositor.c b/src/compositor.c
> > index e7c22db..693df2c 100644
> > --- a/src/compositor.c
> > +++ b/src/compositor.c
> > @@ -1308,7 +1308,7 @@ idle_repaint(void *data)
> > {
> > struct weston_output *output = data;
> >
> > - weston_output_finish_frame(output, weston_compositor_get_time());
> > + output->start_repaint_loop(output);
>
> Very nice to see this get_time() removed.
>
> > }
> >
> > WL_EXPORT void
> > diff --git a/src/compositor.h b/src/compositor.h
> > index 7d1d68e..1e999a6 100644
> > --- a/src/compositor.h
> > +++ b/src/compositor.h
> > @@ -186,6 +186,7 @@ struct weston_output {
> > struct weston_mode *origin;
> > struct wl_list mode_list;
> >
> > + void (*start_repaint_loop)(struct weston_output *output);
> > void (*repaint)(struct weston_output *output,
> > pixman_region32_t *damage);
> > void (*destroy)(struct weston_output *output);
> > --
> > 1.7.10.4
>
> I checked only the rpi specific bits, looking good.
>
>
> Thanks,
> pq
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
More information about the wayland-devel
mailing list