[PATCH] mi: fix software cursor with several X screens

Pierre-Loup A. Griffais pgriffais at nvidia.com
Wed Apr 7 14:08:18 PDT 2010


Thanks for the review; responses inline and attached revised version of the patch:

On 04/06/2010 06:52 PM, Peter Hutterer wrote:
> 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).

Where do we hook the equivalent cleanup function in that case? Having these 
resources being allocated upfront and persisting across enable/disable seems 
more consistent in this context. It also means that toggling a device in a 
resource exhausted scenario will ensure that the existing storage remains 
optimal. Does that make sense?

>
>>> 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.

Good point, done.

>
>>
>>   /*
>>    * 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)

Sure.

>
>> @@ -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.

Done.

>
>> +
>> +        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.

I guess we could, but it seems inconsistent with the rest of the server (not to 
mention it would also broadens the scope of this change by a lot). I can bring 
back a macro that implicitely performs the check if you prefer? (even though I 
didn't like having macros like that with a local scope).

>
>> +
>>   #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

Thanks.

>
> Cheers,
>    Peter


More information about the xorg-devel mailing list