[PATCH] Add hybrid full-size/empty-clip mode to SetRootClip

Daniel Stone daniels at collabora.com
Mon Feb 8 10:50:20 UTC 2016


[Accidentally sent the unannotated version - sorry.]

Hi,

On 23 November 2015 at 07:51, Olivier Fourdan <ofourdan at redhat.com> wrote:
> diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c
> index 5ef444d..2a180f2 100644
> --- a/hw/xwayland/xwayland-output.c
> +++ b/hw/xwayland/xwayland-output.c
> @@ -164,7 +164,7 @@ update_screen_size(struct xwl_output *xwl_output, int width, int height)
>      struct xwl_screen *xwl_screen = xwl_output->xwl_screen;
>      double mmpd;
>
> -    if (xwl_screen->screen->root)
> +    if (!xwl_screen->rootless)
>          SetRootClip(xwl_screen->screen, FALSE);
>
>      xwl_screen->width = width;
> @@ -184,11 +184,13 @@ update_screen_size(struct xwl_output *xwl_output, int width, int height)
>      if (xwl_screen->screen->root) {
>          xwl_screen->screen->root->drawable.width = width;
>          xwl_screen->screen->root->drawable.height = height;
> -        SetRootClip(xwl_screen->screen, TRUE);
>          RRScreenSizeNotify(xwl_screen->screen);
>      }
>
>      update_desktop_dimensions();
> +
> +    if (!xwl_screen->rootless)
> +        SetRootClip(xwl_screen->screen, TRUE);
>  }

Unfortunately this totally breaks output hotplug, regressing the fix
from 216bdbc735. The effect is that if you hotplug an output and move an
XWayland window on to it, it can never receive pointer input, even
though the screen / root window size / RandR information are all updated.

Calling SetRootClip updates the root window's borderSize, so that
RRScreenSizeNotify (through ScreenRestructured) can rebuild the sprite
bounding box against the updated root window borderSize. With the
SetRootClip call removed, the bounding box always remains the same as it
was when the server was started.

It seems like we need a third mode to SetRootClip, diverging in the
middle ('Use REGION_BREAK to ...') branch: update winSize and
borderSize, but keep borderClip and clipList empty. I can't see how to
do this without a DIX change, since calling SetRootClip(TRUE) followed
by SetRootClip(FALSE) would have this desired effect, but generate a
bunch of exposures in between, which would generate invalid writes.

How about this (apply with git am --scissors)? It quite dumbly tries to
preserve API: Bool is really int, so we take the existing TRUE/FALSE
modes as the first two enum parameters, then introduce a new one.

Tested by starting with a single output, manually checking
screenInfo.screens[0]->root->{winSize,borderSize,borderClip,clipList}
with gdb, hotplugging an output, verifying the regions again and also
checking xev receives events on that output, unplugging it and verifying
regions again.

Cheers,
Daniel

--- 8< ---

216bdbc735 removed the SetRootClip call in the XWayland output-hotplug
handler when running rootless (e.g. as a part of Weston/Mutter), since
the root window has no storage, so generating exposures will result in
writes to invalid memory.

Unfortunately, preventing the segfault also breaks sprite confinement.
SetRootClip updates winSize and borderSize for the root window, which
when combined with RRScreenSizeChanged calling ScreenRestructured,
generates a new sprite-confinment area to update it to the whole screen.

Removing this call results in the window geometry being reported
correctly, but winSize/borderSize never changing from their values at
startup, i.e. out of sync with the root window geometry / screen
information in the connection info / XRandR.

This patch introduces a hybrid mode, where we update winSize and
borderSize for the root window, enabling sprite confinement to work
correctly, but keep the clip emptied so exposures are never generated.

Signed-off-by: Daniel Stone <daniels at collabora.com>
Cc: Olivier Fourdan <ofourdan at redhat.com>
Cc: Adam Jackson <ajax at redhat.com>
---
 dix/window.c                  | 18 ++++++++++++------
 hw/xwayland/xwayland-glamor.c |  2 +-
 hw/xwayland/xwayland-output.c | 11 ++++++-----
 hw/xwayland/xwayland-shm.c    |  2 +-
 include/window.h              |  8 +++++++-
 5 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/dix/window.c b/dix/window.c
