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

Daniel Stone daniels at collabora.com
Fri Feb 12 16:36:59 UTC 2016


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>
Tested-by: Olivier Fourdan <ofourdan at redhat.com>
Cc: Adam Jackson <ajax at redhat.com>
---

v2:
  - Renamed enum values for aesthetic purity
  - Better commenting
  - Calculate desired root-clip mode once in XWayland and store it
  - Change FALSE/TRUE in callers to enum values  

 dix/window.c                   | 32 +++++++++++++++++++++-----------
 hw/kdrive/src/kdrive.c         |  4 ++--
 hw/vfb/InitOutput.c            |  4 ++--
 hw/xfree86/common/xf86Helper.c |  4 ++--
 hw/xfree86/modes/xf86RandR12.c |  4 ++--
 hw/xquartz/darwinEvents.c      |  2 +-
 hw/xquartz/quartz.c            |  4 ++--
 hw/xquartz/quartz.h            |  2 +-
 hw/xwayland/xwayland-glamor.c  |  3 ++-
 hw/xwayland/xwayland-output.c  |  9 ++++-----
 hw/xwayland/xwayland-shm.c     |  6 +++---
 hw/xwayland/xwayland.c         |  7 +++++++
 hw/xwayland/xwayland.h         |  1 +
 hw/xwin/winrandr.c             |  4 ++--
 include/window.h               |  8 +++++++-
 15 files changed, 59 insertions(+), 35 deletions(-)

diff --git a/dix/window.c b/dix/window.c
index 25d29ec..ead4dc2 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;
@@ -3679,23 +3680,32 @@ SetRootClip(ScreenPtr pScreen, Bool enable)
         }
     }
 
