[PATCH weston v2 1/2] compositor, backends: weston_compositor_read_presentation_clock
Pekka Paalanen
ppaalanen at gmail.com
Thu Mar 19 02:55:06 PDT 2015
On Wed, 18 Mar 2015 10:58:37 -0700
Bryce Harrington <bryce at osg.samsung.com> wrote:
> On Wed, Mar 18, 2015 at 03:27:21PM +0200, Pekka Paalanen wrote:
> > From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> >
> > Create a new function weston_compositor_read_presentation_clock() to
> > wrap the clock_gettime() call for the Presentation clock.
> >
> > Reading the presentation clock is never supposed to fail, but if it
> > does, this will notify about it. I have not seen it fail yet, though.
> >
> > This prepares for new testing features in the future that might allow
> > controlling the presentation clock. Right now it is just a convenience
> > function for clock_gettime().
> >
> > All presentation clock readers are converted to call this new function
> > except rpi-backend's rpi_flippipe_update_complete(), because it gets its
> > clock id via a thread-safe mechanism. There shouldn't be anything really
> > thread-unsafe in weston_compositor_read_presentation_clock() at the
> > moment, but might be in the future, and weston core is not expected to
> > need to be thread-safe.
> >
> > This is based on the original patch by
> > Cc: Derek Foreman <derekf at osg.samsung.com>
> >
> > Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > ---
> > src/compositor-drm.c | 2 +-
> > src/compositor-fbdev.c | 4 ++--
> > src/compositor-headless.c | 4 ++--
> > src/compositor-rdp.c | 4 ++--
> > src/compositor-rpi.c | 2 +-
> > src/compositor-x11.c | 4 ++--
> > src/compositor.c | 33 +++++++++++++++++++++++++++++++++
> > src/compositor.h | 5 +++++
> > 8 files changed, 48 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> > index ed4eabf..15152b0 100644
> > --- a/src/compositor-drm.c
> > +++ b/src/compositor-drm.c
> > @@ -723,7 +723,7 @@ drm_output_start_repaint_loop(struct weston_output *output_base)
> >
> > finish_frame:
> > /* if we cannot page-flip, immediately finish frame */
> > - clock_gettime(compositor->base.presentation_clock, &ts);
> > + weston_compositor_read_presentation_clock(&compositor->base, &ts);
> > weston_output_finish_frame(output_base, &ts,
> > PRESENTATION_FEEDBACK_INVALID);
> > }
> > diff --git a/src/compositor-fbdev.c b/src/compositor-fbdev.c
> > index eedf252..b4c1390 100644
> > --- a/src/compositor-fbdev.c
> > +++ b/src/compositor-fbdev.c
> > @@ -117,7 +117,7 @@ fbdev_output_start_repaint_loop(struct weston_output *output)
> > {
> > struct timespec ts;
> >
> > - clock_gettime(output->compositor->presentation_clock, &ts);
> > + weston_compositor_read_presentation_clock(output->compositor, &ts);
> > weston_output_finish_frame(output, &ts, PRESENTATION_FEEDBACK_INVALID);
> > }
> >
> > @@ -223,7 +223,7 @@ finish_frame_handler(void *data)
> > struct fbdev_output *output = data;
> > struct timespec ts;
> >
> > - clock_gettime(output->base.compositor->presentation_clock, &ts);
> > + weston_compositor_read_presentation_clock(output->base.compositor, &ts);
> > weston_output_finish_frame(&output->base, &ts, 0);
> >
> > return 1;
> > diff --git a/src/compositor-headless.c b/src/compositor-headless.c
> > index d4d25fa..1b1d327 100644
> > --- a/src/compositor-headless.c
> > +++ b/src/compositor-headless.c
> > @@ -58,7 +58,7 @@ headless_output_start_repaint_loop(struct weston_output *output)
> > {
> > struct timespec ts;
> >
> > - clock_gettime(output->compositor->presentation_clock, &ts);
> > + weston_compositor_read_presentation_clock(output->compositor, &ts);
> > weston_output_finish_frame(output, &ts, PRESENTATION_FEEDBACK_INVALID);
> > }
> >
> > @@ -68,7 +68,7 @@ finish_frame_handler(void *data)
> > struct headless_output *output = data;
> > struct timespec ts;
> >
> > - clock_gettime(output->base.compositor->presentation_clock, &ts);
> > + weston_compositor_read_presentation_clock(output->base.compositor, &ts);
> > weston_output_finish_frame(&output->base, &ts, 0);
> >
> > return 1;
> > diff --git a/src/compositor-rdp.c b/src/compositor-rdp.c
> > index 1c3f988..6955d49 100644
> > --- a/src/compositor-rdp.c
> > +++ b/src/compositor-rdp.c
> > @@ -307,7 +307,7 @@ rdp_output_start_repaint_loop(struct weston_output *output)
> > {
> > struct timespec ts;
> >
> > - clock_gettime(output->compositor->presentation_clock, &ts);
> > + weston_compositor_read_presentation_clock(output->compositor, &ts);
> > weston_output_finish_frame(output, &ts, PRESENTATION_FEEDBACK_INVALID);
> > }
> >
> > @@ -353,7 +353,7 @@ finish_frame_handler(void *data)
> > struct rdp_output *output = data;
> > struct timespec ts;
> >
> > - clock_gettime(output->base.compositor->presentation_clock, &ts);
> > + weston_compositor_read_presentation_clock(output->base.compositor, &ts);
> > weston_output_finish_frame(&output->base, &ts, 0);
> >
> > return 1;
> > diff --git a/src/compositor-rpi.c b/src/compositor-rpi.c
> > index 2d72686..08d5105 100644
> > --- a/src/compositor-rpi.c
> > +++ b/src/compositor-rpi.c
> > @@ -217,7 +217,7 @@ rpi_output_start_repaint_loop(struct weston_output *output)
> > struct timespec ts;
> >
> > /* XXX: do a phony dispmanx update and trigger on its completion? */
> > - clock_gettime(output->compositor->presentation_clock, &ts);
> > + weston_compositor_read_presentation_clock(output->compositor, &ts);
> > weston_output_finish_frame(output, &ts, PRESENTATION_FEEDBACK_INVALID);
> > }
> >
> > diff --git a/src/compositor-x11.c b/src/compositor-x11.c
> > index e9735c5..e89acb7 100644
> > --- a/src/compositor-x11.c
> > +++ b/src/compositor-x11.c
> > @@ -346,7 +346,7 @@ x11_output_start_repaint_loop(struct weston_output *output)
> > {
> > struct timespec ts;
> >
> > - clock_gettime(output->compositor->presentation_clock, &ts);
> > + weston_compositor_read_presentation_clock(output->compositor, &ts);
> > weston_output_finish_frame(output, &ts, PRESENTATION_FEEDBACK_INVALID);
> > }
> >
> > @@ -458,7 +458,7 @@ finish_frame_handler(void *data)
> > struct x11_output *output = data;
> > struct timespec ts;
> >
> > - clock_gettime(output->base.compositor->presentation_clock, &ts);
> > + weston_compositor_read_presentation_clock(output->base.compositor, &ts);
> > weston_output_finish_frame(&output->base, &ts, 0);
> >
> > return 1;
> > diff --git a/src/compositor.c b/src/compositor.c
> > index 60b7ee4..e060be1 100644
> > --- a/src/compositor.c
> > +++ b/src/compositor.c
> > @@ -4618,6 +4618,39 @@ weston_compositor_set_presentation_clock_software(
> > return -1;
> > }
> >
> > +/** Read the current time from the Presentation clock
> > + *
> > + * \param compositor
> > + * \param ts[out] The current time.
> > + *
> > + * \note Reading the current time in user space is always imprecise to some
> > + * degree.
> > + *
> > + * This function is never meant to fail. If reading the clock does fail,
> > + * an error message is logged and a zero time is returned. Callers are not
> > + * supposed to detect or react to failures.
>
> If 0 time is returned, are we certain that's not going to screw up
> callers in some unexpected way?
We don't care too much. Reading the current time is always
somewhat uncertain and the calling code should be implicitly already
dealing with surprising values. Like in the follow-up I have a sanity
check to not delay for more than a second in the worst case.
> If it's never meant to fail, and we don't want to propagate error codes
> from here, why not assert()?
It's not an assert() because there is a good chance it might not be a
bug in Weston's own code. If Weston is used in production, logging the
error and having a glitch is IMHO better than outright dying.
I think we are missing a bit of infrastructure here: an assert-like
macro that in debug builds aborts, but in production builds just logs
the error with as much detail as possible and attempts to continue.
It'd be good to also include rate limiting of logging.
> Otherwise, seems to me like there'd be little harm propagating the error
> by returning false. The current callers do ignore clock_gettime's
> return, but maybe some test someday will want to test conditions where
> clock_gettime wouldn't work properly...
Easy enough to change the return type when such a user appears. Until
then I don't see the point.
> > + */
> > +WL_EXPORT void
> > +weston_compositor_read_presentation_clock(
> > + const struct weston_compositor *compositor,
> > + struct timespec *ts)
> > +{
> > + static bool warned;
> > + int ret;
> > +
> > + ret = clock_gettime(compositor->presentation_clock, ts);
> > + if (ret < 0) {
> > + ts->tv_sec = 0;
> > + ts->tv_nsec = 0;
> > +
> > + if (!warned)
> > + weston_log("Error: failure to read "
> > + "the presentation clock %#x: '%m' (%d)\n",
> > + compositor->presentation_clock, errno);
> > + warned = true;
> > + }
> > +}
> > +
> > WL_EXPORT void
> > weston_version(int *major, int *minor, int *micro)
> > {
> > diff --git a/src/compositor.h b/src/compositor.h
> > index 76b0778..2e6ef9d 100644
> > --- a/src/compositor.h
> > +++ b/src/compositor.h
> > @@ -1343,6 +1343,11 @@ int
> > weston_compositor_set_presentation_clock_software(
> > struct weston_compositor *compositor);
> > void
> > +weston_compositor_read_presentation_clock(
> > + const struct weston_compositor *compositor,
> > + struct timespec *ts);
> > +
> > +void
> > weston_compositor_shutdown(struct weston_compositor *ec);
> > void
> > weston_compositor_exit_with_code(struct weston_compositor *compositor,
>
> Apart from the error handling, rest looks fine:
>
> Reviewed-by: Bryce Harrington <bryce at osg.samsung.com>
Is my explanation satisfactory enough?
Thanks,
pq
More information about the wayland-devel
mailing list