Cursor code problems in randr1.2
Paulo Cesar Pereira de Andrade
pcpa at mandriva.com.br
Fri Aug 10 13:53:16 PDT 2007
Hi, I made this patch for an OEM, to fix crashes when switching VTs
and resuming suspend. 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;
--- 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.
+ */
+static void
+xf86_config_set_cursor(xf86CrtcConfigPtr xf86_config, CursorPtr cursor)
+{
+ if (xf86_config->cursor)
+ --xf86_config->cursor->refcnt;
+ xf86_config->cursor = cursor;
+ if (cursor)
+ ++xf86_config->cursor->refcnt;
+}
+
+/*
* Given a screen coordinate, rotate back to a cursor source coordinate
*/
static void
@@ -419,7 +434,7 @@ xf86_use_hw_cursor (ScreenPtr screen, Cu
xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
xf86CursorInfoPtr cursor_info = xf86_config->cursor_info;
- xf86_config->cursor = cursor;
+ xf86_config_set_cursor(xf86_config, cursor);
if (cursor->bits->width > cursor_info->MaxWidth ||
cursor->bits->height> cursor_info->MaxHeight)
@@ -435,7 +450,7 @@ xf86_use_hw_cursor_argb (ScreenPtr scree
xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
xf86CursorInfoPtr cursor_info = xf86_config->cursor_info;
- xf86_config->cursor = cursor;
+ xf86_config_set_cursor(xf86_config, cursor);
/* Make sure ARGB support is available */
if ((cursor_info->Flags & HARDWARE_CURSOR_ARGB) == 0)
@@ -533,7 +548,7 @@ xf86_cursors_init (ScreenPtr screen, int
}
#endif
- xf86_config->cursor = NULL;
+ xf86_config_set_cursor(xf86_config, NULL);
xf86_hide_cursors (scrn);
return xf86InitCursor (screen, cursor_info);
--
The xf86RandR12.c patch fixes what appears to be randr code trying to
access the framebuffer when the
server insn't in the active VT.
The cursor refcnt patch is what I believe is not the proper patch, but
if not applied the server may crash when returning from a VT switch or
suspend as it will try to deference cursor->bits, but that memory has
been already released (and most likely being used to store some other
kind of data) as the refcnt reached 0.
I also did this debug patch to make sure the refcnt changes would not
end up creating a memory leak as I was not sure if the
increase/decrease would be nested with the requests and higher level
resource handling code.
--
--- xorg-server-1.3.0.0/dix/cursor.c.orig 2007-08-03
11:20:56.000000000 -0300
+++ xorg-server-1.3.0.0/dix/cursor.c 2007-08-03 11:05:43.000000000 -0300
@@ -74,6 +74,60 @@ static GlyphSharePtr sharedGlyphs = (Gly
static CARD32 cursorSerial;
#endif
+#if 0
+struct CursorDebug {
+ CursorPtr cursor;
+ struct CursorDebug *next;
+};
+static int cursor_count;
+static struct CursorDebug *cursor_debug;
+static void debug_add(CursorPtr cursor)
+{
+ struct CursorDebug *ptr = cursor_debug;
+
+ if (cursor == NULL)
+ return;
+ if (ptr) {
+ for (; ptr->next; ptr = ptr->next)
+ if (ptr->cursor == cursor)
+ return;
+ ptr->next = xalloc(sizeof(struct CursorDebug));
+ ptr = ptr->next;
+ }
+ else {
+ cursor_debug = xalloc(sizeof(struct CursorDebug));
+ ptr = cursor_debug;
+ }
+ ptr->cursor = cursor;
+ ptr->next = NULL;
+ ++cursor_count;
+ ErrorF("*** New Cursor %p (%d cursors)\n", cursor, cursor_count);
+}
+static void debug_rem(CursorPtr cursor)
+{
+ struct CursorDebug *ptr = cursor_debug;
+ struct CursorDebug *prev = ptr;
+
+ if (cursor == NULL)
+ return;
+ if (ptr) {
+ for (ptr = ptr->next; ptr; prev = ptr, ptr = ptr->next)
+ if (ptr->cursor == cursor)
+ break;
+ }
+ if (ptr == NULL)
+ ErrorF("*** Cursor %p allocated elsewhere or bogus pointer?\n",
cursor);
+ else {
+ prev->next = ptr->next;
+ xfree(ptr);
+ if (ptr == cursor_debug)
+ cursor_debug = NULL;
+ --cursor_count;
+ ErrorF("*** Del Cursor %p (%d cursors)\n", cursor, cursor_count);
+ }
+}
+#endif /* 0 */
+
static void
FreeCursorBits(CursorBitsPtr bits)
{
@@ -124,6 +178,9 @@ FreeCursor(pointer value, XID cid)
(void)( *pscr->UnrealizeCursor)( pscr, pCurs);
}
FreeCursorBits(pCurs->bits);
+#if 0
+ debug_rem(pCurs);
+#endif
xfree( pCurs);
return(Success);
}
@@ -225,6 +282,9 @@ AllocCursorARGB(unsigned char *psrcbits,
return (CursorPtr)NULL;
}
}
+#if 0
+ debug_add(pCurs);
+#endif
return pCurs;
}
--
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().
I am not sure if there are cases where the cursor may also be released
explicitly elsewhere, but when
running a few programs it seen the number of cursors in the server is
properly restored when finishing the
application.
--
Paulo
More information about the xorg
mailing list