Discussion:
[RFC xserver 0/3] Allow XWM to control Xwayland commits
(too old to reply)
Pekka Paalanen
2016-11-24 13:40:34 UTC
Permalink
From: Pekka Paalanen <***@collabora.co.uk>

Hi,

this is probably the first time I'm sending patches for the xserver, so
pointing out API misuse, coding style issues etc. would be appreciated. The
last patch also has some XXX comments with questions.

The first patch, refactoring, is not absolutely necessary (anymore - I used to
have a use for it while brewing this), but I think it makes things look nicer.

The fundamental problem is that Xwayland and the Wayland compositor (window
manager) are racing, particularly when windows are being mapped. This can lead
to glitches. The specific glitch I bumped into is described at
https://phabricator.freedesktop.org/T7622 .

The proposed solution allows the WM to temporarily prohibit commits.

It would be awkward for Weston to postpone mapping certain windows since it
would leak quite much Xwayland into the heart of the window manager. A Wayland
compositor also cannot ignore commits, because Xwayland will wait for frame
callbacks until it commits again. To not get stuck, one would need to fire
frame callbacks outside of their normal path. It seems much simpler to just
control Xwayland's commits, which also ensures that the first commit will carry
fully drawn window decorations too.

This series is the Xwayland side of the fix to T7622. The Weston side is still
brewing, but I was able to test that the Xwayland code works.


Thanks,
pq

Pekka Paalanen (3):
xwayland: refactor into xwl_window_post_damage()
xwayland: fix 'buffer' may be used uninitialized warning
Xwayland: use _XWAYLAND_ALLOW_COMMITS property

hw/xwayland/xwayland.c | 166 ++++++++++++++++++++++++++++++++++++++++++-------
hw/xwayland/xwayland.h | 3 +
2 files changed, 146 insertions(+), 23 deletions(-)
--
2.7.3

_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman
Pekka Paalanen
2016-11-24 13:40:36 UTC
Permalink
From: Pekka Paalanen <***@collabora.co.uk>

Fix the following warning due to --disable-glamor:

