Re: [PATCH xserver] xwayland: add envvar XWAYLAND_NO_GLAMOR

[Eric Engestrom]

On Wednesday, 2017-03-01 17:45:12 +0100, Olivier Fourdan wrote:
> Not all compositors allow for customizing the Xwayland command line,
> gnome-shell/mutter for example have the command line and path to
> Xwayland binary hardcoded, which makes it harder for users to disable
> glamor acceleration in Xwayland (glamor being used by default).
> 
> Add an environment variable XWAYLAND_NO_GLAMOR t odiable glamor support

"to disable"

The change itself looks good to me.
Reviewed-by: Eric Engestrom 

As to whether it's a good idea to allow this, I'd say it is, but you
might want the opinion of someone who's more involved in this project :)

Cheers,
  Eric

> in Xwayland.
> 
> Signed-off-by: Olivier Fourdan 
> ---
>  hw/xwayland/xwayland-glamor.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/xwayland/xwayland-glamor.c b/hw/xwayland/xwayland-glamor.c
> index b3d0aab..45de54f 100644
> --- a/hw/xwayland/xwayland-glamor.c
> +++ b/hw/xwayland/xwayland-glamor.c
> @@ -549,6 +549,13 @@ Bool
>  xwl_glamor_init(struct xwl_screen *xwl_screen)
>  {
>  ScreenPtr screen = xwl_screen->screen;
> +const char *no_glamor_env;
> +
> +no_glamor_env = getenv("XWAYLAND_NO_GLAMOR");
> +if (no_glamor_env && *no_glamor_env != '0') {

Nit: `XWAYLAND_NO_GLAMOR=` evaluates to true here, disabling glamor.
Not sure how much we care about this case though, or if we do want this
behaviour, but this would fix it:

if (no_glamor_env && *no_glamor_env && *no_glamor_env != '0') {

> +ErrorF("Disabling glamor and dri3 support, XWAYLAND_NO_GLAMOR is 
> set\n");
> +return FALSE;
> +}
>  
>  if (xwl_screen->egl_context == EGL_NO_CONTEXT) {
>  ErrorF("Disabling glamor and dri3, EGL setup failed\n");
> -- 
> 2.9.3
> 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH xserver] xwayland: add envvar XWAYLAND_NO_GLAMOR

[Olivier Fourdan]

Not all compositors allow for customizing the Xwayland command line,
gnome-shell/mutter for example have the command line and path to
Xwayland binary hardcoded, which makes it harder for users to disable
glamor acceleration in Xwayland (glamor being used by default).

Add an environment variable XWAYLAND_NO_GLAMOR t odiable glamor support
in Xwayland.

Signed-off-by: Olivier Fourdan 
---
 hw/xwayland/xwayland-glamor.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/hw/xwayland/xwayland-glamor.c b/hw/xwayland/xwayland-glamor.c
index b3d0aab..45de54f 100644
--- a/hw/xwayland/xwayland-glamor.c
+++ b/hw/xwayland/xwayland-glamor.c
@@ -549,6 +549,13 @@ Bool
 xwl_glamor_init(struct xwl_screen *xwl_screen)
 {
 ScreenPtr screen = xwl_screen->screen;
+const char *no_glamor_env;
+
+no_glamor_env = getenv("XWAYLAND_NO_GLAMOR");
+if (no_glamor_env && *no_glamor_env != '0') {
+ErrorF("Disabling glamor and dri3 support, XWAYLAND_NO_GLAMOR is 
set\n");
+return FALSE;
+}
 
 if (xwl_screen->egl_context == EGL_NO_CONTEXT) {
 ErrorF("Disabling glamor and dri3, EGL setup failed\n");
-- 
2.9.3

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[RFC PATCH xserver] xwayland: RFC Disable glamor with an env var?

[Olivier Fourdan]

Hi all,

I am seeing quite a few Xwayland crashes related to glamor.

Various issues, could be with glamor itself or with the drivers (like the
issues I witness with nv30), whatever.

To investigate those issues (or even unblock affected users) it's often
useful to disable glamor -even temporarily- but Xwayland doesn't use an
xorg.conf and relies solely on the command line.

Not all compositors allow for customizing the command line, gnome-shell
or mutter for example have the command line and path to the Xwayland binary
hardcoded, which makes it harder for users to disable glamor acceleration
in Xwayland by adding -shm to the command line (glamor being used by
default).

Would adding an environment variable such as XWAYLAND_NO_GLAMOR (similar
to what Xephyr does with e.g. XEPHYR_NO_SHM) make sense here? 

I tried and it works with the following patch added, by setting the
environment variable XWAYLAND_NO_GLAMOR in a /etc/profile.d/ script on
Fedora 25.

Cheers,
Olivier

Olivier Fourdan (1):
  xwayland: add envvar XWAYLAND_NO_GLAMOR

 hw/xwayland/xwayland-glamor.c | 7 +++
 1 file changed, 7 insertions(+)

-- 
2.9.3

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston v2 10/11] [RFC] Account for very large repaint window misses

[Daniel Stone]

At the bottom of weston_output_finish_frame(), code exists to account
for flips which have missed the repaint window, by shifting them to lock
on to the next repaint window rather than repainting immediately.

This code only accounted for flips which missed their target by one
repaint window. If they miss by multiples of the repaint window, adjust
them until the next repaint timestamp is in the future.

I was unable to find from discussion of the original development whether
or not this is desirable; I can see an argument both ways. Hence, RFC
tag.

Signed-off-by: Daniel Stone 
Cc: Pekka Paalanen 
Cc: Mario Kleiner 
---
 libweston/compositor.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

v2: New in this revision. Unsure if necessary or not.

diff --git a/libweston/compositor.c b/libweston/compositor.c
index a64e695..511f85a 100644
--- a/libweston/compositor.c
+++ b/libweston/compositor.c
@@ -2485,9 +2485,14 @@ weston_output_finish_frame(struct weston_output *output,
 * the deadline given by repaint_msec? In that case we delay until
 * the deadline of the next frame, to give clients a more predictable
 * timing of the repaint cycle to lock on. */
-   if (presented_flags == WP_PRESENTATION_FEEDBACK_INVALID && msec_rel < 0)
-   timespec_add_nsec(>next_repaint, >next_repaint,
- refresh_nsec);
+   if (presented_flags == WP_PRESENTATION_FEEDBACK_INVALID &&
+   msec_rel < 0) {
+   while (timespec_sub_to_nsec(>next_repaint, ) < 0) {
+   timespec_add_nsec(>next_repaint,
+ >next_repaint,
+ refresh_nsec);
+   }
+   }
 
 out:
output->repaint_status = REPAINT_SCHEDULED;
-- 
2.9.3

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston v2 03/11] timespec: Add timespec_to_msec helper

[Daniel Stone]

Paralleling timespec_to_nsec, converts to milliseconds.

Signed-off-by: Daniel Stone 
Reviewed-by: Pekka Paalanen 
---
 libweston/compositor.c |  2 +-
 shared/timespec-util.h | 11 +++
 tests/timespec-test.c  |  9 +
 3 files changed, 21 insertions(+), 1 deletion(-)

v2: No changes.

diff --git a/libweston/compositor.c b/libweston/compositor.c
index 813f701..a28738e 100644
--- a/libweston/compositor.c
+++ b/libweston/compositor.c
@@ -2388,7 +2388,7 @@ weston_output_finish_frame(struct weston_output *output,
  output->msc,
  presented_flags);
 
-   output->frame_time = stamp->tv_sec * 1000 + stamp->tv_nsec / 100;
+   output->frame_time = timespec_to_msec(stamp);
 
weston_compositor_read_presentation_clock(compositor, );
timespec_sub(, , stamp);
diff --git a/shared/timespec-util.h b/shared/timespec-util.h
index 13948b1..c8e3cd6 100644
--- a/shared/timespec-util.h
+++ b/shared/timespec-util.h
@@ -93,6 +93,17 @@ timespec_to_nsec(const struct timespec *a)
return (int64_t)a->tv_sec * NSEC_PER_SEC + a->tv_nsec;
 }
 
+/* Convert timespec to milliseconds
+ *
+ * \param a timespec
+ * \return milliseconds
+ */
+static inline int64_t
+timespec_to_msec(const struct timespec *a)
+{
+   return (int64_t)a->tv_sec * 1000 + a->tv_nsec / 100;
+}
+
 /* Convert milli-Hertz to nanoseconds
  *
  * \param mhz frequency in mHz, not zero
diff --git a/tests/timespec-test.c b/tests/timespec-test.c
index cd3b1c1..712d1ac 100644
--- a/tests/timespec-test.c
+++ b/tests/timespec-test.c
@@ -60,6 +60,15 @@ ZUC_TEST(timespec_test, timespec_to_nsec)
ZUC_ASSERT_EQ(timespec_to_nsec(), (NSEC_PER_SEC * 4ULL) + 4);
 }
 
+ZUC_TEST(timespec_test, timespec_to_msec)
+{
+   struct timespec a;
+
+   a.tv_sec = 4;
+   a.tv_nsec = 400;
+   ZUC_ASSERT_EQ(timespec_to_msec(), (4000ULL) + 4);
+}
+
 ZUC_TEST(timespec_test, millihz_to_nsec)
 {
ZUC_ASSERT_EQ(millihz_to_nsec(6), 1666);
-- 
2.9.3

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston v2 09/11] Switch to global output repaint timer

[Daniel Stone]

In preparation for grouping output repaint together where possible,
switch the per-output repaint timer, to a global timer which iterates
across all outputs.

This is implemented by storing the absolute time for the next repaint
for each output locally, and maintaining a global timer which iterates
all of them, scheduling the repaint for the first available time.

Signed-off-by: Daniel Stone 
Cc: Mario Kleiner 
Cc: Pekka Paalanen 
---
 libweston/compositor.c | 113 -
 libweston/compositor.h |   6 ++-
 2 files changed, 88 insertions(+), 31 deletions(-)

v2: Rebased on top of previous changes (repaint_status enum).
Suggestions from Pekka:
Keep absolute target repaint time as struct timespec, only using
int64_t nsec for relative comparison to current time, both in
maybe_repaint and finish_frame.
Remove unnecessary output_repaint_timer_arm from schedule_repaint
early-exit path.
Use explict bool rather than 0/~0 time to determine if any outputs
are eligible for repaint in the timer.

diff --git a/libweston/compositor.c b/libweston/compositor.c
index 4aa30da..a64e695 100644
--- a/libweston/compositor.c
+++ b/libweston/compositor.c
@@ -2338,14 +2338,21 @@ weston_output_schedule_repaint_reset(struct 
weston_output *output)
TL_POINT("core_repaint_exit_loop", TLP_OUTPUT(output), TLP_END);
 }
 
-static int
-output_repaint_timer_handler(void *data)
+static void
+weston_output_maybe_repaint(struct weston_output *output,
+   struct timespec *now)
 {
-   struct weston_output *output = data;
struct weston_compositor *compositor = output->compositor;
int ret;
+   int64_t msec_to_repaint;
 
-   assert(output->repaint_status == REPAINT_SCHEDULED);
+   /* We're not ready yet; come back to make a decision later. */
+   if (output->repaint_status != REPAINT_SCHEDULED)
+   return;
+
+   msec_to_repaint = timespec_sub_to_msec(>next_repaint, now);
+   if (msec_to_repaint > 1)
+   return;
 
/* If we're sleeping, drop the repaint machinery entirely; we will
 * explicitly repaint all outputs when we come back. */
@@ -2360,15 +2367,67 @@ output_repaint_timer_handler(void *data)
 
/* If repaint fails, we aren't going to get weston_output_finish_frame
 * to trigger a new repaint, so drop it from repaint and hope
-* something schedules a successful repaint later. */
+* something schedules a successful repaint later. As repainting may
+* take some time, re-read our clock as a courtesy to the next
+* output. */
ret = weston_output_repaint(output);
+   weston_compositor_read_presentation_clock(compositor, now);
if (ret != 0)
goto err;
 
-   return 0;
+   return;
 
 err:
weston_output_schedule_repaint_reset(output);
+}
+
+static void
+output_repaint_timer_arm(struct weston_compositor *compositor)
+{
+   struct weston_output *output;
+   bool any_should_repaint = false;
+   struct timespec now;
+   int64_t msec_to_next;
+
+   weston_compositor_read_presentation_clock(compositor, );
+
+   wl_list_for_each(output, >output_list, link) {
+   int64_t msec_to_this;
+
+   if (output->repaint_status != REPAINT_SCHEDULED)
+   continue;
+
+   msec_to_this = timespec_sub_to_msec(>next_repaint,
+   );
+   if (!any_should_repaint || msec_to_this < msec_to_next)
+   msec_to_next = msec_to_this;
+
+   any_should_repaint = true;
+   }
+
+   if (!any_should_repaint)
+   return;
+
+   /* XXX: This can keep postponing forever, maybe? */
+   if (msec_to_next < 1)
+   msec_to_next = 1;
+
+   wl_event_source_timer_update(compositor->repaint_timer, msec_to_next);
+}
+
+static int
+output_repaint_timer_handler(void *data)
+{
+   struct weston_compositor *compositor = data;
+   struct weston_output *output;
+   struct timespec now;
+
+   weston_compositor_read_presentation_clock(compositor, );
+   wl_list_for_each(output, >output_list, link)
+   weston_output_maybe_repaint(output, );
+
+   output_repaint_timer_arm(compositor);
+
return 0;
 }
 
@@ -2380,9 +2439,7 @@ weston_output_finish_frame(struct weston_output *output,
struct weston_compositor *compositor = output->compositor;
int32_t refresh_nsec;
struct timespec now;
-   struct timespec next;
-   struct timespec remain;
-   int msec_rel = 0;
+   int64_t msec_rel;
 
TL_POINT("core_repaint_finished", TLP_OUTPUT(output),
 TLP_VBLANK(stamp), TLP_END);
@@ -2390,11 +2447,15 @@ weston_output_finish_frame(struct weston_output 

[PATCH weston v2 07/11] Change repaint_needed to bool

[Daniel Stone]

It is only used as a binary value.

Signed-off-by: Daniel Stone 
---
 libweston/compositor-drm.c   | 2 +-
 libweston/compositor-fbdev.c | 2 +-
 libweston/compositor.c   | 4 ++--
 libweston/compositor.h   | 5 -
 4 files changed, 8 insertions(+), 5 deletions(-)

v2: New in this revision. Required by change to avoid normalising
next_repaint values to uint32_t msec in the global repaint timer.

diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
index 9a2df16..2715a87 100644
--- a/libweston/compositor-drm.c
+++ b/libweston/compositor-drm.c
@@ -2931,7 +2931,7 @@ session_notify(struct wl_listener *listener, void *data)
 * pending frame callbacks. */
 
wl_list_for_each(output, >output_list, base.link) {
-   output->base.repaint_needed = 0;
+   output->base.repaint_needed = false;
drmModeSetCursor(b->drm.fd, output->crtc_id, 0, 0, 0);
}
 
diff --git a/libweston/compositor-fbdev.c b/libweston/compositor-fbdev.c
index 44f0cf5..6f976d1 100644
--- a/libweston/compositor-fbdev.c
+++ b/libweston/compositor-fbdev.c
@@ -693,7 +693,7 @@ session_notify(struct wl_listener *listener, void *data)
 
wl_list_for_each(output,
 >output_list, link) {
-   output->repaint_needed = 0;
+   output->repaint_needed = false;
}
}
 }
diff --git a/libweston/compositor.c b/libweston/compositor.c
index 57eafcd..7f3532f 100644
--- a/libweston/compositor.c
+++ b/libweston/compositor.c
@@ -2310,7 +2310,7 @@ weston_output_repaint(struct weston_output *output)
 
pixman_region32_fini(_damage);
 
-   output->repaint_needed = 0;
+   output->repaint_needed = false;
 
weston_compositor_repick(ec);
 
@@ -2549,7 +2549,7 @@ weston_output_schedule_repaint(struct weston_output 
*output)
TL_POINT("core_repaint_req", TLP_OUTPUT(output), TLP_END);
 
loop = wl_display_get_event_loop(compositor->wl_display);
-   output->repaint_needed = 1;
+   output->repaint_needed = true;
if (output->repaint_scheduled)
return;
 
diff --git a/libweston/compositor.h b/libweston/compositor.h
index 9cdd682..7d6e1c9 100644
--- a/libweston/compositor.h
+++ b/libweston/compositor.h
@@ -170,7 +170,10 @@ struct weston_output {
pixman_region32_t region;
 
pixman_region32_t previous_damage;
-   int repaint_needed;
+
+   /** True if damage has occurred since the last repaint for this output;
+*  if set, a repaint will eventually occur. */
+   bool repaint_needed;
int repaint_scheduled;
struct wl_event_source *repaint_timer;
struct weston_output_zoom zoom;
-- 
2.9.3

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston v2 11/11] Allow backends to group repaint flushes

[Daniel Stone]

Implement new repaint_begin and repaint_flush hooks inside
weston_backend, allowing backends to gang together repaints which
trigger at the same time.

Signed-off-by: Daniel Stone 
---
 libweston/compositor-drm.c  |  5 +++--
 libweston/compositor-fbdev.c|  3 ++-
 libweston/compositor-headless.c |  3 ++-
 libweston/compositor-rdp.c  |  3 ++-
 libweston/compositor-wayland.c  |  6 --
 libweston/compositor-x11.c  |  6 --
 libweston/compositor.c  | 46 +
 libweston/compositor.h  | 38 --
 8 files changed, 86 insertions(+), 24 deletions(-)

v2: Add repaint_cancel hook and documentation.

diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
index 2715a87..2776d31 100644
--- a/libweston/compositor-drm.c
+++ b/libweston/compositor-drm.c
@@ -718,7 +718,8 @@ drm_waitvblank_pipe(struct drm_output *output)
 
 static int
 drm_output_repaint(struct weston_output *output_base,
-  pixman_region32_t *damage)
+  pixman_region32_t *damage,
+  void *repaint_data)
 {
struct drm_output *output = to_drm_output(output_base);
struct drm_backend *backend =
@@ -1317,7 +1318,7 @@ drm_output_set_cursor(struct drm_output *output)
 }
 
 static void
-drm_assign_planes(struct weston_output *output_base)
+drm_assign_planes(struct weston_output *output_base, void *repaint_data)
 {
struct drm_backend *b = to_drm_backend(output_base->compositor);
struct drm_output *output = to_drm_output(output_base);
diff --git a/libweston/compositor-fbdev.c b/libweston/compositor-fbdev.c
index 6f976d1..32d71e0 100644
--- a/libweston/compositor-fbdev.c
+++ b/libweston/compositor-fbdev.c
@@ -118,7 +118,8 @@ fbdev_output_start_repaint_loop(struct weston_output 
*output)
 }
 
 static int
-fbdev_output_repaint(struct weston_output *base, pixman_region32_t *damage)
+fbdev_output_repaint(struct weston_output *base, pixman_region32_t *damage,
+void *repaint_data)
 {
struct fbdev_output *output = to_fbdev_output(base);
struct weston_compositor *ec = output->base.compositor;
diff --git a/libweston/compositor-headless.c b/libweston/compositor-headless.c
index a1aec6d..9e42e7f 100644
--- a/libweston/compositor-headless.c
+++ b/libweston/compositor-headless.c
@@ -92,7 +92,8 @@ finish_frame_handler(void *data)
 
 static int
 headless_output_repaint(struct weston_output *output_base,
-  pixman_region32_t *damage)
+  pixman_region32_t *damage,
+  void *repaint_data)
 {
struct headless_output *output = to_headless_output(output_base);
struct weston_compositor *ec = output->base.compositor;
diff --git a/libweston/compositor-rdp.c b/libweston/compositor-rdp.c
index d9668e8..091472b 100644
--- a/libweston/compositor-rdp.c
+++ b/libweston/compositor-rdp.c
@@ -355,7 +355,8 @@ rdp_output_start_repaint_loop(struct weston_output *output)
 }
 
 static int
-rdp_output_repaint(struct weston_output *output_base, pixman_region32_t 
*damage)
+rdp_output_repaint(struct weston_output *output_base, pixman_region32_t 
*damage,
+  void *repaint_data)
 {
struct rdp_output *output = container_of(output_base, struct 
rdp_output, base);
struct weston_compositor *ec = output->base.compositor;
diff --git a/libweston/compositor-wayland.c b/libweston/compositor-wayland.c
index 9d35ef7..ebdbd13 100644
--- a/libweston/compositor-wayland.c
+++ b/libweston/compositor-wayland.c
@@ -488,7 +488,8 @@ wayland_output_start_repaint_loop(struct weston_output 
*output_base)
 #ifdef ENABLE_EGL
 static int
 wayland_output_repaint_gl(struct weston_output *output_base,
- pixman_region32_t *damage)
+ pixman_region32_t *damage,
+ void *repaint_data)
 {
struct wayland_output *output = to_wayland_output(output_base);
struct weston_compositor *ec = output->base.compositor;
@@ -595,7 +596,8 @@ wayland_shm_buffer_attach(struct wayland_shm_buffer *sb)
 
 static int
 wayland_output_repaint_pixman(struct weston_output *output_base,
- pixman_region32_t *damage)
+ pixman_region32_t *damage,
+ void *repaint_data)
 {
struct wayland_output *output = to_wayland_output(output_base);
struct wayland_backend *b =
diff --git a/libweston/compositor-x11.c b/libweston/compositor-x11.c
index f9cb461..02cdf3e 100644
--- a/libweston/compositor-x11.c
+++ b/libweston/compositor-x11.c
@@ -389,7 +389,8 @@ x11_output_start_repaint_loop(struct weston_output *output)
 
 static int
 x11_output_repaint_gl(struct weston_output *output_base,
- pixman_region32_t *damage)
+ pixman_region32_t *damage,
+ void *repaint_data)
 {
struct 

[PATCH weston v2 08/11] Change boolean repaint_scheduled to tristate enum

[Daniel Stone]

repaint_scheduled is actually cleverly a tristate. There are three
possible conditions for the repaint loop to be in at any time:
  - loop idle; no repaint will occur until specifically requested, which
may be never (repaint_scheduled == 0)
  - repaint scheduled: the compositor has definitively scheduled a
repaint request for this output, which will occur in fixed time
  - awaiting repaint completion: the backend has not yet signaled
completion of the last repaint request, and the compositor will not
schedule another until it does so

The last two conditions were previously conflated as
repaint_scheduled == 1, but break them out into separate conditions to
aid clarity, backed up by some asserts.

I would like to further disambiguate between awaiting completion of
start_repaint_loop() and awaiting completion of repaint(), but that
requires more surgery to weston_output_finish_frame() and the backends
than seems currently appropriate.

Signed-off-by: Daniel Stone 
---
 libweston/compositor.c | 20 +---
 libweston/compositor.h |  9 -
 2 files changed, 25 insertions(+), 4 deletions(-)

v2: New in this revision. Required by change to avoid normalising
next_repaint to uint32_t msec in global repaint timer.

diff --git a/libweston/compositor.c b/libweston/compositor.c
index 7f3532f..4aa30da 100644
--- a/libweston/compositor.c
+++ b/libweston/compositor.c
@@ -2311,6 +2311,8 @@ weston_output_repaint(struct weston_output *output)
pixman_region32_fini(_damage);
 
output->repaint_needed = false;
+   if (r == 0)
+   output->repaint_status = REPAINT_AWAITING_COMPLETION;
 
weston_compositor_repick(ec);
 
@@ -2332,7 +2334,7 @@ weston_output_repaint(struct weston_output *output)
 static void
 weston_output_schedule_repaint_reset(struct weston_output *output)
 {
-   output->repaint_scheduled = 0;
+   output->repaint_status = REPAINT_NOT_SCHEDULED;
TL_POINT("core_repaint_exit_loop", TLP_OUTPUT(output), TLP_END);
 }
 
@@ -2343,6 +2345,8 @@ output_repaint_timer_handler(void *data)
struct weston_compositor *compositor = output->compositor;
int ret;
 
+   assert(output->repaint_status == REPAINT_SCHEDULED);
+
/* If we're sleeping, drop the repaint machinery entirely; we will
 * explicitly repaint all outputs when we come back. */
if (compositor->state == WESTON_COMPOSITOR_SLEEPING ||
@@ -2383,6 +2387,7 @@ weston_output_finish_frame(struct weston_output *output,
TL_POINT("core_repaint_finished", TLP_OUTPUT(output),
 TLP_VBLANK(stamp), TLP_END);
 
+   assert(output->repaint_status == REPAINT_AWAITING_COMPLETION);
assert(stamp || (presented_flags & WP_PRESENTATION_FEEDBACK_INVALID));
 
/* If we haven't been supplied any timestamp at all, we don't have a
@@ -2424,6 +2429,8 @@ weston_output_finish_frame(struct weston_output *output,
msec_rel += refresh_nsec / 100;
 
 out:
+   output->repaint_status = REPAINT_SCHEDULED;
+
if (msec_rel < 1)
output_repaint_timer_handler(output);
else
@@ -2435,6 +2442,8 @@ idle_repaint(void *data)
 {
struct weston_output *output = data;
 
+   assert(output->repaint_status == REPAINT_SCHEDULED);
+   output->repaint_status = REPAINT_AWAITING_COMPLETION;
output->start_repaint_loop(output);
 }
 
@@ -2550,11 +2559,16 @@ weston_output_schedule_repaint(struct weston_output 
*output)
 
loop = wl_display_get_event_loop(compositor->wl_display);
output->repaint_needed = true;
-   if (output->repaint_scheduled)
+
+   /* If we already have a repaint scheduled for our idle handler,
+* no need to set it again. If the repaint has been called but
+* not finished, then weston_output_finish_frame() will notice
+* that a repaint is needed and schedule one. */
+   if (output->repaint_status != REPAINT_NOT_SCHEDULED)
return;
 
wl_event_loop_add_idle(loop, idle_repaint, output);
-   output->repaint_scheduled = 1;
+   output->repaint_status = REPAINT_SCHEDULED;
TL_POINT("core_repaint_enter_loop", TLP_OUTPUT(output), TLP_END);
 }
 
diff --git a/libweston/compositor.h b/libweston/compositor.h
index 7d6e1c9..06a0ff5 100644
--- a/libweston/compositor.h
+++ b/libweston/compositor.h
@@ -174,7 +174,14 @@ struct weston_output {
/** True if damage has occurred since the last repaint for this output;
 *  if set, a repaint will eventually occur. */
bool repaint_needed;
-   int repaint_scheduled;
+
+   /** State of the repaint loop */
+   enum {
+   REPAINT_NOT_SCHEDULED = 0, /**< idle; no repaint will occur */
+   REPAINT_SCHEDULED, /**< repaint is scheduled to occur */
+   REPAINT_AWAITING_COMPLETION, /**< last repaint not yet finished 
*/
+   } repaint_status;
+
struct 

[PATCH weston v2 02/11] timespec: Add timespec_add_msec helper

[Daniel Stone]

Add a (timespec) = (timespec) + (msec) helper, to save intermediate
conversions in its users.

Signed-off-by: Daniel Stone 
Reviewed-by: Pekka Paalanen 
---
 shared/timespec-util.h | 12 
 tests/timespec-test.c  | 11 +++
 2 files changed, 23 insertions(+)

v2: Trivially reimplement around timespec_add_nsec.

diff --git a/shared/timespec-util.h b/shared/timespec-util.h
index a1d6881..13948b1 100644
--- a/shared/timespec-util.h
+++ b/shared/timespec-util.h
@@ -70,6 +70,18 @@ timespec_add_nsec(struct timespec *r, const struct timespec 
*a, int64_t b)
}
 }
 
+/* Add a millisecond value to a timespec
+ *
+ * \param r[out] result: a + b
+ * \param a[in] base operand as timespec
+ * \param b[in] operand in milliseconds
+ */
+static inline void
+timespec_add_msec(struct timespec *r, const struct timespec *a, int64_t b)
+{
+   return timespec_add_nsec(r, a, b * 100);
+}
+
 /* Convert timespec to nanoseconds
  *
  * \param a timespec
diff --git a/tests/timespec-test.c b/tests/timespec-test.c
index 91d53f7..cd3b1c1 100644
--- a/tests/timespec-test.c
+++ b/tests/timespec-test.c
@@ -122,3 +122,14 @@ ZUC_TEST(timespec_test, timespec_add_nsec)
ZUC_ASSERT_EQ(16, r.tv_sec);
ZUC_ASSERT_EQ(0, r.tv_nsec);
 }
+
+ZUC_TEST(timespec_test, timespec_add_msec)
+{
+   struct timespec a, r;
+
+   a.tv_sec = 1000;
+   a.tv_nsec = 1;
+   timespec_add_msec(, , 2002);
+   ZUC_ASSERT_EQ(1002, r.tv_sec);
+   ZUC_ASSERT_EQ(201, r.tv_nsec);
+}
-- 
2.9.3

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston v2 06/11] Don't delay initial output paint

[Daniel Stone]

On startup, we cannot lock on to the repaint timer because it is unknown
to us. We deal with this by claiming that the moment of entry into the
repaint loop is the moment a frame returned, causing finish_frame to
delay our initial repaint to (refresh_time - repaint_delay), typically
around 9ms of utterly wasted time.

Add an explicit stamp == NULL, to determine that we are just beginning
our repaint loop, that the timings are in fact totally invalid, and that
it would be beneficial to repaint the output immediately. This will only
trigger when the display had previously been disabled or the previous
state is unknown, e.g. at startup, or coming back from DPMS off.

Signed-off-by: Daniel Stone 
Reviewed-by: Pekka Paalanen 
---
 libweston/compositor-drm.c | 3 +--
 libweston/compositor.c | 9 +
 2 files changed, 10 insertions(+), 2 deletions(-)

v2: Add assert to verify usage (Pekka).

diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
index 7f78699..9a2df16 100644
--- a/libweston/compositor-drm.c
+++ b/libweston/compositor-drm.c
@@ -882,8 +882,7 @@ drm_output_start_repaint_loop(struct weston_output 
*output_base)
 
 finish_frame:
/* if we cannot page-flip, immediately finish frame */
-   weston_compositor_read_presentation_clock(output_base->compositor, );
-   weston_output_finish_frame(output_base, ,
+   weston_output_finish_frame(output_base, NULL,
   WP_PRESENTATION_FEEDBACK_INVALID);
 }
 
diff --git a/libweston/compositor.c b/libweston/compositor.c
index 9eab0e2..57eafcd 100644
--- a/libweston/compositor.c
+++ b/libweston/compositor.c
@@ -2383,6 +2383,14 @@ weston_output_finish_frame(struct weston_output *output,
TL_POINT("core_repaint_finished", TLP_OUTPUT(output),
 TLP_VBLANK(stamp), TLP_END);
 
+   assert(stamp || (presented_flags & WP_PRESENTATION_FEEDBACK_INVALID));
+
+   /* If we haven't been supplied any timestamp at all, we don't have a
+* timebase to work against, so any delay just wastes time. Push a
+* repaint as soon as possible so we can get on with it. */
+   if (!stamp)
+   goto out;
+
refresh_nsec = millihz_to_nsec(output->current_mode->refresh);
weston_presentation_feedback_present_list(>feedback_list,
  output, refresh_nsec, stamp,
@@ -2415,6 +2423,7 @@ weston_output_finish_frame(struct weston_output *output,
if (presented_flags == WP_PRESENTATION_FEEDBACK_INVALID && msec_rel < 0)
msec_rel += refresh_nsec / 100;
 
+out:
if (msec_rel < 1)
output_repaint_timer_handler(output);
else
-- 
2.9.3

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston v2 05/11] Calculate next-frame target time in absolute space

[Daniel Stone]

Rather than determining the time until next-frame repaint in relative
space (time until repaint), determine it first in absolute space, and
then later convert this to relative.

This will later allow us to store these per-output, so we can have a
single idle timer which will allow us to aggregate multiple repaints
together when timing allows.

Signed-off-by: Daniel Stone 
Reviewed-by: Pekka Paalanen 
---
 libweston/compositor.c | 28 +++-
 1 file changed, 15 insertions(+), 13 deletions(-)

v2: No changes.

diff --git a/libweston/compositor.c b/libweston/compositor.c
index a28738e..9eab0e2 100644
--- a/libweston/compositor.c
+++ b/libweston/compositor.c
@@ -2376,8 +2376,9 @@ weston_output_finish_frame(struct weston_output *output,
struct weston_compositor *compositor = output->compositor;
int32_t refresh_nsec;
struct timespec now;
-   struct timespec gone;
-   int msec;
+   struct timespec next;
+   struct timespec remain;
+   int msec_rel = 0;
 
TL_POINT("core_repaint_finished", TLP_OUTPUT(output),
 TLP_VBLANK(stamp), TLP_END);
@@ -2389,34 +2390,35 @@ weston_output_finish_frame(struct weston_output *output,
  presented_flags);
 
output->frame_time = timespec_to_msec(stamp);
-
weston_compositor_read_presentation_clock(compositor, );
-   timespec_sub(, , stamp);
-   msec = (refresh_nsec - timespec_to_nsec()) / 100; /* floor */
-   msec -= compositor->repaint_msec;
 
-   if (msec < -1000 || msec > 1000) {
+   timespec_add_nsec(, stamp, refresh_nsec);
+   timespec_add_msec(, , -compositor->repaint_msec);
+   timespec_sub(, , );
+   msec_rel = timespec_to_msec();
+
+   if (msec_rel < -1000 || msec_rel > 1000) {
static bool warned;
 
if (!warned)
weston_log("Warning: computed repaint delay is "
-  "insane: %d msec\n", msec);
+  "insane: %d msec\n", msec_rel);
warned = true;
 
-   msec = 0;
+   msec_rel = 0;
}
 
/* Called from restart_repaint_loop and restart happens already after
 * the deadline given by repaint_msec? In that case we delay until
 * the deadline of the next frame, to give clients a more predictable
 * timing of the repaint cycle to lock on. */
-   if (presented_flags == WP_PRESENTATION_FEEDBACK_INVALID && msec < 0)
-   msec += refresh_nsec / 100;
+   if (presented_flags == WP_PRESENTATION_FEEDBACK_INVALID && msec_rel < 0)
+   msec_rel += refresh_nsec / 100;
 
-   if (msec < 1)
+   if (msec_rel < 1)
output_repaint_timer_handler(output);
else
-   wl_event_source_timer_update(output->repaint_timer, msec);
+   wl_event_source_timer_update(output->repaint_timer, msec_rel);
 }
 
 static void
-- 
2.9.3

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston v2 04/11] timespec: Add timespec subtraction helpers

[Daniel Stone]

Add helpers to subtract two timespecs, then return the difference in
either milliseconds or nanoseconds. These will be used to compare
timestamps during the repaint cycle.

Signed-off-by: Daniel Stone 
Suggested-by: Pekka Paalanen 
---
 shared/timespec-util.h | 26 ++
 tests/timespec-test.c  | 22 ++
 2 files changed, 48 insertions(+)

v2: New in this series. Suggested by Pekka, however the wesgr code was
not applicable as it normalised to an unsigned range, which is
inappropriate for our uses. Implemented from scratch.

diff --git a/shared/timespec-util.h b/shared/timespec-util.h
index c8e3cd6..95e0056 100644
--- a/shared/timespec-util.h
+++ b/shared/timespec-util.h
@@ -93,6 +93,20 @@ timespec_to_nsec(const struct timespec *a)
return (int64_t)a->tv_sec * NSEC_PER_SEC + a->tv_nsec;
 }
 
+/* Subtract timespecs and return result in nanoseconds
+ *
+ * \param a[in] operand
+ * \param b[in] operand
+ * \return to_nanoseconds(a - b)
+ */
+static inline int64_t
+timespec_sub_to_nsec(const struct timespec *a, const struct timespec *b)
+{
+   struct timespec r;
+   timespec_sub(, a, b);
+   return timespec_to_nsec();
+}
+
 /* Convert timespec to milliseconds
  *
  * \param a timespec
@@ -104,6 +118,18 @@ timespec_to_msec(const struct timespec *a)
return (int64_t)a->tv_sec * 1000 + a->tv_nsec / 100;
 }
 
+/* Subtract timespecs and return result in milliseconds
+ *
+ * \param a[in] operand
+ * \param b[in] operand
+ * \return to_milliseconds(a - b)
+ */
+static inline int64_t
+timespec_sub_to_msec(const struct timespec *a, const struct timespec *b)
+{
+   return timespec_sub_to_nsec(a, b) / 100;
+}
+
 /* Convert milli-Hertz to nanoseconds
  *
  * \param mhz frequency in mHz, not zero
diff --git a/tests/timespec-test.c b/tests/timespec-test.c
index 712d1ac..a503911 100644
--- a/tests/timespec-test.c
+++ b/tests/timespec-test.c
@@ -142,3 +142,25 @@ ZUC_TEST(timespec_test, timespec_add_msec)
ZUC_ASSERT_EQ(1002, r.tv_sec);
ZUC_ASSERT_EQ(201, r.tv_nsec);
 }
+
+ZUC_TEST(timespec_test, timespec_sub_to_nsec)
+{
+   struct timespec a, b;
+
+   a.tv_sec = 1000;
+   a.tv_nsec = 1;
+   b.tv_sec = 1;
+   b.tv_nsec = 2;
+   ZUC_ASSERT_EQ((999L * NSEC_PER_SEC) - 1, timespec_sub_to_nsec(, ));
+}
+
+ZUC_TEST(timespec_test, timespec_sub_to_msec)
+{
+   struct timespec a, b;
+
+   a.tv_sec = 1000;
+   a.tv_nsec = 200L;
+   b.tv_sec = 2;
+   b.tv_nsec = 100L;
+   ZUC_ASSERT_EQ((998 * 1000) + 1, timespec_sub_to_msec(, ));
+}
-- 
2.9.3

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston v2 00/11] Grouped output repaint

[Daniel Stone]

Hi,
I'm submitting v2 of this series after Pekka's review. At this point,
we now keep all time values in absolute timespec structs, only using
normalisation to nsec for relative comparisons. This required the
addition of subtraction helpers, suggested by Pekka, but reimplemented
to work with signed rather than unsigned values.

Doing this required prising apart weston_output::repaint_scheduled
somewhat, which is no bad thing. Previously it was a tri-state:
0 meant that no repaint would happen without damage occurring, and
1 meant that either a repaint was scheduled to occur at a fixed
time in the future, or that the previous repaint had not yet
completed, and a repaint would not occur until the next call to
weston_output_finish_frame. The only way to differentiate the two
was to combine with the state of the output's repaint timer, which
as of this series no longer exists.

repaint_scheduled has now been prised apart into a tri-state enum,
allowing us to differentiate between the latter two cases, and
documented slightly better. This, I think, is no bad thing.

There is also an RFC patch for adjusting repaint timings after
multi-frame repaint-window misses. Previously we would only adjust the
target repaint time by one frame, if we had missed it. If we had missed
the target time by multiple frames, we would schedule a repaint to
happen immediately, instead of at the next repaint window. This patch
changes the behaviour to target the next repaint window regardless of
how many frames we were late.

I am unsure if this behaviour is desirable, so have tagged it RFC: it
does not affect the series, and is something I noticed only by
inspection.

Cheers,
Daniel

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston v2 01/11] timespec: Add timespec_add_nsec helper

[Daniel Stone]

Add a (timespec) = (timespec) + (nsec) helper, to save intermediate
conversions to nanoseconds in its users.

Signed-off-by: Daniel Stone 
Reviewed-by: Pekka Paalanen 
---
 Makefile.am|  10 
 shared/timespec-util.h |  21 +
 tests/timespec-test.c  | 124 +
 3 files changed, 155 insertions(+)
 create mode 100644 tests/timespec-test.c

v2: Ensure timespec is normalised to 0 <= nsec < NSEC_PER_SEC.

diff --git a/Makefile.am b/Makefile.am
index dbdeea2..519d911 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1194,6 +1194,7 @@ internal_tests =  \
 
 shared_tests = \
config-parser.test  \
+   timespec.test   \
string.test \
vertex-clip.test\
zuctest
@@ -1307,6 +1308,15 @@ config_parser_test_CFLAGS =  \
$(AM_CFLAGS)\
-I$(top_srcdir)/tools/zunitc/inc
 
+timespec_test_SOURCES = tests/timespec-test.c
+timespec_test_LDADD =  \
+   libshared.la\
+   libzunitc.la\
+   libzunitcmain.la
+timespec_test_CFLAGS = \
+   $(AM_CFLAGS)\
+   -I$(top_srcdir)/tools/zunitc/inc
+
 string_test_SOURCES = \
tests/string-test.c \
shared/string-helpers.h
diff --git a/shared/timespec-util.h b/shared/timespec-util.h
index edd4ec1..a1d6881 100644
--- a/shared/timespec-util.h
+++ b/shared/timespec-util.h
@@ -49,6 +49,27 @@ timespec_sub(struct timespec *r,
}
 }
 
+/* Add a nanosecond value to a timespec
+ *
+ * \param r[out] result: a + b
+ * \param a[in] base operand as timespec
+ * \param b[in] operand in nanoseconds
+ */
+static inline void
+timespec_add_nsec(struct timespec *r, const struct timespec *a, int64_t b)
+{
+   r->tv_sec = a->tv_sec + (b / NSEC_PER_SEC);
+   r->tv_nsec = a->tv_nsec + (b % NSEC_PER_SEC);
+
+   if (r->tv_nsec >= NSEC_PER_SEC) {
+   r->tv_sec++;
+   r->tv_nsec -= NSEC_PER_SEC;
+   } else if (r->tv_nsec < 0) {
+   r->tv_sec--;
+   r->tv_nsec += NSEC_PER_SEC;
+   }
+}
+
 /* Convert timespec to nanoseconds
  *
  * \param a timespec
diff --git a/tests/timespec-test.c b/tests/timespec-test.c
new file mode 100644
index 000..91d53f7
--- /dev/null
+++ b/tests/timespec-test.c
@@ -0,0 +1,124 @@
+/*
+ * Copyright © 2016 Collabora, Ltd.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining
+ * a copy of this software and associated documentation files (the
+ * "Software"), to deal in the Software without restriction, including
+ * without limitation the rights to use, copy, modify, merge, publish,
+ * distribute, sublicense, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial
+ * portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT.  IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include "config.h"
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "timespec-util.h"
+
+#include "shared/helpers.h"
+#include "zunitc/zunitc.h"
+
+ZUC_TEST(timespec_test, timespec_sub)
+{
+   struct timespec a, b, r;
+
+   a.tv_sec = 1;
+   a.tv_nsec = 1;
+   b.tv_sec = 0;
+   b.tv_nsec = 2;
+   timespec_sub(, , );
+   ZUC_ASSERT_EQ(r.tv_sec, 0);
+   ZUC_ASSERT_EQ(r.tv_nsec, NSEC_PER_SEC - 1);
+}
+
+ZUC_TEST(timespec_test, timespec_to_nsec)
+{
+   struct timespec a;
+
+   a.tv_sec = 4;
+   a.tv_nsec = 4;
+   ZUC_ASSERT_EQ(timespec_to_nsec(), (NSEC_PER_SEC * 4ULL) + 4);
+}
+
+ZUC_TEST(timespec_test, millihz_to_nsec)
+{
+   ZUC_ASSERT_EQ(millihz_to_nsec(6), 1666);
+}
+
+ZUC_TEST(timespec_test, timespec_add_nsec)
+{
+   struct timespec a, r;
+
+   a.tv_sec = 0;
+   a.tv_nsec = NSEC_PER_SEC - 1;
+   timespec_add_nsec(, , 1);
+   ZUC_ASSERT_EQ(1, r.tv_sec);
+   ZUC_ASSERT_EQ(0, r.tv_nsec);
+
+   timespec_add_nsec(, , 2);
+   ZUC_ASSERT_EQ(1, r.tv_sec);
+   ZUC_ASSERT_EQ(1, r.tv_nsec);
+
+   timespec_add_nsec(, , (NSEC_PER_SEC * 2ULL));
+   ZUC_ASSERT_EQ(2, r.tv_sec);
+  

Re: [PATCH weston 19/68] compositor-drm: Refcount drm_fb

[Daniel Stone]

Hi,

On 1 March 2017 at 10:29, Pekka Paalanen  wrote:
> On Tue, 28 Feb 2017 14:46:21 + Daniel Stone  wrote:
>> On 28 February 2017 at 10:59, Pekka Paalanen  wrote:
>> > The important thing to document about this change is that is exchanges
>> > a (almost never hit) double-unref into a (always hit) leak of the
>> > drm_fb. It would be even better to mention what patch shall fix the
>> > introduced leak.
>>
>> I ... don't think it does leak? At least not if it gets successfully 
>> displayed.
>
> Hmm, yes, on a second look after sleeping several nights, I see calls
> to drm_fb_get_from_bo() are pretty much balanced with drm_fb_unref.
>
> But because this patch adds refcounting, and adds only calls to
> drm_fb_ref() without adding corresponding calls to unref, then I think
> the code just before this patch was somehow broken, because this patch
> as is definitely changes behaviour.

Sure, I can move the drm_fb_ref() addition into a follow-up patch if
you'd prefer. Let me know.

>> Every drm_fb_get_from_bo() whilst the BO is live, sure. But these are
>> balanced by drm_fb_unref (formerly drm_output_release_fb) calls in,
>> say, vblank_handler. So I can't really see a leak, and even if there
>> were, planes are disabled by default. Maybe scanout, but that's
>> trivially broken right now anyway, so not the biggest of regressions
>> as far as I'm concerned, if it even is one - and I can't quite see it.
>
> Scanout is broken? Since when?
>
> I used it successfully when reproducing
> https://bugs.freedesktop.org/show_bug.cgi?id=98833 or is that exactly
> the breakage you refer to?

ISTR the problem was that, if you repeatedly hit the repaint loop (due
to damage, even if everything was occluded), the buffer currently on
scanout could get released too early? It's hard to remember though; so
many bugs, and this series has been going such a long time.

>> Other libweston users can and do use multiple views ...
>
> Then they should have been broken to begin with, no? As you said,
> planes are disabled by default.

Indeed they have been!

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston 19/68] compositor-drm: Refcount drm_fb

[Pekka Paalanen]

On Tue, 28 Feb 2017 14:46:21 +
Daniel Stone  wrote:

> Hi,
> 
> On 28 February 2017 at 10:59, Pekka Paalanen  wrote:
> > On Mon, 27 Feb 2017 22:58:48 + Daniel Stone  
> > wrote:  
> >> It's indeed almost certainly a bugfix: if you have one client buffer
> >> with multiple views, both of which could be promoted to scanout,
> >> you'll get the same user data (drm_fb) handed back for the same BO,
> >> and could attempt to destroy it twice. Previously you'd probably
> >> mostly luck out due to the memory not yet being reused, but now you'd
> >> be near-guaranteed to hit the assert in drm_fb_unref, which seemed
> >> harsh. So I squashed it in here.
> >>
> >> drm_fb_ref doesn't exist before this patch, so the split would have to
> >> be this in a later patch. Would that work for you? I don't mind either
> >> way.  
> >
> > I think I'd be fine if those points were explained in the commit
> > message. To me, it's important to justify every hunk in a patch if it
> > somehow stands out as not being part of the main topic. And this one
> > caugh my eye as an unexplained change.  
> 
> Fair enough. I think the change from unref to actually performing an
> unref makes it more or less balancing, but sure.
> 
> > The important thing to document about this change is that is exchanges
> > a (almost never hit) double-unref into a (always hit) leak of the
> > drm_fb. It would be even better to mention what patch shall fix the
> > introduced leak.  
> 
> I ... don't think it does leak? At least not if it gets successfully 
> displayed.

Hmm, yes, on a second look after sleeping several nights, I see calls
to drm_fb_get_from_bo() are pretty much balanced with drm_fb_unref.

But because this patch adds refcounting, and adds only calls to
drm_fb_ref() without adding corresponding calls to unref, then I think
the code just before this patch was somehow broken, because this patch
as is definitely changes behaviour.

Was it so that before this patch adding the refcounting, the drm_fbs
were destroyed and re-created repeatedly, and it no longer happens?

I'm trying to find the explanation on the change of behaviour caused by
this patch, because I cannot understand how adding a ref but not adding
unref could not change the behaviour.

> > To clarify, the leak I am imagining is: drm_fb gets created with
> > refcount=1, then every drm_fb_get_from_bo() will increment the
> > refcount, but according to the old not-refcounted architecture, there
> > is only ever a single call to unref, which means it will never be
> > released. And that would apply to *all* drm_fbs ever used. Am I wrong?
> >
> > Seems like a fairly big temporary regression compared to the
> > double-free which can never happen in Weston since we don't use
> > multiple views (yet).  
> 
> Every drm_fb_get_from_bo() whilst the BO is live, sure. But these are
> balanced by drm_fb_unref (formerly drm_output_release_fb) calls in,
> say, vblank_handler. So I can't really see a leak, and even if there
> were, planes are disabled by default. Maybe scanout, but that's
> trivially broken right now anyway, so not the biggest of regressions
> as far as I'm concerned, if it even is one - and I can't quite see it.

Scanout is broken? Since when?

I used it successfully when reproducing
https://bugs.freedesktop.org/show_bug.cgi?id=98833 or is that exactly
the breakage you refer to?

> Other libweston users can and do use multiple views ...

Then they should have been broken to begin with, no? As you said,
planes are disabled by default.


Thanks,
pq


pgpidiirhxRro.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH libinput] touchpad: ignore hovering touches for the software button state

[Hans de Goede]

HI,

On 01-03-17 02:48, Peter Hutterer wrote:

If a touch started hovering in the main area, the button state would start
with AREA and never move to the real button state, despite the finger
triggering the pressure thresholds correctly in one of the areas.

This could even happen across touch sequences if a touch went below pressure
in the software button area, it changed to hovering and the button state
changed to NONE. On the next event, the touch is still hovering and the
current position of the touch is taken for the button state machine.

https://bugs.freedesktop.org/show_bug.cgi?id=99976

Signed-off-by: Peter Hutterer 


Patch looks good to me:

Reviewed-by: Hans de Goede 

Regards,

Hans




---
 src/evdev-mt-touchpad-buttons.c |  2 +-
 test/test-touchpad-buttons.c| 32 
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/src/evdev-mt-touchpad-buttons.c b/src/evdev-mt-touchpad-buttons.c
index 176b431..895cf0e 100644
--- a/src/evdev-mt-touchpad-buttons.c
+++ b/src/evdev-mt-touchpad-buttons.c
@@ -457,7 +457,7 @@ tp_button_handle_state(struct tp_dispatch *tp, uint64_t 
time)
struct tp_touch *t;

tp_for_each_touch(tp, t) {
-   if (t->state == TOUCH_NONE)
+   if (t->state == TOUCH_NONE || t->state == TOUCH_HOVERING)
continue;

if (t->state == TOUCH_END) {
diff --git a/test/test-touchpad-buttons.c b/test/test-touchpad-buttons.c
index 820d8f0..0037ba7 100644
--- a/test/test-touchpad-buttons.c
+++ b/test/test-touchpad-buttons.c
@@ -1476,6 +1476,37 @@ START_TEST(clickpad_softbutton_right_to_left)
 }
 END_TEST

+START_TEST(clickpad_softbutton_hover_into_buttons)
+{
+   struct litest_device *dev = litest_current_device();
+   struct libinput *li = dev->libinput;
+
+   litest_drain_events(li);
+
+   litest_hover_start(dev, 0, 50, 50);
+   libinput_dispatch(li);
+   litest_hover_move_to(dev, 0, 50, 50, 90, 90, 10, 0);
+   libinput_dispatch(li);
+
+   litest_touch_move_to(dev, 0, 90, 90, 91, 91, 1, 0);
+
+   litest_button_click(dev, BTN_LEFT, true);
+   libinput_dispatch(li);
+
+   litest_assert_button_event(li,
+  BTN_RIGHT,
+  LIBINPUT_BUTTON_STATE_PRESSED);
+   litest_assert_empty_queue(li);
+
+   litest_button_click(dev, BTN_LEFT, false);
+   litest_touch_up(dev, 0);
+
+   litest_assert_button_event(li,
+  BTN_RIGHT,
+  LIBINPUT_BUTTON_STATE_RELEASED);
+}
+END_TEST
+
 START_TEST(clickpad_topsoftbuttons_left)
 {
struct litest_device *dev = litest_current_device();
@@ -1962,6 +1993,7 @@ litest_setup_tests_touchpad_buttons(void)
litest_add("touchpad:softbutton", clickpad_softbutton_left_2nd_fg_move, 
LITEST_CLICKPAD, LITEST_APPLE_CLICKPAD);
litest_add("touchpad:softbutton", clickpad_softbutton_left_to_right, 
LITEST_CLICKPAD, LITEST_APPLE_CLICKPAD);
litest_add("touchpad:softbutton", clickpad_softbutton_right_to_left, 
LITEST_CLICKPAD, LITEST_APPLE_CLICKPAD);
+   litest_add("touchpad:softbutton", 
clickpad_softbutton_hover_into_buttons, LITEST_CLICKPAD|LITEST_HOVER, 
LITEST_APPLE_CLICKPAD);

litest_add("touchpad:topsoftbuttons", clickpad_topsoftbuttons_left, 
LITEST_TOPBUTTONPAD, LITEST_ANY);
litest_add("touchpad:topsoftbuttons", clickpad_topsoftbuttons_right, 
LITEST_TOPBUTTONPAD, LITEST_ANY);


___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel