]> err.no Git - linux-2.6/commitdiff
drm: fix driver deadlock with AIGLX and reclaim_buffers_locked
authorThomas Hellstrom <thomas-at-tungstengraphics-dot-com>
Fri, 23 Mar 2007 02:28:33 +0000 (13:28 +1100)
committerDave Airlie <airlied@linux.ie>
Fri, 23 Mar 2007 02:28:33 +0000 (13:28 +1100)
Bugzilla Bug #9457

Add refcounting of user waiters to the DRM hardware lock, so that we can use
DRM_LOCK_CONT flag more conservatively.

Also add a kernel waiter refcount that if nonzero transfers the lock for the
kernel context when it is released. This is useful when waiting for idle and can be used for very simple fence object driver implementations for the new memory manager

Signed-off-by: Dave Airlie <airlied@linux.ie>
drivers/char/drm/drmP.h
drivers/char/drm/drm_fops.c
drivers/char/drm/drm_irq.c
drivers/char/drm/drm_lock.c
drivers/char/drm/drm_stub.c
drivers/char/drm/sis_drv.c
drivers/char/drm/via_drv.c

index 09705da8cdd7489d9f975bd028e7b31f11483271..80041d5b792df40bfb7e027f76a235940575eebb 100644 (file)
@@ -414,6 +414,10 @@ typedef struct drm_lock_data {
        struct file *filp;              /**< File descr of lock holder (0=kernel) */
        wait_queue_head_t lock_queue;   /**< Queue of blocked processes */
        unsigned long lock_time;        /**< Time of last lock in jiffies */
+       spinlock_t spinlock;
+       uint32_t kernel_waiters;
+       uint32_t user_waiters;
+       int idle_has_lock;
 } drm_lock_data_t;
 
 /**
@@ -590,6 +594,8 @@ struct drm_driver {
        void (*reclaim_buffers) (struct drm_device * dev, struct file * filp);
        void (*reclaim_buffers_locked) (struct drm_device *dev,
                                        struct file *filp);
+       void (*reclaim_buffers_idlelocked) (struct drm_device *dev,
+                                       struct file * filp);
        unsigned long (*get_map_ofs) (drm_map_t * map);
        unsigned long (*get_reg_ofs) (struct drm_device * dev);
        void (*set_version) (struct drm_device * dev, drm_set_version_t * sv);
@@ -915,9 +921,18 @@ extern int drm_lock(struct inode *inode, struct file *filp,
                    unsigned int cmd, unsigned long arg);
 extern int drm_unlock(struct inode *inode, struct file *filp,
                      unsigned int cmd, unsigned long arg);
-extern int drm_lock_take(__volatile__ unsigned int *lock, unsigned int context);
-extern int drm_lock_free(drm_device_t * dev,
-                        __volatile__ unsigned int *lock, unsigned int context);
+extern int drm_lock_take(drm_lock_data_t *lock_data, unsigned int context);
+extern int drm_lock_free(drm_lock_data_t *lock_data, unsigned int context);
+extern void drm_idlelock_take(drm_lock_data_t *lock_data);
+extern void drm_idlelock_release(drm_lock_data_t *lock_data);
+
+/*
+ * These are exported to drivers so that they can implement fencing using
+ * DMA quiscent + idle. DMA quiescent usually requires the hardware lock.
+ */
+
+extern int drm_i_have_hw_lock(struct file *filp);
+extern int drm_kernel_take_hw_lock(struct file *filp);
 
                                /* Buffer management support (drm_bufs.h) */
 extern int drm_addbufs_agp(drm_device_t * dev, drm_buf_desc_t * request);
index 314abd9d651083dcd3318c9be20ad218cd2f1962..3b159cab3bc807c8395f2c58d13b77a79cd225f8 100644 (file)
@@ -356,58 +356,56 @@ int drm_release(struct inode *inode, struct file *filp)
                  current->pid, (long)old_encode_dev(priv->head->device),
                  dev->open_count);
 
