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

[Roland Scheidegger]

On 10.03.2010 12:02, Michel Dänzer wrote:
> 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.
Right. Still, (old) drivers are replacing this generic code with their
own versions.

> 
>> 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.
You're right I suppose (I know I did that in the vmware driver but I
wasn't really sure it was actually necessary).

> 
> This can be done simpler as below, right?
Yes, certainly, looks even better.

Roland

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