CC Xwayland-xwayland.o
In file included from /home/pq/local/include/wayland-client.h:40:0,
from xwayland.h:35,
from xwayland.c:26:
xwayland.c: In function ‘block_handler’:
/home/pq/local/include/wayland-client-protocol.h:3446:2: warning: ‘buffer’ may be used uninitialized in this function [-Wmaybe-uninitialized]
wl_proxy_marshal((struct wl_proxy *) wl_surface,
^
xwayland.c:466:23: note: ‘buffer’ was declared here
struct wl_buffer *buffer;
^

Signed-off-by: Pekka Paalanen <***@collabora.co.uk>
---
hw/xwayland/xwayland.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index c2ac4b3..9d79554 100644
--- a/hw/xwayland/xwayland.c
+++ b/hw/xwayland/xwayland.c
@@ -474,8 +474,8 @@ xwl_window_post_damage(struct xwl_window *xwl_window)
#if GLAMOR_HAS_GBM
if (xwl_screen->glamor)
buffer = xwl_glamor_pixmap_get_wl_buffer(pixmap);
+ else
#endif
- if (!xwl_screen->glamor)
buffer = xwl_shm_pixmap_get_wl_buffer(pixmap);

wl_surface_attach(xwl_window->surface, buffer, 0, 0);
--
2.7.3

_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-dev
Olivier Fourdan
2016-12-05 15:07:07 UTC
Permalink
Post by Pekka Paalanen
CC Xwayland-xwayland.o
In file included from /home/pq/local/include/wayland-client.h:40:0,
from xwayland.h:35,
/home/pq/local/include/wayland-client-protocol.h:3446:2: warning: ‘buffer’
may be used uninitialized in this function [-Wmaybe-uninitialized]
wl_proxy_marshal((struct wl_proxy *) wl_surface,
^
xwayland.c:466:23: note: ‘buffer’ was declared here
struct wl_buffer *buffer;
^
---
hw/xwayland/xwayland.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index c2ac4b3..9d79554 100644
--- a/hw/xwayland/xwayland.c
+++ b/hw/xwayland/xwayland.c
@@ -474,8 +474,8 @@ xwl_window_post_damage(struct xwl_window *xwl_window)
#if GLAMOR_HAS_GBM
if (xwl_screen->glamor)
buffer = xwl_glamor_pixmap_get_wl_buffer(pixmap);
+ else
#endif
- if (!xwl_screen->glamor)
buffer = xwl_shm_pixmap_get_wl_buffer(pixmap);
wl_surface_attach(xwl_window->surface, buffer, 0, 0);
--
2.7.3
Reviewed-by: Olivier Fourdan <***@redhat.com>

Cheers,
Olivier
_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists
Pekka Paalanen
2016-11-24 13:40:35 UTC
Permalink
From: Pekka Paalanen <***@collabora.co.uk>

Refactor xwl_screen_post_damage() and split the window specific code
into a new function xwl_window_post_damage().

This is a pure refactoring, there are no behavioral changes. An assert
is added to xwl_window_post_damage() to ensure frame callbacks are not
leaked if a future patch changes the call.

Signed-off-by: Pekka Paalanen <***@collabora.co.uk>
---
hw/xwayland/xwayland.c | 56 +++++++++++++++++++++++++++++---------------------
1 file changed, 33 insertions(+), 23 deletions(-)

diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index 9e1ecf8..c2ac4b3 100644
--- a/hw/xwayland/xwayland.c
+++ b/hw/xwayland/xwayland.c
@@ -458,44 +458,54 @@ static const struct wl_callback_listener frame_listener = {
};

static void
-xwl_screen_post_damage(struct xwl_screen *xwl_screen)
+xwl_window_post_damage(struct xwl_window *xwl_window)
{
- struct xwl_window *xwl_window, *next_xwl_window;
+ struct xwl_screen *xwl_screen = xwl_window->xwl_screen;
RegionPtr region;
BoxPtr box;
struct wl_buffer *buffer;
PixmapPtr pixmap;

- xorg_list_for_each_entry_safe(xwl_window, next_xwl_window,
- &xwl_screen->damage_window_list, link_damage) {
- /* If we're waiting on a frame callback from the server,
- * don't attach a new buffer. */
- if (xwl_window->frame_callback)
- continue;
+ assert(!xwl_window->frame_callback);

- region = DamageRegion(xwl_window->damage);
- pixmap = (*xwl_screen->screen->GetWindowPixmap) (xwl_window->window);
+ region = DamageRegion(xwl_window->damage);
+ pixmap = (*xwl_screen->screen->GetWindowPixmap) (xwl_window->window);

#if GLAMOR_HAS_GBM
- if (xwl_screen->glamor)
- buffer = xwl_glamor_pixmap_get_wl_buffer(pixmap);
+ if (xwl_screen->glamor)
+ buffer = xwl_glamor_pixmap_get_wl_buffer(pixmap);
#endif
- if (!xwl_screen->glamor)
- buffer = xwl_shm_pixmap_get_wl_buffer(pixmap);
+ if (!xwl_screen->glamor)
+ buffer = xwl_shm_pixmap_get_wl_buffer(pixmap);

- wl_surface_attach(xwl_window->surface, buffer, 0, 0);
+ wl_surface_attach(xwl_window->surface, buffer, 0, 0);

- box = RegionExtents(region);
- wl_surface_damage(xwl_window->surface, box->x1, box->y1,
- box->x2 - box->x1, box->y2 - box->y1);
+ box = RegionExtents(region);
+ wl_surface_damage(xwl_window->surface, box->x1, box->y1,
+ box->x2 - box->x1, box->y2 - box->y1);

- xwl_window->frame_callback = wl_surface_frame(xwl_window->surface);
- wl_callback_add_listener(xwl_window->frame_callback, &frame_listener, xwl_window);
+ xwl_window->frame_callback = wl_surface_frame(xwl_window->surface);
+ wl_callback_add_listener(xwl_window->frame_callback, &frame_listener, xwl_window);

- wl_surface_commit(xwl_window->surface);
- DamageEmpty(xwl_window->damage);
+ wl_surface_commit(xwl_window->surface);
+ DamageEmpty(xwl_window->damage);

- xorg_list_del(&xwl_window->link_damage);
+ xorg_list_del(&xwl_window->link_damage);
+}
+
+static void
+xwl_screen_post_damage(struct xwl_screen *xwl_screen)
+{
+ struct xwl_window *xwl_window, *next_xwl_window;
+
+ xorg_list_for_each_entry_safe(xwl_window, next_xwl_window,
+ &xwl_screen->damage_window_list, link_damage) {
+ /* If we're waiting on a frame callback from the server,
+ * don't attach a new buffer. */
+ if (xwl_window->frame_callback)
+ continue;
+
+ xwl_window_post_damage(xwl_window);
}
}
--
2.7.3

_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https:/
Olivier Fourdan
2016-12-05 15:06:27 UTC
Permalink
Post by Pekka Paalanen
Refactor xwl_screen_post_damage() and split the window specific code
into a new function xwl_window_post_damage().
This is a pure refactoring, there are no behavioral changes. An assert
is added to xwl_window_post_damage() to ensure frame callbacks are not
leaked if a future patch changes the call.
---
hw/xwayland/xwayland.c | 56
+++++++++++++++++++++++++++++---------------------
1 file changed, 33 insertions(+), 23 deletions(-)
diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index 9e1ecf8..c2ac4b3 100644
--- a/hw/xwayland/xwayland.c
+++ b/hw/xwayland/xwayland.c
@@ -458,44 +458,54 @@ static const struct wl_callback_listener frame_listener = {
};
static void
-xwl_screen_post_damage(struct xwl_screen *xwl_screen)
+xwl_window_post_damage(struct xwl_window *xwl_window)
{
- struct xwl_window *xwl_window, *next_xwl_window;
+ struct xwl_screen *xwl_screen = xwl_window->xwl_screen;
RegionPtr region;
BoxPtr box;
struct wl_buffer *buffer;
PixmapPtr pixmap;
- xorg_list_for_each_entry_safe(xwl_window, next_xwl_window,
- &xwl_screen->damage_window_list, link_damage) {
- /* If we're waiting on a frame callback from the server,
- * don't attach a new buffer. */
- if (xwl_window->frame_callback)
- continue;
+ assert(!xwl_window->frame_callback);
- region = DamageRegion(xwl_window->damage);
- pixmap = (*xwl_screen->screen->GetWindowPixmap)
(xwl_window->window);
+ region = DamageRegion(xwl_window->damage);
+ pixmap = (*xwl_screen->screen->GetWindowPixmap) (xwl_window->window);
#if GLAMOR_HAS_GBM
- if (xwl_screen->glamor)
- buffer = xwl_glamor_pixmap_get_wl_buffer(pixmap);
+ if (xwl_screen->glamor)
+ buffer = xwl_glamor_pixmap_get_wl_buffer(pixmap);
#endif
- if (!xwl_screen->glamor)
- buffer = xwl_shm_pixmap_get_wl_buffer(pixmap);
+ if (!xwl_screen->glamor)
+ buffer = xwl_shm_pixmap_get_wl_buffer(pixmap);
- wl_surface_attach(xwl_window->surface, buffer, 0, 0);
+ wl_surface_attach(xwl_window->surface, buffer, 0, 0);
- box = RegionExtents(region);
- wl_surface_damage(xwl_window->surface, box->x1, box->y1,
- box->x2 - box->x1, box->y2 - box->y1);
+ box = RegionExtents(region);
+ wl_surface_damage(xwl_window->surface, box->x1, box->y1,
+ box->x2 - box->x1, box->y2 - box->y1);
- xwl_window->frame_callback = wl_surface_frame(xwl_window->surface);
- wl_callback_add_listener(xwl_window->frame_callback,
&frame_listener, xwl_window);
+ xwl_window->frame_callback = wl_surface_frame(xwl_window->surface);
+ wl_callback_add_listener(xwl_window->frame_callback, &frame_listener, xwl_window);
- wl_surface_commit(xwl_window->surface);
- DamageEmpty(xwl_window->damage);
+ wl_surface_commit(xwl_window->surface);
+ DamageEmpty(xwl_window->damage);
- xorg_list_del(&xwl_window->link_damage);
+ xorg_list_del(&xwl_window->link_damage);
+}
+
+static void
+xwl_screen_post_damage(struct xwl_screen *xwl_screen)
+{
+ struct xwl_window *xwl_window, *next_xwl_window;
+
+ xorg_list_for_each_entry_safe(xwl_window, next_xwl_window,
+ &xwl_screen->damage_window_list, link_damage) {
+ /* If we're waiting on a frame callback from the server,
+ * don't attach a new buffer. */
+ if (xwl_window->frame_callback)
+ continue;
+
+ xwl_window_post_damage(xwl_window);
}
}
--
2.7.3
Reviewed-by: Olivier Fourdan <***@redhat.com>
_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/lis
Pekka Paalanen
2016-11-24 13:40:37 UTC
Permalink
From: Pekka Paalanen <***@collabora.co.uk>

The X11 window manager (XWM) of a Wayland compositor can use the
_XWAYLAND_ALLOW_COMMITS property to control when Xwayland sends
wl_surface.commit requests. If the property is not set, the behaviour
remains what it was.

XWM uses the property to inhibit commits until the window is ready to be
shown. This gives XWM time to set up the window decorations and internal
state before Xwayland does the first commit. XWM can use this to ensure
the first commit carries fully drawn decorations and the window
management state is correct when the window becomes visible.

Setting the property to zero inhibits further commits, and setting it to
non-zero allows commits. Deleting the property allows commits.

When the property is changed from zero to non-zero, there will be a
commit on next block_handler() call provided that some damage has been
recorded.

Without this patch (i.e. with the old behaviour) Xwayland can and will
commit the surface very soon as the application window has been realized
and drawn into. This races with XWM and may cause visible glitches.

Signed-off-by: Pekka Paalanen <***@collabora.co.uk>
---
hw/xwayland/xwayland.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++++
hw/xwayland/xwayland.h | 3 ++
2 files changed, 113 insertions(+)

diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index 9d79554..0c41726 100644
--- a/hw/xwayland/xwayland.c
+++ b/hw/xwayland/xwayland.c
@@ -34,6 +34,8 @@
#include <glx_extinit.h>
#include <os.h>
#include <xserver_poll.h>
+#include <xacestr.h>
+#include <propertyst.h>

#ifdef XF86VIDMODE
#include <X11/extensions/xf86vmproto.h>
@@ -262,6 +264,107 @@ xwl_pixmap_get(PixmapPtr pixmap)
}

static void
+xwl_window_set_allow_commits_from_property(struct xwl_window *xwl_window,
+ PropertyPtr prop)
+{
+ static Bool warned = FALSE;
+ CARD32 data;
+
+ /* XXX: check prop->type? */
+ if (prop->format != 32 || prop->size != 1) {
+ /* Not properly set, so fall back to safe and glitchy */
+ xwl_window->allow_commits = TRUE;
+
+ if (!warned) {
+ LogMessage(X_WARNING, "Window manager is misusing property %s.\n",
+ NameForAtom(xwl_window->xwl_screen->allow_commits_prop));
+ warned = TRUE;
+ }
+ return;
+ }
+
+ /* XXX: is this memcpy really the right thing to do? */
+ memcpy(&data, prop->data, 4);
+ xwl_window->allow_commits = !!data;
+ DebugF("xwayland: win %d allow_commits = %d\n",
+ xwl_window->window->drawable.id, xwl_window->allow_commits);
+}
+
+static void
+xwl_window_init_allow_commits(struct xwl_window *xwl_window)
+{
+ PropertyPtr prop = NULL;
+ int ret;
+
+ ret = dixLookupProperty(&prop, xwl_window->window,
+ xwl_window->xwl_screen->allow_commits_prop,
+ serverClient, DixReadAccess);
+ if (ret == Success && prop) {
+ xwl_window_set_allow_commits_from_property(xwl_window, prop);
+ }
+ else {
+ xwl_window->allow_commits = TRUE;
+ DebugF("xwayland: win %d allow_commit is uncontrolled.\n",
+ xwl_window->window->drawable.id);
+ }
+}
+
+static void
+xwl_property_callback(CallbackListPtr *pcbl, void *unused,
+ void *calldata)
+{
+ XacePropertyAccessRec *rec = calldata;
+ PropertyPtr prop = *rec->ppProp;
+ ScreenPtr screen = rec->pWin->drawable.pScreen;
+ struct xwl_screen *xwl_screen = xwl_screen_get(screen);
+ struct xwl_window *xwl_window;
+ Bool old_allow_commits;
+
+ if (prop->propertyName != xwl_screen->allow_commits_prop)
+ return;
+
+ xwl_window = xwl_window_get(rec->pWin);
+ if (!xwl_window)
+ return;
+
+ /* XXX: check the access is from the XWM client or serverClient? */
+
+ old_allow_commits = xwl_window->allow_commits;
+
+ if (rec->access_mode & DixWriteAccess)
+ xwl_window_set_allow_commits_from_property(xwl_window, prop);
+
+ if (rec->access_mode & DixDestroyAccess) {
+ xwl_window->allow_commits = TRUE;
+ DebugF("xwayland: win %d allow_commit reverts to uncontrolled.\n",
+ xwl_window->window->drawable.id);
+ }
+
+ /* If allow_commits turned from off to on, discard any frame
+ * callback we might be waiting for so that a new buffer is posted
+ * immediately through block_handler() if there is damage to post.
+ */
+ if (!old_allow_commits && xwl_window->allow_commits) {
+ if (xwl_window->frame_callback) {
+ wl_callback_destroy(xwl_window->frame_callback);
+ xwl_window->frame_callback = NULL;
+ }
+ }
+}
+
+static void
+xwl_screen_hook_property_access(struct xwl_screen *xwl_screen)
+{
+ static const char allow_commits[] = "_XWAYLAND_ALLOW_COMMITS";
+
+ xwl_screen->allow_commits_prop = MakeAtom(allow_commits,
+ strlen(allow_commits),
+ TRUE);
+ XaceRegisterCallback(XACE_PROPERTY_ACCESS, xwl_property_callback,
+ NULL);
+}
+
+static void
send_surface_id_event(struct xwl_window *xwl_window)
{
static const char atom_name[] = "WL_SURFACE_ID";
@@ -376,6 +479,8 @@ xwl_realize_window(WindowPtr window)
dixSetPrivate(&window->devPrivates, &xwl_window_private_key, xwl_window);
xorg_list_init(&xwl_window->link_damage);

+ xwl_window_init_allow_commits(xwl_window);
+
return ret;

err_surf:
@@ -505,6 +610,9 @@ xwl_screen_post_damage(struct xwl_screen *xwl_screen)
if (xwl_window->frame_callback)
continue;

+ if (!xwl_window->allow_commits)
+ continue;
+
xwl_window_post_damage(xwl_window);
}
}
@@ -849,6 +957,8 @@ xwl_screen_init(ScreenPtr pScreen, int argc, char **argv)
pScreen->CursorWarpedTo = xwl_cursor_warped_to;
pScreen->CursorConfinedTo = xwl_cursor_confined_to;

+ xwl_screen_hook_property_access(xwl_screen);
+
return ret;
}

diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
index 5e5624b..91b7620 100644
--- a/hw/xwayland/xwayland.h
+++ b/hw/xwayland/xwayland.h
@@ -99,6 +99,8 @@ struct xwl_screen {
void *egl_display, *egl_context;
struct gbm_device *gbm;
struct glamor_context *glamor_ctx;
+
+ Atom allow_commits_prop;
};

struct xwl_window {
@@ -109,6 +111,7 @@ struct xwl_window {
DamagePtr damage;
struct xorg_list link_damage;
struct wl_callback *frame_callback;
+ Bool allow_commits;
};

#define MODIFIER_META 0x01
--
2.7.3

_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailma
Pekka Paalanen
2016-12-05 12:03:04 UTC
Permalink
On Thu, 24 Nov 2016 15:40:37 +0200
Post by Pekka Paalanen
The X11 window manager (XWM) of a Wayland compositor can use the
_XWAYLAND_ALLOW_COMMITS property to control when Xwayland sends
wl_surface.commit requests. If the property is not set, the behaviour
remains what it was.
XWM uses the property to inhibit commits until the window is ready to be
shown. This gives XWM time to set up the window decorations and internal
state before Xwayland does the first commit. XWM can use this to ensure
the first commit carries fully drawn decorations and the window
management state is correct when the window becomes visible.
Setting the property to zero inhibits further commits, and setting it to
non-zero allows commits. Deleting the property allows commits.
When the property is changed from zero to non-zero, there will be a
commit on next block_handler() call provided that some damage has been
recorded.
Without this patch (i.e. with the old behaviour) Xwayland can and will
commit the surface very soon as the application window has been realized
and drawn into. This races with XWM and may cause visible glitches.
---
hw/xwayland/xwayland.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++++
hw/xwayland/xwayland.h | 3 ++
2 files changed, 113 insertions(+)
Hi all,

the reason this patch is RFC are the XXX comments in the code, and the
question: is it ok for Xwayland to hook up to properties and implement
functionality that way?

This patch was based on some whacky code from 2010, so the conventions
might be off.

I've highlighted the XXX comments below.

Patches 1 and 2 OTOH would be ready for merging on my behalf.

Olivier asked about _NET_WM_SYNC_REQUEST - do you want me to fully
implement the basic sync protocol too before accepting this series?
Post by Pekka Paalanen
diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index 9d79554..0c41726 100644
--- a/hw/xwayland/xwayland.c
+++ b/hw/xwayland/xwayland.c
@@ -34,6 +34,8 @@
#include <glx_extinit.h>
#include <os.h>
#include <xserver_poll.h>
+#include <xacestr.h>
+#include <propertyst.h>
#ifdef XF86VIDMODE
#include <X11/extensions/xf86vmproto.h>
@@ -262,6 +264,107 @@ xwl_pixmap_get(PixmapPtr pixmap)
}
static void
+xwl_window_set_allow_commits_from_property(struct xwl_window *xwl_window,
+ PropertyPtr prop)
+{
+ static Bool warned = FALSE;
+ CARD32 data;
+
+ /* XXX: check prop->type? */
+ if (prop->format != 32 || prop->size != 1) {
Should I check that prop->type is XA_CARDINAL?
Post by Pekka Paalanen
+ /* Not properly set, so fall back to safe and glitchy */
+ xwl_window->allow_commits = TRUE;
+
+ if (!warned) {
+ LogMessage(X_WARNING, "Window manager is misusing property %s.\n",
+ NameForAtom(xwl_window->xwl_screen->allow_commits_prop));
+ warned = TRUE;
+ }
+ return;
+ }
+
+ /* XXX: is this memcpy really the right thing to do? */
+ memcpy(&data, prop->data, 4);
Is memcpy to a CARD32 the right way to access the data? Is it necessary
to memcpy?
Post by Pekka Paalanen
+ xwl_window->allow_commits = !!data;
+ DebugF("xwayland: win %d allow_commits = %d\n",
+ xwl_window->window->drawable.id, xwl_window->allow_commits);
+}
+
+static void
+xwl_window_init_allow_commits(struct xwl_window *xwl_window)
+{
+ PropertyPtr prop = NULL;
+ int ret;
+
+ ret = dixLookupProperty(&prop, xwl_window->window,
+ xwl_window->xwl_screen->allow_commits_prop,
+ serverClient, DixReadAccess);
+ if (ret == Success && prop) {
+ xwl_window_set_allow_commits_from_property(xwl_window, prop);
+ }
+ else {
+ xwl_window->allow_commits = TRUE;
+ DebugF("xwayland: win %d allow_commit is uncontrolled.\n",
+ xwl_window->window->drawable.id);
+ }
+}
+
+static void
+xwl_property_callback(CallbackListPtr *pcbl, void *unused,
+ void *calldata)
+{
+ XacePropertyAccessRec *rec = calldata;
+ PropertyPtr prop = *rec->ppProp;
+ ScreenPtr screen = rec->pWin->drawable.pScreen;
+ struct xwl_screen *xwl_screen = xwl_screen_get(screen);
+ struct xwl_window *xwl_window;
+ Bool old_allow_commits;
+
+ if (prop->propertyName != xwl_screen->allow_commits_prop)
+ return;
+
+ xwl_window = xwl_window_get(rec->pWin);
+ if (!xwl_window)
+ return;
+
+ /* XXX: check the access is from the XWM client or serverClient? */
I expect the property to be written only from the XWM. Should I verify
that somehow? Can I even identify the XWM here?


Thanks,
pq
Post by Pekka Paalanen
+
+ old_allow_commits = xwl_window->allow_commits;
+
+ if (rec->access_mode & DixWriteAccess)
+ xwl_window_set_allow_commits_from_property(xwl_window, prop);
+
+ if (rec->access_mode & DixDestroyAccess) {
+ xwl_window->allow_commits = TRUE;
+ DebugF("xwayland: win %d allow_commit reverts to uncontrolled.\n",
+ xwl_window->window->drawable.id);
+ }
+
+ /* If allow_commits turned from off to on, discard any frame
+ * callback we might be waiting for so that a new buffer is posted
+ * immediately through block_handler() if there is damage to post.
+ */
+ if (!old_allow_commits && xwl_window->allow_commits) {
+ if (xwl_window->frame_callback) {
+ wl_callback_destroy(xwl_window->frame_callback);
+ xwl_window->frame_callback = NULL;
+ }
+ }
+}
+
+static void
+xwl_screen_hook_property_access(struct xwl_screen *xwl_screen)
+{
+ static const char allow_commits[] = "_XWAYLAND_ALLOW_COMMITS";
+
+ xwl_screen->allow_commits_prop = MakeAtom(allow_commits,
+ strlen(allow_commits),
+ TRUE);
+ XaceRegisterCallback(XACE_PROPERTY_ACCESS, xwl_property_callback,
+ NULL);
+}
+
+static void
send_surface_id_event(struct xwl_window *xwl_window)
{
static const char atom_name[] = "WL_SURFACE_ID";
@@ -376,6 +479,8 @@ xwl_realize_window(WindowPtr window)
dixSetPrivate(&window->devPrivates, &xwl_window_private_key, xwl_window);
xorg_list_init(&xwl_window->link_damage);
+ xwl_window_init_allow_commits(xwl_window);
+
return ret;
@@ -505,6 +610,9 @@ xwl_screen_post_damage(struct xwl_screen *xwl_screen)
if (xwl_window->frame_callback)
continue;
+ if (!xwl_window->allow_commits)
+ continue;
+
xwl_window_post_damage(xwl_window);
}
}
@@ -849,6 +957,8 @@ xwl_screen_init(ScreenPtr pScreen, int argc, char **argv)
pScreen->CursorWarpedTo = xwl_cursor_warped_to;
pScreen->CursorConfinedTo = xwl_cursor_confined_to;
+ xwl_screen_hook_property_access(xwl_screen);
+
return ret;
}
diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
index 5e5624b..91b7620 100644
--- a/hw/xwayland/xwayland.h
+++ b/hw/xwayland/xwayland.h
@@ -99,6 +99,8 @@ struct xwl_screen {
void *egl_display, *egl_context;
struct gbm_device *gbm;
struct glamor_context *glamor_ctx;
+
+ Atom allow_commits_prop;
};
struct xwl_window {
@@ -109,6 +111,7 @@ struct xwl_window {
DamagePtr damage;
struct xorg_list link_damage;
struct wl_callback *frame_callback;
+ Bool allow_commits;
};
#define MODIFIER_META 0x01
Pekka Paalanen
2016-12-05 12:30:15 UTC
Permalink
On Mon, 5 Dec 2016 14:03:04 +0200
Post by Pekka Paalanen
On Thu, 24 Nov 2016 15:40:37 +0200
Post by Pekka Paalanen
The X11 window manager (XWM) of a Wayland compositor can use the
_XWAYLAND_ALLOW_COMMITS property to control when Xwayland sends
wl_surface.commit requests. If the property is not set, the behaviour
remains what it was.
XWM uses the property to inhibit commits until the window is ready to be
shown. This gives XWM time to set up the window decorations and internal
state before Xwayland does the first commit. XWM can use this to ensure
the first commit carries fully drawn decorations and the window
management state is correct when the window becomes visible.
Setting the property to zero inhibits further commits, and setting it to
non-zero allows commits. Deleting the property allows commits.
When the property is changed from zero to non-zero, there will be a
commit on next block_handler() call provided that some damage has been
recorded.
Without this patch (i.e. with the old behaviour) Xwayland can and will
commit the surface very soon as the application window has been realized
and drawn into. This races with XWM and may cause visible glitches.
---
hw/xwayland/xwayland.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++++
hw/xwayland/xwayland.h | 3 ++
2 files changed, 113 insertions(+)
Hi all,
the reason this patch is RFC are the XXX comments in the code, and the
question: is it ok for Xwayland to hook up to properties and implement
functionality that way?
This patch was based on some whacky code from 2010, so the conventions
might be off.
I've highlighted the XXX comments below.
Patches 1 and 2 OTOH would be ready for merging on my behalf.
Olivier asked about _NET_WM_SYNC_REQUEST - do you want me to fully
implement the basic sync protocol too before accepting this series?
Hi,

forgot to mention that the WIP Weston counterpart for this series is:
https://patchwork.freedesktop.org/series/16109/
where the last patch uses the new property.


Thanks,
pq
Olivier Fourdan
2016-12-05 14:59:46 UTC
Permalink
Hi Pekka,
Post by Pekka Paalanen
[...]
Patches 1 and 2 OTOH would be ready for merging on my behalf.
Yes, I think the two first patches are fine.
Post by Pekka Paalanen
Olivier asked about _NET_WM_SYNC_REQUEST - do you want me to fully
implement the basic sync protocol too before accepting this series?
I was hoping/wondering if using _XWAYLAND_ALLOW_COMMITS could help the X11 window manager/X11 compositor to better sync the updates from the client with the refresh of the server side decorations (which includes drop shadows) to reduce the artefacts with server side decorations and drop shadows when resizing apps implementing the _NET_WM_SYNC_REQUEST protocol under Xwayland

But if anything, imho, it's up to the X11 window manager/X11 compositor to use both mechanism when dealing with Xwayland, since the window manager/X11 compositor "knows" about both properties and when it's best to paint.

So FWIW, I don't think it's necessary to add _NET_WM_SYNC_REQUEST to Xwayland, when using _XWAYLAND_ALLOW_COMMITS. I'll play with mutter and _XWAYLAND_ALLOW_COMMITS to see how that goes :)
Post by Pekka Paalanen
[...]
I expect the property to be written only from the XWM. Should I verify
that somehow? Can I even identify the XWM here?
I guess any client (or even user playing with xprop [1]) could add/change the property to any toplevel X11 window and Xwayland would stop committing the surfaces, but what would be the point of doing that?

Cheers,
Olivier

[1] "xprop -id ... -format _XWAYLAND_ALLOW_COMMITS 32c -set _XWAYLAND_ALLOW_COMMITS 0" brings the update to a halt :)
_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/
Pekka Paalanen
2016-12-05 15:46:53 UTC
Permalink
On Mon, 5 Dec 2016 09:59:46 -0500 (EST)
Post by Olivier Fourdan
Hi Pekka,
Post by Pekka Paalanen
[...]
Patches 1 and 2 OTOH would be ready for merging on my behalf.
Yes, I think the two first patches are fine.
Hi Olivier,

thank you for the reviews!
Post by Olivier Fourdan
Post by Pekka Paalanen
Olivier asked about _NET_WM_SYNC_REQUEST - do you want me to fully
implement the basic sync protocol too before accepting this series?
I was hoping/wondering if using _XWAYLAND_ALLOW_COMMITS could help
the X11 window manager/X11 compositor to better sync the updates from
the client with the refresh of the server side decorations (which
includes drop shadows) to reduce the artefacts with server side
decorations and drop shadows when resizing apps implementing the
_NET_WM_SYNC_REQUEST protocol under Xwayland
But if anything, imho, it's up to the X11 window manager/X11
compositor to use both mechanism when dealing with Xwayland, since
the window manager/X11 compositor "knows" about both properties and
when it's best to paint.
Right, however I wonder if the XWM always can reliably stop the commits
before the X11 client starts drawing the new size. I suppose with
interactive resizes using _NET_WM_MOVERESIZE and resizes initiated by
the XWM would be fine since the XWM knows there will be a resize before
telling the X11 client. But what if the client resizes spontaneously?

