[PATCH] mi: fix software cursor with several X screens
Peter Hutterer
peter.hutterer at who-t.net
Tue Apr 6 18:52:01 PDT 2010
On Tue, Apr 06, 2010 at 03:32:49PM -0700, Pierre-Loup A. Griffais wrote:
> Now with a proper GC cleanup sequence instead of freeing the same GC in a loop.
>
> Thanks,
> - Pierre-Loup
>
> On 04/06/2010 02:44 PM, Pierre-Loup A. Griffais wrote:
> >(disregard previous message sent too soon)
> >
> >Attached is a tentative patch that cleans that particular code up and fixes the
> >issue.
> >
> >It would seem cleaner to perform the screen looping in ActivateDevice(), but
> >that would also mean changing miPointerDeviceInitialize and
> >miSpriteDeviceCursorInitialize to only perform their own setup once in that
> >case. Opinions?
InitializeSprite is probably the better place since a device doesn't
necessarily get a sprite even when activated (attach SDs for example) and it
may get a sprite without being deactivated first (set floating).
> >Thanks,
> > - Pierre-Loup
> >
> >On 04/06/2010 08:20 AM, Tiago Vignatti wrote:
> >>On Tue, Apr 06, 2010 at 03:52:07AM +0200, ext Pierre-Loup A. Griffais wrote:
> >>>The DC code is broken for setups with several screens. Devs only have one pSave
> >>>pixmap and there's no code to thrash them like p[Save|Restore]GC.
> >>>
> >>>That means if you have two X screens and force SW cursor on both, the server
> >>>will end up passing a bunch of CopyAreas with mismatching screens to the driver,
> >>>which can fail horribly if the driver does a good job abstracting screens away
> >>>from each other.
> >>
> >>If helps you, I can confirm this happening here also.
> >>
> >> Tiago
> From e7c5552be9cc217866ea7362c44884baa2964d85 Mon Sep 17 00:00:00 2001
> From: Pierre-Loup A. Griffais <pgriffais at nvidia.com>
> Date: Tue, 6 Apr 2010 15:30:30 -0700
> Subject: [PATCH] [PATCH] mi: don't thrash resources when displaying the software cursor across screens
>
> This changes the DC layer to maintain a persistent set of GCs/pixmaps/pictures
> for each pScreen instead of failing to thrash between them when changing
> screens.
>
> Signed-off-by: Pierre-Loup A. Griffais <pgriffais at nvidia.com>
> ---
> mi/midispcur.c | 263 +++++++++++++++++++++++---------------------------------
> 1 files changed, 108 insertions(+), 155 deletions(-)
>
> diff --git a/mi/midispcur.c b/mi/midispcur.c
> index 3fb7e02..7d8f876 100644
> --- a/mi/midispcur.c
> +++ b/mi/midispcur.c
> @@ -59,9 +59,9 @@ static DevPrivateKey miDCScreenKey = &miDCScreenKeyIndex;
>
> static Bool miDCCloseScreen(int index, ScreenPtr pScreen);
>
> -/* per device private data */
> -static int miDCSpriteKeyIndex;
> -static DevPrivateKey miDCSpriteKey = &miDCSpriteKeyIndex;
> +/* per device per-screen private data */
> +static int miDCSpriteKeyIndex[MAXSCREENS];
> +static DevPrivateKey miDCSpriteKey = miDCSpriteKeyIndex;
>
> typedef struct {
> GCPtr pSourceGC, pMaskGC;
> @@ -77,8 +77,8 @@ typedef struct {
>
> #define MIDCBUFFER(dev) \
> ((DevHasCursor(dev)) ? \
> - (miDCBufferPtr)dixLookupPrivate(&dev->devPrivates, miDCSpriteKey) : \
> - (miDCBufferPtr)dixLookupPrivate(&dev->u.master->devPrivates, miDCSpriteKey))
> + (miDCBufferPtr)dixLookupPrivate(&dev->devPrivates, miDCSpriteKey + (pScreen)->myNum) : \
> + (miDCBufferPtr)dixLookupPrivate(&dev->u.master->devPrivates, miDCSpriteKey + (pScreen)->myNum))
I think it's better to change the macro to take (dev, pScreen) instead of
magically relying on pScreen to be there.
>
> /*
> * The core pointer buffer will point to the index of the virtual core pointer
> @@ -158,10 +158,6 @@ miDCInitialize (ScreenPtr pScreen, miPointerScreenFuncPtr screenFuncs)
> return TRUE;
> }
>
> -#define tossGC(gc) (gc ? FreeGC (gc, (GContext) 0) : 0)
> -#define tossPix(pix) (pix ? (*pScreen->DestroyPixmap) (pix) : TRUE)
> -#define tossPict(pict) (pict ? FreePicture (pict, 0) : 0)
> -
> static Bool
> miDCCloseScreen (int index, ScreenPtr pScreen)
> {
> @@ -183,7 +179,6 @@ miDCRealizeCursor (ScreenPtr pScreen, CursorPtr pCursor)
> }
>
> #ifdef ARGB_CURSOR
> -#define EnsurePicture(picture,draw,win) (picture || miDCMakePicture(&picture,draw,win))
>
> static VisualPtr
> miDCGetWindowVisual (WindowPtr pWin)
> @@ -415,12 +410,8 @@ miDCPutBits (
> (*maskGC->ops->PushPixels) (maskGC, pPriv->maskBits, pDrawable, w, h, x, y);
> }
>
> -#define EnsureGC(gc,win) (gc || miDCMakeGC(&gc, win))
> -
> static GCPtr
> -miDCMakeGC(
> - GCPtr *ppGC,
> - WindowPtr pWin)
> +miDCMakeGC(ScreenPtr pScreen)
> {
> GCPtr pGC;
> int status;
> @@ -428,10 +419,9 @@ miDCMakeGC(
>
> gcvals[0] = IncludeInferiors;
> gcvals[1] = FALSE;
> - pGC = CreateGC((DrawablePtr)pWin,
> + pGC = CreateGC((DrawablePtr)WindowTable[pScreen->myNum],
> GCSubwindowMode|GCGraphicsExposures, gcvals, &status,
> (XID)0, serverClient);
> - *ppGC = pGC;
> return pGC;
> }
I don't really see the need for this change - why not let the window be
passed in instead of accessing the global WindowTable. Especially since
looking at the caller below you don't gain much from it (see my comment
in place there)
> @@ -461,17 +451,6 @@ miDCPutUpCursor (DeviceIntPtr pDev, ScreenPtr pScreen, CursorPtr pCursor,
> #ifdef ARGB_CURSOR
> if (pPriv->pPicture)
> {
> - /* see comment in miDCPutUpCursor */
> - if (pBuffer->pRootPicture &&
> - pBuffer->pRootPicture->pDrawable &&
> - pBuffer->pRootPicture->pDrawable->pScreen != pScreen)
> - {
> - tossPict(pBuffer->pRootPicture);
> - pBuffer->pRootPicture = NULL;
> - }
> -
> - if (!EnsurePicture(pBuffer->pRootPicture, &pWin->drawable, pWin))
> - return FALSE;
> CompositePicture (PictOpOver,
> pPriv->pPicture,
> NULL,
> @@ -484,33 +463,6 @@ miDCPutUpCursor (DeviceIntPtr pDev, ScreenPtr pScreen, CursorPtr pCursor,
> else
> #endif
> {
> - /**
> - * XXX: Before MPX, the sourceGC and maskGC were attached to the
> - * screen, and would switch as the screen switches. With mpx we have
> - * the GC's attached to the device now, so each time we switch screen
> - * we need to make sure the GC's are allocated on the new screen.
> - * This is ... not optimal. (whot)
> - */
> - if (pBuffer->pSourceGC && pScreen != pBuffer->pSourceGC->pScreen)
> - {
> - tossGC(pBuffer->pSourceGC);
> - pBuffer->pSourceGC = NULL;
> - }
> -
> - if (pBuffer->pMaskGC && pScreen != pBuffer->pMaskGC->pScreen)
> - {
> - tossGC(pBuffer->pMaskGC);
> - pBuffer->pMaskGC = NULL;
> - }
> -
> - if (!EnsureGC(pBuffer->pSourceGC, pWin))
> - return FALSE;
> - if (!EnsureGC(pBuffer->pMaskGC, pWin))
> - {
> - FreeGC (pBuffer->pSourceGC, (GContext) 0);
> - pBuffer->pSourceGC = 0;
> - return FALSE;
> - }
> miDCPutBits ((DrawablePtr)pWin, pPriv,
> pBuffer->pSourceGC, pBuffer->pMaskGC,
> x, y, pCursor->bits->width, pCursor->bits->height,
> @@ -544,14 +496,7 @@ miDCSaveUnderCursor (DeviceIntPtr pDev, ScreenPtr pScreen,
> if (!pSave)
> return FALSE;
> }
> - /* see comment in miDCPutUpCursor */
> - if (pBuffer->pSaveGC && pBuffer->pSaveGC->pScreen != pScreen)
> - {
> - tossGC(pBuffer->pSaveGC);
> - pBuffer->pSaveGC = NULL;
> - }
> - if (!EnsureGC(pBuffer->pSaveGC, pWin))
> - return FALSE;
> +
> pGC = pBuffer->pSaveGC;
> if (pSave->drawable.serialNumber != pGC->serialNumber)
> ValidateGC ((DrawablePtr) pSave, pGC);
> @@ -578,14 +523,7 @@ miDCRestoreUnderCursor (DeviceIntPtr pDev, ScreenPtr pScreen,
> pWin = WindowTable[pScreen->myNum];
> if (!pSave)
> return FALSE;
> - /* see comment in miDCPutUpCursor */
> - if (pBuffer->pRestoreGC && pBuffer->pRestoreGC->pScreen != pScreen)
> - {
> - tossGC(pBuffer->pRestoreGC);
> - pBuffer->pRestoreGC = NULL;
> - }
> - if (!EnsureGC(pBuffer->pRestoreGC, pWin))
> - return FALSE;
> +
> pGC = pBuffer->pRestoreGC;
> if (pWin->drawable.serialNumber != pGC->serialNumber)
> ValidateGC ((DrawablePtr) pWin, pGC);
> @@ -616,14 +554,7 @@ miDCChangeSave (DeviceIntPtr pDev, ScreenPtr pScreen,
> */
> if (!pSave)
> return FALSE;
> - /* see comment in miDCPutUpCursor */
> - if (pBuffer->pRestoreGC && pBuffer->pRestoreGC->pScreen != pScreen)
> - {
> - tossGC(pBuffer->pRestoreGC);
> - pBuffer->pRestoreGC = NULL;
> - }
> - if (!EnsureGC(pBuffer->pRestoreGC, pWin))
> - return FALSE;
> +
> pGC = pBuffer->pRestoreGC;
> if (pWin->drawable.serialNumber != pGC->serialNumber)
> ValidateGC ((DrawablePtr) pWin, pGC);
> @@ -662,14 +593,7 @@ miDCChangeSave (DeviceIntPtr pDev, ScreenPtr pScreen,
> (*pGC->ops->CopyArea) ((DrawablePtr) pSave, (DrawablePtr) pWin, pGC,
> 0, sourcey, -dx, copyh, x + dx, desty);
> }
> - /* see comment in miDCPutUpCursor */
> - if (pBuffer->pSaveGC && pBuffer->pSaveGC->pScreen != pScreen)
> - {
> - tossGC(pBuffer->pSaveGC);
> - pBuffer->pSaveGC = NULL;
> - }
> - if (!EnsureGC(pBuffer->pSaveGC, pWin))
> - return FALSE;
> +
> pGC = pBuffer->pSaveGC;
> if (pSave->drawable.serialNumber != pGC->serialNumber)
> ValidateGC ((DrawablePtr) pSave, pGC);
> @@ -770,7 +694,7 @@ miDCMoveCursor (DeviceIntPtr pDev, ScreenPtr pScreen, CursorPtr pCursor,
>
> pTemp = pBuffer->pTemp;
> if (!pTemp ||
> - pTemp->drawable.width != pBuffer->pSave->drawable.width ||
> + pTemp->drawable.width != pBuffer->pSave->drawable.width ||
> pTemp->drawable.height != pBuffer->pSave->drawable.height)
> {
> if (pTemp)
> @@ -809,17 +733,9 @@ miDCMoveCursor (DeviceIntPtr pDev, ScreenPtr pScreen, CursorPtr pCursor,
> #ifdef ARGB_CURSOR
> if (pPriv->pPicture)
> {
> - /* see comment in miDCPutUpCursor */
> - if (pBuffer->pTempPicture &&
> - pBuffer->pTempPicture->pDrawable &&
> - pBuffer->pTempPicture->pDrawable->pScreen != pScreen)
> - {
> - tossPict(pBuffer->pTempPicture);
> - pBuffer->pTempPicture = NULL;
> - }
> + if (!pBuffer->pTempPicture)
> + miDCMakePicture(&pBuffer->pTempPicture, &pTemp->drawable, pWin);
>
> - if (!EnsurePicture(pBuffer->pTempPicture, &pTemp->drawable, pWin))
> - return FALSE;
> CompositePicture (PictOpOver,
> pPriv->pPicture,
> NULL,
> @@ -832,38 +748,12 @@ miDCMoveCursor (DeviceIntPtr pDev, ScreenPtr pScreen, CursorPtr pCursor,
> else
> #endif
> {
> - if (!pBuffer->pPixSourceGC)
> - {
> - pBuffer->pPixSourceGC = CreateGC ((DrawablePtr)pTemp,
> - GCGraphicsExposures, &gcval, &status, (XID)0, serverClient);
> - if (!pBuffer->pPixSourceGC)
> - return FALSE;
> - }
> - if (!pBuffer->pPixMaskGC)
> - {
> - pBuffer->pPixMaskGC = CreateGC ((DrawablePtr)pTemp,
> - GCGraphicsExposures, &gcval, &status, (XID)0, serverClient);
> - if (!pBuffer->pPixMaskGC)
> - return FALSE;
> - }
> miDCPutBits ((DrawablePtr)pTemp, pPriv,
> pBuffer->pPixSourceGC, pBuffer->pPixMaskGC,
> dx, dy, pCursor->bits->width, pCursor->bits->height,
> source, mask);
> }
>
> - /* see comment in miDCPutUpCursor */
> - if (pBuffer->pRestoreGC && pBuffer->pRestoreGC->pScreen != pScreen)
> - {
> - tossGC(pBuffer->pRestoreGC);
> - pBuffer->pRestoreGC = NULL;
> - }
> - /*
> - * copy the temporary pixmap onto the screen
> - */
> -
> - if (!EnsureGC(pBuffer->pRestoreGC, pWin))
> - return FALSE;
> pGC = pBuffer->pRestoreGC;
> if (pWin->drawable.serialNumber != pGC->serialNumber)
> ValidateGC ((DrawablePtr) pWin, pGC);
> @@ -877,51 +767,114 @@ miDCMoveCursor (DeviceIntPtr pDev, ScreenPtr pScreen, CursorPtr pCursor,
> static Bool
> miDCDeviceInitialize(DeviceIntPtr pDev, ScreenPtr pScreen)
> {
> - miDCBufferPtr pBuffer;
> -
> - pBuffer = xalloc(sizeof(miDCBufferRec));
> - dixSetPrivate(&pDev->devPrivates, miDCSpriteKey, pBuffer);
> -
> - pBuffer->pSourceGC =
> - pBuffer->pMaskGC =
> - pBuffer->pSaveGC =
> - pBuffer->pRestoreGC =
> - pBuffer->pMoveGC =
> - pBuffer->pPixSourceGC =
> - pBuffer->pPixMaskGC = NULL;
> + miDCBufferPtr pBuffer;
> + WindowPtr pWin;
> + XID gcval = FALSE;
> + int status;
> + int i;
> +
> + if (!DevHasCursor(pDev))
> + return TRUE;
> +
> + for (i = 0; i < screenInfo.numScreens; i++)
> + {
> + pScreen = screenInfo.screens[i];
> +
> + pBuffer = xalloc(sizeof(miDCBufferRec));
> + if (!pBuffer)
> + goto failure;
> +
> + dixSetPrivate(&pDev->devPrivates, miDCSpriteKey + pScreen->myNum, pBuffer);
> +
> + pBuffer->pSourceGC = miDCMakeGC(pScreen);
> + if (!pBuffer->pSourceGC)
> + goto failure;
> +
> + pBuffer->pMaskGC = miDCMakeGC(pScreen);
> + if (!pBuffer->pMaskGC)
> + goto failure;
> +
> + pBuffer->pSaveGC = miDCMakeGC(pScreen);
> + if (!pBuffer->pSaveGC)
> + goto failure;
> +
> + pBuffer->pRestoreGC = miDCMakeGC(pScreen);
> + if (!pBuffer->pRestoreGC)
> + goto failure;
> +
> + pWin = WindowTable[pScreen->myNum];
if you move this line up to after the dixSetPrivate, you can just leave the
miDCMakeGC API as-is and the code is equally readable.
> +
> + pBuffer->pMoveGC = CreateGC ((DrawablePtr)pWin,
> + GCGraphicsExposures, &gcval, &status, (XID)0, serverClient);
> + if (!pBuffer->pMoveGC)
> + goto failure;
> +
> + pBuffer->pPixSourceGC = CreateGC ((DrawablePtr)pWin,
> + GCGraphicsExposures, &gcval, &status, (XID)0, serverClient);
> + if (!pBuffer->pPixSourceGC)
> + goto failure;
> +
> + pBuffer->pPixMaskGC = CreateGC ((DrawablePtr)pWin,
> + GCGraphicsExposures, &gcval, &status, (XID)0, serverClient);
> + if (!pBuffer->pPixMaskGC)
> + goto failure;
> +
> #ifdef ARGB_CURSOR
> - pBuffer->pRootPicture = NULL;
> - pBuffer->pTempPicture = NULL;
> + miDCMakePicture(&pBuffer->pRootPicture, &pWin->drawable, pWin);
> + if (!pBuffer->pRootPicture)
> + goto failure;
> +
> + pBuffer->pTempPicture = NULL;
> #endif
> - pBuffer->pSave = pBuffer->pTemp = NULL;
> +
> + // these get (re)allocated lazily depending on the cursor size
> + pBuffer->pSave = pBuffer->pTemp = NULL;
> + }
>
> return TRUE;
> +
> +failure:
> +
> + miDCDeviceCleanup(pDev, pScreen);
> +
> + return FALSE;
> }
>
> static void
> miDCDeviceCleanup(DeviceIntPtr pDev, ScreenPtr pScreen)
> {
> miDCBufferPtr pBuffer;
> + int i;
>
> if (DevHasCursor(pDev))
> {
> - pBuffer = MIDCBUFFER(pDev);
> - tossGC (pBuffer->pSourceGC);
> - tossGC (pBuffer->pMaskGC);
> - tossGC (pBuffer->pSaveGC);
> - tossGC (pBuffer->pRestoreGC);
> - tossGC (pBuffer->pMoveGC);
> - tossGC (pBuffer->pPixSourceGC);
> - tossGC (pBuffer->pPixMaskGC);
> - tossPix (pBuffer->pSave);
> - tossPix (pBuffer->pTemp);
> + for (i = 0; i < screenInfo.numScreens; i++)
> + {
> + pScreen = screenInfo.screens[i];
> +
> + pBuffer = MIDCBUFFER(pDev);
> +
> + if (pBuffer)
> + {
> + if (pBuffer->pSourceGC) FreeGC(pBuffer->pSourceGC, (GContext) 0);
> + if (pBuffer->pMaskGC) FreeGC(pBuffer->pMaskGC, (GContext) 0);
> + if (pBuffer->pSaveGC) FreeGC(pBuffer->pSaveGC, (GContext) 0);
> + if (pBuffer->pRestoreGC) FreeGC(pBuffer->pRestoreGC, (GContext) 0);
> + if (pBuffer->pMoveGC) FreeGC(pBuffer->pMoveGC, (GContext) 0);
> + if (pBuffer->pPixSourceGC) FreeGC(pBuffer->pPixSourceGC, (GContext) 0);
> + if (pBuffer->pPixMaskGC) FreeGC(pBuffer->pPixMaskGC, (GContext) 0);
could we make FreeGC handle a null pointer?
that would clean the code up a bit.
> +
> #ifdef ARGB_CURSOR
> -#if 0 /* This has been free()d before */
> - tossPict (pScreenPriv->pRootPicture);
> -#endif
> - tossPict (pBuffer->pTempPicture);
> + if (pBuffer->pRootPicture) FreePicture(pBuffer->pRootPicture, 0);
> + if (pBuffer->pTempPicture) FreePicture(pBuffer->pTempPicture, 0);
> #endif
> - xfree(pBuffer);
> - dixSetPrivate(&pDev->devPrivates, miDCSpriteKey, NULL);
> +
> + if (pBuffer->pSave) (*pScreen->DestroyPixmap)(pBuffer->pSave);
> + if (pBuffer->pTemp) (*pScreen->DestroyPixmap)(pBuffer->pTemp);
> +
> + xfree(pBuffer);
> + dixSetPrivate(&pDev->devPrivates, miDCSpriteKey + pScreen->myNum, NULL);
> + }
> + }
> }
> }
> --
> 1.7.0
other than this, looks good to me in principle
Cheers,
Peter
More information about the xorg-devel
mailing list