-    /*
-     * Use REGION_BREAK to avoid optimizations in ValidateTree
-     * that assume the root borderClip can't change well, normally
-     * it doesn't...)
-     */
-    if (enable) {
+    if (mode != ROOT_CLIP_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;
+
+        /*
+         * Use REGION_BREAK to avoid optimizations in ValidateTree
+         * that assume the root borderClip can't change well, normally
+         * it doesn't...)
+         */
         RegionBreak(&pWin->clipList);
+
+	/* For INPUT_ONLY, empty the borderClip so no rendering will ever
+	 * be attempted to the screen pixmap (only redirected windows),
+	 * but we keep borderSize as full regardless. */
+        if (WasViewable && mode == ROOT_CLIP_FULL)
+            RegionReset(&pWin->borderClip, &box);
+        else
+            RegionEmpty(&pWin->borderClip);
     }
     else {
         RegionEmpty(&pWin->borderClip);
diff --git a/hw/kdrive/src/kdrive.c b/hw/kdrive/src/kdrive.c
index dddbe6e..582ff66 100644
--- a/hw/kdrive/src/kdrive.c
+++ b/hw/kdrive/src/kdrive.c
@@ -99,7 +99,7 @@ KdDisableScreen(ScreenPtr pScreen)
     if (!pScreenPriv->enabled)
         return;
     if (!pScreenPriv->closed)
-        SetRootClip(pScreen, FALSE);
+        SetRootClip(pScreen, ROOT_CLIP_NONE);
     KdDisableColormap(pScreen);
     if (!pScreenPriv->screen->dumb && pScreenPriv->card->cfuncs->disableAccel)
         (*pScreenPriv->card->cfuncs->disableAccel) (pScreen);
@@ -182,7 +182,7 @@ KdEnableScreen(ScreenPtr pScreen)
     if (!pScreenPriv->screen->dumb && pScreenPriv->card->cfuncs->enableAccel)
         (*pScreenPriv->card->cfuncs->enableAccel) (pScreen);
     KdEnableColormap(pScreen);
-    SetRootClip(pScreen, TRUE);
+    SetRootClip(pScreen, ROOT_CLIP_FULL);
     if (pScreenPriv->card->cfuncs->dpms)
         (*pScreenPriv->card->cfuncs->dpms) (pScreen, pScreenPriv->dpmsState);
     return TRUE;
diff --git a/hw/vfb/InitOutput.c b/hw/vfb/InitOutput.c
index 9e8dd7a..01bb631 100644
--- a/hw/vfb/InitOutput.c
+++ b/hw/vfb/InitOutput.c
@@ -763,7 +763,7 @@ vfbRRScreenSetSize(ScreenPtr  pScreen,
                    CARD32     mmHeight)
 {
     // Prevent screen updates while we change things around
-    SetRootClip(pScreen, FALSE);
+    SetRootClip(pScreen, ROOT_CLIP_NONE);
 
     pScreen->width = width;
     pScreen->height = height;
@@ -771,7 +771,7 @@ vfbRRScreenSetSize(ScreenPtr  pScreen,
     pScreen->mmHeight = mmHeight;
 
     // Restore the ability to update screen, now with new dimensions
-    SetRootClip(pScreen, TRUE);
+    SetRootClip(pScreen, ROOT_CLIP_FULL);
 
     RRScreenSizeNotify (pScreen);
     RRTellChanged(pScreen);
diff --git a/hw/xfree86/common/xf86Helper.c b/hw/xfree86/common/xf86Helper.c
index c42e93e..3b01a49 100644
--- a/hw/xfree86/common/xf86Helper.c
+++ b/hw/xfree86/common/xf86Helper.c
@@ -1076,14 +1076,14 @@ xf86EnableDisableFBAccess(ScrnInfoPtr pScrnInfo, Bool enable)
          * Restore all of the clip lists on the screen
          */
         if (!xf86Resetting)
-            SetRootClip(pScreen, TRUE);
+            SetRootClip(pScreen, ROOT_CLIP_FULL);
 
     }
     else {
         /*
          * Empty all of the clip lists on the screen
          */
-        SetRootClip(pScreen, FALSE);
+        SetRootClip(pScreen, ROOT_CLIP_NONE);
     }
 }
 
diff --git a/hw/xfree86/modes/xf86RandR12.c b/hw/xfree86/modes/xf86RandR12.c
index eae7016..60d2254 100644
--- a/hw/xfree86/modes/xf86RandR12.c
+++ b/hw/xfree86/modes/xf86RandR12.c
@@ -1848,13 +1848,13 @@ xf86RandR14ProviderSetOutputSource(ScreenPtr pScreen,
     if (provider->output_source == source_provider)
         return TRUE;
 
-    SetRootClip(source_provider->pScreen, FALSE);
+    SetRootClip(source_provider->pScreen, ROOT_CLIP_NONE);
 
     DetachUnboundGPU(pScreen);
     AttachOutputGPU(source_provider->pScreen, pScreen);
 
     provider->output_source = source_provider;
-    SetRootClip(source_provider->pScreen, TRUE);
+    SetRootClip(source_provider->pScreen, ROOT_CLIP_FULL);
     return TRUE;
 }
 
diff --git a/hw/xquartz/darwinEvents.c b/hw/xquartz/darwinEvents.c
index 5577297..7f34e0c 100644
--- a/hw/xquartz/darwinEvents.c
+++ b/hw/xquartz/darwinEvents.c
@@ -275,7 +275,7 @@ DarwinEventHandler(int screenNum, InternalEvent *ie, DeviceIntPtr dev)
         break;
 
     case kXquartzSetRootClip:
-        QuartzSetRootClip((Bool)e->data[0]);
+        QuartzSetRootClip(e->data[0]);
         break;
 
     case kXquartzQuit:
diff --git a/hw/xquartz/quartz.c b/hw/xquartz/quartz.c
index d3ec133..c8b6f96 100644
--- a/hw/xquartz/quartz.c
+++ b/hw/xquartz/quartz.c
@@ -485,7 +485,7 @@ QuartzHide(void)
  *  Enable or disable rendering to the X screen.
  */
 void
-QuartzSetRootClip(BOOL enable)
+QuartzSetRootClip(int mode)
 {
     int i;
 
@@ -494,7 +494,7 @@ QuartzSetRootClip(BOOL enable)
 
     for (i = 0; i < screenInfo.numScreens; i++) {
         if (screenInfo.screens[i]) {
-            SetRootClip(screenInfo.screens[i], enable);
+            SetRootClip(screenInfo.screens[i], mode);
         }
     }
 }
diff --git a/hw/xquartz/quartz.h b/hw/xquartz/quartz.h
index 47c4416..ddbf2e7 100644
--- a/hw/xquartz/quartz.h
+++ b/hw/xquartz/quartz.h
@@ -149,7 +149,7 @@ QuartzShow(void);
 void
 QuartzHide(void);
 void
-QuartzSetRootClip(BOOL enable);
+QuartzSetRootClip(int mode);
 void
 QuartzSpaceChanged(uint32_t space_id);
 
diff --git a/hw/xwayland/xwayland-glamor.c b/hw/xwayland/xwayland-glamor.c
index 7f6fb9a..8e77a84 100644
--- a/hw/xwayland/xwayland-glamor.c
+++ b/hw/xwayland/xwayland-glamor.c
@@ -236,7 +236,6 @@ xwl_glamor_create_screen_resources(ScreenPtr screen)
     if (xwl_screen->rootless) {
         screen->devPrivate =
             fbCreatePixmap(screen, 0, 0, screen->rootDepth, 0);
-        SetRootClip(screen, FALSE);
     }
     else {
         screen->devPrivate =
@@ -247,6 +246,8 @@ xwl_glamor_create_screen_resources(ScreenPtr screen)
             glamor_set_screen_pixmap(screen->devPrivate, NULL);
     }
 
+    SetRootClip(screen, xwl_screen->root_clip_mode);
+
     return screen->devPrivate != NULL;
 }
 
diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c
index e9ec190..5263cb3 100644
--- a/hw/xwayland/xwayland-output.c
+++ b/hw/xwayland/xwayland-output.c
@@ -164,8 +164,8 @@ 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);
+    if (xwl_screen->root_clip_mode == ROOT_CLIP_FULL)
+        SetRootClip(xwl_screen->screen, ROOT_CLIP_NONE);
 
     xwl_screen->width = width;
     xwl_screen->height = height;
