[RFC xserver 3/3] Xwayland: use _XWAYLAND_ALLOW_COMMITS property
Pekka Paalanen
ppaalanen at gmail.com
Mon Dec 5 12:03:04 UTC 2016
On Thu, 24 Nov 2016 15:40:37 +0200
Pekka Paalanen <ppaalanen at gmail.com> wrote:
> From: Pekka Paalanen <pekka.paalanen at 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 <pekka.paalanen at collabora.co.uk>
> ---
> 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?
> 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?
> + /* 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?
> + 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
> +
> + 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.x.org/archives/xorg-devel/attachments/20161205/ffe09f19/attachment.sig>
More information about the xorg-devel
mailing list