[PATCH weston] Make backends always specify output repaint time

Jonas Ådahl jadahl at gmail.com
Mon Apr 8 10:06:09 PDT 2013


On Sun, Apr 7, 2013 at 4:24 PM, Pekka Paalanen <ppaalanen at gmail.com> 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?
>

As far as I understand it, yes.


>
> > +             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 for the review.

Jonas


>
>
> Thanks,
> pq
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20130408/2f871110/attachment-0001.html>


More information about the wayland-devel mailing list