[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