index 25d29ec..9726ade 100644
--- a/dix/window.c
+++ b/dix/window.c
@@ -3647,7 +3647,7 @@ WindowParentHasDeviceCursor(WindowPtr pWin,
  *	all of the windows
  */
 void
-SetRootClip(ScreenPtr pScreen, Bool enable)
+SetRootClip(ScreenPtr pScreen, int enable)
 {
     WindowPtr pWin = pScreen->root;
     WindowPtr pChild;
@@ -3655,6 +3655,7 @@ SetRootClip(ScreenPtr pScreen, Bool enable)
     Bool anyMarked = FALSE;
     WindowPtr pLayerWin;
     BoxRec box;
+    enum RootClipMode mode = enable;
 
     if (!pWin)
         return;
@@ -3684,18 +3685,23 @@ SetRootClip(ScreenPtr pScreen, Bool enable)
      * that assume the root borderClip can't change well, normally
      * it doesn't...)
      */
-    if (enable) {
+    if (mode != ROOT_SIZE_NONE) {
+        pWin->drawable.width = pScreen->width;
+        pWin->drawable.height = pScreen->height;
+
         box.x1 = 0;
         box.y1 = 0;
         box.x2 = pScreen->width;
         box.y2 = pScreen->height;
+
         RegionInit(&pWin->winSize, &box, 1);
         RegionInit(&pWin->borderSize, &box, 1);
-        if (WasViewable)
-            RegionReset(&pWin->borderClip, &box);
-        pWin->drawable.width = pScreen->width;
-        pWin->drawable.height = pScreen->height;
         RegionBreak(&pWin->clipList);
+
+	if (WasViewable && mode == ROOT_SIZE_SCREEN)
+            RegionReset(&pWin->borderClip, &box);
+	else
+            RegionEmpty(&pWin->borderClip);
     }
     else {
         RegionEmpty(&pWin->borderClip);
diff --git a/hw/xwayland/xwayland-glamor.c b/hw/xwayland/xwayland-glamor.c
index 7f6fb9a..5557818 100644
--- a/hw/xwayland/xwayland-glamor.c
+++ b/hw/xwayland/xwayland-glamor.c
@@ -236,7 +236,7 @@ xwl_glamor_create_screen_resources(ScreenPtr screen)
     if (xwl_screen->rootless) {
         screen->devPrivate =
             fbCreatePixmap(screen, 0, 0, screen->rootDepth, 0);
-        SetRootClip(screen, FALSE);
+        SetRootClip(screen, ROOT_SIZE_SCREEN_EMPTY);
     }
     else {
         screen->devPrivate =
diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c
index e9ec190..f5c7194 100644
--- a/hw/xwayland/xwayland-output.c
+++ b/hw/xwayland/xwayland-output.c
@@ -164,8 +164,7 @@ update_screen_size(struct xwl_output *xwl_output, int width, int height)
     struct xwl_screen *xwl_screen = xwl_output->xwl_screen;
     double mmpd;
 
-    if (!xwl_screen->rootless)
-        SetRootClip(xwl_screen->screen, FALSE);
+    SetRootClip(xwl_screen->screen, FALSE);
 
     xwl_screen->width = width;
     xwl_screen->height = height;
@@ -181,6 +180,11 @@ update_screen_size(struct xwl_output *xwl_output, int width, int height)
         xwl_screen->screen->mmHeight = height * mmpd;
     }
 
+    if (xwl_screen->rootless)
+        SetRootClip(xwl_screen->screen, ROOT_SIZE_SCREEN_EMPTY);
+    else
+        SetRootClip(xwl_screen->screen, ROOT_SIZE_SCREEN);
+
     if (xwl_screen->screen->root) {
         xwl_screen->screen->root->drawable.width = width;
         xwl_screen->screen->root->drawable.height = height;
@@ -188,9 +192,6 @@ update_screen_size(struct xwl_output *xwl_output, int width, int height)
     }
 
     update_desktop_dimensions();
-
-    if (!xwl_screen->rootless)
-        SetRootClip(xwl_screen->screen, TRUE);
 }
 
 static void
diff --git a/hw/xwayland/xwayland-shm.c b/hw/xwayland/xwayland-shm.c
index 7072be4..2fbe74d 100644
--- a/hw/xwayland/xwayland-shm.c
+++ b/hw/xwayland/xwayland-shm.c
@@ -282,7 +282,7 @@ xwl_shm_create_screen_resources(ScreenPtr screen)
     if (xwl_screen->rootless) {
         screen->devPrivate =
             fbCreatePixmap(screen, 0, 0, screen->rootDepth, 0);
-        SetRootClip(screen, FALSE);
+        SetRootClip(screen, ROOT_SIZE_SCREEN_EMPTY);
     }
     else
         screen->devPrivate =
diff --git a/include/window.h b/include/window.h
index f13ed51..67c9f10 100644
--- a/include/window.h
+++ b/include/window.h
@@ -72,6 +72,12 @@ struct _Cursor;
 typedef struct _BackingStore *BackingStorePtr;
 typedef struct _Window *WindowPtr;
 
+enum RootClipMode {
+	ROOT_SIZE_NONE = 0, /**< resize the root window to 0x0 */
+	ROOT_SIZE_SCREEN = 1, /**< resize the root window to fit screen */
+	ROOT_SIZE_SCREEN_EMPTY = 2, /**< as above, but empty clip */
+};
+
 typedef int (*VisitWindowProcPtr) (WindowPtr pWin,
                                    void *data);
 
@@ -221,7 +227,7 @@ extern _X_EXPORT RegionPtr CreateBoundingShape(WindowPtr /* pWin */ );
 
 extern _X_EXPORT RegionPtr CreateClipShape(WindowPtr /* pWin */ );
 
-extern _X_EXPORT void SetRootClip(ScreenPtr pScreen, Bool enable);
+extern _X_EXPORT void SetRootClip(ScreenPtr pScreen, int enable);
 extern _X_EXPORT void PrintWindowTree(void);
 extern _X_EXPORT void PrintPassiveGrabs(void);
 
-- 
2.5.0



More information about the xorg-devel mailing list