<div dir="ltr"><div class="gmail_extra"> <div class="gmail_quote">On Sun, Apr 7, 2013 at 4:24 PM, Pekka Paalanen <<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Fri, 5 Apr 2013 23:07:11 +0200 Jonas Ådahl <<a href="mailto:jadahl@gmail.com">jadahl@gmail.com</a>> 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. </div>Good idea. <div><div class="h5"> > The drm backend implementation is from the patch by Ander Conselvan de > Oliveira found here: > <a href="http://lists.freedesktop.org/archives/wayland-devel/2013-February/007393.html" target="_blank">http://lists.freedesktop.org/archives/wayland-devel/2013-February/007393.html</a> > > Signed-off-by: Jonas Ådahl <<a href="mailto:jadahl@gmail.com">jadahl@gmail.com</a>> > --- > > 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] <a href="http://lists.freedesktop.org/archives/wayland-devel/2013-February/007393.html" target="_blank">http://lists.freedesktop.org/archives/wayland-devel/2013-February/007393.html</a> > > 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"); </div></div>Btw, does this cause one refresh cycle (scanout frame) delay in starting the repaint loop? </blockquote><div> </div><div>As far as I understand it, yes. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div class="h5"> > + 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; </div></div><div class="im">> 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 */ </div>See below. <div class="im"> > + /* 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 */ </div>This "manufacture" comment should stay here, not be moved. Otherwise ok. <div><div class="h5"> > - /* 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) </div></div>The rpi bits look good, apart from that one comment. <div class="im"> > 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); </div>Very nice to see this get_time() removed. <div class="im"> > } > > 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 </div>I checked only the rpi specific bits, looking good. </blockquote><div> </div><div>Thanks for the review. </div><div>Jonas </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Thanks, pq </blockquote></div> </div></div>