Gitweb:     
http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=040ac32048d5efabd557c1e0a6ab8aec2c710c56
Commit:     040ac32048d5efabd557c1e0a6ab8aec2c710c56
Parent:     4b560fde06aeb342f3ff0bce924627ab722d251a
Author:     Thomas Hellstrom <thomas-at-tungstengraphics-dot-com>
AuthorDate: Fri Mar 23 13:28:33 2007 +1100
Committer:  Dave Airlie <[EMAIL PROTECTED]>
CommitDate: Fri Mar 23 13:28:33 2007 +1100

    drm: fix driver deadlock with AIGLX and reclaim_buffers_locked
    
    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 <[EMAIL PROTECTED]>
---
 drivers/char/drm/drmP.h     |   21 ++++++-
 drivers/char/drm/drm_fops.c |   90 ++++++++++++++---------------
 drivers/char/drm/drm_irq.c  |    4 +-
 drivers/char/drm/drm_lock.c |  134 ++++++++++++++++++++++++++++++++++---------
 drivers/char/drm/drm_stub.c |    1 +
 drivers/char/drm/sis_drv.c  |    2 +-
 drivers/char/drm/via_drv.c  |    3 +-
 7 files changed, 174 insertions(+), 81 deletions(-)

diff --git a/drivers/char/drm/drmP.h b/drivers/char/drm/drmP.h
index 09705da..80041d5 100644
--- a/drivers/char/drm/drmP.h
+++ b/drivers/char/drm/drmP.h
@@ -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);
diff --git a/drivers/char/drm/drm_fops.c b/drivers/char/drm/drm_fops.c
index 314abd9..3b159ca 100644
--- a/drivers/char/drm/drm_fops.c
+++ b/drivers/char/drm/drm_fops.c
@@ -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);
diff --git a/drivers/char/drm/drm_irq.c b/drivers/char/drm/drm_irq.c
index 9d00c51..2e75331 100644
--- a/drivers/char/drm/drm_irq.c
+++ b/drivers/char/drm/drm_irq.c
@@ -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;
diff --git a/drivers/char/drm/drm_lock.c b/drivers/char/drm/drm_lock.c
index e9993ba..befd1af 100644
--- a/drivers/char/drm/drm_lock.c
+++ b/drivers/char/drm/drm_lock.c
@@ -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);
diff --git a/drivers/char/drm/drm_stub.c b/drivers/char/drm/drm_stub.c
index 120d102..19408ad 100644
--- a/drivers/char/drm/drm_stub.c
+++ b/drivers/char/drm/drm_stub.c
@@ -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);
diff --git a/drivers/char/drm/sis_drv.c b/drivers/char/drm/sis_drv.c
index 3d5b321..690e0af 100644
--- a/drivers/char/drm/sis_drv.c
+++ b/drivers/char/drm/sis_drv.c
@@ -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,
diff --git a/drivers/char/drm/via_drv.c b/drivers/char/drm/via_drv.c
index bb9dde8..2d4957a 100644
--- a/drivers/char/drm/via_drv.c
+++ b/drivers/char/drm/via_drv.c
@@ -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,
-
To unsubscribe from this list: send the line "unsubscribe git-commits-head" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to