I assumed we would need some _NET_WM_SYNC_REQUEST support in Xwayland
always, but maybe we don't and doing it in XWM is enough?
Post by Olivier Fourdan
So FWIW, I don't think it's necessary to add _NET_WM_SYNC_REQUEST to
Xwayland, when using _XWAYLAND_ALLOW_COMMITS. I'll play with mutter
and _XWAYLAND_ALLOW_COMMITS to see how that goes :)
Excellent!
Post by Olivier Fourdan
Post by Pekka Paalanen
[...]
I expect the property to be written only from the XWM. Should I
verify that somehow? Can I even identify the XWM here?
I guess any client (or even user playing with xprop [1]) could
add/change the property to any toplevel X11 window and Xwayland would
stop committing the surfaces, but what would be the point of doing
that?
Indeed. I suppose X11 is designed to shoot your own foot if you aim and
pull the trigger. ;-)
Post by Olivier Fourdan
Cheers,
Olivier
[1] "xprop -id ... -format _XWAYLAND_ALLOW_COMMITS 32c -set
_XWAYLAND_ALLOW_COMMITS 0" brings the update to a halt :)
Thanks,
pq
Olivier Fourdan
2016-11-25 08:47:56 UTC
Permalink
Hi Pekka,
Post by Pekka Paalanen
this is probably the first time I'm sending patches for the xserver, so
pointing out API misuse, coding style issues etc. would be appreciated. The
last patch also has some XXX comments with questions.
The first patch, refactoring, is not absolutely necessary (anymore - I used to
have a use for it while brewing this), but I think it makes things look nicer.
The fundamental problem is that Xwayland and the Wayland compositor (window
manager) are racing, particularly when windows are being mapped. This can lead
to glitches. The specific glitch I bumped into is described at
https://phabricator.freedesktop.org/T7622 .
The proposed solution allows the WM to temporarily prohibit commits.
It would be awkward for Weston to postpone mapping certain windows since it
would leak quite much Xwayland into the heart of the window manager. A Wayland
compositor also cannot ignore commits, because Xwayland will wait for frame
callbacks until it commits again. To not get stuck, one would need to fire
frame callbacks outside of their normal path. It seems much simpler to just
control Xwayland's commits, which also ensures that the first commit will carry
fully drawn window decorations too.
This series is the Xwayland side of the fix to T7622. The Weston side is still
brewing, but I was able to test that the Xwayland code works.
I've taken a look at https://phabricator.freedesktop.org/T7622 but I am not sure how that proposal would play with apps that implement the _NET_WM_SYNC protocol, using _NET_WM_SYNC_REQUEST and sync counters as described in EMWH [1]

GNOME (mutter,gtk3) go a bit farther even and use an extended version of this protocol described by Owen [2].

I suspect these make the de-syncronization with the Xwayland toplevel decorated by the X window manager even more visible under Wayland, as the repaint is scheduled according to the app content and not sync'ed with Xwayland and repaint of the shadows by the XWM.

Would your proposal help there?

Cheers,
Olivier

[1] https://specifications.freedesktop.org/wm-spec/wm-spec-latest.html#idm140200472538288
[2] http://fishsoup.net/misc/wm-spec-synchronization.html
[3] https://bugzilla.gnome.org/show_bug.cgi?id=767212
[4] https://bugs.freedesktop.org/show_bug.cgi?id=81275
_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xor
Pekka Paalanen
2016-11-25 10:30:18 UTC
Permalink
On Fri, 25 Nov 2016 03:47:56 -0500 (EST)
Post by Olivier Fourdan
Hi Pekka,
Post by Pekka Paalanen
this is probably the first time I'm sending patches for the xserver, so
pointing out API misuse, coding style issues etc. would be appreciated. The
last patch also has some XXX comments with questions.
The first patch, refactoring, is not absolutely necessary (anymore - I used to
have a use for it while brewing this), but I think it makes things look nicer.
The fundamental problem is that Xwayland and the Wayland compositor (window
manager) are racing, particularly when windows are being mapped. This can lead
to glitches. The specific glitch I bumped into is described at
https://phabricator.freedesktop.org/T7622 .
The proposed solution allows the WM to temporarily prohibit commits.
It would be awkward for Weston to postpone mapping certain windows since it
would leak quite much Xwayland into the heart of the window manager. A Wayland
compositor also cannot ignore commits, because Xwayland will wait for frame
callbacks until it commits again. To not get stuck, one would need to fire
frame callbacks outside of their normal path. It seems much simpler to just
control Xwayland's commits, which also ensures that the first commit will carry
fully drawn window decorations too.
This series is the Xwayland side of the fix to T7622. The Weston side is still
brewing, but I was able to test that the Xwayland code works.
I've taken a look at https://phabricator.freedesktop.org/T7622 but I
am not sure how that proposal would play with apps that implement the
_NET_WM_SYNC protocol, using _NET_WM_SYNC_REQUEST and sync counters
as described in EMWH [1]
Hi,

I have no idea. I don't know how that protocol works and Weston does
not use it. Yet.
Post by Olivier Fourdan
GNOME (mutter,gtk3) go a bit farther even and use an extended version
of this protocol described by Owen [2].
I suspect these make the de-syncronization with the Xwayland toplevel
decorated by the X window manager even more visible under Wayland, as
the repaint is scheduled according to the app content and not sync'ed
with Xwayland and repaint of the shadows by the XWM.
Would your proposal help there?
I would hope so, but given that I don't know how the sync protocol
works, I can't say. So far I'm just following the plan Daniel made.

From what Daniel wrote about the sync protocol in T7622, it sounds like
"my" proposal is the first step toward making the sync protocol really
successful.

I've been wondering though whether the client content sync
protocol should be implemented in the WM or could/should it be
implemented in Xwayland? Somehow it would feel natural to me, being a
foreigner to X11, that Xwayland would throttle its wl_surface.commits
based on both the client content sync protocol and the WM sync protocol
(this series). I don't think we would want to route every single commit
through the WM.


Thanks,
pq
Post by Olivier Fourdan
[1]
https://specifications.freedesktop.org/wm-spec/wm-spec-latest.html#idm140200472538288
[2] http://fishsoup.net/misc/wm-spec-synchronization.html [3]
https://bugzilla.gnome.org/show_bug.cgi?id=767212 [4]
https://bugs.freedesktop.org/show_bug.cgi?id=81275
Pekka Paalanen
2016-11-28 13:47:11 UTC
Permalink
On Fri, 25 Nov 2016 12:30:18 +0200
Post by Pekka Paalanen
On Fri, 25 Nov 2016 03:47:56 -0500 (EST)
Post by Olivier Fourdan
Hi Pekka,
Post by Pekka Paalanen
this is probably the first time I'm sending patches for the xserver, so
pointing out API misuse, coding style issues etc. would be appreciated. The
last patch also has some XXX comments with questions.
The first patch, refactoring, is not absolutely necessary (anymore - I used to
have a use for it while brewing this), but I think it makes things look nicer.
The fundamental problem is that Xwayland and the Wayland compositor (window
manager) are racing, particularly when windows are being mapped. This can lead
to glitches. The specific glitch I bumped into is described at
https://phabricator.freedesktop.org/T7622 .
The proposed solution allows the WM to temporarily prohibit commits.
It would be awkward for Weston to postpone mapping certain windows since it
would leak quite much Xwayland into the heart of the window manager. A Wayland
compositor also cannot ignore commits, because Xwayland will wait for frame
callbacks until it commits again. To not get stuck, one would need to fire
frame callbacks outside of their normal path. It seems much simpler to just
control Xwayland's commits, which also ensures that the first commit will carry
fully drawn window decorations too.
This series is the Xwayland side of the fix to T7622. The Weston side is still
brewing, but I was able to test that the Xwayland code works.
I've taken a look at https://phabricator.freedesktop.org/T7622 but I
am not sure how that proposal would play with apps that implement the
_NET_WM_SYNC protocol, using _NET_WM_SYNC_REQUEST and sync counters
as described in EMWH [1]
Hi,
I have no idea. I don't know how that protocol works and Weston does
not use it. Yet.
Post by Olivier Fourdan
GNOME (mutter,gtk3) go a bit farther even and use an extended version
of this protocol described by Owen [2].
I suspect these make the de-syncronization with the Xwayland toplevel
decorated by the X window manager even more visible under Wayland, as
the repaint is scheduled according to the app content and not sync'ed
with Xwayland and repaint of the shadows by the XWM.
Would your proposal help there?
I would hope so, but given that I don't know how the sync protocol
works, I can't say. So far I'm just following the plan Daniel made.
From what Daniel wrote about the sync protocol in T7622, it sounds like
"my" proposal is the first step toward making the sync protocol really
successful.
I've been wondering though whether the client content sync
protocol should be implemented in the WM or could/should it be
implemented in Xwayland? Somehow it would feel natural to me, being a
foreigner to X11, that Xwayland would throttle its wl_surface.commits
based on both the client content sync protocol and the WM sync protocol
(this series). I don't think we would want to route every single commit
through the WM.
Thanks,
pq
Post by Olivier Fourdan
[1]
https://specifications.freedesktop.org/wm-spec/wm-spec-latest.html#idm140200472538288
[2] http://fishsoup.net/misc/wm-spec-synchronization.html
Hi,

having read the above two specs, it is very much not obvious how to
connect all the dots. It needs experimenting.

Would it be acceptable to build the knowledge of all these sync
protocols into Xwayland server?

The thing there is that Xwayland is issuing the wl_surface.commits, and
having XWM to explicitly trigger all commits always seems like
unnecessary overhead. However, Xwayland does not know when to commit if
it doesn't understand the X11 sync protocols. Either every commit
for every window has to roundtrip through XWM, or Xwayland needs to
understand stuff. Or the third option, which I like the least, is to
have the Wayland compositor special-case all surface from Xwayland and
put them through a completely different and new code paths not shared
with anything.

The basic frame counter only throttles resizes, so that would be mostly
between XWM and the X11 app. Xwayland could automatically stop
committing when the counter indicates the app is busy reacting to a
resize, and resume committing when the app catches up.

The extended frame counter seems fairly straightforward at first hand
to integrate as a commit prohibitor, like _XWAYLAND_ALLOW_COMMITS.

I'm not sure if _NET_WM_FRAME_DRAWN should correspond to Wayland frame
callbacks (which Xwayland could then handle on its own) or sent by XWM
when it has drawn the decorations. The former would be more
straightforward, the latter might allow using FRAME_DRAWN for
triggering the commit but somehow I have a feeling it might not be a
good idea...

_NET_WM_FRAME_TIMINGS looks like something we could provide with the
Presentation feedback protocol straight from Xwayland.

Anyway, we have three different kinds of X11 apps:
- no sync
- basic sync
- extended sync

For the no-sync kind I think we still need something like my patch
series. The other sync types would build on top, either by extending or
replacing _XWAYLAND_ALLOW_COMMITS - or even just leaving it to XWM as a
master switch of commits.

Hence, at the moment, I believe this series would be a useful step
forward, if the idea of the server inspecting properties is acceptable.

Mind, I am mostly thinking this in Weston XWM terms, which draws the
window decorations through X11 like a normal window manager. I'm not
sure how things would change if the Wayland compositor drew the
decorations directly, internally, as a part of composition, but I would
hope it just means some steps in the sync protocols simply become
no-ops.

CC'd Owen in case he has opinions.


Thanks,
pq
Post by Pekka Paalanen
Post by Olivier Fourdan
[3]
https://bugzilla.gnome.org/show_bug.cgi?id=767212 [4]
https://bugs.freedesktop.org/show_bug.cgi?id=81275
Owen Taylor
2016-11-30 20:12:54 UTC
Permalink
Hi Pekka,

I don't have a lot of of commentary to add here. Certainly getting the
frame-sync protocols right does require integration between Xwayland and
the compositing manager. I don't think there's that much virtue in spending
time on the extended version of the sync protocol and
_NET_WM_FRAME_TIMINGS, because, to my knowledge, those are implemented only
by GTK+ 3, and most GTK+ 3 apps will be running as native Wayland apps. On
the other hand, gtk2 and qt4 X clients will be around to exercise the basic
version of the protocol for the forseeable future.

