Cursor code problems in randr1.2

[Keith Packard]

On Fri, 2007-08-10 at 17:53 -0300, Paulo Cesar Pereira de Andrade wrote:
> Hi, I made this patch for an OEM, to fix crashes when switching VTs 
> and resuming suspend.

Thanks much; we've had a few reports about this, but no way to
understand what was going on.

>  I believe the
> patch is fixing the problem, but not the proper way, so I am submitting 
> it here so it should make it clear for
> people working with the related code what is the real cause of the crashes.
> --
> --- xorg-server-1.3.0.0/hw/xfree86/modes/xf86RandR12.c.orig    
> 2007-08-03 10:53:11.000000000 -0300
> +++ xorg-server-1.3.0.0/hw/xfree86/modes/xf86RandR12.c    2007-08-03 
> 10:53:43.000000000 -0300
> @@ -339,6 +339,9 @@ xf86RandR12ScreenSetSize (ScreenPtr    pScr
>      WindowPtr        pRoot = WindowTable[pScreen->myNum];
>      Bool        ret = FALSE;
>  
> +    if (!pScrn->vtSema)
> +    return FALSE;
> +
>      if (randrp->virtualX == -1 || randrp->virtualY == -1)
>      {
>      randrp->virtualX = pScrn->virtualX;

I don't think this is what we want; it will make changing modes
impossible while the server is switched away. Instead, it should suffice
to have the calls to EnableDisableFBAccess protected with tests for
vtSema. This patch is entirely untested, but should do the trick:

diff --git a/hw/xfree86/modes/xf86RandR12.c b/hw/xfree86/modes/xf86RandR12.c
index 9d74e53..ae0a2ce 100644
--- a/hw/xfree86/modes/xf86RandR12.c
+++ b/hw/xfree86/modes/xf86RandR12.c
@@ -345,7 +345,7 @@ xf86RandR12ScreenSetSize (ScreenPtr pScreen,
        randrp->virtualX = pScrn->virtualX;
        randrp->virtualY = pScrn->virtualY;
     }
-    if (pRoot)
+    if (pRoot && pScrn->vtSema)
        (*pScrn->EnableDisableFBAccess) (pScreen->myNum, FALSE);
 
     /* Let the driver update virtualX and virtualY */
@@ -363,7 +363,7 @@ xf86RandR12ScreenSetSize (ScreenPtr pScreen,
     xf86SetViewport (pScreen, 0, 0);
 
 finish:
-    if (pRoot)
+    if (pRoot && pScrn->vtSema)
        (*pScrn->EnableDisableFBAccess) (pScreen->myNum, TRUE);
 #if RANDR_12_INTERFACE
     if (WindowTable[pScreen->myNum] && ret)

> --- xorg-server-1.3.0.0/hw/xfree86/modes/xf86Cursors.c.orig    
> 2007-08-03 10:53:59.000000000 -0300
> +++ xorg-server-1.3.0.0/hw/xfree86/modes/xf86Cursors.c    2007-08-03 
> 10:59:36.000000000 -0300
> @@ -47,6 +47,21 @@
>  #include "cursorstr.h"
>  
>  /*
> + * This function tries to make sure the driver/server code will not
> + * used memory no longer pointing to cursor data by updating the
> + * cursor reference counter accordingly.

Yes, this is a good plan. However, you must use FreeCursor instead of
decrementing the cursor refcnt. FreeCursor will actually free the cursor
when the refcount hits zero.

Something like this entirely untested patch:

diff --git a/hw/xfree86/modes/xf86Cursors.c b/hw/xfree86/modes/xf86Cursors.c
index 396bf30..92b90a9 100644
--- a/hw/xfree86/modes/xf86Cursors.c
+++ b/hw/xfree86/modes/xf86Cursors.c
@@ -447,7 +447,10 @@ xf86_use_hw_cursor (ScreenPtr screen, CursorPtr cursor)
     xf86CrtcConfigPtr   xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
     xf86CursorInfoPtr  cursor_info = xf86_config->cursor_info;

+    if (xf86_config->cursor)
+       FreeCursor (xf86_config->cursor, None);
     xf86_config->cursor = cursor;
+    ++cursor->refcnt;

     if (cursor->bits->width > cursor_info->MaxWidth ||
        cursor->bits->height> cursor_info->MaxHeight)
@@ -463,7 +466,10 @@ xf86_use_hw_cursor_argb (ScreenPtr screen, CursorPtr cursor)
     xf86CrtcConfigPtr   xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
     xf86CursorInfoPtr  cursor_info = xf86_config->cursor_info;

+    if (xf86_config->cursor)
+       FreeCursor (xf86_config->cursor, None);
     xf86_config->cursor = cursor;
+    ++cursor->refcnt;

     /* Make sure ARGB support is available */
     if ((cursor_info->Flags & HARDWARE_CURSOR_ARGB) == 0)
@@ -632,4 +638,9 @@ xf86_cursors_fini (ScreenPtr screen)
        xfree (xf86_config->cursor_image);
        xf86_config->cursor_image = NULL;
     }
+    if (xf86_config->cursor)
+    {
+       FreeCursor (xf86_config->cursor, None);
+       xf86_config->cursor = NULL;
+    }
 }

>   This patch also serves as a hint that something else is wrong, as 
> there are significant cursors being
> deallocated in dix.c:FreeCursor() but not being allocated in 
> dix.c:AllocCursor().

No, that's correct -- FreeCursor decrements the cursor reference count,
and so will be called many times the the same cursor as it gets added
and removed from various windows.

I'd appreciate some testing of the above patches to see if they solve
your problems.

And, thanks much for sending proposed patches along; I like to see
development done this way.

-- 
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
URL: <http://lists.x.org/archives/xorg/attachments/20070810/9a8aec64/attachment.pgp>