[PATCH] Add hybrid full-size/empty-clip mode to SetRootClip
Olivier Fourdan
ofourdan at redhat.com
Tue Feb 9 15:16:06 UTC 2016
Hi Daniel,
----- Original Message -----
> [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);
The original source seems to use spaces instead of tabs, might want to keep it that way.
(unless this is our MUA playing tricks on me)
> }
> 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);
So we take advantage that Bool is actually an int so we can change the semantic and pass an enum while retaining API/ABI compatility.
Maybe use ROOT_SIZE_NONE instead of FALSE here (just like you did a few lines above) so we don't end up too confused :)
> 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);
I'm not a big fan of the Bool -> int -> enum trickery but I'm not sure if we could afford to have an enum without breaking API/ABI (although I reckoned most compilers would use an int for an enum, no?), but if we have no other choice then, fine.
Anyway, it works for me so:
Tested-by: Olivier Fourdan <ofourdan at redhat.com>
Cheers,
Olivier
More information about the xorg-devel
mailing list