[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