-       if (priv->lock_count && dev->lock.hw_lock &&
-           _DRM_LOCK_IS_HELD(dev->lock.hw_lock->lock) &&
-           dev->lock.filp == filp) {
-               DRM_DEBUG("File %p released, freeing lock for context %d\n",
-                         filp, _DRM_LOCKING_CONTEXT(dev->lock.hw_lock->lock));
-
-               if (dev->driver->reclaim_buffers_locked)
+       if (dev->driver->reclaim_buffers_locked && dev->lock.hw_lock) {
+               if (drm_i_have_hw_lock(filp)) {
                        dev->driver->reclaim_buffers_locked(dev, filp);
-
-               drm_lock_free(dev, &dev->lock.hw_lock->lock,
-                             _DRM_LOCKING_CONTEXT(dev->lock.hw_lock->lock));
-
-               /* FIXME: may require heavy-handed reset of
-                  hardware at this point, possibly
-                  processed via a callback to the X
-                  server. */
-       } else if (dev->driver->reclaim_buffers_locked && priv->lock_count
-                  && dev->lock.hw_lock) {
-               /* The lock is required to reclaim buffers */
-               DECLARE_WAITQUEUE(entry, current);
-
-               add_wait_queue(&dev->lock.lock_queue, &entry);
-               for (;;) {
-                       __set_current_state(TASK_INTERRUPTIBLE);
-                       if (!dev->lock.hw_lock) {
-                               /* Device has been unregistered */
-                               retcode = -EINTR;
-                               break;
+               } else {
+                       unsigned long _end=jiffies + 3*DRM_HZ;
+                       int locked = 0;
+
+                       drm_idlelock_take(&dev->lock);
+
+                       /*
+                        * Wait for a while.
+                        */
+
+                       do{
+                               spin_lock(&dev->lock.spinlock);
+                               locked = dev->lock.idle_has_lock;
+                               spin_unlock(&dev->lock.spinlock);
+                               if (locked)
+                                       break;
+                               schedule();
+                       } while (!time_after_eq(jiffies, _end));
+
+                       if (!locked) {
+                               DRM_ERROR("reclaim_buffers_locked() deadlock. Please rework this\n"
+                                         "\tdriver to use reclaim_buffers_idlelocked() instead.\n"
+                                         "\tI will go on reclaiming the buffers anyway.\n");
                        }
-                       if (drm_lock_take(&dev->lock.hw_lock->lock,
-                                         DRM_KERNEL_CONTEXT)) {
-                               dev->lock.filp = filp;
-                               dev->lock.lock_time = jiffies;
-                               atomic_inc(&dev->counts[_DRM_STAT_LOCKS]);
-                               break;  /* Got lock */
-                       }
-                       /* Contention */
-                       schedule();
-                       if (signal_pending(current)) {
-                               retcode = -ERESTARTSYS;
-                               break;
-                       }
-               }
-               __set_current_state(TASK_RUNNING);
-               remove_wait_queue(&dev->lock.lock_queue, &entry);
-               if (!retcode) {
+
                        dev->driver->reclaim_buffers_locked(dev, filp);
-                       drm_lock_free(dev, &dev->lock.hw_lock->lock,
-                                     DRM_KERNEL_CONTEXT);
+                       drm_idlelock_release(&dev->lock);
                }
        }
 
+       if (dev->driver->reclaim_buffers_idlelocked && dev->lock.hw_lock) {
+
+               drm_idlelock_take(&dev->lock);
+               dev->driver->reclaim_buffers_idlelocked(dev, filp);
+               drm_idlelock_release(&dev->lock);
+
+       }
+
+       if (drm_i_have_hw_lock(filp)) {
+               DRM_DEBUG("File %p released, freeing lock for context %d\n",
+                         filp, _DRM_LOCKING_CONTEXT(dev->lock.hw_lock->lock));
+
+               drm_lock_free(&dev->lock,
+                             _DRM_LOCKING_CONTEXT(dev->lock.hw_lock->lock));
+       }
+
+
        if (drm_core_check_feature(dev, DRIVER_HAVE_DMA) &&
            !dev->driver->reclaim_buffers_locked) {
                dev->driver->reclaim_buffers(dev, filp);
index 9d00c51fe2c44790000f59551bf05947a2d829f2..2e75331fd83e7c36bf297e06c3351fcb5d294598 100644 (file)
@@ -424,7 +424,7 @@ static void drm_locked_tasklet_func(unsigned long data)
        spin_lock_irqsave(&dev->tasklet_lock, irqflags);
 
        if (!dev->locked_tasklet_func ||
-           !drm_lock_take(&dev->lock.hw_lock->lock,
+           !drm_lock_take(&dev->lock,
                           DRM_KERNEL_CONTEXT)) {
                spin_unlock_irqrestore(&dev->tasklet_lock, irqflags);
                return;
@@ -435,7 +435,7 @@ static void drm_locked_tasklet_func(unsigned long data)
 
        dev->locked_tasklet_func(dev);
 
-       drm_lock_free(dev, &dev->lock.hw_lock->lock,
+       drm_lock_free(&dev->lock,
                      DRM_KERNEL_CONTEXT);
 
        dev->locked_tasklet_func = NULL;
index e9993ba461a2ba85e39229ebfc9d46dbe240d13a..befd1af19dfe0962ff7915c7cd0e1573bf7270e6 100644 (file)
@@ -35,9 +35,6 @@
 
 #include "drmP.h"
 
-static int drm_lock_transfer(drm_device_t * dev,
-                            __volatile__ unsigned int *lock,
-                            unsigned int context);
 static int drm_notifier(void *priv);
 
 /**
@@ -80,6 +77,9 @@ int drm_lock(struct inode *inode, struct file *filp,
                        return -EINVAL;
 
        add_wait_queue(&dev->lock.lock_queue, &entry);
+       spin_lock(&dev->lock.spinlock);
+       dev->lock.user_waiters++;
+       spin_unlock(&dev->lock.spinlock);
        for (;;) {
                __set_current_state(TASK_INTERRUPTIBLE);
                if (!dev->lock.hw_lock) {
@@ -87,7 +87,7 @@ int drm_lock(struct inode *inode, struct file *filp,
                        ret = -EINTR;
                        break;
                }
-               if (drm_lock_take(&dev->lock.hw_lock->lock, lock.context)) {
+               if (drm_lock_take(&dev->lock, lock.context)) {
                        dev->lock.filp = filp;
                        dev->lock.lock_time = jiffies;
                        atomic_inc(&dev->counts[_DRM_STAT_LOCKS]);
@@ -101,12 +101,14 @@ int drm_lock(struct inode *inode, struct file *filp,
                        break;
                }
        }
+       spin_lock(&dev->lock.spinlock);
+       dev->lock.user_waiters--;
+       spin_unlock(&dev->lock.spinlock);
        __set_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;
+       DRM_DEBUG( "%d %s\n", lock.context, ret ? "interrupted" : "has lock" );
+       if (ret) return ret;
 
        sigemptyset(&dev->sigmask);
        sigaddset(&dev->sigmask, SIGSTOP);
@@ -127,14 +129,12 @@ int drm_lock(struct inode *inode, struct file *filp,
                }
        }
 
-       /* dev->driver->kernel_context_switch isn't used by any of the x86
-        *  drivers but is used by the Sparc driver.
-        */
        if (dev->driver->kernel_context_switch &&
            dev->last_context != lock.context) {
                dev->driver->kernel_context_switch(dev, dev->last_context,
                                                   lock.context);
        }
+
        return 0;
 }
 
@@ -184,12 +184,8 @@ int drm_unlock(struct inode *inode, struct file *filp,
        if (dev->driver->kernel_context_switch_unlock)
                dev->driver->kernel_context_switch_unlock(dev);
        else {
-               drm_lock_transfer(dev, &dev->lock.hw_lock->lock,
-                                 DRM_KERNEL_CONTEXT);
-
-               if (drm_lock_free(dev, &dev->lock.hw_lock->lock,
-                                 DRM_KERNEL_CONTEXT)) {
-                       DRM_ERROR("\n");
+               if (drm_lock_free(&dev->lock,lock.context)) {
+                       /* FIXME: Should really bail out here. */
                }
        }
 
@@ -206,18 +202,26 @@ int drm_unlock(struct inode *inode, struct file *filp,
  *
  * Attempt to mark the lock as held by the given context, via the \p cmpxchg instruction.
  */
-int drm_lock_take(__volatile__ unsigned int *lock, unsigned int context)
+int drm_lock_take(drm_lock_data_t *lock_data,
+                 unsigned int context)
 {
        unsigned int old, new, prev;
+       volatile unsigned int *lock = &lock_data->hw_lock->lock;
 
+       spin_lock(&lock_data->spinlock);
        do {
                old = *lock;
                if (old & _DRM_LOCK_HELD)
                        new = old | _DRM_LOCK_CONT;
-               else
-                       new = context | _DRM_LOCK_HELD;
+               else {
+                       new = context | _DRM_LOCK_HELD |
+                               ((lock_data->user_waiters + lock_data->kernel_waiters > 1) ?
+                                _DRM_LOCK_CONT : 0);
+               }
                prev = cmpxchg(lock, old, new);
        } while (prev != old);
+       spin_unlock(&lock_data->spinlock);
+
        if (_DRM_LOCKING_CONTEXT(old) == context) {
                if (old & _DRM_LOCK_HELD) {
                        if (context != DRM_KERNEL_CONTEXT) {
@@ -227,7 +231,8 @@ int drm_lock_take(__volatile__ unsigned int *lock, unsigned int context)
                        return 0;
                }
        }
-       if (new == (context | _DRM_LOCK_HELD)) {
+
+       if ((_DRM_LOCKING_CONTEXT(new)) == context && (new & _DRM_LOCK_HELD)) {
                /* Have lock */
                return 1;
        }
@@ -246,13 +251,13 @@ int drm_lock_take(__volatile__ unsigned int *lock, unsigned int context)
  * Resets the lock file pointer.
  * Marks the lock as held by the given context, via the \p cmpxchg instruction.
  */
-static int drm_lock_transfer(drm_device_t * dev,
-                            __volatile__ unsigned int *lock,
+static int drm_lock_transfer(drm_lock_data_t *lock_data,
                             unsigned int context)
 {
        unsigned int old, new, prev;
+       volatile unsigned int *lock = &lock_data->hw_lock->lock;
 
-       dev->lock.filp = NULL;
+       lock_data->filp = NULL;
        do {
                old = *lock;
                new = context | _DRM_LOCK_HELD;
@@ -272,23 +277,32 @@ static int drm_lock_transfer(drm_device_t * dev,
  * Marks the lock as not held, via the \p cmpxchg instruction. Wakes any task
  * waiting on the lock queue.
  */
-int drm_lock_free(drm_device_t * dev,
-                 __volatile__ unsigned int *lock, unsigned int context)
+int drm_lock_free(drm_lock_data_t *lock_data, unsigned int context)
 {
        unsigned int old, new, prev;
+       volatile unsigned int *lock = &lock_data->hw_lock->lock;
+
+       spin_lock(&lock_data->spinlock);
+       if (lock_data->kernel_waiters != 0) {
+               drm_lock_transfer(lock_data, 0);
+               lock_data->idle_has_lock = 1;
+               spin_unlock(&lock_data->spinlock);
+               return 1;
+       }
+       spin_unlock(&lock_data->spinlock);
 
-       dev->lock.filp = NULL;
        do {
                old = *lock;
-               new = 0;
+               new = _DRM_LOCKING_CONTEXT(old);
                prev = cmpxchg(lock, old, new);
        } while (prev != old);
+
        if (_DRM_LOCK_IS_HELD(old) && _DRM_LOCKING_CONTEXT(old) != context) {
                DRM_ERROR("%d freed heavyweight lock held by %d\n",
                          context, _DRM_LOCKING_CONTEXT(old));
                return 1;
        }
-       wake_up_interruptible(&dev->lock.lock_queue);
+       wake_up_interruptible(&lock_data->lock_queue);
        return 0;
 }
 
@@ -322,3 +336,67 @@ static int drm_notifier(void *priv)
        } while (prev != old);
        return 0;
 }
+
+/**
+ * This function returns immediately and takes the hw lock
+ * with the kernel context if it is free, otherwise it gets the highest priority when and if
+ * it is eventually released.
+ *
+ * This guarantees that the kernel will _eventually_ have the lock _unless_ it is held
+ * by a blocked process. (In the latter case an explicit wait for the hardware lock would cause
+ * a deadlock, which is why the "idlelock" was invented).
+ *
+ * This should be sufficient to wait for GPU idle without
+ * having to worry about starvation.
+ */
+
+void drm_idlelock_take(drm_lock_data_t *lock_data)
+{
+       int ret = 0;
+
+       spin_lock(&lock_data->spinlock);
+       lock_data->kernel_waiters++;
+       if (!lock_data->idle_has_lock) {
+
+               spin_unlock(&lock_data->spinlock);
+               ret = drm_lock_take(lock_data, DRM_KERNEL_CONTEXT);
+               spin_lock(&lock_data->spinlock);
+
+               if (ret == 1)
+                       lock_data->idle_has_lock = 1;
+       }
+       spin_unlock(&lock_data->spinlock);
+}
+EXPORT_SYMBOL(drm_idlelock_take);
+
+void drm_idlelock_release(drm_lock_data_t *lock_data)
+{
+       unsigned int old, prev;
+       volatile unsigned int *lock = &lock_data->hw_lock->lock;
+
+       spin_lock(&lock_data->spinlock);
+       if (--lock_data->kernel_waiters == 0) {
+               if (lock_data->idle_has_lock) {
+                       do {
+                               old = *lock;
+                               prev = cmpxchg(lock, old, DRM_KERNEL_CONTEXT);
+                       } while (prev != old);
+                       wake_up_interruptible(&lock_data->lock_queue);
+                       lock_data->idle_has_lock = 0;
+               }
+       }
+       spin_unlock(&lock_data->spinlock);
+}
+EXPORT_SYMBOL(drm_idlelock_release);
+
+
+int drm_i_have_hw_lock(struct file *filp)
+{
+       DRM_DEVICE;
+
+       return (priv->lock_count && dev->lock.hw_lock &&
+               _DRM_LOCK_IS_HELD(dev->lock.hw_lock->lock) &&
+               dev->lock.filp == filp);
+}
+
+EXPORT_SYMBOL(drm_i_have_hw_lock);
index 120d10256feb6d5371da2d3c9652f8e26ce73aa4..19408adcc7755c396f83839986013773eb0a48c9 100644 (file)
@@ -62,6 +62,7 @@ static int drm_fill_in_dev(drm_device_t * dev, struct pci_dev *pdev,
        spin_lock_init(&dev->count_lock);
        spin_lock_init(&dev->drw_lock);
        spin_lock_init(&dev->tasklet_lock);
+       spin_lock_init(&dev->lock.spinlock);
        init_timer(&dev->timer);
        mutex_init(&dev->struct_mutex);
        mutex_init(&dev->ctxlist_mutex);
index 3d5b3218b6ff02fa5da3e377276be76174692151..690e0af8e7c2f3db9408d25869e9446e59c68ae2 100644 (file)
@@ -71,7 +71,7 @@ static struct drm_driver driver = {
        .context_dtor = NULL,
        .dma_quiescent = sis_idle,
        .reclaim_buffers = NULL,
-       .reclaim_buffers_locked = sis_reclaim_buffers_locked,
+       .reclaim_buffers_idlelocked = sis_reclaim_buffers_locked,
        .lastclose = sis_lastclose,
        .get_map_ofs = drm_core_get_map_ofs,
        .get_reg_ofs = drm_core_get_reg_ofs,
index bb9dde8b1911138445caf8cbf665f479dfc1fe42..2d4957ab256a39410b9cfec8d06a9e4bab5c7e40 100644 (file)
@@ -52,7 +52,8 @@ static struct drm_driver driver = {
        .dma_quiescent = via_driver_dma_quiescent,
        .dri_library_name = dri_library_name,
        .reclaim_buffers = drm_core_reclaim_buffers,
-       .reclaim_buffers_locked = via_reclaim_buffers_locked,
+       .reclaim_buffers_locked = NULL,
+       .reclaim_buffers_idlelocked = via_reclaim_buffers_locked,
        .lastclose = via_lastclose,
        .get_map_ofs = drm_core_get_map_ofs,
        .get_reg_ofs = drm_core_get_reg_ofs,