[PATCH] move reference counting out of the UseHWCursor/UseHWCursorARGB functions

[Michel Dänzer]

On Tue, 2010-03-09 at 18:38 +0100, Roland Scheidegger wrote: 
> The problem is that the xf86_use_hw_cursor(_argb) functions may get this
> correctly now, some drivers will replace this with their own functions.

To be pedantic, it's not so much that the drivers 'replace' these as
that they're generic implementations provided by the server for RandR
1.2 capable drivers.

> It is pretty insane to expect them to do reference counting of the
> cursor (as an example, look at driver/xf86-video-vmware to see how that
> looks like as a workaround). There are even places in xserver itself
> which replace these two functions.
> 
> FWIW, the segfaults are caused because the reference count of the cursor
> reached zero, hence the cursor was freed, however
> xf86CursorEnableDisableFBAccess() brought it back to life from the dead
> (from the SavedCursor), at this point obviously anything can happen.
> This patch hence adds reference counting in xf86CursorSetCursor.
> In theory with this it should be possible to remove the reference
> counting in the UseHwCursor functions I think, though it should also be
> safe to keep them.
> I can't guarantee though this is fully correct as the reference counting
> looks a bit fishy overall, hopefully it won't create a memleak...

I think it's missing some code in xf86CursorCloseScreen() to prevent a
possible leak of one CursorRec per server generation.

> 
> 
> diff --git a/hw/xfree86/ramdac/xf86Cursor.c
> b/hw/xfree86/ramdac/xf86Cursor.c index 6b71f46..109d037 100644 ---
> a/hw/xfree86/ramdac/xf86Cursor.c +++ b/hw/xfree86/ramdac/xf86Cursor.c
> @@ -317,13 +317,18 @@ xf86CursorSetCursor(DeviceIntPtr pDev, ScreenPtr
> pScreen, CursorPtr pCurs, if (pDev == inputInfo.pointer || (!
> IsMaster(pDev) && pDev->u.master == inputInfo.pointer)) { + CursorPtr
> pOldCursor = ScreenPriv->CurrentCursor; ScreenPriv->CurrentCursor =
> pCurs; + pCurs->refcnt++; ScreenPriv->x = x; ScreenPriv->y = y;
> ScreenPriv->CursorToRestore = NULL; ScreenPriv->HotX =
> pCurs->bits->xhot; ScreenPriv->HotY = pCurs->bits->yhot; + if
> (pOldCursor) + FreeCursor(pOldCursor, None); + if (!
> infoPtr->pScrn->vtSema) ScreenPriv->SavedCursor = pCurs;

This can be done simpler as below, right?


diff --git a/hw/xfree86/ramdac/xf86Cursor.c b/hw/xfree86/ramdac/xf86Cursor.c
index 896ed37..4b33582 100644
--- a/hw/xfree86/ramdac/xf86Cursor.c
+++ b/hw/xfree86/ramdac/xf86Cursor.c
@@ -129,6 +129,9 @@ xf86CursorCloseScreen(int i, ScreenPtr pScreen)
     if (ScreenPriv->isUp && pScrn->vtSema)
 	xf86SetCursor(pScreen, NullCursor, ScreenPriv->x, ScreenPriv->y);
 
+    if (ScreenPriv->CurrentCursor)
+	FreeCursor(ScreenPriv->CurrentCursor, None);
+
     pScreen->CloseScreen = ScreenPriv->CloseScreen;
     pScreen->QueryBestSize = ScreenPriv->QueryBestSize;
     pScreen->RecolorCursor = ScreenPriv->RecolorCursor;
@@ -317,6 +320,9 @@ xf86CursorSetCursor(DeviceIntPtr pDev, ScreenPtr pScreen, CursorPtr pCurs,
     if (pDev == inputInfo.pointer ||
         (!pDev->isMaster && pDev->u.master == inputInfo.pointer))
     {
+	pCurs->refcnt++;
+	if (ScreenPriv->CurrentCursor)
+	    FreeCursor(ScreenPriv->CurrentCursor, None);
 	ScreenPriv->CurrentCursor = pCurs;
 	ScreenPriv->x = x;
 	ScreenPriv->y = y;


-- 
Earthling Michel Dänzer           |                http://www.vmware.com
Libre software enthusiast         |          Debian, X and DRI developer