[Patch] Re: X server steals DRI lock

[Thomas Hellström]

Thomas Hellström wrote: Since the error messages I get indicate that indeed the filp associated with the HW lock has changed, the stealing has probably occured in the kernel DRM code. Hmm, This is a nasty one. What seems to happen is that when the X server waits in the kernel for the lock to become available, it is interrupted by a signal. However, the X server sometimes sets the DMA_QUIESCENT lock flag which calls the dma_quiescent function which usually returns zero, and the kernel IOCTL falsely returns success. The X server goes on thinking it has the lock, and later on releases it for the dri client really holding it, while the dri client still thinks it has the lock. The attached patch fixes the problem for me. Could someone pls review, as I'm not sure the return before "sigemptyset" is correctly placed. The bug affects intel, mga and via drivers since they all provide dma_quiescent functions. /Thomas

Index: linux/drm_drv.h
===================================================================
RCS file: /cvs/dri/drm/linux/drm_drv.h,v
retrieving revision 1.93
diff -u -r1.93 drm_drv.h
--- linux/drm_drv.h     31 Oct 2004 15:16:43 -0000      1.93
+++ linux/drm_drv.h     19 Apr 2005 19:08:41 -0000
@@ -1048,7 +1048,10 @@
        }
        current->state = TASK_RUNNING;
        remove_wait_queue( &dev->lock.lock_queue, &entry );
-       
+
+        DRM_DEBUG( "%d %s\n", lock.context, ret ? "interrupted" : "has lock" );
+       if (ret) return ret;
+
        sigemptyset( &dev->sigmask );
        sigaddset( &dev->sigmask, SIGSTOP );
        sigaddset( &dev->sigmask, SIGTSTP );
@@ -1062,19 +1065,19 @@
        if (dev->fn_tbl.dma_ready && (lock.flags & _DRM_LOCK_READY))
                dev->fn_tbl.dma_ready(dev);
        
-       if ( dev->fn_tbl.dma_quiescent && (lock.flags & _DRM_LOCK_QUIESCENT ))
-               return dev->fn_tbl.dma_quiescent(dev);
-       
-       
+       if (dev->fn_tbl.dma_quiescent && (lock.flags & _DRM_LOCK_QUIESCENT)) {
+               if (dev->fn_tbl.dma_quiescent(dev)) {
+                       DRM_DEBUG( "%d waiting for DMA quiescent\n", 
lock.context);
+                       return DRM_ERR(EBUSY);
+               }
+       }       
+
        if ( dev->fn_tbl.kernel_context_switch && dev->last_context != 
lock.context ) {
                dev->fn_tbl.kernel_context_switch(dev, dev->last_context,
                                                  lock.context);
        }
        
-
-        DRM_DEBUG( "%d %s\n", lock.context, ret ? "interrupted" : "has lock" );
-
-        return ret;
+        return 0;
 }
 
 /** 
Index: linux-core/drm_lock.c
===================================================================
RCS file: /cvs/dri/drm/linux-core/drm_lock.c,v
retrieving revision 1.14
diff -u -r1.14 drm_lock.c
--- linux-core/drm_lock.c       18 Oct 2004 14:16:41 -0000      1.14
+++ linux-core/drm_lock.c       19 Apr 2005 19:08:41 -0000
@@ -99,6 +99,9 @@
        current->state = TASK_RUNNING;
        remove_wait_queue(&dev->lock.lock_queue, &entry);
 
+        DRM_DEBUG( "%d %s\n", lock.context, ret ? "interrupted" : "has lock" );
+       if (ret) return ret;
+
        sigemptyset(&dev->sigmask);
        sigaddset(&dev->sigmask, SIGSTOP);
        sigaddset(&dev->sigmask, SIGTSTP);
@@ -111,8 +114,12 @@
        if (dev->driver->dma_ready && (lock.flags & _DRM_LOCK_READY))
                dev->driver->dma_ready(dev);
 
-       if (dev->driver->dma_quiescent && (lock.flags & _DRM_LOCK_QUIESCENT))
-               return dev->driver->dma_quiescent(dev);
+       if (dev->driver->dma_quiescent && (lock.flags & _DRM_LOCK_QUIESCENT)) {
+               if (dev->driver->dma_quiescent(dev)) {
+                       DRM_DEBUG( "%d waiting for DMA quiescent\n", 
lock.context);
+                       return DRM_ERR(EBUSY);
+               }
+       }
 
        if (dev->driver->kernel_context_switch
            && dev->last_context != lock.context) {
@@ -120,9 +127,7 @@
                                                   lock.context);
        }
 
-       DRM_DEBUG("%d %s\n", lock.context, ret ? "interrupted" : "has lock");
-
-       return ret;
+       return 0;
 }
 
 /**