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

On 04/20/2010 06:47 PM, Peter Hutterer wrote:
This commit leads to a segfault on the very first XTS test case.

Backtrace:
0: /opt/xorg/bin/Xorg (xorg_backtrace+0x3b) [0x80a33db]
1: /opt/xorg/bin/Xorg (0x8048000+0x62a75) [0x80aaa75]
2: (vdso) (__kernel_rt_sigreturn+0x0) [0x5d140c]
3: /lib/libc.so.6 (0x9bb000+0x73579) [0xa2e579]
4: /lib/libc.so.6 (realloc+0xe0) [0xa2e830]
5: /opt/xorg/bin/Xorg (Xrealloc+0x33) [0x80a3f33]
6: /opt/xorg/bin/Xorg (0x8048000+0x1ab79) [0x8062b79]
7: /opt/xorg/bin/Xorg (0x8048000+0x1ac4e) [0x8062c4e]
8: /opt/xorg/bin/Xorg (RegisterExtensionNames+0x2ce) [0x8062fbe]
9: /opt/xorg/bin/Xorg (AddExtension+0x19a) [0x807bd7a]
10: /opt/xorg//lib/xorg/modules/extensions/libextmod.so (0x728000+0x1169a)
[0x73969a]
11: /opt/xorg/bin/Xorg (InitExtensions+0x85) [0x80c0eb5]
12: /opt/xorg/bin/Xorg (0x8048000+0x1a51d) [0x806251d]
13: /lib/libc.so.6 (__libc_start_main+0xe6) [0x9d1bb6]
14: /opt/xorg/bin/Xorg (0x8048000+0x1a2a1) [0x80622a1]
Segmentation fault at address 0x10b2d5f8

valgrind output:
==5069== Invalid read of size 4
==5069==    at 0x80F928D: FreePicture (picture.c:1531)
==5069==    by 0x818DDEF: miDCDeviceCleanup (midispcur.c:867)
==5069==    by 0x81B97F0: miSpriteDeviceCursorCleanup (misprite.c:968)
==5069==    by 0x80995FA: miPointerDeviceCleanup (mipointer.c:292)
==5069==    by 0x807973E: CloseDevice (devices.c:840)
==5069==    by 0x80799B6: CloseDownDevices (devices.c:933)
==5069==    by 0x8062705: main (main.c:309)
==5069==  Address 0x4cce844 is 12 bytes inside a block of size 84 free'd
==5069==    at 0x40057F6: free (vg_replace_malloc.c:325)
==5069==    by 0x80A3DE0: Xfree (utils.c:1154)
==5069==    by 0x80F9332: FreePicture (picture.c:1576)
==5069==    by 0x80FBB4B: PictureDestroyWindow (picture.c:69)
==5069==    by 0x810B1A3: damageDestroyWindow (damage.c:1840)
==5069==    by 0x80864F1: FreeWindowResources (window.c:846)
==5069==    by 0x8086812: DeleteWindow (window.c:925)
==5069==    by 0x806B53E: FreeClientResources (resource.c:806)
==5069==    by 0x806B60F: FreeAllResources (resource.c:823)
==5069==    by 0x80626E4: main (main.c:299)
==5069==
==5069== Invalid write of size 4
==5069==    at 0x80F9295: FreePicture (picture.c:1531)
==5069==    by 0x818DDEF: miDCDeviceCleanup (midispcur.c:867)
==5069==    by 0x81B97F0: miSpriteDeviceCursorCleanup (misprite.c:968)
==5069==    by 0x80995FA: miPointerDeviceCleanup (mipointer.c:292)
==5069==    by 0x807973E: CloseDevice (devices.c:840)
==5069==    by 0x80799B6: CloseDownDevices (devices.c:933)
==5069==    by 0x8062705: main (main.c:309)
==5069==  Address 0x4cce844 is 12 bytes inside a block of size 84 free'd
==5069==    at 0x40057F6: free (vg_replace_malloc.c:325)
==5069==    by 0x80A3DE0: Xfree (utils.c:1154)
==5069==    by 0x80F9332: FreePicture (picture.c:1576)
==5069==    by 0x80FBB4B: PictureDestroyWindow (picture.c:69)
==5069==    by 0x810B1A3: damageDestroyWindow (damage.c:1840)
==5069==    by 0x80864F1: FreeWindowResources (window.c:846)
==5069==    by 0x8086812: DeleteWindow (window.c:925)
==5069==    by 0x806B53E: FreeClientResources (resource.c:806)
==5069==    by 0x806B60F: FreeAllResources (resource.c:823)
==5069==    by 0x80626E4: main (main.c:299)