@@ -181,6 +181,8 @@ update_screen_size(struct xwl_output *xwl_output, int width, int height)
         xwl_screen->screen->mmHeight = height * mmpd;
     }
 
+    SetRootClip(xwl_screen->screen, xwl_screen->root_clip_mode);
+
     if (xwl_screen->screen->root) {
         xwl_screen->screen->root->drawable.width = width;
         xwl_screen->screen->root->drawable.height = height;
@@ -188,9 +190,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..1b388f0 100644
--- a/hw/xwayland/xwayland-shm.c
+++ b/hw/xwayland/xwayland-shm.c
@@ -279,16 +279,16 @@ xwl_shm_create_screen_resources(ScreenPtr screen)
     if (!ret)
         return ret;
 
-    if (xwl_screen->rootless) {
+    if (xwl_screen->rootless)
         screen->devPrivate =
             fbCreatePixmap(screen, 0, 0, screen->rootDepth, 0);
-        SetRootClip(screen, FALSE);
-    }
     else
         screen->devPrivate =
             xwl_shm_create_pixmap(screen, screen->width, screen->height,
                                   screen->rootDepth,
                                   CREATE_PIXMAP_USAGE_BACKING_PIXMAP);
 
+    SetRootClip(screen, xwl_screen->root_clip_mode);
+
     return screen->devPrivate != NULL;
 }
diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index 3d36205..c97f57d 100644
--- a/hw/xwayland/xwayland.c
+++ b/hw/xwayland/xwayland.c
@@ -590,6 +590,13 @@ xwl_screen_init(ScreenPtr pScreen, int argc, char **argv)
         }
     }
 
+    /* In rootless mode, we don't have any screen storage, and the only
+     * rendering should be to redirected mode. */
+    if (xwl_screen->rootless)
+        xwl_screen->root_clip_mode = ROOT_CLIP_INPUT_ONLY;
+    else
+        xwl_screen->root_clip_mode = ROOT_CLIP_FULL;
+
     if (xwl_screen->listen_fd_count > 0) {
         if (xwl_screen->wm_fd >= 0)
             AddCallback(&SelectionCallback, wm_selection_callback, xwl_screen);
diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
index a7d7119..4fcdee5 100644
--- a/hw/xwayland/xwayland.h
+++ b/hw/xwayland/xwayland.h
@@ -49,6 +49,7 @@ struct xwl_screen {
     ScreenPtr screen;
     WindowPtr pointer_limbo_window;
     int expecting_event;
+    enum RootClipMode root_clip_mode;
 
     int wm_fd;
     int listen_fds[5];
diff --git a/hw/xwin/winrandr.c b/hw/xwin/winrandr.c
index f4ba054..c29d7b3 100644
--- a/hw/xwin/winrandr.c
+++ b/hw/xwin/winrandr.c
@@ -75,7 +75,7 @@ winDoRandRScreenSetSize(ScreenPtr pScreen,
     WindowPtr pRoot = pScreen->root;
 
     // Prevent screen updates while we change things around
-    SetRootClip(pScreen, FALSE);
+    SetRootClip(pScreen, ROOT_CLIP_NONE);
 
     /* Update the screen size as requested */
     pScreenInfo->dwWidth = width;
@@ -101,7 +101,7 @@ winDoRandRScreenSetSize(ScreenPtr pScreen,
     // does this emit a ConfigureNotify??
 
     // Restore the ability to update screen, now with new dimensions
-    SetRootClip(pScreen, TRUE);
+    SetRootClip(pScreen, ROOT_CLIP_FULL);
 
     // and arrange for it to be repainted
     pScreen->PaintWindow(pRoot, &pRoot->borderClip, PW_BACKGROUND);
diff --git a/include/window.h b/include/window.h
index f13ed51..7a22feb 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_CLIP_NONE = 0, /**< resize the root window to 0x0 */
+    ROOT_CLIP_FULL = 1, /**< resize the root window to fit screen */
+    ROOT_CLIP_INPUT_ONLY = 2, /**< as above, but no rendering to screen */
+};
+
 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