[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