Regards,
Owen
Post by Pekka Paalanen
On Fri, 25 Nov 2016 12:30:18 +0200
Post by Pekka Paalanen
On Fri, 25 Nov 2016 03:47:56 -0500 (EST)
Post by Olivier Fourdan
Hi Pekka,
Post by Pekka Paalanen
this is probably the first time I'm sending patches for the xserver,
so
Post by Pekka Paalanen
Post by Olivier Fourdan
Post by Pekka Paalanen
pointing out API misuse, coding style issues etc. would be
appreciated. The
Post by Pekka Paalanen
Post by Olivier Fourdan
Post by Pekka Paalanen
last patch also has some XXX comments with questions.
The first patch, refactoring, is not absolutely necessary (anymore -
I used
Post by Pekka Paalanen
Post by Olivier Fourdan
Post by Pekka Paalanen
to
have a use for it while brewing this), but I think it makes things
look
Post by Pekka Paalanen
Post by Olivier Fourdan
Post by Pekka Paalanen
nicer.
The fundamental problem is that Xwayland and the Wayland compositor
(window
Post by Pekka Paalanen
Post by Olivier Fourdan
Post by Pekka Paalanen
manager) are racing, particularly when windows are being mapped.
This can
Post by Pekka Paalanen
Post by Olivier Fourdan
Post by Pekka Paalanen
lead
to glitches. The specific glitch I bumped into is described at
https://phabricator.freedesktop.org/T7622 .
The proposed solution allows the WM to temporarily prohibit commits.
It would be awkward for Weston to postpone mapping certain windows
since it
Post by Pekka Paalanen
Post by Olivier Fourdan
Post by Pekka Paalanen
would leak quite much Xwayland into the heart of the window manager.
A
Post by Pekka Paalanen
Post by Olivier Fourdan
Post by Pekka Paalanen
Wayland
compositor also cannot ignore commits, because Xwayland will wait
for frame
Post by Pekka Paalanen
Post by Olivier Fourdan
Post by Pekka Paalanen
callbacks until it commits again. To not get stuck, one would need
to fire
Post by Pekka Paalanen
Post by Olivier Fourdan
Post by Pekka Paalanen
frame callbacks outside of their normal path. It seems much simpler
to just
Post by Pekka Paalanen
Post by Olivier Fourdan
Post by Pekka Paalanen
control Xwayland's commits, which also ensures that the first commit
will
Post by Pekka Paalanen
Post by Olivier Fourdan
Post by Pekka Paalanen
carry
fully drawn window decorations too.
This series is the Xwayland side of the fix to T7622. The Weston
side is
Post by Pekka Paalanen
Post by Olivier Fourdan
Post by Pekka Paalanen
still
brewing, but I was able to test that the Xwayland code works.
I've taken a look at https://phabricator.freedesktop.org/T7622 but I
am not sure how that proposal would play with apps that implement the
_NET_WM_SYNC protocol, using _NET_WM_SYNC_REQUEST and sync counters
as described in EMWH [1]
Hi,
I have no idea. I don't know how that protocol works and Weston does
not use it. Yet.
Post by Olivier Fourdan
GNOME (mutter,gtk3) go a bit farther even and use an extended version
of this protocol described by Owen [2].
I suspect these make the de-syncronization with the Xwayland toplevel
decorated by the X window manager even more visible under Wayland, as
the repaint is scheduled according to the app content and not sync'ed
with Xwayland and repaint of the shadows by the XWM.
Would your proposal help there?
I would hope so, but given that I don't know how the sync protocol
works, I can't say. So far I'm just following the plan Daniel made.
From what Daniel wrote about the sync protocol in T7622, it sounds like
"my" proposal is the first step toward making the sync protocol really
successful.
I've been wondering though whether the client content sync
protocol should be implemented in the WM or could/should it be
implemented in Xwayland? Somehow it would feel natural to me, being a
foreigner to X11, that Xwayland would throttle its wl_surface.commits
based on both the client content sync protocol and the WM sync protocol
(this series). I don't think we would want to route every single commit
through the WM.
Thanks,
pq
Post by Olivier Fourdan
[1]
https://specifications.freedesktop.org/wm-spec/wm-spec-latest.html#
idm140200472538288
Post by Pekka Paalanen
Post by Olivier Fourdan
[2] http://fishsoup.net/misc/wm-spec-synchronization.html
Hi,
having read the above two specs, it is very much not obvious how to
connect all the dots. It needs experimenting.
Would it be acceptable to build the knowledge of all these sync
protocols into Xwayland server?
The thing there is that Xwayland is issuing the wl_surface.commits, and
having XWM to explicitly trigger all commits always seems like
unnecessary overhead. However, Xwayland does not know when to commit if
it doesn't understand the X11 sync protocols. Either every commit
for every window has to roundtrip through XWM, or Xwayland needs to
understand stuff. Or the third option, which I like the least, is to
have the Wayland compositor special-case all surface from Xwayland and
put them through a completely different and new code paths not shared
with anything.
The basic frame counter only throttles resizes, so that would be mostly
between XWM and the X11 app. Xwayland could automatically stop
committing when the counter indicates the app is busy reacting to a
resize, and resume committing when the app catches up.
The extended frame counter seems fairly straightforward at first hand
to integrate as a commit prohibitor, like _XWAYLAND_ALLOW_COMMITS.
I'm not sure if _NET_WM_FRAME_DRAWN should correspond to Wayland frame
callbacks (which Xwayland could then handle on its own) or sent by XWM
when it has drawn the decorations. The former would be more
straightforward, the latter might allow using FRAME_DRAWN for
triggering the commit but somehow I have a feeling it might not be a
good idea...
_NET_WM_FRAME_TIMINGS looks like something we could provide with the
Presentation feedback protocol straight from Xwayland.
- no sync
- basic sync
- extended sync
For the no-sync kind I think we still need something like my patch
series. The other sync types would build on top, either by extending or
replacing _XWAYLAND_ALLOW_COMMITS - or even just leaving it to XWM as a
master switch of commits.
Hence, at the moment, I believe this series would be a useful step
forward, if the idea of the server inspecting properties is acceptable.
Mind, I am mostly thinking this in Weston XWM terms, which draws the
window decorations through X11 like a normal window manager. I'm not
sure how things would change if the Wayland compositor drew the
decorations directly, internally, as a part of composition, but I would
hope it just means some steps in the sync protocols simply become
no-ops.
CC'd Owen in case he has opinions.
Thanks,
pq
Post by Pekka Paalanen
Post by Olivier Fourdan
[3]
https://bugzilla.gnome.org/show_bug.cgi?id=767212 [4]
https://bugs.freedesktop.org/show_bug.cgi?id=81275
Pekka Paalanen
2016-12-01 09:53:58 UTC
Permalink
On Wed, 30 Nov 2016 15:12:54 -0500
Post by Olivier Fourdan
Hi Pekka,
I don't have a lot of of commentary to add here. Certainly getting the
frame-sync protocols right does require integration between Xwayland and
the compositing manager. I don't think there's that much virtue in spending
time on the extended version of the sync protocol and
_NET_WM_FRAME_TIMINGS, because, to my knowledge, those are implemented only
by GTK+ 3, and most GTK+ 3 apps will be running as native Wayland apps. On
the other hand, gtk2 and qt4 X clients will be around to exercise the basic
version of the protocol for the forseeable future.
Hi Owen,

that's valuable, saves me the effort of designing for and maybe
implementing the extended version. :-)

I'm kind of curious though why no other toolkit saw the benefit of the
extended sync protocol.


Thanks,
pq
Owen Taylor
2016-12-01 15:15:11 UTC
Permalink
Probably a combination of factors:
* It never got into the official spec (I didn't want to push it in without
some review and hopefully a second implementation)
* It requires coordinated window manager and toolkit changes to get any
benefit
* The original spec fixed a huge problem with opaque resizing where the
application could lag arbitrarily behind the window manager - the extended
spec just makes things look better when you look close.
* People who really care about these things were already beginning to put
their efforts into Wayland

- Owen
Post by Pekka Paalanen
On Wed, 30 Nov 2016 15:12:54 -0500
Post by Olivier Fourdan
Hi Pekka,
I don't have a lot of of commentary to add here. Certainly getting the
frame-sync protocols right does require integration between Xwayland and
the compositing manager. I don't think there's that much virtue in
spending
Post by Olivier Fourdan
time on the extended version of the sync protocol and
_NET_WM_FRAME_TIMINGS, because, to my knowledge, those are implemented
only
Post by Olivier Fourdan
by GTK+ 3, and most GTK+ 3 apps will be running as native Wayland apps.
On
Post by Olivier Fourdan
the other hand, gtk2 and qt4 X clients will be around to exercise the
basic
Post by Olivier Fourdan
version of the protocol for the forseeable future.
Hi Owen,
that's valuable, saves me the effort of designing for and maybe
implementing the extended version. :-)
I'm kind of curious though why no other toolkit saw the benefit of the
extended sync protocol.
Thanks,
pq
Adam Jackson
2016-12-05 21:43:24 UTC
Permalink
(apologies for being so slow to get to this thread, this is great stuff)
Post by Pekka Paalanen
Hi,
having read the above two specs, it is very much not obvious how to
connect all the dots. It needs experimenting.
Would it be acceptable to build the knowledge of all these sync
protocols into Xwayland server?
Sure. I'd started down these lines a while ago:

https://cgit.freedesktop.org/~ajax/xserver/log/?h=wayland-deferred-surface

Please do steal from that, specifically the bit where it adds a real
property callback. Your patch series hangs that off XACE, which is
still optional (and merely requiring it for Xwayland is a bit awkward
since it means you can't do both XACE-enabled and -disabled servers
from the same build pass).
Post by Pekka Paalanen
The thing there is that Xwayland is issuing the wl_surface.commits, and
having XWM to explicitly trigger all commits always seems like
unnecessary overhead. However, Xwayland does not know when to commit if
it doesn't understand the X11 sync protocols. Either every commit
for every window has to roundtrip through XWM, or Xwayland needs to
understand stuff. Or the third option, which I like the least, is to
have the Wayland compositor special-case all surface from Xwayland and
put them through a completely different and new code paths not shared
with anything.
Agreed, having the wm uncork commits is insane. xserver is best
positioned to make these decisions, and if that means teaching it about
client IPC conventions, okay.
Post by Pekka Paalanen
- no sync
- basic sync
- extended sync
For the no-sync kind I think we still need something like my patch
series. The other sync types would build on top, either by extending or
replacing _XWAYLAND_ALLOW_COMMITS - or even just leaving it to XWM as a
master switch of commits.
As mentioned further downthread the extended sync case isn't that
interesting since it's really only gtk3.

For the no-sync case I think xwayland gets to invent whatever semantics
it likes best. Without a zwl_x11_compat protocol (see below) I think
that means resizes necessarily flush commits through but that surface
content updates can be corked until the next frame. And then for basic
sync clients, resizes wait for the netwm protocol before flushing
instead.
Post by Pekka Paalanen
Mind, I am mostly thinking this in Weston XWM terms, which draws the
window decorations through X11 like a normal window manager.
That's fine. Honestly I'd prefer a world where the wayland server was
not also the window manager, you should be able to run twm if you want
(and have it work about as well as, say, twm on win32). That'd need an
x11 compat protocol to really handle well, but again, okay. I think
that's _better_ than the current model, where mutter just magically
knows to get X geometry info through this side channel and therefore
has to worry about races, instead of Xwayland always presenting a
logically consistent view of its world to the wayland server.

- ajax
_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: htt
Olivier Fourdan
2016-12-06 07:39:01 UTC
Permalink
Hey
Post by Adam Jackson
(apologies for being so slow to get to this thread, this is great stuff)
[...]
Post by Pekka Paalanen
Mind, I am mostly thinking this in Weston XWM terms, which draws the
window decorations through X11 like a normal window manager.
That's fine. Honestly I'd prefer a world where the wayland server was
not also the window manager, you should be able to run twm if you want
(and have it work about as well as, say, twm on win32). That'd need an
x11 compat protocol to really handle well, but again, okay. I think
that's _better_ than the current model, where mutter just magically
knows to get X geometry info through this side channel and therefore
has to worry about races, instead of Xwayland always presenting a
logically consistent view of its world to the wayland server.
Putting my /other desktop/ swimming suit here, I would love to see such a
model, it would greatly help with a wider Wayland adoption imho.

Cheers,
Olivier
Pekka Paalanen
2016-12-07 10:49:51 UTC
Permalink
On Mon, 05 Dec 2016 16:43:24 -0500
Post by Adam Jackson
(apologies for being so slow to get to this thread, this is great stuff)
Post by Pekka Paalanen
Hi,
having read the above two specs, it is very much not obvious how to
connect all the dots. It needs experimenting.
Would it be acceptable to build the knowledge of all these sync
protocols into Xwayland server?
https://cgit.freedesktop.org/~ajax/xserver/log/?h=wayland-deferred-surface
Please do steal from that, specifically the bit where it adds a real
property callback. Your patch series hangs that off XACE, which is
still optional (and merely requiring it for Xwayland is a bit awkward
since it means you can't do both XACE-enabled and -disabled servers
from the same build pass).
Hi Adam,

nice! Should I take only the property stuff, or should I try to
learn and integrate everything you have there?

I suppose I should start with only the property stuff and make my
mechanism work with that first.
Post by Adam Jackson
Post by Pekka Paalanen
The thing there is that Xwayland is issuing the wl_surface.commits, and
having XWM to explicitly trigger all commits always seems like
unnecessary overhead. However, Xwayland does not know when to commit if
it doesn't understand the X11 sync protocols. Either every commit
for every window has to roundtrip through XWM, or Xwayland needs to
understand stuff. Or the third option, which I like the least, is to
have the Wayland compositor special-case all surface from Xwayland and
put them through a completely different and new code paths not shared
with anything.
Agreed, having the wm uncork commits is insane. xserver is best
positioned to make these decisions, and if that means teaching it about
client IPC conventions, okay.
Very good.
Post by Adam Jackson
Post by Pekka Paalanen
- no sync
- basic sync
- extended sync
For the no-sync kind I think we still need something like my patch
series. The other sync types would build on top, either by extending or
replacing _XWAYLAND_ALLOW_COMMITS - or even just leaving it to XWM as a
master switch of commits.
As mentioned further downthread the extended sync case isn't that
interesting since it's really only gtk3.
For the no-sync case I think xwayland gets to invent whatever semantics
it likes best. Without a zwl_x11_compat protocol (see below) I think
that means resizes necessarily flush commits through but that surface
content updates can be corked until the next frame. And then for basic
sync clients, resizes wait for the netwm protocol before flushing
instead.
I'm not quite sure I understood the "flush commits through" part.
Post by Adam Jackson
Post by Pekka Paalanen
Mind, I am mostly thinking this in Weston XWM terms, which draws the
window decorations through X11 like a normal window manager.
That's fine. Honestly I'd prefer a world where the wayland server was
not also the window manager, you should be able to run twm if you want
(and have it work about as well as, say, twm on win32). That'd need an
x11 compat protocol to really handle well, but again, okay. I think
that's _better_ than the current model, where mutter just magically
knows to get X geometry info through this side channel and therefore
has to worry about races, instead of Xwayland always presenting a
logically consistent view of its world to the wayland server.
Indeed, the races are nasty. When looking into Weston XWM and how it
determines the input region and geometry of a surface (decorations
minus shadows) I was wondering if we could get Xwayland to just set the
right input region via Wayland. That would be just core Wayland, even.
Then we'd have the geometry sync'd with the right wl_surface.commits.

As for everything else, I couldn't say where the policies should be
set. That is, would zwl_x11_compat be written mainly in Wayland or X11
terms? I.e. who should be determining all the window types from X11
information which is lower level than what Wayland/desktop handles? It
is also possible that Wayland systems other than those using desktop
shell interfaces and concepts would still want to support Xwayland.
That is a quest I am unlikely to take on.


Thanks,
pq
Adam Jackson
2016-12-07 20:57:40 UTC
Permalink
Post by Pekka Paalanen
nice! Should I take only the property stuff, or should I try to
learn and integrate everything you have there?
I suppose I should start with only the property stuff and make my
mechanism work with that first.
I guess I should have said "steal whatever you need from that". The
property callback I'll insist on, the rest you can take or leave as you
wish.
Post by Pekka Paalanen
Post by Adam Jackson
For the no-sync case I think xwayland gets to invent whatever semantics
it likes best. Without a zwl_x11_compat protocol (see below) I think
that means resizes necessarily flush commits through but that surface
content updates can be corked until the next frame. And then for basic
sync clients, resizes wait for the netwm protocol before flushing
instead.
I'm not quite sure I understood the "flush commits through" part.
A commit that doesn't change surface dimensions is something X can
withhold from Wayland until the next frame (or whenever). So you might
have a sequence like:

A: PutImage
B: PutImage
C: ConfigureWindow

A and B would merely mark X's view of the surface as dirty, and we'd
commit that at the next frame; but if C happens before the next frame,
then because the wayland server is getting geometry via the X protocol
the wayland view of the surface needs to match that geometry, and thus
we would need to commit immediately, "flushing" the putimages through.
Post by Pekka Paalanen
Indeed, the races are nasty. When looking into Weston XWM and how it
determines the input region and geometry of a surface (decorations
minus shadows) I was wondering if we could get Xwayland to just set the
right input region via Wayland. That would be just core Wayland, even.
Then we'd have the geometry sync'd with the right wl_surface.commits.
Probably. Doesn't handle explicit window coordinates though.
Post by Pekka Paalanen
As for everything else, I couldn't say where the policies should be
set. That is, would zwl_x11_compat be written mainly in Wayland or X11
terms? I.e. who should be determining all the window types from X11
information which is lower level than what Wayland/desktop handles?
I don't think that's something I could answer in advance of actually
having written the protocol. (Or rather: answering it definitively
would require having written something that works correctly.)
Post by Pekka Paalanen
It
is also possible that Wayland systems other than those using desktop
shell interfaces and concepts would still want to support Xwayland.
That is a quest I am unlikely to take on.
Sure why not. I think what I'm saying is, if that wayland server
exposes at least some shell protocol and the compat extension, then
X+twm should work like it does on (say) cygwin, and if that ends up
breaking your shell metaphor, don't expose the compat extension.
Rootful mode will work either way.

The wayland server would of course be free to only expose the compat
extension to clients that it knows are Xwayland, if it really wanted to
make native clients work worse than X11.

- ajax
_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/
Pekka Paalanen
2016-12-09 12:24:11 UTC
Permalink
From: Pekka Paalanen <***@collabora.co.uk>

Hi,

this is v2 of _XWAYLAND_ALLOW_COMMITS property support in Xwayland to allow the
window manager to control when wl_surface.commits occur. The first revision was
posted here:
https://lists.x.org/archives/xorg-devel/2016-November/051913.html

The fundamental problem is that Xwayland and the Wayland compositor (window
manager) are racing, particularly when windows are being mapped. This can lead
to glitches. The specific glitch I bumped into is described at
https://phabricator.freedesktop.org/T7622 .

The proposed solution allows the WM to temporarily prohibit commits.

In this v2 series:
- patches 1 and 2 are new and needed to stop using XACE hooks
- patches 3 and 4 are the same as v1, R-b Olivier
- patch 5 changes are listed in the commit message

The v2 series is also available as a branch:
https://git.collabora.com/cgit/user/pq/xserver.git/log/?h=allow_commits-v2

This patch series is the Xwayland side of the fix to T7622. The Weston series
is here:
https://patchwork.freedesktop.org/series/16109/


Thanks,
pq

Adam Jackson (2):
dix: Pass the whole property into deliverPropertyNotifyEvent
dix: Add a callback chain for window property state change

