Discussion:
[PATCH weston v2 0/2] New repaint scheduling algorithm
(too old to reply)
Pekka Paalanen
2015-03-18 13:27:20 UTC
Permalink
From: Pekka Paalanen <***@collabora.co.uk>

Hi,

this is the v2 of
http://lists.freedesktop.org/archives/wayland-devel/2015-February/019924.html

Please see
http://ppaalanen.blogspot.fi/2015/02/weston-repaint-scheduling.html
for the details.

From the v1 series, the first 3 patches landed, and the remaining two needed
revising.

Here I have squashed the remaining 2 patches together and addressed review
comments. The first patch in this new series is a new one, except the idea was
stolen from
http://patchwork.freedesktop.org/patch/39502/


Thanks,
pq

Pekka Paalanen (2):
compositor, backends: weston_compositor_read_presentation_clock
compositor: add repaint delay timer

man/weston.ini.man | 10 ++++
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 | 116 +++++++++++++++++++++++++++++++++++++++++-----
src/compositor.h | 7 +++
9 files changed, 132 insertions(+), 21 deletions(-)
--
2.0.5
Pekka Paalanen
2015-03-18 13:27:21 UTC
Permalink
This post might be inappropriate. Click to display it.
Bryce Harrington
2015-03-18 17:58:37 UTC
Permalink
Post by Pekka Paalanen
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
---
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)
/* 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?

If it's never meant to fail, and we don't want to propagate error codes
from here, why not assert()?

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...
Post by Pekka Paalanen
+ */
+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 <***@osg.samsung.com>
Pekka Paalanen
2015-03-19 09:55:06 UTC
Permalink
On Wed, 18 Mar 2015 10:58:37 -0700
Post by Bryce Harrington
Post by Pekka Paalanen
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
---
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)
/* 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.
Post by Bryce Harrington
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.
Post by Bryce Harrington
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.
Post by Bryce Harrington
Post by Pekka Paalanen
+ */
+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,
Is my explanation satisfactory enough?


Thanks,
pq
Bryce Harrington
2015-03-19 18:29:52 UTC
Permalink
Post by Pekka Paalanen
On Wed, 18 Mar 2015 10:58:37 -0700
Post by Bryce Harrington
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.
Post by Bryce Harrington
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.
The man page of assert() indicates that for non-debug builds (NDEBUG not
defined), the assert() macro does nothing. Aside from logging an error
message, it seems to do what you want?
Post by Pekka Paalanen
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.
Yes, I'd love to see this, I think it'd solve the need nicely.
Post by Pekka Paalanen
Post by Bryce Harrington
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.
Is my explanation satisfactory enough?
Welll, it didn't change my mind. :-) But the issue is pretty minor, and
not worth blocking on if you want to land it as-is.

Bryce
Pekka Paalanen
2015-03-20 10:18:13 UTC
Permalink
On Thu, 19 Mar 2015 11:29:52 -0700
Post by Bryce Harrington
Post by Pekka Paalanen
On Wed, 18 Mar 2015 10:58:37 -0700
Post by Bryce Harrington
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.
Post by Bryce Harrington
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.
The man page of assert() indicates that for non-debug builds (NDEBUG not
defined), the assert() macro does nothing. Aside from logging an error
message, it seems to do what you want?
Non-debug means NDEBUG is defined - does anyone actually use that?
It prevents running our test suite, too.
Post by Bryce Harrington
Post by Pekka Paalanen
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.
Yes, I'd love to see this, I think it'd solve the need nicely.
Post by Pekka Paalanen
Post by Bryce Harrington
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.
Is my explanation satisfactory enough?
Welll, it didn't change my mind. :-) But the issue is pretty minor, and
not worth blocking on if you want to land it as-is.
Ok, cool. If we get the infra, it's an easy grepping for 'warned' or
even weston_log and assert to find the places that will benefit.