XTS test case: Xproto pAllocColor

This reverts commit 00b8b7ad61b6f818271fb4d1e383113170309d72.

Signed-off-by: Peter Hutterer<peter.hutte...@who-t.net>
---
  mi/midispcur.c |  270 +++++++++++++++++++++++++++++++++-----------------------
  1 files changed, 159 insertions(+), 111 deletions(-)

diff --git a/mi/midispcur.c b/mi/midispcur.c
index 3a31b74..3fb7e02 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 per-screen private data */
-static int miDCSpriteKeyIndex[MAXSCREENS];
-static DevPrivateKey miDCSpriteKey = miDCSpriteKeyIndex;
+/* per device private data */
+static int miDCSpriteKeyIndex;
+static DevPrivateKey miDCSpriteKey =&miDCSpriteKeyIndex;

  typedef struct {
      GCPtr          pSourceGC, pMaskGC;
@@ -75,10 +75,10 @@ typedef struct {
  #endif
  } miDCBufferRec, *miDCBufferPtr;

-#define MIDCBUFFER(dev, screen) \
+#define MIDCBUFFER(dev) \
   ((DevHasCursor(dev)) ? \
-  (miDCBufferPtr)dixLookupPrivate(&dev->devPrivates, miDCSpriteKey + 
(screen)->myNum) : \
-  (miDCBufferPtr)dixLookupPrivate(&dev->u.master->devPrivates, miDCSpriteKey + 
(screen)->myNum))
+  (miDCBufferPtr)dixLookupPrivate(&dev->devPrivates, miDCSpriteKey) : \
+  (miDCBufferPtr)dixLookupPrivate(&dev->u.master->devPrivates, miDCSpriteKey))

  /*
   * The core pointer buffer will point to the index of the virtual core pointer
@@ -158,6 +158,10 @@ 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)
  {
@@ -179,6 +183,7 @@ miDCRealizeCursor (ScreenPtr pScreen, CursorPtr pCursor)
  }

  #ifdef ARGB_CURSOR
+#define EnsurePicture(picture,draw,win) (picture || 
miDCMakePicture(&picture,draw,win))

  static VisualPtr
  miDCGetWindowVisual (WindowPtr pWin)
@@ -410,8 +415,12 @@ miDCPutBits (
      (*maskGC->ops->PushPixels) (maskGC, pPriv->maskBits, pDrawable, w, h, x, 
y);
  }

+#define EnsureGC(gc,win) (gc || miDCMakeGC(&gc, win))
+
  static GCPtr
-miDCMakeGC(WindowPtr pWin)
+miDCMakeGC(
+    GCPtr      *ppGC,
+    WindowPtr  pWin)
  {
      GCPtr pGC;
      int   status;
@@ -422,6 +431,7 @@ miDCMakeGC(WindowPtr pWin)
      pGC = CreateGC((DrawablePtr)pWin,
                    GCSubwindowMode|GCGraphicsExposures, gcvals,&status,
                    (XID)0, serverClient);
+    *ppGC = pGC;
      return pGC;
  }

@@ -446,11 +456,22 @@ miDCPutUpCursor (DeviceIntPtr pDev, ScreenPtr pScreen, 
CursorPtr pCursor,
      pScreenPriv = (miDCScreenPtr)dixLookupPrivate(&pScreen->devPrivates,
                                                   miDCScreenKey);
      pWin = WindowTable[pScreen->myNum];
-    pBuffer = MIDCBUFFER(pDev, pScreen);
+    pBuffer = MIDCBUFFER(pDev);

  #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,
@@ -463,6 +484,33 @@ 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,
@@ -483,7 +531,7 @@ miDCSaveUnderCursor (DeviceIntPtr pDev, ScreenPtr pScreen,

      pScreenPriv = (miDCScreenPtr)dixLookupPrivate(&pScreen->devPrivates,
                                                   miDCScreenKey);
-    pBuffer = MIDCBUFFER(pDev, pScreen);
+    pBuffer = MIDCBUFFER(pDev);

      pSave = pBuffer->pSave;
      pWin = WindowTable[pScreen->myNum];
@@ -496,7 +544,14 @@ 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);
@@ -517,13 +572,20 @@ miDCRestoreUnderCursor (DeviceIntPtr pDev, ScreenPtr 
pScreen,

      pScreenPriv = (miDCScreenPtr)dixLookupPrivate(&pScreen->devPrivates,
                                                   miDCScreenKey);
-    pBuffer = MIDCBUFFER(pDev, pScreen);
+    pBuffer = MIDCBUFFER(pDev);
      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);
@@ -545,7 +607,7 @@ miDCChangeSave (DeviceIntPtr pDev, ScreenPtr pScreen,

      pScreenPriv = (miDCScreenPtr)dixLookupPrivate(&pScreen->devPrivates,
                                                   miDCScreenKey);
-    pBuffer = MIDCBUFFER(pDev, pScreen);
+    pBuffer = MIDCBUFFER(pDev);

      pSave = pBuffer->pSave;
      pWin = WindowTable[pScreen->myNum];
@@ -554,7 +616,14 @@ 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);
@@ -593,7 +662,14 @@ 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);
@@ -690,7 +766,7 @@ miDCMoveCursor (DeviceIntPtr pDev, ScreenPtr pScreen, 
CursorPtr pCursor,
      pScreenPriv = (miDCScreenPtr)dixLookupPrivate(&pScreen->devPrivates,
                                                   miDCScreenKey);
      pWin = WindowTable[pScreen->myNum];
-    pBuffer = MIDCBUFFER(pDev, pScreen);
+    pBuffer = MIDCBUFFER(pDev);

      pTemp = pBuffer->pTemp;
      if (!pTemp ||
@@ -733,9 +809,17 @@ miDCMoveCursor (DeviceIntPtr pDev, ScreenPtr pScreen, 
CursorPtr pCursor,
  #ifdef ARGB_CURSOR
      if (pPriv->pPicture)
      {
-       if (!pBuffer->pTempPicture)
-            miDCMakePicture(&pBuffer->pTempPicture,&pTemp->drawable, pWin);
+        /* see comment in miDCPutUpCursor */
+        if (pBuffer->pTempPicture&&
+                pBuffer->pTempPicture->pDrawable&&
+                pBuffer->pTempPicture->pDrawable->pScreen != pScreen)
+        {
+            tossPict(pBuffer->pTempPicture);
+            pBuffer->pTempPicture = NULL;
+        }