Pekka Paalanen (3):
xwayland: refactor into xwl_window_post_damage()
xwayland: fix 'buffer' may be used uninitialized warning
xwayland: use _XWAYLAND_ALLOW_COMMITS property

dix/property.c | 23 +++++--
hw/xwayland/xwayland.c | 170 ++++++++++++++++++++++++++++++++++++++++++-------
hw/xwayland/xwayland.h | 3 +
include/property.h | 8 +++
4 files changed, 174 insertions(+), 30 deletions(-)
--
2.7.3

_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/m
Pekka Paalanen
2016-12-09 12:24:12 UTC
Permalink
From: Adam Jackson <***@redhat.com>

Instead of just the atom. No functional change.

Signed-off-by: Adam Jackson <***@redhat.com>
Signed-off-by: Pekka Paalanen <***@collabora.co.uk>
---
dix/property.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/dix/property.c b/dix/property.c
index fa4da2d..3447bca 100644
--- a/dix/property.c
+++ b/dix/property.c
@@ -106,14 +106,14 @@ dixLookupProperty(PropertyPtr *result, WindowPtr pWin, Atom propertyName,
}

static void
-deliverPropertyNotifyEvent(WindowPtr pWin, int state, Atom atom)
+deliverPropertyNotifyEvent(WindowPtr pWin, int state, PropertyPtr pProp)
{
xEvent event;
UpdateCurrentTimeIf();
event = (xEvent) {
.u.property.window = pWin->drawable.id,
.u.property.state = state,
- .u.property.atom = atom,
+ .u.property.atom = pProp->propertyName,
.u.property.time = currentTime.milliseconds,
};
event.u.u.type = PropertyNotify;
@@ -175,7 +175,7 @@ ProcRotateProperties(ClientPtr client)
delta += stuff->nAtoms;
for (i = 0; i < stuff->nAtoms; i++) {
j = (i + delta) % stuff->nAtoms;
- deliverPropertyNotifyEvent(pWin, PropertyNewValue, atoms[i]);
+ deliverPropertyNotifyEvent(pWin, PropertyNewValue, props[i]);

/* Preserve name and devPrivates */
props[j]->type = saved[i].type;
@@ -351,7 +351,7 @@ dixChangeWindowProperty(ClientPtr pClient, WindowPtr pWin, Atom property,
return rc;

if (sendevent)
- deliverPropertyNotifyEvent(pWin, PropertyNewValue, pProp->propertyName);
+ deliverPropertyNotifyEvent(pWin, PropertyNewValue, pProp);

return Success;
}
@@ -380,7 +380,7 @@ DeleteProperty(ClientPtr client, WindowPtr pWin, Atom propName)
prevProp->next = pProp->next;
}

- deliverPropertyNotifyEvent(pWin, PropertyDelete, pProp->propertyName);
+ deliverPropertyNotifyEvent(pWin, PropertyDelete, pProp);
free(pProp->data);
dixFreeObjectWithPrivates(pProp, PRIVATE_PROPERTY);
}
@@ -394,7 +394,7 @@ DeleteAllWindowProperties(WindowPtr pWin)

pProp = wUserProps(pWin);
while (pProp) {
- deliverPropertyNotifyEvent(pWin, PropertyDelete, pProp->propertyName);
+ deliverPropertyNotifyEvent(pWin, PropertyDelete, pProp);
pNextProp = pProp->next;
free(pProp->data);
dixFreeObjectWithPrivates(pProp, PRIVATE_PROPERTY);
@@ -517,7 +517,7 @@ ProcGetProperty(ClientPtr client)
};

if (stuff->delete && (reply.bytesAfter == 0))
- deliverPropertyNotifyEvent(pWin, PropertyDelete, pProp->propertyName);
+ deliverPropertyNotifyEvent(pWin, PropertyDelete, pProp);