Thanks,
pq
Pekka Paalanen
2015-03-18 13:27:22 UTC
Permalink
This post might be inappropriate. Click to display it.
Derek Foreman
2015-03-18 20:53:33 UTC
Permalink
Post by Pekka Paalanen
This timer delays the output_repaint towards the end of the refresh
period, reducing the time from repaint to present.
The length of the repaint window can be set in weston.ini.
The call to weston_output_schedule_repaint_reset() is delayed by one
more period. If we exit the continuous repaint loop (set
output->repaint_scheduled to false) in finish_frame, we may call
start_repaint_loop() unnecessarily. The problem case was actually
observed with two outputs on the DRM backend at 60 Hz, and 7 ms
repaint-window. During a window move, one output was constantly falling
off the continuous repaint loop and introducing additional one frame
latency, leading to jerky window motion. This code now avoids the
problem.
- Rename repaint_delay_timer to repaint_timer and
output_repaint_delay_handler to output_repaint_timer_handler.
- When computing the delay, take the current time into account. The timer
uses a relative timeout, so we have to subtract any time already gone.
Note, that 'gone' may also be negative. DRM has a habit of predicting
the page flip timestamp so it may be still in the future when we get the
completion event.
- Do also a sanity check 'msec > 1000'. In the unlikely case that
something fails to provide a good timestamp, never delay for more than
one second.
---
man/weston.ini.man | 10 +++++++
src/compositor.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++--------
src/compositor.h | 2 ++
3 files changed, 84 insertions(+), 11 deletions(-)
diff --git a/man/weston.ini.man b/man/weston.ini.man
index 7a353c9..d7df0e4 100644
--- a/man/weston.ini.man
+++ b/man/weston.ini.man
.fi
.RE
.TP 7
+.BI "repaint-window=" N
+Set the approximate length of the repaint window in milliseconds. The repaint
+window is used to control and reduce the output latency for clients. If the
+window is longer than the output refresh period, the repaint will be done
+immediately when the previous repaint finishes, not processing client requests
+in between. If the repaint window is too short, the compositor may miss the
+target vertical blank, increasing output latency. The default value is 7
+milliseconds. The allowed range is from -10 to 1000 milliseconds. Using a
+negative value will force the compositor to always miss the target vblank.
+.TP 7
.BI "gbm-format="format
sets the GBM format used for the framebuffer for the GBM backend. Can be
.B xrgb8888,
diff --git a/src/compositor.c b/src/compositor.c
index e060be1..3f6aa7d 100644
--- a/src/compositor.c
+++ b/src/compositor.c
@@ -62,6 +62,28 @@
#include "git-version.h"
#include "version.h"
+#define DEFAULT_REPAINT_WINDOW 7 /* milliseconds */
+
+#define NSEC_PER_SEC 1000000000
+
+static void
+timespec_sub(struct timespec *r,
+ const struct timespec *a, const struct timespec *b)
+{
+ r->tv_sec = a->tv_sec - b->tv_sec;
+ r->tv_nsec = a->tv_nsec - b->tv_nsec;
+ if (r->tv_nsec < 0) {
+ r->tv_sec--;
+ r->tv_nsec += NSEC_PER_SEC;
+ }
+}
+
+static int64_t
+timespec_to_nsec(const struct timespec *a)
+{
+ return (int64_t)a->tv_sec * NSEC_PER_SEC + a->tv_nsec;
+}
+
static struct wl_list child_process_list;
static struct weston_compositor *segv_compositor;
@@ -2346,19 +2368,38 @@ weston_output_schedule_repaint_reset(struct weston_output *output)
weston_compositor_read_input, compositor);
}
+static int
+output_repaint_timer_handler(void *data)
+{
+ struct weston_output *output = data;
+ struct weston_compositor *compositor = output->compositor;
+
+ if (output->repaint_needed &&
+ compositor->state != WESTON_COMPOSITOR_SLEEPING &&
+ compositor->state != WESTON_COMPOSITOR_OFFSCREEN &&
+ weston_output_repaint(output) == 0)
+ return 0;
+
+ weston_output_schedule_repaint_reset(output);
+
+ return 0;
+}
+
WL_EXPORT void
weston_output_finish_frame(struct weston_output *output,
const struct timespec *stamp,
uint32_t presented_flags)
{
struct weston_compositor *compositor = output->compositor;
- int r;
- uint32_t refresh_nsec;
+ int32_t refresh_nsec;
+ struct timespec now;
+ struct timespec gone;
+ int msec;
TL_POINT("core_repaint_finished", TLP_OUTPUT(output),
TLP_VBLANK(stamp), TLP_END);
- refresh_nsec = 1000000000000UL / output->current_mode->refresh;
+ refresh_nsec = 1000000000000LL / output->current_mode->refresh;
weston_presentation_feedback_present_list(&output->feedback_list,
output, refresh_nsec, stamp,
output->msc,
@@ -2366,15 +2407,16 @@ weston_output_finish_frame(struct weston_output *output,
output->frame_time = stamp->tv_sec * 1000 + stamp->tv_nsec / 1000000;
- if (output->repaint_needed &&
- compositor->state != WESTON_COMPOSITOR_SLEEPING &&
- compositor->state != WESTON_COMPOSITOR_OFFSCREEN) {
- r = weston_output_repaint(output);
- if (!r)
- return;
- }
+ weston_compositor_read_presentation_clock(compositor, &now);
+ timespec_sub(&gone, &now, stamp);
+ msec = (refresh_nsec - timespec_to_nsec(&gone)) / 1000000; /* floor */
+ msec -= compositor->repaint_msec;
- weston_output_schedule_repaint_reset(output);
+ /* Also sanity check. */
+ if (msec < 1 || msec > 1000)
What's the msec > 1000 for? When doing fake presentation clock later I
think this will be a problem. I guess at that point I can make the "too
long" test conditional on test mode being off...

Under normal circumstances maybe we should log (perhaps just the first
time) the > 1000 case, as it seems to indicate something is rather wrong?

This all looks pretty good to me though,
Post by Pekka Paalanen
+ output_repaint_timer_handler(output);
+ else
+ wl_event_source_timer_update(output->repaint_timer, msec);
}
static void
@@ -3873,6 +3915,8 @@ weston_output_destroy(struct weston_output *output)
output->destroying = 1;
+ wl_event_source_remove(output->repaint_timer);
+
weston_presentation_feedback_discard_list(&output->feedback_list);
weston_compositor_remove_output(output->compositor, output);
@@ -4033,6 +4077,8 @@ weston_output_init(struct weston_output *output, struct weston_compositor *c,
int x, int y, int mm_width, int mm_height, uint32_t transform,
int32_t scale)
{
+ struct wl_event_loop *loop;
+
output->compositor = c;
output->x = x;
output->y = y;
@@ -4053,6 +4099,10 @@ weston_output_init(struct weston_output *output, struct weston_compositor *c,
wl_list_init(&output->resource_list);
wl_list_init(&output->feedback_list);
+ loop = wl_display_get_event_loop(c->wl_display);
+ output->repaint_timer = wl_event_loop_add_timer(loop,
+ output_repaint_timer_handler, output);
+
output->id = ffs(~output->compositor->output_id_pool) - 1;
output->compositor->output_id_pool |= 1 << output->id;
@@ -4517,6 +4567,17 @@ weston_compositor_init(struct weston_compositor *ec,
weston_compositor_add_debug_binding(ec, KEY_T,
timeline_key_binding_handler, ec);
+ s = weston_config_get_section(ec->config, "core", NULL, NULL);
+ weston_config_section_get_int(s, "repaint-window", &ec->repaint_msec,
+ DEFAULT_REPAINT_WINDOW);
+ if (ec->repaint_msec < -10 || ec->repaint_msec > 1000) {
+ weston_log("Invalid repaint_window value in config: %d\n",
+ ec->repaint_msec);
+ ec->repaint_msec = DEFAULT_REPAINT_WINDOW;
+ }
+ weston_log("Output repaint window is %d ms maximum.\n",
+ ec->repaint_msec);
+
weston_compositor_schedule_repaint(ec);
return 0;
diff --git a/src/compositor.h b/src/compositor.h
index 2e6ef9d..a451ba3 100644
--- a/src/compositor.h
+++ b/src/compositor.h
@@ -196,6 +196,7 @@ struct weston_output {
pixman_region32_t previous_damage;
int repaint_needed;
int repaint_scheduled;
+ struct wl_event_source *repaint_timer;
struct weston_output_zoom zoom;
int dirty;
struct wl_signal frame_signal;
@@ -683,6 +684,7 @@ struct weston_compositor {
int32_t kb_repeat_delay;
clockid_t presentation_clock;
+ int32_t repaint_msec;
int exit_code;
};
Pekka Paalanen
2015-03-19 10:15:16 UTC
Permalink
On Wed, 18 Mar 2015 15:53:33 -0500
Post by Derek Foreman
Post by Pekka Paalanen
This timer delays the output_repaint towards the end of the refresh
period, reducing the time from repaint to present.
The length of the repaint window can be set in weston.ini.
The call to weston_output_schedule_repaint_reset() is delayed by one
more period. If we exit the continuous repaint loop (set
output->repaint_scheduled to false) in finish_frame, we may call
start_repaint_loop() unnecessarily. The problem case was actually
observed with two outputs on the DRM backend at 60 Hz, and 7 ms
repaint-window. During a window move, one output was constantly falling
off the continuous repaint loop and introducing additional one frame
latency, leading to jerky window motion. This code now avoids the
problem.
- Rename repaint_delay_timer to repaint_timer and
output_repaint_delay_handler to output_repaint_timer_handler.
- When computing the delay, take the current time into account. The timer
uses a relative timeout, so we have to subtract any time already gone.
Note, that 'gone' may also be negative. DRM has a habit of predicting
the page flip timestamp so it may be still in the future when we get the
completion event.
- Do also a sanity check 'msec > 1000'. In the unlikely case that
something fails to provide a good timestamp, never delay for more than
one second.
---
man/weston.ini.man | 10 +++++++
src/compositor.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++--------
src/compositor.h | 2 ++
3 files changed, 84 insertions(+), 11 deletions(-)
diff --git a/man/weston.ini.man b/man/weston.ini.man
index 7a353c9..d7df0e4 100644
--- a/man/weston.ini.man
+++ b/man/weston.ini.man
.fi
.RE
.TP 7
+.BI "repaint-window=" N
+Set the approximate length of the repaint window in milliseconds. The repaint
+window is used to control and reduce the output latency for clients. If the
+window is longer than the output refresh period, the repaint will be done
+immediately when the previous repaint finishes, not processing client requests
+in between. If the repaint window is too short, the compositor may miss the
+target vertical blank, increasing output latency. The default value is 7
+milliseconds. The allowed range is from -10 to 1000 milliseconds. Using a
+negative value will force the compositor to always miss the target vblank.
+.TP 7
.BI "gbm-format="format
sets the GBM format used for the framebuffer for the GBM backend. Can be
.B xrgb8888,
diff --git a/src/compositor.c b/src/compositor.c
index e060be1..3f6aa7d 100644
--- a/src/compositor.c
+++ b/src/compositor.c
@@ -62,6 +62,28 @@
#include "git-version.h"
#include "version.h"
+#define DEFAULT_REPAINT_WINDOW 7 /* milliseconds */
+
+#define NSEC_PER_SEC 1000000000
+
+static void
+timespec_sub(struct timespec *r,
+ const struct timespec *a, const struct timespec *b)
+{
+ r->tv_sec = a->tv_sec - b->tv_sec;
+ r->tv_nsec = a->tv_nsec - b->tv_nsec;
+ if (r->tv_nsec < 0) {
+ r->tv_sec--;
+ r->tv_nsec += NSEC_PER_SEC;
+ }
+}
+
+static int64_t
+timespec_to_nsec(const struct timespec *a)
+{
+ return (int64_t)a->tv_sec * NSEC_PER_SEC + a->tv_nsec;
+}
+
static struct wl_list child_process_list;
static struct weston_compositor *segv_compositor;
@@ -2346,19 +2368,38 @@ weston_output_schedule_repaint_reset(struct weston_output *output)
weston_compositor_read_input, compositor);
}
+static int
+output_repaint_timer_handler(void *data)
+{
+ struct weston_output *output = data;
+ struct weston_compositor *compositor = output->compositor;
+
+ if (output->repaint_needed &&
+ compositor->state != WESTON_COMPOSITOR_SLEEPING &&
+ compositor->state != WESTON_COMPOSITOR_OFFSCREEN &&
+ weston_output_repaint(output) == 0)
+ return 0;
+
+ weston_output_schedule_repaint_reset(output);
+
+ return 0;
+}
+
WL_EXPORT void
weston_output_finish_frame(struct weston_output *output,
const struct timespec *stamp,
uint32_t presented_flags)
{
struct weston_compositor *compositor = output->compositor;
- int r;
- uint32_t refresh_nsec;
+ int32_t refresh_nsec;
+ struct timespec now;
+ struct timespec gone;
+ int msec;
TL_POINT("core_repaint_finished", TLP_OUTPUT(output),
TLP_VBLANK(stamp), TLP_END);
- refresh_nsec = 1000000000000UL / output->current_mode->refresh;
+ refresh_nsec = 1000000000000LL / output->current_mode->refresh;
weston_presentation_feedback_present_list(&output->feedback_list,
output, refresh_nsec, stamp,
output->msc,
@@ -2366,15 +2407,16 @@ weston_output_finish_frame(struct weston_output *output,
output->frame_time = stamp->tv_sec * 1000 + stamp->tv_nsec / 1000000;
- if (output->repaint_needed &&
- compositor->state != WESTON_COMPOSITOR_SLEEPING &&
- compositor->state != WESTON_COMPOSITOR_OFFSCREEN) {
- r = weston_output_repaint(output);
- if (!r)
- return;
- }
+ weston_compositor_read_presentation_clock(compositor, &now);
+ timespec_sub(&gone, &now, stamp);
+ msec = (refresh_nsec - timespec_to_nsec(&gone)) / 1000000; /* floor */
+ msec -= compositor->repaint_msec;
- weston_output_schedule_repaint_reset(output);
+ /* Also sanity check. */
+ if (msec < 1 || msec > 1000)
What's the msec > 1000 for? When doing fake presentation clock later I
think this will be a problem. I guess at that point I can make the "too
long" test conditional on test mode being off...
That's half of the sanity check in case something return a bogus time
value which might make us wait for days.

Just don't advance the presentation clock too much at a time? I think a
bigger problem will be the timer which you have to fudge into honouring
the fake clock.
Post by Derek Foreman
Under normal circumstances maybe we should log (perhaps just the first
time) the > 1000 case, as it seems to indicate something is rather wrong?
Yeah, logging would be ok. I'll add that in a follow-up patch so you
don't have to read the rest again.


Thanks,
pq
Post by Derek Foreman
This all looks pretty good to me though,
Post by Pekka Paalanen
+ output_repaint_timer_handler(output);
+ else
+ wl_event_source_timer_update(output->repaint_timer, msec);
}
static void
@@ -3873,6 +3915,8 @@ weston_output_destroy(struct weston_output *output)
output->destroying = 1;
+ wl_event_source_remove(output->repaint_timer);
+
weston_presentation_feedback_discard_list(&output->feedback_list);
weston_compositor_remove_output(output->compositor, output);
@@ -4033,6 +4077,8 @@ weston_output_init(struct weston_output *output, struct weston_compositor *c,
int x, int y, int mm_width, int mm_height, uint32_t transform,
int32_t scale)
{
+ struct wl_event_loop *loop;
+
output->compositor = c;
output->x = x;
output->y = y;
@@ -4053,6 +4099,10 @@ weston_output_init(struct weston_output *output, struct weston_compositor *c,
wl_list_init(&output->resource_list);
wl_list_init(&output->feedback_list);
+ loop = wl_display_get_event_loop(c->wl_display);
+ output->repaint_timer = wl_event_loop_add_timer(loop,
+ output_repaint_timer_handler, output);
+
output->id = ffs(~output->compositor->output_id_pool) - 1;
output->compositor->output_id_pool |= 1 << output->id;
@@ -4517,6 +4567,17 @@ weston_compositor_init(struct weston_compositor *ec,
weston_compositor_add_debug_binding(ec, KEY_T,
timeline_key_binding_handler, ec);
+ s = weston_config_get_section(ec->config, "core", NULL, NULL);
+ weston_config_section_get_int(s, "repaint-window", &ec->repaint_msec,
+ DEFAULT_REPAINT_WINDOW);
+ if (ec->repaint_msec < -10 || ec->repaint_msec > 1000) {
+ weston_log("Invalid repaint_window value in config: %d\n",
+ ec->repaint_msec);
+ ec->repaint_msec = DEFAULT_REPAINT_WINDOW;
+ }
+ weston_log("Output repaint window is %d ms maximum.\n",
+ ec->repaint_msec);
+
weston_compositor_schedule_repaint(ec);
return 0;
diff --git a/src/compositor.h b/src/compositor.h
index 2e6ef9d..a451ba3 100644
--- a/src/compositor.h
+++ b/src/compositor.h
@@ -196,6 +196,7 @@ struct weston_output {
pixman_region32_t previous_damage;
int repaint_needed;
int repaint_scheduled;
+ struct wl_event_source *repaint_timer;
struct weston_output_zoom zoom;
int dirty;
struct wl_signal frame_signal;
@@ -683,6 +684,7 @@ struct weston_compositor {
int32_t kb_repeat_delay;
clockid_t presentation_clock;
+ int32_t repaint_msec;
int exit_code;
};
Pekka Paalanen
2015-03-19 10:33:15 UTC
Permalink
From: Pekka Paalanen <***@collabora.co.uk>

Make the sanity check more explicit and log a warning if it happens.

Small negative values are ok because it just means the compositor is
lagging behind, or more likely the user specified a too long repaint
window.

Signed-off-by: Pekka Paalanen <***@collabora.co.uk>
---
src/compositor.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/src/compositor.c b/src/compositor.c
index 3f6aa7d..be2309a 100644
--- a/src/compositor.c
+++ b/src/compositor.c
@@ -2412,8 +2412,18 @@ weston_output_finish_frame(struct weston_output *output,
msec = (refresh_nsec - timespec_to_nsec(&gone)) / 1000000; /* floor */
msec -= compositor->repaint_msec;

- /* Also sanity check. */
- if (msec < 1 || msec > 1000)
+ if (msec < -1000 || msec > 1000) {
+ static bool warned;
+
+ if (!warned)
+ weston_log("Warning: computed repaint delay is "
+ "insane: %d msec\n", msec);
+ warned = true;
+
+ msec = 0;
+ }
+
+ if (msec < 1)
output_repaint_timer_handler(output);
else
wl_event_source_timer_update(output->repaint_timer, msec);
--
2.0.5
Derek Foreman
2015-03-19 13:13:42 UTC
Permalink
Post by Pekka Paalanen
Make the sanity check more explicit and log a warning if it happens.
Small negative values are ok because it just means the compositor is
lagging behind, or more likely the user specified a too long repaint
window.
---
src/compositor.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/compositor.c b/src/compositor.c
index 3f6aa7d..be2309a 100644
--- a/src/compositor.c
+++ b/src/compositor.c
@@ -2412,8 +2412,18 @@ weston_output_finish_frame(struct weston_output *output,
msec = (refresh_nsec - timespec_to_nsec(&gone)) / 1000000; /* floor */
msec -= compositor->repaint_msec;
- /* Also sanity check. */
- if (msec < 1 || msec > 1000)
+ if (msec < -1000 || msec > 1000) {
+ static bool warned;
+
+ if (!warned)
+ weston_log("Warning: computed repaint delay is "
+ "insane: %d msec\n", msec);
+ warned = true;
+
+ msec = 0;
+ }
+
+ if (msec < 1)
output_repaint_timer_handler(output);
else
wl_event_source_timer_update(output->repaint_timer, msec);
Bryce Harrington
2015-03-19 18:16:33 UTC
Permalink
Post by Pekka Paalanen
Post by Pekka Paalanen
Make the sanity check more explicit and log a warning if it happens.
Small negative values are ok because it just means the compositor is
lagging behind, or more likely the user specified a too long repaint
window.
---
src/compositor.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/compositor.c b/src/compositor.c
index 3f6aa7d..be2309a 100644
--- a/src/compositor.c
+++ b/src/compositor.c
@@ -2412,8 +2412,18 @@ weston_output_finish_frame(struct weston_output *output,
msec = (refresh_nsec - timespec_to_nsec(&gone)) / 1000000; /* floor */
msec -= compositor->repaint_msec;
- /* Also sanity check. */
- if (msec < 1 || msec > 1000)
+ if (msec < -1000 || msec > 1000) {
+ static bool warned;
+
+ if (!warned)
+ weston_log("Warning: computed repaint delay is "
+ "insane: %d msec\n", msec);
+ warned = true;
+
+ msec = 0;
+ }
+
+ if (msec < 1)
output_repaint_timer_handler(output);
else
wl_event_source_timer_update(output->repaint_timer, msec);
_______________________________________________
wayland-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Pekka Paalanen
2015-03-20 10:22:17 UTC
Permalink
On Thu, 19 Mar 2015 11:16:33 -0700
Post by Pekka Paalanen
Make the sanity check more explicit and log a warning if it happens.
Small negative values are ok because it just means the compositor is
lagging behind, or more likely the user specified a too long repaint
window.
All three pushed:
b407024..8fd4de4 master -> master


Thanks,
pq

Bill Spitzak
2015-03-18 21:44:52 UTC
Permalink
Would it be possible to compute this repaint window as how long it took
to paint the previous frame, plus some small user-adjustable constant?
Or a more complex weighted average of recent frames?

I'm not sure if perhaps the necessary clock information is not
available, or that this idea would mess up the presentation feedback.
Post by Pekka Paalanen
This timer delays the output_repaint towards the end of the refresh
period, reducing the time from repaint to present.
The length of the repaint window can be set in weston.ini.
Pekka Paalanen
2015-03-19 10:07:51 UTC
Permalink
On Wed, 18 Mar 2015 14:44:52 -0700
Post by Bill Spitzak
Would it be possible to compute this repaint window as how long it took
to paint the previous frame, plus some small user-adjustable constant?
Or a more complex weighted average of recent frames?
I'm not sure if perhaps the necessary clock information is not
available, or that this idea would mess up the presentation feedback.
Currently we don't know when the frame compositing completes and how
much is just waiting for the next vblank to flip it on screen. Maybe we
will have a nice way in the future. Right now all we have AFAIK is
glFlush which simply does not do what we need, and glFinish which is a
blocking call so we'd need a thread to run it which requires moving all
EGL/GL calls into that thread basically and oh god the horror.

This topic was touched in the blog post comments at
http://ppaalanen.blogspot.fi/2015/02/weston-repaint-scheduling.html

Thanks,
pq
Post by Bill Spitzak
Post by Pekka Paalanen
This timer delays the output_repaint towards the end of the refresh
period, reducing the time from repaint to present.
The length of the repaint window can be set in weston.ini.
Bryce Harrington
2015-03-18 21:45:34 UTC
Permalink
Post by Pekka Paalanen
This timer delays the output_repaint towards the end of the refresh
period, reducing the time from repaint to present.
The length of the repaint window can be set in weston.ini.
The call to weston_output_schedule_repaint_reset() is delayed by one
more period. If we exit the continuous repaint loop (set
output->repaint_scheduled to false) in finish_frame, we may call
start_repaint_loop() unnecessarily. The problem case was actually
observed with two outputs on the DRM backend at 60 Hz, and 7 ms
repaint-window. During a window move, one output was constantly falling
off the continuous repaint loop and introducing additional one frame
latency, leading to jerky window motion. This code now avoids the
problem.
- Rename repaint_delay_timer to repaint_timer and
output_repaint_delay_handler to output_repaint_timer_handler.
- When computing the delay, take the current time into account. The timer
uses a relative timeout, so we have to subtract any time already gone.
Note, that 'gone' may also be negative. DRM has a habit of predicting
the page flip timestamp so it may be still in the future when we get the
completion event.
- Do also a sanity check 'msec > 1000'. In the unlikely case that
something fails to provide a good timestamp, never delay for more than
one second.
Integer 32 math with numbers up in the billions kind of worries me about
out-of-range errors, but the math all appears to work out. Might be
nice to have tests for timespec_sub and timespec_to_nsec anyway just to
Post by Pekka Paalanen
---
man/weston.ini.man | 10 +++++++
src/compositor.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++--------
src/compositor.h | 2 ++
3 files changed, 84 insertions(+), 11 deletions(-)
diff --git a/man/weston.ini.man b/man/weston.ini.man
index 7a353c9..d7df0e4 100644
--- a/man/weston.ini.man
+++ b/man/weston.ini.man
.fi
.RE
.TP 7
+.BI "repaint-window=" N
+Set the approximate length of the repaint window in milliseconds. The repaint
+window is used to control and reduce the output latency for clients. If the
+window is longer than the output refresh period, the repaint will be done
+immediately when the previous repaint finishes, not processing client requests
+in between. If the repaint window is too short, the compositor may miss the
+target vertical blank, increasing output latency. The default value is 7
+milliseconds. The allowed range is from -10 to 1000 milliseconds. Using a
+negative value will force the compositor to always miss the target vblank.
+.TP 7
.BI "gbm-format="format
sets the GBM format used for the framebuffer for the GBM backend. Can be
.B xrgb8888,
diff --git a/src/compositor.c b/src/compositor.c
index e060be1..3f6aa7d 100644
--- a/src/compositor.c
+++ b/src/compositor.c
@@ -62,6 +62,28 @@
#include "git-version.h"
#include "version.h"
+#define DEFAULT_REPAINT_WINDOW 7 /* milliseconds */
+
+#define NSEC_PER_SEC 1000000000
+
+static void
+timespec_sub(struct timespec *r,
+ const struct timespec *a, const struct timespec *b)
+{
+ r->tv_sec = a->tv_sec - b->tv_sec;
+ r->tv_nsec = a->tv_nsec - b->tv_nsec;
+ if (r->tv_nsec < 0) {
+ r->tv_sec--;
+ r->tv_nsec += NSEC_PER_SEC;
+ }
+}
+
+static int64_t
+timespec_to_nsec(const struct timespec *a)
+{
+ return (int64_t)a->tv_sec * NSEC_PER_SEC + a->tv_nsec;
+}
+
static struct wl_list child_process_list;
static struct weston_compositor *segv_compositor;
@@ -2346,19 +2368,38 @@ weston_output_schedule_repaint_reset(struct weston_output *output)
weston_compositor_read_input, compositor);
}
+static int
+output_repaint_timer_handler(void *data)
+{
+ struct weston_output *output = data;
+ struct weston_compositor *compositor = output->compositor;
+
+ if (output->repaint_needed &&
+ compositor->state != WESTON_COMPOSITOR_SLEEPING &&
+ compositor->state != WESTON_COMPOSITOR_OFFSCREEN &&
+ weston_output_repaint(output) == 0)
+ return 0;
+
+ weston_output_schedule_repaint_reset(output);
+
+ return 0;
+}
+
WL_EXPORT void
weston_output_finish_frame(struct weston_output *output,
const struct timespec *stamp,
uint32_t presented_flags)
{
struct weston_compositor *compositor = output->compositor;
- int r;
- uint32_t refresh_nsec;
+ int32_t refresh_nsec;
+ struct timespec now;
+ struct timespec gone;
+ int msec;
TL_POINT("core_repaint_finished", TLP_OUTPUT(output),
TLP_VBLANK(stamp), TLP_END);
- refresh_nsec = 1000000000000UL / output->current_mode->refresh;
+ refresh_nsec = 1000000000000LL / output->current_mode->refresh;
weston_presentation_feedback_present_list(&output->feedback_list,
output, refresh_nsec, stamp,
output->msc,
@@ -2366,15 +2407,16 @@ weston_output_finish_frame(struct weston_output *output,
output->frame_time = stamp->tv_sec * 1000 + stamp->tv_nsec / 1000000;
- if (output->repaint_needed &&
- compositor->state != WESTON_COMPOSITOR_SLEEPING &&
- compositor->state != WESTON_COMPOSITOR_OFFSCREEN) {
- r = weston_output_repaint(output);
- if (!r)
- return;
- }
+ weston_compositor_read_presentation_clock(compositor, &now);
+ timespec_sub(&gone, &now, stamp);
+ msec = (refresh_nsec - timespec_to_nsec(&gone)) / 1000000; /* floor */
+ msec -= compositor->repaint_msec;
- weston_output_schedule_repaint_reset(output);
+ /* Also sanity check. */
+ if (msec < 1 || msec > 1000)
+ output_repaint_timer_handler(output);
+ else
+ wl_event_source_timer_update(output->repaint_timer, msec);
}
static void
@@ -3873,6 +3915,8 @@ weston_output_destroy(struct weston_output *output)
output->destroying = 1;
+ wl_event_source_remove(output->repaint_timer);
+
weston_presentation_feedback_discard_list(&output->feedback_list);
weston_compositor_remove_output(output->compositor, output);
@@ -4033,6 +4077,8 @@ weston_output_init(struct weston_output *output, struct weston_compositor *c,
int x, int y, int mm_width, int mm_height, uint32_t transform,
int32_t scale)
{
+ struct wl_event_loop *loop;
+
output->compositor = c;
output->x = x;
output->y = y;
@@ -4053,6 +4099,10 @@ weston_output_init(struct weston_output *output, struct weston_compositor *c,
wl_list_init(&output->resource_list);
wl_list_init(&output->feedback_list);
+ loop = wl_display_get_event_loop(c->wl_display);
+ output->repaint_timer = wl_event_loop_add_timer(loop,
+ output_repaint_timer_handler, output);
+
output->id = ffs(~output->compositor->output_id_pool) - 1;
output->compositor->output_id_pool |= 1 << output->id;
@@ -4517,6 +4567,17 @@ weston_compositor_init(struct weston_compositor *ec,
weston_compositor_add_debug_binding(ec, KEY_T,
timeline_key_binding_handler, ec);
+ s = weston_config_get_section(ec->config, "core", NULL, NULL);
+ weston_config_section_get_int(s, "repaint-window", &ec->repaint_msec,
+ DEFAULT_REPAINT_WINDOW);
+ if (ec->repaint_msec < -10 || ec->repaint_msec > 1000) {
+ weston_log("Invalid repaint_window value in config: %d\n",
+ ec->repaint_msec);
+ ec->repaint_msec = DEFAULT_REPAINT_WINDOW;
+ }
+ weston_log("Output repaint window is %d ms maximum.\n",
+ ec->repaint_msec);
+
weston_compositor_schedule_repaint(ec);
return 0;
diff --git a/src/compositor.h b/src/compositor.h
index 2e6ef9d..a451ba3 100644
--- a/src/compositor.h
+++ b/src/compositor.h
@@ -196,6 +196,7 @@ struct weston_output {
pixman_region32_t previous_damage;
int repaint_needed;
int repaint_scheduled;
+ struct wl_event_source *repaint_timer;
struct weston_output_zoom zoom;
int dirty;
struct wl_signal frame_signal;
@@ -683,6 +684,7 @@ struct weston_compositor {
int32_t kb_repeat_delay;
clockid_t presentation_clock;
+ int32_t repaint_msec;
int exit_code;
};
--
2.0.5
_______________________________________________
wayland-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Pekka Paalanen
2015-03-19 10:01:08 UTC
Permalink
On Wed, 18 Mar 2015 14:45:34 -0700
Post by Bryce Harrington
Post by Pekka Paalanen
This timer delays the output_repaint towards the end of the refresh
period, reducing the time from repaint to present.
The length of the repaint window can be set in weston.ini.
The call to weston_output_schedule_repaint_reset() is delayed by one
more period. If we exit the continuous repaint loop (set
output->repaint_scheduled to false) in finish_frame, we may call
start_repaint_loop() unnecessarily. The problem case was actually
observed with two outputs on the DRM backend at 60 Hz, and 7 ms
repaint-window. During a window move, one output was constantly falling
off the continuous repaint loop and introducing additional one frame
latency, leading to jerky window motion. This code now avoids the
problem.
- Rename repaint_delay_timer to repaint_timer and
output_repaint_delay_handler to output_repaint_timer_handler.
- When computing the delay, take the current time into account. The timer
uses a relative timeout, so we have to subtract any time already gone.
Note, that 'gone' may also be negative. DRM has a habit of predicting
the page flip timestamp so it may be still in the future when we get the
completion event.
- Do also a sanity check 'msec > 1000'. In the unlikely case that
something fails to provide a good timestamp, never delay for more than
one second.
Integer 32 math with numbers up in the billions kind of worries me about
out-of-range errors, but the math all appears to work out. Might be
nice to have tests for timespec_sub and timespec_to_nsec anyway just to
Yeah, I lifted timespec_sub() from wesgr. There I have much more
rigorous testing of timestamp validity. tv_nsec is of type 'long' which
may be 32-bit (tv_sec *might* be 32-bit too, we have no guarantees).
For valid timestamps, tv_nsec is always in the range [0, 999999999] so
the math is safe for valid timestamps, and produces valid timestamps.

I've been thinking of putting all timespec helpers in a new shared file
with tests and exact documentation, but there isn't enough of them yet.


Thanks,
pq
Post by Bryce Harrington
Post by Pekka Paalanen
---
man/weston.ini.man | 10 +++++++
src/compositor.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++--------
src/compositor.h | 2 ++
3 files changed, 84 insertions(+), 11 deletions(-)
diff --git a/man/weston.ini.man b/man/weston.ini.man
index 7a353c9..d7df0e4 100644
--- a/man/weston.ini.man
+++ b/man/weston.ini.man
.fi
.RE
.TP 7
+.BI "repaint-window=" N
+Set the approximate length of the repaint window in milliseconds. The repaint
+window is used to control and reduce the output latency for clients. If the
+window is longer than the output refresh period, the repaint will be done
+immediately when the previous repaint finishes, not processing client requests
+in between. If the repaint window is too short, the compositor may miss the
+target vertical blank, increasing output latency. The default value is 7
+milliseconds. The allowed range is from -10 to 1000 milliseconds. Using a
+negative value will force the compositor to always miss the target vblank.
+.TP 7
.BI "gbm-format="format
sets the GBM format used for the framebuffer for the GBM backend. Can be
.B xrgb8888,
diff --git a/src/compositor.c b/src/compositor.c
index e060be1..3f6aa7d 100644
--- a/src/compositor.c
+++ b/src/compositor.c
@@ -62,6 +62,28 @@
#include "git-version.h"
#include "version.h"
+#define DEFAULT_REPAINT_WINDOW 7 /* milliseconds */
+
+#define NSEC_PER_SEC 1000000000
+
+static void
+timespec_sub(struct timespec *r,
+ const struct timespec *a, const struct timespec *b)
+{
+ r->tv_sec = a->tv_sec - b->tv_sec;
+ r->tv_nsec = a->tv_nsec - b->tv_nsec;
+ if (r->tv_nsec < 0) {
+ r->tv_sec--;
+ r->tv_nsec += NSEC_PER_SEC;
+ }
+}
+
+static int64_t
+timespec_to_nsec(const struct timespec *a)
+{
+ return (int64_t)a->tv_sec * NSEC_PER_SEC + a->tv_nsec;
+}
+
static struct wl_list child_process_list;
static struct weston_compositor *segv_compositor;
@@ -2346,19 +2368,38 @@ weston_output_schedule_repaint_reset(struct weston_output *output)
weston_compositor_read_input, compositor);
}
+static int
+output_repaint_timer_handler(void *data)
+{
+ struct weston_output *output = data;
+ struct weston_compositor *compositor = output->compositor;
+
+ if (output->repaint_needed &&
+ compositor->state != WESTON_COMPOSITOR_SLEEPING &&
+ compositor->state != WESTON_COMPOSITOR_OFFSCREEN &&
+ weston_output_repaint(output) == 0)
+ return 0;
+
+ weston_output_schedule_repaint_reset(output);
+
+ return 0;
+}
+
WL_EXPORT void
weston_output_finish_frame(struct weston_output *output,
const struct timespec *stamp,
uint32_t presented_flags)
{
struct weston_compositor *compositor = output->compositor;
- int r;
- uint32_t refresh_nsec;
+ int32_t refresh_nsec;
+ struct timespec now;
+ struct timespec gone;
+ int msec;
TL_POINT("core_repaint_finished", TLP_OUTPUT(output),
TLP_VBLANK(stamp), TLP_END);
- refresh_nsec = 1000000000000UL / output->current_mode->refresh;
+ refresh_nsec = 1000000000000LL / output->current_mode->refresh;
weston_presentation_feedback_present_list(&output->feedback_list,
output, refresh_nsec, stamp,
output->msc,
@@ -2366,15 +2407,16 @@ weston_output_finish_frame(struct weston_output *output,
output->frame_time = stamp->tv_sec * 1000 + stamp->tv_nsec / 1000000;
- if (output->repaint_needed &&
- compositor->state != WESTON_COMPOSITOR_SLEEPING &&
- compositor->state != WESTON_COMPOSITOR_OFFSCREEN) {
- r = weston_output_repaint(output);
- if (!r)
- return;
- }
+ weston_compositor_read_presentation_clock(compositor, &now);
+ timespec_sub(&gone, &now, stamp);
+ msec = (refresh_nsec - timespec_to_nsec(&gone)) / 1000000; /* floor */
+ msec -= compositor->repaint_msec;
- weston_output_schedule_repaint_reset(output);
+ /* Also sanity check. */
+ if (msec < 1 || msec > 1000)
+ output_repaint_timer_handler(output);
+ else
+ wl_event_source_timer_update(output->repaint_timer, msec);
}
static void
@@ -3873,6 +3915,8 @@ weston_output_destroy(struct weston_output *output)
output->destroying = 1;
+ wl_event_source_remove(output->repaint_timer);
+
weston_presentation_feedback_discard_list(&output->feedback_list);
weston_compositor_remove_output(output->compositor, output);
@@ -4033,6 +4077,8 @@ weston_output_init(struct weston_output *output, struct weston_compositor *c,
int x, int y, int mm_width, int mm_height, uint32_t transform,
int32_t scale)
{
+ struct wl_event_loop *loop;
+
output->compositor = c;
output->x = x;
output->y = y;
@@ -4053,6 +4099,10 @@ weston_output_init(struct weston_output *output, struct weston_compositor *c,
wl_list_init(&output->resource_list);
wl_list_init(&output->feedback_list);
+ loop = wl_display_get_event_loop(c->wl_display);
+ output->repaint_timer = wl_event_loop_add_timer(loop,
+ output_repaint_timer_handler, output);
+
output->id = ffs(~output->compositor->output_id_pool) - 1;
output->compositor->output_id_pool |= 1 << output->id;
@@ -4517,6 +4567,17 @@ weston_compositor_init(struct weston_compositor *ec,
weston_compositor_add_debug_binding(ec, KEY_T,
timeline_key_binding_handler, ec);
+ s = weston_config_get_section(ec->config, "core", NULL, NULL);
+ weston_config_section_get_int(s, "repaint-window", &ec->repaint_msec,
+ DEFAULT_REPAINT_WINDOW);
+ if (ec->repaint_msec < -10 || ec->repaint_msec > 1000) {
+ weston_log("Invalid repaint_window value in config: %d\n",
+ ec->repaint_msec);
+ ec->repaint_msec = DEFAULT_REPAINT_WINDOW;
+ }
+ weston_log("Output repaint window is %d ms maximum.\n",
+ ec->repaint_msec);
+
weston_compositor_schedule_repaint(ec);
return 0;
diff --git a/src/compositor.h b/src/compositor.h
index 2e6ef9d..a451ba3 100644
--- a/src/compositor.h
+++ b/src/compositor.h
@@ -196,6 +196,7 @@ struct weston_output {
pixman_region32_t previous_damage;
int repaint_needed;
int repaint_scheduled;
+ struct wl_event_source *repaint_timer;
struct weston_output_zoom zoom;
int dirty;
struct wl_signal frame_signal;
@@ -683,6 +684,7 @@ struct weston_compositor {
int32_t kb_repeat_delay;
clockid_t presentation_clock;
+ int32_t repaint_msec;
int exit_code;
};
--
2.0.5
_______________________________________________
wayland-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Loading...