[PATCH] Revert "mi: don't thrash resources when displaying the software cursor across screens"

Peter Hutterer peter.hutterer at who-t.net
Thu Apr 22 16:22:14 PDT 2010


On Wed, Apr 21, 2010 at 04:51:17PM -0700, Pierre-Loup A. Griffais wrote:
> Since the revert is already pushed, here's a new version of the
> change as Peter pushed it including the teardown crash fix.
> 
> Thanks,
>  - Pierre-Loup
> 

> From 7ffae8aaa4ac445a727e663e1e1bf05a2510cd35 Mon Sep 17 00:00:00 2001
> From: Pierre-Loup A. Griffais <pgriffais at nvidia.com>
> Date: Wed, 21 Apr 2010 16:46:17 -0700
> Subject: [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>
> Reviewed-by: Peter Hutterer <peter.hutterer at who-t.net>
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---
>  mi/midispcur.c |  269 ++++++++++++++++++++++---------------------------------
>  1 files changed, 108 insertions(+), 161 deletions(-)
> 
> diff --git a/mi/midispcur.c b/mi/midispcur.c
> index 3fb7e02..9041630 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;
> @@ -75,10 +75,10 @@ typedef struct {
>  #endif
>  } miDCBufferRec, *miDCBufferPtr;
>  
> -#define MIDCBUFFER(dev) \
> +#define MIDCBUFFER(dev, screen) \
>   ((DevHasCursor(dev)) ? \
> -  (miDCBufferPtr)dixLookupPrivate(&dev->devPrivates, miDCSpriteKey) : \
> -  (miDCBufferPtr)dixLookupPrivate(&dev->u.master->devPrivates, miDCSpriteKey))
> +  (miDCBufferPtr)dixLookupPrivate(&dev->devPrivates, miDCSpriteKey + (screen)->myNum) : \
> +  (miDCBufferPtr)dixLookupPrivate(&dev->u.master->devPrivates, miDCSpriteKey + (screen)->myNum))
>  
>  /* 
>   * 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(WindowPtr pWin)
>  {
>      GCPtr pGC;
>      int   status;
> @@ -431,7 +422,6 @@ miDCMakeGC(
>      pGC = CreateGC((DrawablePtr)pWin,
>  		   GCSubwindowMode|GCGraphicsExposures, gcvals, &status,
>  		   (XID)0, serverClient);
> -    *ppGC = pGC;
>      return pGC;
>  }
>  
> @@ -456,22 +446,11 @@ miDCPutUpCursor (DeviceIntPtr pDev, ScreenPtr pScreen, CursorPtr pCursor,
>      pScreenPriv = (miDCScreenPtr)dixLookupPrivate(&pScreen->devPrivates,
>  						  miDCScreenKey);
>      pWin = WindowTable[pScreen->myNum];
> -    pBuffer = MIDCBUFFER(pDev);
> +    pBuffer = MIDCBUFFER(pDev, pScreen);
>  
>  #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,
> @@ -531,7 +483,7 @@ miDCSaveUnderCursor (DeviceIntPtr pDev, ScreenPtr pScreen,
>  
>      pScreenPriv = (miDCScreenPtr)dixLookupPrivate(&pScreen->devPrivates,
>  						  miDCScreenKey);
> -    pBuffer = MIDCBUFFER(pDev);
> +    pBuffer = MIDCBUFFER(pDev, pScreen);
>  
>      pSave = pBuffer->pSave;
>      pWin = WindowTable[pScreen->myNum];
> @@ -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);
> @@ -572,20 +517,13 @@ miDCRestoreUnderCursor (DeviceIntPtr pDev, ScreenPtr pScreen,
>  
>      pScreenPriv = (miDCScreenPtr)dixLookupPrivate(&pScreen->devPrivates,
>  						  miDCScreenKey);
> -    pBuffer = MIDCBUFFER(pDev);
> +    pBuffer = MIDCBUFFER(pDev, pScreen);
>      pSave = pBuffer->pSave;
>  
>      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);
> @@ -607,7 +545,7 @@ miDCChangeSave (DeviceIntPtr pDev, ScreenPtr pScreen,
>  
>      pScreenPriv = (miDCScreenPtr)dixLookupPrivate(&pScreen->devPrivates,
>  						  miDCScreenKey);
> -    pBuffer = MIDCBUFFER(pDev);
> +    pBuffer = MIDCBUFFER(pDev, pScreen);
>  
>      pSave = pBuffer->pSave;
>      pWin = WindowTable[pScreen->myNum];
> @@ -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);
> @@ -766,7 +690,7 @@ miDCMoveCursor (DeviceIntPtr pDev, ScreenPtr pScreen, CursorPtr pCursor,
>      pScreenPriv = (miDCScreenPtr)dixLookupPrivate(&pScreen->devPrivates,
>  						  miDCScreenKey);
>      pWin = WindowTable[pScreen->myNum];
> -    pBuffer = MIDCBUFFER(pDev);
> +    pBuffer = MIDCBUFFER(pDev, pScreen);
>  
>      pTemp = pBuffer->pTemp;
>      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,108 @@ 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);
> +        pWin = WindowTable[pScreen->myNum];
> +
> +        pBuffer->pSourceGC = miDCMakeGC(pWin);
> +        if (!pBuffer->pSourceGC)
> +            goto failure;
> +
> +        pBuffer->pMaskGC = miDCMakeGC(pWin);
> +        if (!pBuffer->pMaskGC)
> +            goto failure;
> +
> +        pBuffer->pSaveGC = miDCMakeGC(pWin);
> +        if (!pBuffer->pSaveGC)
> +            goto failure;
> +
> +        pBuffer->pRestoreGC = miDCMakeGC(pWin);
> +        if (!pBuffer->pRestoreGC)
> +            goto failure;
> +
> +        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);
> -#ifdef ARGB_CURSOR
> -#if 0				/* This has been free()d before */
> -        tossPict (pScreenPriv->pRootPicture);
> -#endif 
> -        tossPict (pBuffer->pTempPicture);
> -#endif
> -        xfree(pBuffer);
> -        dixSetPrivate(&pDev->devPrivates, miDCSpriteKey, NULL);
> +        for (i = 0; i < screenInfo.numScreens; i++)
> +        {
> +            pScreen = screenInfo.screens[i];
> +
> +            pBuffer = MIDCBUFFER(pDev, pScreen);
> +
> +            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);
> +
> +                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.4


confirming that this doesn't crash anymore.
Tested-by: Peter Hutterer <peter.hutterer at who-t.net>

though I didn't test the actual problem it is supposed to fix (the screen
crossing)

Cheers,
  Peter


More information about the xorg-devel mailing list