Problem with new Xorg, radeon & compiz/Kwin revisited

[matt]

On 08/07/12 22:35, matt wrote:
> On 08/07/12 19:15, matt wrote:
>>
>> A while back, I was having a strange issue with the new Xorg and compiz.
>> In a nutshell, closing certain windows caused Xorg to exit on nasty
>> signals. At the same time, a few users with Radeon cards were having
>> issues with Kwin disabling opengl for no apparent reason.
>>
>> After some research, I had posted a patch seen in a debian mailing list
>> as a possible solution, and it did indeed solve the immediate problem of
>> crashing Xorg (and later I found out Kwin worked with OpenGL effects
>> when the patch is applied).
>>
>> It was requested I file a PR but first investigate what issues such a
>> patch might cause. I'm glad I waited, because there are a few. The
>> medicine is a little worse than the disease. First, with Compiz & Xfce4,
>> if X was told to shutdown while the previously affected programs
>> (xfce4-terminal running under compiz & xfce4 was obvious), the machine
>> would immediately reboot. This is obviously not good. So usually I would
>> only logout after closing all windows (except Xfce4-panel and the root
>> window). This would work, and the machine could be shut down from the 
>> TTY.
>>
>> Recently, I installed KDE4. I was so impressed that I was trying to use
>> it as my "daily desktop". OpenGL Kwin was working fine. However,
>> engaging the screen lock would reboot the machine, as well as the
>> machine occasionally spontaneously rebooting. This was much worse than
>> the above issue.
>>
>> The patch had one other odd effect, which was that Dri2 was disabled for
>> some reason.
>>
>> I reverted the patch to xorg-server I had been running, and now Kwin
>> will not start OpenGL mode, because it can't find an ARGBGLX visual. If
>> Kwin is started alone, OpenGL suprisingly works. Xfce4+compiz crashes
>> Xorg as before, however Dri2 is now enabled. The signal is a segfault at
>> 0x0. AIGLX on/off have no effect.
>>
>> Here was the patch (I applied it manually looking at the file, the
>> offsets are wrong for us):
>>
>> diff --git a/glx/glxdri.c b/glx/glxdri.c
>> index 326f539..f6ef784 100644
>> --- a/glx/glxdri.c
>> +++ b/glx/glxdri.c
>> @@ -230,7 +230,7 @@ __glXDRIdrawableDestroy(__GLXdrawable *drawable)
>>
>>      /* If the X window was destroyed, the dri DestroyWindow hook will
>>       * aready have taken care of this, so only call if pDraw isn't
>> NULL. */
>> -    if (drawable->pDraw != NULL) {
>> +    if (drawable->pDraw != NULL&& drawable->pDraw->type ==
>> DRAWABLE_WINDOW) {
>>      screen = (__GLXDRIscreen *) glxGetScreen(drawable->pDraw->pScreen);
>> (*screen->core->destroyDrawable)(private->driDrawable);
>>
>> It seems like this shouldn't be having as much effect as it does. I can
>> only guess the reboots are from memory problems resulting from lingering
>> drawables? I can't explain the Dri2 interaction, nor can I explain
>> Kwin's behavior (fairly new to KDE, last time I used it was last century
>> :) ).
>>
>> For what it's worth, my driver is xf86-video-ati on a Radeon HD4650.
>> I'm thoroughly confused by this problem and I'd welcome some help trying
>> to solve the bug. Anyone else seeing problems with RV710 radeons and
>> OpenGL compositing?
>>
>> Matt
>>
>>
>>
>>
> A quick follow up...dri2 issue was something else, it works with the 
> patch. I've tried base gcc as well as gcc47 no difference for 
> xorg-server and the above behavior. The initial check for pDraw != 
> NULL was to prevent a double free, and the patch adds a check to 
> prevent an instance. My best guess now is that there is a double free 
> situation that passes the DRAWABLE_WINDOW check.
>
> Matt
I fixed the issues for radeon using WITH_NEW_XORG (at least for me)

1. The attached patch from Andriy Gaipon fixes an issue with refcounting 
when releasing pixmaps
2. Apply the other patch that checks for pDraw->type == DRAWABLE_WINDOW 
that I posted before

This has been stable with both KWin and compiz.

Was Gaipon's patch previously applied in FreeBSD's xorg?

Matt
-------------- next part --------------
--- glx/glxdrawable.h.orig	2011-06-11 19:55:47.000000000 +0300
+++ glx/glxdrawable.h	2011-07-30 14:52:16.730791751 +0300
@@ -53,6 +53,7 @@ struct __GLXdrawable {
 
     DrawablePtr pDraw;
     XID drawId;
+    XID otherId; /* for glx1.3 we need to track the original Drawable as well */
 
     /*
     ** Either GLX_DRAWABLE_PIXMAP, GLX_DRAWABLE_WINDOW or
--- glx/glxcmds.c.orig	2011-06-11 19:55:47.000000000 +0300
+++ glx/glxcmds.c	2011-07-30 14:51:33.374788616 +0300
@@ -1095,6 +1095,7 @@ __glXDrawableInit(__GLXdrawable *drawabl
     drawable->pDraw = pDraw;
     drawable->type = type;
     drawable->drawId = drawId;
+    drawable->otherId = 0;
     drawable->config = config;
     drawable->eventMask = 0;
 
@@ -1131,10 +1132,13 @@ DoCreateGLXDrawable(ClientPtr client, __
      * Windows aren't refcounted, so track both the X and the GLX window
      * so we get called regardless of destruction order.
      */
-    if (drawableId != glxDrawableId && type == GLX_DRAWABLE_WINDOW &&
-	!AddResource(pDraw->id, __glXDrawableRes, pGlxDraw)) {
-	pGlxDraw->destroy (pGlxDraw);
-	return BadAlloc;
+    if (drawableId != glxDrawableId &&
+        (type == GLX_DRAWABLE_WINDOW || type == GLX_DRAWABLE_PIXMAP)) {
+        if (!AddResource(drawableId, __glXDrawableRes, pGlxDraw)) {
+	    pGlxDraw->destroy (pGlxDraw);
+	    return BadAlloc;
+	}
+	pGlxDraw->otherId = drawableId;
     }
 
     return Success;
--- glx/glxext.c.orig	2011-06-11 19:55:47.000000000 +0300
+++ glx/glxext.c	2011-07-30 14:51:03.306788511 +0300
@@ -124,14 +124,16 @@ static Bool DrawableGone(__GLXdrawable *
 {
     __GLXcontext *c, *next;
 
-    if (glxPriv->type == GLX_DRAWABLE_WINDOW) {
+    if (glxPriv->type == GLX_DRAWABLE_WINDOW || glxPriv->type == GLX_DRAWABLE_PIXMAP) {
         /* If this was created by glXCreateWindow, free the matching resource */
-        if (glxPriv->drawId != glxPriv->pDraw->id) {
-            if (xid == glxPriv->drawId)
-                FreeResourceByType(glxPriv->pDraw->id, __glXDrawableRes, TRUE);
-            else
-                FreeResourceByType(glxPriv->drawId, __glXDrawableRes, TRUE);
-        }
+        if (glxPriv->otherId) {
+            XID other = glxPriv->otherId;
+            glxPriv->otherId = 0;
+            if (xid == other)
+                FreeResourceByType(glxPriv->drawId, __glXDrawableRes, TRUE);
+            else
+                FreeResourceByType(other, __glXDrawableRes, TRUE);
+        }
         /* otherwise this window was implicitly created by MakeCurrent */
     }