+       if (!EnsurePicture(pBuffer->pTempPicture,&pTemp->drawable, pWin))
+           return FALSE;
         CompositePicture (PictOpOver,
                           pPriv->pPicture,
                           NULL,
@@ -748,12 +832,38 @@ 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);
@@ -767,113 +877,51 @@ miDCMoveCursor (DeviceIntPtr pDev, ScreenPtr pScreen, 
CursorPtr pCursor,
  static Bool
  miDCDeviceInitialize(DeviceIntPtr pDev, ScreenPtr pScreen)
  {
-    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;
-
+    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;
  #ifdef ARGB_CURSOR
-        miDCMakePicture(&pBuffer->pRootPicture,&pWin->drawable, pWin);
-        if (!pBuffer->pRootPicture)
-            goto failure;
-
-        pBuffer->pTempPicture = NULL;
+    pBuffer->pRootPicture = NULL;
+    pBuffer->pTempPicture = NULL;
  #endif
-
-        // these get (re)allocated lazily depending on the cursor size
-        pBuffer->pSave = pBuffer->pTemp = NULL;
-    }
+    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))
      {
-        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);
-
+        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 (pBuffer->pRootPicture) FreePicture(pBuffer->pRootPicture, 
0);
-                if (pBuffer->pTempPicture) FreePicture(pBuffer->pTempPicture, 
0);
+#if 0                          /* This has been free()d before */
+        tossPict (pScreenPriv->pRootPicture);
+#endif
+        tossPict (pBuffer->pTempPicture);
  #endif
-
-                if (pBuffer->pSave) (*pScreen->DestroyPixmap)(pBuffer->pSave);
-                if (pBuffer->pTemp) (*pScreen->DestroyPixmap)(pBuffer->pTemp);
-
-                xfree(pBuffer);
-                dixSetPrivate(&pDev->devPrivates, miDCSpriteKey + 
pScreen->myNum, NULL);
-            }
-        }
+        xfree(pBuffer);
+        dixSetPrivate(&pDev->devPrivates, miDCSpriteKey, NULL);
      }
  }
--
1.6.6.1

>From 7ffae8aaa4ac445a727e663e1e1bf05a2510cd35 Mon Sep 17 00:00:00 2001
From: Pierre-Loup A. Griffais <pgriff...@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 <pgriff...@nvidia.com>
Reviewed-by: Peter Hutterer <peter.hutte...@who-t.net>
Signed-off-by: Peter Hutterer <peter.hutte...@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

_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Reply via email to