WriteReplyToClient(client, sizeof(xGenericReply), &reply);
if (len) {
--
2.7.3

_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://
Pekka Paalanen
2016-12-09 12:24:14 UTC
Permalink
From: Pekka Paalanen <***@collabora.co.uk>

Refactor xwl_screen_post_damage() and split the window specific code
into a new function xwl_window_post_damage().

This is a pure refactoring, there are no behavioral changes. An assert
is added to xwl_window_post_damage() to ensure frame callbacks are not
leaked if a future patch changes the call.

Signed-off-by: Pekka Paalanen <***@collabora.co.uk>
Reviewed-by: Olivier Fourdan <***@redhat.com>
---
hw/xwayland/xwayland.c | 56 +++++++++++++++++++++++++++++---------------------
1 file changed, 33 insertions(+), 23 deletions(-)

diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index 9e1ecf8..c2ac4b3 100644
--- a/hw/xwayland/xwayland.c
+++ b/hw/xwayland/xwayland.c
@@ -458,44 +458,54 @@ static const struct wl_callback_listener frame_listener = {
};

static void
-xwl_screen_post_damage(struct xwl_screen *xwl_screen)
+xwl_window_post_damage(struct xwl_window *xwl_window)
{
- struct xwl_window *xwl_window, *next_xwl_window;
+ struct xwl_screen *xwl_screen = xwl_window->xwl_screen;
RegionPtr region;
BoxPtr box;
struct wl_buffer *buffer;
PixmapPtr pixmap;

- xorg_list_for_each_entry_safe(xwl_window, next_xwl_window,
- &xwl_screen->damage_window_list, link_damage) {
- /* If we're waiting on a frame callback from the server,
- * don't attach a new buffer. */
- if (xwl_window->frame_callback)
- continue;
+ assert(!xwl_window->frame_callback);

- region = DamageRegion(xwl_window->damage);
- pixmap = (*xwl_screen->screen->GetWindowPixmap) (xwl_window->window);
+ region = DamageRegion(xwl_window->damage);
+ pixmap = (*xwl_screen->screen->GetWindowPixmap) (xwl_window->window);

#if GLAMOR_HAS_GBM
- if (xwl_screen->glamor)
- buffer = xwl_glamor_pixmap_get_wl_buffer(pixmap);
+ if (xwl_screen->glamor)
+ buffer = xwl_glamor_pixmap_get_wl_buffer(pixmap);
#endif
- if (!xwl_screen->glamor)
- buffer = xwl_shm_pixmap_get_wl_buffer(pixmap);
+ if (!xwl_screen->glamor)
+ buffer = xwl_shm_pixmap_get_wl_buffer(pixmap);

- wl_surface_attach(xwl_window->surface, buffer, 0, 0);
+ wl_surface_attach(xwl_window->surface, buffer, 0, 0);

- box = RegionExtents(region);
- wl_surface_damage(xwl_window->surface, box->x1, box->y1,
- box->x2 - box->x1, box->y2 - box->y1);
+ box = RegionExtents(region);
+ wl_surface_damage(xwl_window->surface, box->x1, box->y1,
+ box->x2 - box->x1, box->y2 - box->y1);

- xwl_window->frame_callback = wl_surface_frame(xwl_window->surface);
- wl_callback_add_listener(xwl_window->frame_callback, &frame_listener, xwl_window);
+ xwl_window->frame_callback = wl_surface_frame(xwl_window->surface);
+ wl_callback_add_listener(xwl_window->frame_callback, &frame_listener, xwl_window);

- wl_surface_commit(xwl_window->surface);
- DamageEmpty(xwl_window->damage);
+ wl_surface_commit(xwl_window->surface);
+ DamageEmpty(xwl_window->damage);

- xorg_list_del(&xwl_window->link_damage);
+ xorg_list_del(&xwl_window->link_damage);
+}
+
+static void
+xwl_screen_post_damage(struct xwl_screen *xwl_screen)
+{
+ struct xwl_window *xwl_window, *next_xwl_window;
+
+ xorg_list_for_each_entry_safe(xwl_window, next_xwl_window,
+ &xwl_screen->damage_window_list, link_damage) {
+ /* If we're waiting on a frame callback from the server,
+ * don't attach a new buffer. */
+ if (xwl_window->frame_callback)
+ continue;
+
+ xwl_window_post_damage(xwl_window);
}
}
--
2.7.3

_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists
Pekka Paalanen
2016-12-09 12:24:13 UTC
Permalink
From: Adam Jackson <***@redhat.com>

This will be used by in-server features that need to react to property
changes. The first one will be _XWAYLAND_ALLOW_COMMITS.

Signed-off-by: Adam Jackson <***@redhat.com>
[Pekka: add commit message body]
Signed-off-by: Pekka Paalanen <***@collabora.co.uk>
---
dix/property.c | 9 +++++++++
include/property.h | 8 ++++++++
2 files changed, 17 insertions(+)

diff --git a/dix/property.c b/dix/property.c
index 3447bca..ff1d669 100644
--- a/dix/property.c
+++ b/dix/property.c
@@ -105,10 +105,17 @@ dixLookupProperty(PropertyPtr *result, WindowPtr pWin, Atom propertyName,
return rc;
}

+CallbackListPtr PropertyStateCallback;
+
static void
deliverPropertyNotifyEvent(WindowPtr pWin, int state, PropertyPtr pProp)
{
xEvent event;
+ PropertyStateRec rec = {
+ .win = pWin,
+ .prop = pProp,
+ .state = state
+ };
UpdateCurrentTimeIf();
event = (xEvent) {
.u.property.window = pWin->drawable.id,
@@ -117,6 +124,8 @@ deliverPropertyNotifyEvent(WindowPtr pWin, int state, PropertyPtr pProp)
.u.property.time = currentTime.milliseconds,
};
event.u.u.type = PropertyNotify;
+
+ CallCallbacks(&PropertyStateCallback, &rec);
DeliverEvents(pWin, &event, 1, (WindowPtr) NULL);
}

diff --git a/include/property.h b/include/property.h
index be875e9..d7ccff3 100644
--- a/include/property.h
+++ b/include/property.h
@@ -51,6 +51,14 @@ SOFTWARE.

typedef struct _Property *PropertyPtr;

+typedef struct _PropertyStateRec {
+ WindowPtr win;
+ PropertyPtr prop;
+ int state;
+} PropertyStateRec;
+
+extern CallbackListPtr PropertyStateCallback;
+
extern _X_EXPORT int dixLookupProperty(PropertyPtr * /*result */ ,
WindowPtr /*pWin */ ,
Atom /*proprty */ ,
--
2.7.3

_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org
Pekka Paalanen
2016-12-09 12:24:15 UTC
Permalink
From: Pekka Paalanen <***@collabora.co.uk>

Fix the following warning due to --disable-glamor:

CC Xwayland-xwayland.o
In file included from /home/pq/local/include/wayland-client.h:40:0,
from xwayland.h:35,
from xwayland.c:26:
xwayland.c: In function ‘block_handler’:
/home/pq/local/include/wayland-client-protocol.h:3446:2: warning: ‘buffer’ may be used uninitialized in this function [-Wmaybe-uninitialized]
wl_proxy_marshal((struct wl_proxy *) wl_surface,
^
xwayland.c:466:23: note: ‘buffer’ was declared here
struct wl_buffer *buffer;
^

Signed-off-by: Pekka Paalanen <***@collabora.co.uk>
Reviewed-by: Olivier Fourdan <***@redhat.com>
---
hw/xwayland/xwayland.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index c2ac4b3..9d79554 100644
--- a/hw/xwayland/xwayland.c
+++ b/hw/xwayland/xwayland.c
@@ -474,8 +474,8 @@ xwl_window_post_damage(struct xwl_window *xwl_window)
#if GLAMOR_HAS_GBM
if (xwl_screen->glamor)
buffer = xwl_glamor_pixmap_get_wl_buffer(pixmap);
+ else
#endif
- if (!xwl_screen->glamor)
buffer = xwl_shm_pixmap_get_wl_buffer(pixmap);

wl_surface_attach(xwl_window->surface, buffer, 0, 0);
--
2.7.3

_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Pekka Paalanen
2016-12-09 12:24:16 UTC
Permalink
From: Pekka Paalanen <***@collabora.co.uk>

The X11 window manager (XWM) of a Wayland compositor can use the
_XWAYLAND_ALLOW_COMMITS property to control when Xwayland sends
wl_surface.commit requests. If the property is not set, the behaviour
remains what it was.

XWM uses the property to inhibit commits until the window is ready to be
shown. This gives XWM time to set up the window decorations and internal
state before Xwayland does the first commit. XWM can use this to ensure
the first commit carries fully drawn decorations and the window
management state is correct when the window becomes visible.

Setting the property to zero inhibits further commits, and setting it to
non-zero allows commits. Deleting the property allows commits.

When the property is changed from zero to non-zero, there will be a
commit on next block_handler() call provided that some damage has been
recorded.

Without this patch (i.e. with the old behaviour) Xwayland can and will
commit the surface very soon as the application window has been realized
and drawn into. This races with XWM and may cause visible glitches.

v2:
- use PropertyStateCallback instead of XACE, based on the patch
"xwayland: Track per-window support for netwm frame sync" by
Adam Jackson
- check property type is XA_CARDINAL
- drop a useless memcpy()

Weston Bug: https://phabricator.freedesktop.org/T7622
Signed-off-by: Pekka Paalanen <***@collabora.co.uk>
---
hw/xwayland/xwayland.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++++
hw/xwayland/xwayland.h | 3 ++
2 files changed, 117 insertions(+)

diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index 9d79554..04a068f 100644
--- a/hw/xwayland/xwayland.c
+++ b/hw/xwayland/xwayland.c
@@ -27,6 +27,7 @@

#include <stdio.h>

+#include <X11/Xatom.h>
#include <selection.h>
#include <micmap.h>
#include <misyncshm.h>
@@ -34,6 +35,7 @@
#include <glx_extinit.h>
#include <os.h>
#include <xserver_poll.h>
+#include <propertyst.h>

#ifdef XF86VIDMODE
#include <X11/extensions/xf86vmproto.h>
@@ -115,6 +117,86 @@ xwl_screen_get(ScreenPtr screen)
return dixLookupPrivate(&screen->devPrivates, &xwl_screen_private_key);
}

+static void
+xwl_window_set_allow_commits_from_property(struct xwl_window *xwl_window,
+ PropertyPtr prop)
+{
+ static Bool warned = FALSE;
+ CARD32 *propdata;
+
+ if (prop->propertyName != xwl_window->xwl_screen->allow_commits_prop)
+ FatalError("Xwayland internal error: prop mismatch in %s.\n", __func__);
+
+ if (prop->type != XA_CARDINAL || prop->format != 32 || prop->size != 1) {
+ /* Not properly set, so fall back to safe and glitchy */
+ xwl_window->allow_commits = TRUE;
+
+ if (!warned) {
+ LogMessage(X_WARNING, "Window manager is misusing property %s.\n",
+ NameForAtom(prop->propertyName));
+ warned = TRUE;
+ }
+ return;
+ }
+
+ propdata = prop->data;
+ xwl_window->allow_commits = !!propdata[0];
+ DebugF("xwayland: win %d allow_commits = %d\n",
+ xwl_window->window->drawable.id, xwl_window->allow_commits);
+}
+
+static void
+xwl_property_callback(CallbackListPtr *pcbl, void *closure,
+ void *calldata)
+{
+ ScreenPtr screen = closure;
+ PropertyStateRec *rec = calldata;
+ struct xwl_screen *xwl_screen;
+ struct xwl_window *xwl_window;
+ Bool old_allow_commits;
+
+ if (rec->win->drawable.pScreen != screen)
+ return;
+
+ xwl_window = xwl_window_get(rec->win);
+ if (!xwl_window)
+ return;
+
+ xwl_screen = xwl_screen_get(screen);
+ if (rec->prop->propertyName != xwl_screen->allow_commits_prop)
+ return;
+
+ /* Handle _XWAYLAND_ALLOW_COMMITS property changes */
+
+ old_allow_commits = xwl_window->allow_commits;
+
+ switch (rec->state) {
+ case PropertyNewValue:
+ xwl_window_set_allow_commits_from_property(xwl_window, rec->prop);
+ break;
+
+ case PropertyDelete:
+ xwl_window->allow_commits = TRUE;
+ DebugF("xwayland: win %d allow_commit reverts to uncontrolled.\n",
+ xwl_window->window->drawable.id);
+ break;
+
+ default:
+ break;
+ }
+
+ /* If allow_commits turned from off to on, discard any frame
+ * callback we might be waiting for so that a new buffer is posted
+ * immediately through block_handler() if there is damage to post.
+ */
+ if (!old_allow_commits && xwl_window->allow_commits) {
+ if (xwl_window->frame_callback) {
+ wl_callback_destroy(xwl_window->frame_callback);
+ xwl_window->frame_callback = NULL;
+ }
+ }
+}
+
static Bool
xwl_close_screen(ScreenPtr screen)
{
@@ -122,6 +204,8 @@ xwl_close_screen(ScreenPtr screen)
struct xwl_output *xwl_output, *next_xwl_output;
struct xwl_seat *xwl_seat, *next_xwl_seat;

+ DeleteCallback(&PropertyStateCallback, xwl_property_callback, screen);
+
xorg_list_for_each_entry_safe(xwl_output, next_xwl_output,
&xwl_screen->output_list, link)
xwl_output_destroy(xwl_output);
@@ -262,6 +346,25 @@ xwl_pixmap_get(PixmapPtr pixmap)
}

static void
+xwl_window_init_allow_commits(struct xwl_window *xwl_window)
+{
+ PropertyPtr prop = NULL;
+ int ret;
+
+ ret = dixLookupProperty(&prop, xwl_window->window,
+ xwl_window->xwl_screen->allow_commits_prop,
+ serverClient, DixReadAccess);
+ if (ret == Success && prop) {
+ xwl_window_set_allow_commits_from_property(xwl_window, prop);
+ }
+ else {
+ xwl_window->allow_commits = TRUE;
+ DebugF("xwayland: win %d allow_commit is uncontrolled.\n",
+ xwl_window->window->drawable.id);
+ }
+}
+
+static void
send_surface_id_event(struct xwl_window *xwl_window)
{
static const char atom_name[] = "WL_SURFACE_ID";
@@ -376,6 +479,8 @@ xwl_realize_window(WindowPtr window)
dixSetPrivate(&window->devPrivates, &xwl_window_private_key, xwl_window);
xorg_list_init(&xwl_window->link_damage);

+ xwl_window_init_allow_commits(xwl_window);
+
return ret;

err_surf:
@@ -505,6 +610,9 @@ xwl_screen_post_damage(struct xwl_screen *xwl_screen)
if (xwl_window->frame_callback)
continue;

+ if (!xwl_window->allow_commits)
+ continue;
+
xwl_window_post_damage(xwl_window);
}
}
@@ -694,6 +802,7 @@ wm_selection_callback(CallbackListPtr *p, void *data, void *arg)
static Bool
xwl_screen_init(ScreenPtr pScreen, int argc, char **argv)
{
+ static const char allow_commits[] = "_XWAYLAND_ALLOW_COMMITS";
struct xwl_screen *xwl_screen;
Pixel red_mask, blue_mask, green_mask;
int ret, bpc, green_bpc, i;
@@ -849,6 +958,11 @@ xwl_screen_init(ScreenPtr pScreen, int argc, char **argv)
pScreen->CursorWarpedTo = xwl_cursor_warped_to;
pScreen->CursorConfinedTo = xwl_cursor_confined_to;

+ xwl_screen->allow_commits_prop = MakeAtom(allow_commits,
+ strlen(allow_commits),
+ TRUE);
+ AddCallback(&PropertyStateCallback, xwl_property_callback, pScreen);
+
return ret;
}

diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
index 5e5624b..91b7620 100644
--- a/hw/xwayland/xwayland.h
+++ b/hw/xwayland/xwayland.h
@@ -99,6 +99,8 @@ struct xwl_screen {
void *egl_display, *egl_context;
struct gbm_device *gbm;
struct glamor_context *glamor_ctx;
+
+ Atom allow_commits_prop;
};

struct xwl_window {
@@ -109,6 +111,7 @@ struct xwl_window {
DamagePtr damage;
struct xorg_list link_damage;
struct wl_callback *frame_callback;
+ Bool allow_commits;
};

#define MODIFIER_META 0x01
--
2.7.3

_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info:
Adam Jackson
2017-01-02 21:17:27 UTC
Permalink
Post by Pekka Paalanen
The X11 window manager (XWM) of a Wayland compositor can use the
_XWAYLAND_ALLOW_COMMITS property to control when Xwayland sends
wl_surface.commit requests. If the property is not set, the behaviour
remains what it was.
I'm still thinking over whether I like this or whether I'd rather have
this keyed off the netwm sync props. (Did we think that was a thing
that could work? Forgive me, I'm freshly back from holidays.) Still,
some style notes below.
Post by Pekka Paalanen
+static void
+xwl_window_set_allow_commits_from_property(struct xwl_window *xwl_window,
+                                           PropertyPtr prop)
+{
+    static Bool warned = FALSE;
+    CARD32 *propdata;
+
+    if (prop->propertyName != xwl_window->xwl_screen->allow_commits_prop)
+        FatalError("Xwayland internal error: prop mismatch in %s.\n", __func__);
+
+    if (prop->type != XA_CARDINAL || prop->format != 32 || prop->size != 1) {
+        /* Not properly set, so fall back to safe and glitchy */
+        xwl_window->allow_commits = TRUE;
+
+        if (!warned) {
+            LogMessage(X_WARNING, "Window manager is misusing property %s.\n",
+                       NameForAtom(prop->propertyName));
+            warned = TRUE;
+        }
+        return;
+    }
+
+    propdata = prop->data;
+    xwl_window->allow_commits = !!propdata[0];
+    DebugF("xwayland: win %d allow_commits = %d\n",
+           xwl_window->window->drawable.id, xwl_window->allow_commits);
+}
This seems odd. The primitive concept here is "allow commits or not",
not "inspect this property to tell me whether commits are okay". I
think this would be better factored as:

void xwl_window_allow_commits(struct xwl_window *win, Bool allow);
Post by Pekka Paalanen
+static void
+xwl_property_callback(CallbackListPtr *pcbl, void *closure,
+                      void *calldata)
+{
+    ScreenPtr screen = closure;
+    PropertyStateRec *rec = calldata;
+    struct xwl_screen *xwl_screen;
+    struct xwl_window *xwl_window;
+    Bool old_allow_commits;
+
+    if (rec->win->drawable.pScreen != screen)
+        return;
+
+    xwl_window = xwl_window_get(rec->win);
+    if (!xwl_window)
+        return;
+
+    xwl_screen = xwl_screen_get(screen);
+    if (rec->prop->propertyName != xwl_screen->allow_commits_prop)
+        return;
... change this to a table lookup for known property names? Maybe not a
thing we need yet. Either way, don't repeat this in the _allow_commits,
which you wouldn't be able to do anyway with the new signature.
Post by Pekka Paalanen
+    /* Handle _XWAYLAND_ALLOW_COMMITS property changes */
+
+    old_allow_commits = xwl_window->allow_commits;
+
+    switch (rec->state) {
+        xwl_window_set_allow_commits_from_property(xwl_window, rec->prop);
+        break;
+
+        xwl_window->allow_commits = TRUE;
+        DebugF("xwayland: win %d allow_commit reverts to uncontrolled.\n",
+               xwl_window->window->drawable.id);
+        break;
This would refactor as:

void xwl_property_allow_commits(struct xwl_window *, PropertyStateRec *);

which computes the allow boolean to give xwl_allow_commits.
Post by Pekka Paalanen
@@ -262,6 +346,25 @@ xwl_pixmap_get(PixmapPtr pixmap)
 }
 
 static void
+xwl_window_init_allow_commits(struct xwl_window *xwl_window)
+{
+    PropertyPtr prop = NULL;
+    int ret;
+
+    ret = dixLookupProperty(&prop, xwl_window->window,
+                            xwl_window->xwl_screen->allow_commits_prop,
+                            serverClient, DixReadAccess);
+    if (ret == Success && prop) {
+        xwl_window_set_allow_commits_from_property(xwl_window, prop);
+    }
+    else {
+        xwl_window->allow_commits = TRUE;
+        DebugF("xwayland: win %d allow_commit is uncontrolled.\n",
+               xwl_window->window->drawable.id);
+    }
+}
Likewise here you wouldn't need to look up the property first and could
just initialize. That assumes the property atom actually exists, so...
Post by Pekka Paalanen
@@ -694,6 +802,7 @@ wm_selection_callback(CallbackListPtr *p, void *data, void *arg)
 static Bool
 xwl_screen_init(ScreenPtr pScreen, int argc, char **argv)
 {
+    static const char allow_commits[] = "_XWAYLAND_ALLOW_COMMITS";
     struct xwl_screen *xwl_screen;
     Pixel red_mask, blue_mask, green_mask;
     int ret, bpc, green_bpc, i;
@@ -849,6 +958,11 @@ xwl_screen_init(ScreenPtr pScreen, int argc, char **argv)
     pScreen->CursorWarpedTo = xwl_cursor_warped_to;
     pScreen->CursorConfinedTo = xwl_cursor_confined_to;
 
+    xwl_screen->allow_commits_prop = MakeAtom(allow_commits,
+                                              strlen(allow_commits),
+                                              TRUE);
+    AddCallback(&PropertyStateCallback, xwl_property_callback, pScreen);
+
     return ret;
 }
... here you would check that MakeAtom didn't return BAD_RESOURCE and
return false if it did. (If MakeAtom fails in screen init you're doomed
anyway.)

- ajax
_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-d
Olivier Fourdan
2017-01-03 09:31:39 UTC
Permalink
Hi,

----- Original Message -----
Post by Adam Jackson
Post by Pekka Paalanen
The X11 window manager (XWM) of a Wayland compositor can use the
_XWAYLAND_ALLOW_COMMITS property to control when Xwayland sends
wl_surface.commit requests. If the property is not set, the behaviour
remains what it was.
I'm still thinking over whether I like this or whether I'd rather have
this keyed off the netwm sync props. (Did we think that was a thing
that could work? Forgive me, I'm freshly back from holidays.) [...]
Maybe it's two different goals. If I understand correctly, this patch was initially intended for the initial map of the window.

FWIW I spent quite a few days before the holidays experimenting with this, trying to see if that could be used to help with the black border issue reported in mutter with server-side decorations [1] on applications implementing the NET_WM_SYNC extended protocol [2].

Unfortunately, I didn't have much success in doing so.

Freezing the commit in either meta_window_actor_freeze() or meta_window_actor_pre_paint() and re-allowing the commit in meta_window_actor_thaw() or meta_window_actor_post_paint() didn't help at all with the visible black border during resize, and made the resize more sluggish because while the commit is frozen, the surface is not resized but can still be moved so resizing from the left or top looks awful, from a user stand point.

I think I could trace the black border issue in mutter back to meta_window_actor_update_opaque_region(), but I was not able to use the _XWAYLAND_ALLOW_COMMITS property to fix or even improve the problem, so I am not even sure _XWAYLAND_ALLOW_COMMITS is the solution for the problem I am trying to fix - Granted, I am not an expert in this area of mutter, but I start to wonder if _XWAYLAND_ALLOW_COMMITS is the solution to the specific issue I am trying to solve.

So, we might eventually need both, something like _XWAYLAND_ALLOW_COMMITS for the initial map and also have Xwayland monitoring NET_WM_SYNC properties.

Cheers,
Olivier

[1] https://bugzilla.gnome.org/show_bug.cgi?id=767212
[2] http://fishsoup.net/misc/wm-spec-synchronization.html

_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: htt
Pekka Paalanen
2017-01-12 14:27:10 UTC
Permalink
On Tue, 3 Jan 2017 04:31:39 -0500 (EST)
Post by Olivier Fourdan
Hi,
----- Original Message -----
Post by Adam Jackson
Post by Pekka Paalanen
The X11 window manager (XWM) of a Wayland compositor can use the
_XWAYLAND_ALLOW_COMMITS property to control when Xwayland sends
wl_surface.commit requests. If the property is not set, the behaviour
remains what it was.
I'm still thinking over whether I like this or whether I'd rather have
this keyed off the netwm sync props. (Did we think that was a thing
that could work? Forgive me, I'm freshly back from holidays.) [...]
Maybe it's two different goals. If I understand correctly, this patch
was initially intended for the initial map of the window.
Hi,

yes, that's correct.

Btw. I would love to write a documentation patch explaining what
_XWAYLAND_ALLOW_COMMITS is meant for, how it works, and how it should
be used. Where would such documentation live? I might suggest putting
it in the Wayland docbook, but that would be a bit odd, since it is
documenting X.org's Xwayland implementation. But I bet it would make
reviewing this patch easier.

Originally I was working on letting -geometry option of X11 programs to
allow setting the initial position of the window. I got it implemented
in Weston, but with the regression that all decorated windows would
appear to jump once when mapped (without -geometry, even). The root
cause for that was the racing between Xwayland and the XWM/compositor.

Xwayland did the first commit before the decorations were drawn, that
caused the compositor to position and show the window, then the
decorations appeared and the window needed to move to account for the
added decorations. Except that is not the whole truth, because if it
happened like that, I would see the decorations appear around the
window, but instead I see the decorated window jump to another
position, so there is probably also a drawing/compositing race going
on. Plus I am not quite sure if Xwayland is still single-buffered to
towards the compositor which would be incorrect behaviour of a Wayland
client (but was originally written so, because maintaining multiple
buffers would have cost significantly more and X11 drawing does not
have a concept of "completed frames" anyway...).

Anyway, the initial mapping is a mess if Xwayland commits before XWM is
ready, and it would be quite difficult for XWM to internally postpone
the mapping in the compositor. I hope we agreed at least that much
already.

This patch is particularly for the initial mapping, especially for apps
and compositors that do not use NET_WM_SYNC (yet).

Adding support for NET_WM_SYNC protocol would be the next step (in
Weston), but I am not sure when I would get there. I suppose the XWM
could drive it via _XWAYLAND_ALLOW_COMMITS or Xwayland could hook up to
the NET_WM_SYNC protocol messages itself like Adam and Daniel discussed.

However, would NET_WM_SYNC apply to initial mapping of windows for
clients/XWM that do support it?

I read the _NET_WM_SYNC_REQUEST spec [1] and it does not say, it only
talks about resizing and ConfigureNotify. Do apps generally set up
NET_WM_SYNC before they map the window?

[1] https://specifications.freedesktop.org/wm-spec/wm-spec-latest.html#idm140200472538288
Post by Olivier Fourdan
FWIW I spent quite a few days before the holidays experimenting with
this, trying to see if that could be used to help with the black
border issue reported in mutter with server-side decorations [1] on
applications implementing the NET_WM_SYNC extended protocol [2].
Unfortunately, I didn't have much success in doing so.
Freezing the commit in either meta_window_actor_freeze() or
meta_window_actor_pre_paint() and re-allowing the commit in
meta_window_actor_thaw() or meta_window_actor_post_paint() didn't
help at all with the visible black border during resize, and made the
resize more sluggish because while the commit is frozen, the surface
is not resized but can still be moved so resizing from the left or
top looks awful, from a user stand point.
I think I could trace the black border issue in mutter back to
meta_window_actor_update_opaque_region(), but I was not able to use
the _XWAYLAND_ALLOW_COMMITS property to fix or even improve the
problem, so I am not even sure _XWAYLAND_ALLOW_COMMITS is the
solution for the problem I am trying to fix - Granted, I am not an
expert in this area of mutter, but I start to wonder if
_XWAYLAND_ALLOW_COMMITS is the solution to the specific issue I am
trying to solve.
Olivier, when we talked in IRC, you said you weren't sure at which
point mutter is actually emitting the X11 commands to draw the
decorations. Did you find out? I believe it would be very important to
draw decorations while commits are disabled.

Also mind that drawing decorations goes to X11, while compositing the
decorations happens much later when the content comes back from
Xwayland with a commit. So if you draw decors and immediately
composite, you likely will be compositing outdated content. If you do
that and use the new state instead of the old, I think you'd be pretty
guaranteed to see bad rendering. Can you check if that is happening?

Weston has some machinery to set up internal pending state, instead of
current state, when XWM updates decorations etc. via X11. That pending
state gets latched in on the next commit from Xwayland. But even that
has a false assumption that the next commit is right one - Xwayland
could be committing old stuff around the same time and be mistaken as
the commit XWM expected.

The latter problem is particularly with input regions in Weston
(theoretical - I'm not sure there are actually noticeable effects).
I've been thinking if we could give the Wayland state update to
Xwayland so that Xwayland could push it out with the correct commit.
Right now, Xwayland never uses wl_surface.set_input_region (or
set_opaque_region) which is part of the Wayland core protocol. That is
a whole another story though.
Post by Olivier Fourdan
So, we might eventually need both, something like
_XWAYLAND_ALLOW_COMMITS for the initial map and also have Xwayland
monitoring NET_WM_SYNC properties.
Yeah. I think it should be pretty easy and not hugely too bad to have
XWM use _XWAYLAND_ALLOW_COMMITS also for NET_WM_SYNC so we didn't need
the NET_WM_SYNC support in Xwayland. Of course, it would be more
efficient in Xwayland proper.
Post by Olivier Fourdan
Cheers,
Olivier
[1] https://bugzilla.gnome.org/show_bug.cgi?id=767212
[2] http://fishsoup.net/misc/wm-spec-synchronization.html
Thanks,
pq
Pekka Paalanen
2017-01-18 13:55:10 UTC
Permalink
On Thu, 12 Jan 2017 16:27:10 +0200
Post by Pekka Paalanen
On Tue, 3 Jan 2017 04:31:39 -0500 (EST)
Post by Olivier Fourdan
Hi,
----- Original Message -----
Post by Adam Jackson
Post by Pekka Paalanen
The X11 window manager (XWM) of a Wayland compositor can use the
_XWAYLAND_ALLOW_COMMITS property to control when Xwayland sends
wl_surface.commit requests. If the property is not set, the behaviour
remains what it was.
I'm still thinking over whether I like this or whether I'd rather have
this keyed off the netwm sync props. (Did we think that was a thing
that could work? Forgive me, I'm freshly back from holidays.) [...]
Maybe it's two different goals. If I understand correctly, this patch
was initially intended for the initial map of the window.
Hi,
yes, that's correct.
Btw. I would love to write a documentation patch explaining what
_XWAYLAND_ALLOW_COMMITS is meant for, how it works, and how it should
be used. Where would such documentation live? I might suggest putting
it in the Wayland docbook, but that would be a bit odd, since it is
documenting X.org's Xwayland implementation. But I bet it would make
reviewing this patch easier.
Originally I was working on letting -geometry option of X11 programs to
allow setting the initial position of the window. I got it implemented
in Weston, but with the regression that all decorated windows would
appear to jump once when mapped (without -geometry, even). The root
cause for that was the racing between Xwayland and the XWM/compositor.
Xwayland did the first commit before the decorations were drawn, that
caused the compositor to position and show the window, then the
decorations appeared and the window needed to move to account for the
added decorations. Except that is not the whole truth, because if it
happened like that, I would see the decorations appear around the
window, but instead I see the decorated window jump to another
position, so there is probably also a drawing/compositing race going
on. Plus I am not quite sure if Xwayland is still single-buffered to
towards the compositor which would be incorrect behaviour of a Wayland
client (but was originally written so, because maintaining multiple
buffers would have cost significantly more and X11 drawing does not
have a concept of "completed frames" anyway...).
Anyway, the initial mapping is a mess if Xwayland commits before XWM is
ready, and it would be quite difficult for XWM to internally postpone
the mapping in the compositor. I hope we agreed at least that much
already.
This patch is particularly for the initial mapping, especially for apps
and compositors that do not use NET_WM_SYNC (yet).
Adding support for NET_WM_SYNC protocol would be the next step (in
Weston), but I am not sure when I would get there. I suppose the XWM
could drive it via _XWAYLAND_ALLOW_COMMITS or Xwayland could hook up to
the NET_WM_SYNC protocol messages itself like Adam and Daniel discussed.
Hi all,

here is an update on the Weston side:
https://lists.freedesktop.org/archives/wayland-devel/2017-January/032712.html

The related Weston patches series has shrunk from 24 to 3 patches as
lots of them have been merged. The stuff about _XWAYLAND_ALLOW_COMMITS
is still pending.

Given the development on Weston side, would you demand implementing
NET_WM_SYNC_REQUEST support in Weston before deciding whether to merge
_XWAYLAND_ALLOW_COMMITS support in Xwayland, or is the rationale in the
remaining Weston patches enough to justify it already?

The window jumping I talked about no longer happens, due to reordering
the patch series, but I believe that is just the race being won rather
than lost most of the time and that the race which
_XWAYLAND_ALLOW_COMMITS will remove still exists.


Thanks,
pq
Olivier Fourdan
2017-01-18 15:42:22 UTC
Permalink
Hi Pekka,

----- Original Message -----
Post by Pekka Paalanen
https://lists.freedesktop.org/archives/wayland-devel/2017-January/032712.html
The related Weston patches series has shrunk from 24 to 3 patches as
lots of them have been merged. The stuff about _XWAYLAND_ALLOW_COMMITS
is still pending.
Given the development on Weston side, would you demand implementing
NET_WM_SYNC_REQUEST support in Weston before deciding whether to merge
_XWAYLAND_ALLOW_COMMITS support in Xwayland, or is the rationale in the
remaining Weston patches enough to justify it already?
I meant to reply your previous email but didn't quite finish it, sorry...

What I meant to say there is NET_WM_SYNC and _XWAYLAND_ALLOW_COMMITS are two different things and I don't think you can reach the same goals using NET_WM_SYNC_REQUEST.

The goal of NET_WM_SYNC is to make sure the window manager is not flooding the client with configure requests while the user resizes the window. Without NET_WM_SYNC, you can easily see the client window/repaint lagging behind when resizing even with an X11 compositor - That's quite different from what _XWAYLAND_ALLOW_COMMITS is meant for.

Not all apps use NET_WM_SYNC, actually few do (mostly gtk and qt based apps) when considering the large number of X11 apps available, so you cannot rely on NET_WM_SYNC being available, whereas having _XWAYLAND_ALLOW_COMMITS in Xwayland make it available for all X11 clients running on Wayland with a compositor taking advantage of it.

So, *IMHO* _XWAYLAND_ALLOW_COMMITS should not depend on weston implementing NET_WM_SYNC_REQUEST. Sorry if I caused some confusion.
Post by Pekka Paalanen
The window jumping I talked about no longer happens, due to reordering
the patch series, but I believe that is just the race being won rather
than lost most of the time and that the race which
_XWAYLAND_ALLOW_COMMITS will remove still exists.
Cheers,
Olivier
_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo
Pekka Paalanen
2017-01-19 10:28:02 UTC
Permalink
On Wed, 18 Jan 2017 10:42:22 -0500 (EST)
Post by Olivier Fourdan
Hi Pekka,
----- Original Message -----
Post by Pekka Paalanen
https://lists.freedesktop.org/archives/wayland-devel/2017-January/032712.html
The related Weston patches series has shrunk from 24 to 3 patches as
lots of them have been merged. The stuff about _XWAYLAND_ALLOW_COMMITS
is still pending.
Given the development on Weston side, would you demand implementing
NET_WM_SYNC_REQUEST support in Weston before deciding whether to merge
_XWAYLAND_ALLOW_COMMITS support in Xwayland, or is the rationale in the
remaining Weston patches enough to justify it already?
I meant to reply your previous email but didn't quite finish it, sorry...
What I meant to say there is NET_WM_SYNC and _XWAYLAND_ALLOW_COMMITS
are two different things and I don't think you can reach the same
goals using NET_WM_SYNC_REQUEST.
The goal of NET_WM_SYNC is to make sure the window manager is not
flooding the client with configure requests while the user resizes
the window. Without NET_WM_SYNC, you can easily see the client
window/repaint lagging behind when resizing even with an X11
compositor - That's quite different from what _XWAYLAND_ALLOW_COMMITS
is meant for.
Not all apps use NET_WM_SYNC, actually few do (mostly gtk and qt
based apps) when considering the large number of X11 apps available,
so you cannot rely on NET_WM_SYNC being available, whereas having
_XWAYLAND_ALLOW_COMMITS in Xwayland make it available for all X11
clients running on Wayland with a compositor taking advantage of it.
So, *IMHO* _XWAYLAND_ALLOW_COMMITS should not depend on weston
implementing NET_WM_SYNC_REQUEST. Sorry if I caused some confusion.
Hi Olivier,

I'm glad we agree. :-)

In my current Weston implementation, ALLOW_COMMITS is only used for the
initial map. I suppose it could also be used to guarantee flicker-free
decorations: just before sending Configure disable commits, and
enable commits after the decorations have been "drawn" in the new size.
Do you think that might be a good idea to implement in the future?

Then there is the question of whether NET_WM_SYNC should automatically
disable/enable commits in Xwayland or do we just implement that in XWM
via _XWAYLAND_ALLOW_COMMITS, but that's completely orthogonal.


Thanks,
pq
Pekka Paalanen
2017-02-17 11:56:41 UTC
Permalink
On Thu, 19 Jan 2017 12:28:02 +0200
Post by Pekka Paalanen
On Wed, 18 Jan 2017 10:42:22 -0500 (EST)
Post by Olivier Fourdan
Hi Pekka,
----- Original Message -----
Post by Pekka Paalanen
https://lists.freedesktop.org/archives/wayland-devel/2017-January/032712.html
The related Weston patches series has shrunk from 24 to 3 patches as
lots of them have been merged. The stuff about _XWAYLAND_ALLOW_COMMITS
is still pending.
Given the development on Weston side, would you demand implementing
NET_WM_SYNC_REQUEST support in Weston before deciding whether to merge
_XWAYLAND_ALLOW_COMMITS support in Xwayland, or is the rationale in the
remaining Weston patches enough to justify it already?
I meant to reply your previous email but didn't quite finish it, sorry...
What I meant to say there is NET_WM_SYNC and _XWAYLAND_ALLOW_COMMITS
are two different things and I don't think you can reach the same
goals using NET_WM_SYNC_REQUEST.
The goal of NET_WM_SYNC is to make sure the window manager is not
flooding the client with configure requests while the user resizes
the window. Without NET_WM_SYNC, you can easily see the client
window/repaint lagging behind when resizing even with an X11
compositor - That's quite different from what _XWAYLAND_ALLOW_COMMITS
is meant for.
Not all apps use NET_WM_SYNC, actually few do (mostly gtk and qt
based apps) when considering the large number of X11 apps available,
so you cannot rely on NET_WM_SYNC being available, whereas having
_XWAYLAND_ALLOW_COMMITS in Xwayland make it available for all X11
clients running on Wayland with a compositor taking advantage of it.
So, *IMHO* _XWAYLAND_ALLOW_COMMITS should not depend on weston
implementing NET_WM_SYNC_REQUEST. Sorry if I caused some confusion.
Hi Olivier,
I'm glad we agree. :-)
In my current Weston implementation, ALLOW_COMMITS is only used for the
initial map. I suppose it could also be used to guarantee flicker-free
decorations: just before sending Configure disable commits, and
enable commits after the decorations have been "drawn" in the new size.
Do you think that might be a good idea to implement in the future?
Then there is the question of whether NET_WM_SYNC should automatically
disable/enable commits in Xwayland or do we just implement that in XWM
via _XWAYLAND_ALLOW_COMMITS, but that's completely orthogonal.
Hi Adam,

you asked for a branch and here it is, but note: do NOT pull this directly.

It contains a patch to fix the build that I needed to verify the
branch, but probably should not be merged as is. More of that topic
here:
https://lists.x.org/archives/xorg-devel/2017-February/052770.html

I can trivially do a re-spin after the build issues are fixed upstream,
if you wish.

The following changes since commit 461aa95591fef046d3b9e8ab9182fd750a6f1de2:

tests: fixes for non-enable-xorg build (2017-02-17 12:02:53 +0200)

are available in the git repository at:

git://git.collabora.co.uk/git/user/pq/xserver.git allow_commits-v3-rebased

for you to fetch changes up to d90c0ca4132bef2376ffafb4f1a2269be6278c70:

xwayland: use _XWAYLAND_ALLOW_COMMITS property (2017-02-17 12:21:17 +0200)

----------------------------------------------------------------
Adam Jackson (2):
dix: Pass the whole property into deliverPropertyNotifyEvent
dix: Add a callback chain for window property state change

Pekka Paalanen (3):
xwayland: refactor into xwl_window_post_damage()
xwayland: fix 'buffer' may be used uninitialized warning
xwayland: use _XWAYLAND_ALLOW_COMMITS property

dix/property.c | 23 +++++++----
hw/xwayland/xwayland.c | 177 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------
hw/xwayland/xwayland.h | 3 ++
include/property.h | 8 ++++
4 files changed, 181 insertions(+), 30 deletions(-)


The last patch is still missing anyone's review.

This branch is the same as the v3 submission, just rebased on top of
current master:
https://patchwork.freedesktop.org/series/16610/

The last time we were still wondering if this is was a good enough idea
or needed more use first. You haven't commented on the latest my and
Olivier's discussion or Daniel's reply.

I would warmly welcome more acks for the last patch.

The corresponding Weston patches are still here:
https://lists.freedesktop.org/archives/wayland-devel/2017-January/032712.html
https://patchwork.freedesktop.org/series/18180/


Thanks,
pq
Adam Jackson
2017-02-23 18:53:31 UTC
Permalink
  tests: fixes for non-enable-xorg build (2017-02-17 12:02:53 +0200)
  git://git.collabora.co.uk/git/user/pq/xserver.git allow_commits-v3-rebased
  xwayland: use _XWAYLAND_ALLOW_COMMITS property (2017-02-17 12:21:17 +0200)
----------------------------------------------------------------
      dix: Pass the whole property into deliverPropertyNotifyEvent
      dix: Add a callback chain for window property state change
      xwayland: refactor into xwl_window_post_damage()
      xwayland: fix 'buffer' may be used uninitialized warning
      xwayland: use _XWAYLAND_ALLOW_COMMITS property
 dix/property.c         |  23 +++++++----
 hw/xwayland/xwayland.c | 177 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------
 hw/xwayland/xwayland.h |   3 ++
 include/property.h     |   8 ++++
 4 files changed, 181 insertions(+), 30 deletions(-)
The last patch is still missing anyone's review.
This branch is the same as the v3 submission, just rebased on top of
https://patchwork.freedesktop.org/series/16610/
The last time we were still wondering if this is was a good enough idea
or needed more use first. You haven't commented on the latest my and
Olivier's discussion or Daniel's reply.
Nah, I'm convinced. Merged:

remote: I: patch #126568 updated using rev 50bcea8be337ea983e464f2b5b8b2dc6d1024532.
remote: I: patch #126570 updated using rev 8e3f9ce6c06e7605832c55dfd180396f66ec8b66.
remote: I: patch #123670 updated using rev f7b8560f23ac5582e2f97dc9f6de32a42e61e520.
remote: I: patch #123669 updated using rev a6308cea602f688ac653e3466cd57767e02093a9.
remote: I: patch #133671 updated using rev 9f4d308cdad957a354912f17febe5bcad7f44812.
remote: I: 5 patch(es) updated to state Accepted.
To ssh://git.freedesktop.org/git/xorg/xserver
fde5cd7..9f4d308 master -> master

- ajax
_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-dev

Daniel Stone
2017-01-03 10:31:24 UTC
Permalink
Hi,
Post by Adam Jackson
Post by Pekka Paalanen
The X11 window manager (XWM) of a Wayland compositor can use the
_XWAYLAND_ALLOW_COMMITS property to control when Xwayland sends
wl_surface.commit requests. If the property is not set, the behaviour
remains what it was.
I'm still thinking over whether I like this or whether I'd rather have
this keyed off the netwm sync props. (Did we think that was a thing
that could work? Forgive me, I'm freshly back from holidays.) Still,
some style notes below.
Hm. So, is what you're proposing that the server would intercept
_NET_WM_SYNC_REQUEST ClientMessages from WM -> client and not send any
further commits for that window's surface, until the last SYNC_REQUEST
sent and the SYNC_REQUEST_COUNTER become coherent? I think that'd
largely work; it does exclude NetWM clients, but eh.

Cheers,
Daniel
_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Pekka Paalanen
2017-01-12 11:56:14 UTC
Permalink
On Mon, 02 Jan 2017 16:17:27 -0500
Post by Adam Jackson
Post by Pekka Paalanen
The X11 window manager (XWM) of a Wayland compositor can use the
_XWAYLAND_ALLOW_COMMITS property to control when Xwayland sends
wl_surface.commit requests. If the property is not set, the behaviour
remains what it was.
I'm still thinking over whether I like this or whether I'd rather have
this keyed off the netwm sync props. (Did we think that was a thing
that could work? Forgive me, I'm freshly back from holidays.) Still,
some style notes below.
Hi Adam,

I'll continue that discussion in a reply to Olivier, replies on the
mechanics below. Just fresh (all paged out) from the holidays myself.
Post by Adam Jackson
Post by Pekka Paalanen
+static void
+xwl_window_set_allow_commits_from_property(struct xwl_window *xwl_window,
+                                           PropertyPtr prop)
+{
+    static Bool warned = FALSE;
+    CARD32 *propdata;
+
+    if (prop->propertyName != xwl_window->xwl_screen->allow_commits_prop)
+        FatalError("Xwayland internal error: prop mismatch in %s.\n", __func__);
This check is purely defensive programming in case someone later edits
the code. It's essentially an assert() to say "never call this function
with any other property". Do you not like such "documentation"?
Post by Adam Jackson
Post by Pekka Paalanen
+
+    if (prop->type != XA_CARDINAL || prop->format != 32 || prop->size != 1) {
+        /* Not properly set, so fall back to safe and glitchy */
+        xwl_window->allow_commits = TRUE;
+
+        if (!warned) {
+            LogMessage(X_WARNING, "Window manager is misusing property %s.\n",
+                       NameForAtom(prop->propertyName));
+            warned = TRUE;
+        }
+        return;
+    }
+
+    propdata = prop->data;
+    xwl_window->allow_commits = !!propdata[0];
+    DebugF("xwayland: win %d allow_commits = %d\n",
+           xwl_window->window->drawable.id, xwl_window->allow_commits);
+}
This seems odd. The primitive concept here is "allow commits or not",
not "inspect this property to tell me whether commits are okay". I
Well, every place where this is called, really needs to inspect the
property, because it may or may not be set, and it may or may not be set
with the correct types etc.
Post by Adam Jackson
void xwl_window_allow_commits(struct xwl_window *win, Bool allow);
That function would be a single assignment, and I was lazy writing it,
but I can do that - it will be easier to maintain with a function
instead of a plain assignment. I would still prefer keeping the
different debug messages for the cases where the value is set
implicitly instead of from the property.
Post by Adam Jackson
Post by Pekka Paalanen
+static void
+xwl_property_callback(CallbackListPtr *pcbl, void *closure,
+                      void *calldata)
+{
+    ScreenPtr screen = closure;
+    PropertyStateRec *rec = calldata;
+    struct xwl_screen *xwl_screen;
+    struct xwl_window *xwl_window;
+    Bool old_allow_commits;
+
+    if (rec->win->drawable.pScreen != screen)
+        return;
+
+    xwl_window = xwl_window_get(rec->win);
+    if (!xwl_window)
+        return;
+
+    xwl_screen = xwl_screen_get(screen);
+    if (rec->prop->propertyName != xwl_screen->allow_commits_prop)
+        return;
... change this to a table lookup for known property names? Maybe not a
thing we need yet. Either way, don't repeat this in the _allow_commits,
which you wouldn't be able to do anyway with the new signature.
Indeed, I did think of something to allow easily adding more properties
we might want to watch, but since we don't yet, I decided to leave that for
when (if) we actually do. When we do, the below stuff would be split into a
separate function "handle the allow_commits_prop stuff". I can do that
already, too.

The repeated check however I explained above, since this is not the
only call site of xwl_window_set_allow_commits_from_property().
Post by Adam Jackson
Post by Pekka Paalanen
+    /* Handle _XWAYLAND_ALLOW_COMMITS property changes */
+
+    old_allow_commits = xwl_window->allow_commits;
+
+    switch (rec->state) {
+        xwl_window_set_allow_commits_from_property(xwl_window, rec->prop);
+        break;
+
+        xwl_window->allow_commits = TRUE;
+        DebugF("xwayland: win %d allow_commit reverts to uncontrolled.\n",
+               xwl_window->window->drawable.id);
+        break;
void xwl_property_allow_commits(struct xwl_window *, PropertyStateRec *);
which computes the allow boolean to give xwl_allow_commits.
Yup.
Post by Adam Jackson
Post by Pekka Paalanen
@@ -262,6 +346,25 @@ xwl_pixmap_get(PixmapPtr pixmap)
 }
 
 static void
+xwl_window_init_allow_commits(struct xwl_window *xwl_window)
+{
+    PropertyPtr prop = NULL;
+    int ret;
+
+    ret = dixLookupProperty(&prop, xwl_window->window,
+                            xwl_window->xwl_screen->allow_commits_prop,
+                            serverClient, DixReadAccess);
+    if (ret == Success && prop) {
+        xwl_window_set_allow_commits_from_property(xwl_window, prop);
+    }
+    else {
+        xwl_window->allow_commits = TRUE;
+        DebugF("xwayland: win %d allow_commit is uncontrolled.\n",
+               xwl_window->window->drawable.id);
+    }
+}
Likewise here you wouldn't need to look up the property first and could
just initialize.
I'm not sure what you mean, because I really want to initialize from
the current value of the property if it exists, not unconditionally to
a constant. The WM can (and IIRC normally will) set the property before
the window is realized.

Ah, it must be set before the window is realized because otherwise the
first commit would race against setting ALLOW_COMMITS to false.

Struct xwl_window gets created when the window is realized, so it
cannot be initialized earlier.
Post by Adam Jackson
That assumes the property atom actually exists, so...
Post by Pekka Paalanen
@@ -694,6 +802,7 @@ wm_selection_callback(CallbackListPtr *p, void *data, void *arg)
 static Bool
 xwl_screen_init(ScreenPtr pScreen, int argc, char **argv)
 {
+    static const char allow_commits[] = "_XWAYLAND_ALLOW_COMMITS";
     struct xwl_screen *xwl_screen;
     Pixel red_mask, blue_mask, green_mask;
     int ret, bpc, green_bpc, i;
@@ -849,6 +958,11 @@ xwl_screen_init(ScreenPtr pScreen, int argc, char **argv)
     pScreen->CursorWarpedTo = xwl_cursor_warped_to;
     pScreen->CursorConfinedTo = xwl_cursor_confined_to;
 
+    xwl_screen->allow_commits_prop = MakeAtom(allow_commits,
+                                              strlen(allow_commits),
+                                              TRUE);
+    AddCallback(&PropertyStateCallback, xwl_property_callback, pScreen);
+
     return ret;
 }
... here you would check that MakeAtom didn't return BAD_RESOURCE and
return false if it did. (If MakeAtom fails in screen init you're doomed
anyway.)
I did wonder if that might fail, I'll add the check.


Thanks,
pq
Pekka Paalanen
2017-01-17 10:18:14 UTC
Permalink
From: Pekka Paalanen <***@collabora.co.uk>

The X11 window manager (XWM) of a Wayland compositor can use the
_XWAYLAND_ALLOW_COMMITS property to control when Xwayland sends
wl_surface.commit requests. If the property is not set, the behaviour
remains what it was.

XWM uses the property to inhibit commits until the window is ready to be
shown. This gives XWM time to set up the window decorations and internal
state before Xwayland does the first commit. XWM can use this to ensure
the first commit carries fully drawn decorations and the window
management state is correct when the window becomes visible.

Setting the property to zero inhibits further commits, and setting it to
non-zero allows commits. Deleting the property allows commits.

When the property is changed from zero to non-zero, there will be a
commit on next block_handler() call provided that some damage has been
recorded.

Without this patch (i.e. with the old behaviour) Xwayland can and will
commit the surface very soon as the application window has been realized
and drawn into. This races with XWM and may cause visible glitches.

v3:
- introduced a simple setter for xwl_window::allow_commits
- split xwl_window_property_allow_commits() out of
xwl_property_callback()
- check MakeAtom(_XWAYLAND_ALLOW_COMMITS)

v2:
- use PropertyStateCallback instead of XACE, based on the patch
"xwayland: Track per-window support for netwm frame sync" by
Adam Jackson
- check property type is XA_CARDINAL
- drop a useless memcpy()

Weston Bug: https://phabricator.freedesktop.org/T7622
Signed-off-by: Pekka Paalanen <***@collabora.co.uk>
---
hw/xwayland/xwayland.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++
hw/xwayland/xwayland.h | 3 ++
2 files changed, 124 insertions(+)

diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index 9d79554..ed60035 100644
--- a/hw/xwayland/xwayland.c
+++ b/hw/xwayland/xwayland.c
@@ -27,6 +27,7 @@

#include <stdio.h>

+#include <X11/Xatom.h>
#include <selection.h>
#include <micmap.h>
#include <misyncshm.h>
@@ -34,6 +35,7 @@
#include <glx_extinit.h>
#include <os.h>
#include <xserver_poll.h>
+#include <propertyst.h>

#ifdef XF86VIDMODE
#include <X11/extensions/xf86vmproto.h>
@@ -115,6 +117,94 @@ xwl_screen_get(ScreenPtr screen)
return dixLookupPrivate(&screen->devPrivates, &xwl_screen_private_key);
}

+static void
+xwl_window_set_allow_commits(struct xwl_window *xwl_window, Bool allow,
+ const char *debug_msg)
+{
+ xwl_window->allow_commits = allow;
+ DebugF("xwayland: win %d allow_commits = %d (%s)\n",
+ xwl_window->window->drawable.id, allow, debug_msg);
+}
+
+static void
+xwl_window_set_allow_commits_from_property(struct xwl_window *xwl_window,
+ PropertyPtr prop)
+{
+ static Bool warned = FALSE;
+ CARD32 *propdata;
+
+ if (prop->propertyName != xwl_window->xwl_screen->allow_commits_prop)
+ FatalError("Xwayland internal error: prop mismatch in %s.\n", __func__);
+
+ if (prop->type != XA_CARDINAL || prop->format != 32 || prop->size != 1) {
+ /* Not properly set, so fall back to safe and glitchy */
+ xwl_window_set_allow_commits(xwl_window, TRUE, "WM fault");
+
+ if (!warned) {
+ LogMessage(X_WARNING, "Window manager is misusing property %s.\n",
+ NameForAtom(prop->propertyName));
+ warned = TRUE;
+ }
+ return;
+ }
+
+ propdata = prop->data;
+ xwl_window_set_allow_commits(xwl_window, !!propdata[0], "from property");
+}
+
+static void
+xwl_window_property_allow_commits(struct xwl_window *xwl_window,
+ PropertyStateRec *propstate)
+{
+ Bool old_allow_commits = xwl_window->allow_commits;
+
+ switch (propstate->state) {
+ case PropertyNewValue:
+ xwl_window_set_allow_commits_from_property(xwl_window, propstate->prop);
+ break;
+
+ case PropertyDelete:
+ xwl_window_set_allow_commits(xwl_window, TRUE, "property deleted");
+ break;
+
+ default:
+ break;
+ }
+
+ /* If allow_commits turned from off to on, discard any frame
+ * callback we might be waiting for so that a new buffer is posted
+ * immediately through block_handler() if there is damage to post.
+ */
+ if (!old_allow_commits && xwl_window->allow_commits) {
+ if (xwl_window->frame_callback) {
+ wl_callback_destroy(xwl_window->frame_callback);
+ xwl_window->frame_callback = NULL;
+ }
+ }
+}
+
+static void
+xwl_property_callback(CallbackListPtr *pcbl, void *closure,
+ void *calldata)
+{
+ ScreenPtr screen = closure;
+ PropertyStateRec *rec = calldata;
+ struct xwl_screen *xwl_screen;
+ struct xwl_window *xwl_window;
+
+ if (rec->win->drawable.pScreen != screen)
+ return;
+
+ xwl_window = xwl_window_get(rec->win);
+ if (!xwl_window)
+ return;
+
+ xwl_screen = xwl_screen_get(screen);
+
+ if (rec->prop->propertyName == xwl_screen->allow_commits_prop)
+ xwl_window_property_allow_commits(xwl_window, rec);
+}
+
static Bool
xwl_close_screen(ScreenPtr screen)
{
@@ -122,6 +212,8 @@ xwl_close_screen(ScreenPtr screen)
struct xwl_output *xwl_output, *next_xwl_output;
struct xwl_seat *xwl_seat, *next_xwl_seat;

+ DeleteCallback(&PropertyStateCallback, xwl_property_callback, screen);
+
xorg_list_for_each_entry_safe(xwl_output, next_xwl_output,
&xwl_screen->output_list, link)
xwl_output_destroy(xwl_output);
@@ -262,6 +354,21 @@ xwl_pixmap_get(PixmapPtr pixmap)
}

static void
+xwl_window_init_allow_commits(struct xwl_window *xwl_window)
+{
+ PropertyPtr prop = NULL;
+ int ret;
+
+ ret = dixLookupProperty(&prop, xwl_window->window,
+ xwl_window->xwl_screen->allow_commits_prop,
+ serverClient, DixReadAccess);
+ if (ret == Success && prop)
+ xwl_window_set_allow_commits_from_property(xwl_window, prop);
+ else
+ xwl_window_set_allow_commits(xwl_window, TRUE, "no property");
+}
+
+static void
send_surface_id_event(struct xwl_window *xwl_window)
{
static const char atom_name[] = "WL_SURFACE_ID";
@@ -376,6 +483,8 @@ xwl_realize_window(WindowPtr window)
dixSetPrivate(&window->devPrivates, &xwl_window_private_key, xwl_window);
xorg_list_init(&xwl_window->link_damage);

+ xwl_window_init_allow_commits(xwl_window);
+
return ret;

err_surf:
@@ -505,6 +614,9 @@ xwl_screen_post_damage(struct xwl_screen *xwl_screen)
if (xwl_window->frame_callback)
continue;

+ if (!xwl_window->allow_commits)
+ continue;
+
xwl_window_post_damage(xwl_window);
}
}
@@ -694,6 +806,7 @@ wm_selection_callback(CallbackListPtr *p, void *data, void *arg)
static Bool
xwl_screen_init(ScreenPtr pScreen, int argc, char **argv)
{
+ static const char allow_commits[] = "_XWAYLAND_ALLOW_COMMITS";
struct xwl_screen *xwl_screen;
Pixel red_mask, blue_mask, green_mask;
int ret, bpc, green_bpc, i;
@@ -849,6 +962,14 @@ xwl_screen_init(ScreenPtr pScreen, int argc, char **argv)
pScreen->CursorWarpedTo = xwl_cursor_warped_to;
pScreen->CursorConfinedTo = xwl_cursor_confined_to;

+ xwl_screen->allow_commits_prop = MakeAtom(allow_commits,
+ strlen(allow_commits),
+ TRUE);
+ if (xwl_screen->allow_commits_prop == BAD_RESOURCE)
+ return FALSE;
+
+ AddCallback(&PropertyStateCallback, xwl_property_callback, pScreen);
+
return ret;
}

diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
index 5e5624b..91b7620 100644
--- a/hw/xwayland/xwayland.h
+++ b/hw/xwayland/xwayland.h
@@ -99,6 +99,8 @@ struct xwl_screen {
void *egl_display, *egl_context;
struct gbm_device *gbm;
struct glamor_context *glamor_ctx;
+
+ Atom allow_commits_prop;
};

struct xwl_window {
@@ -109,6 +111,7 @@ struct xwl_window {
DamagePtr damage;
struct xorg_list link_damage;
struct wl_callback *frame_callback;
+ Bool allow_commits;
};

#define MODIFIER_META 0x01
--
2.10.2

_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman
Loading...