Bugzilla Bug #9457

Add refcounting of user waiters to the DRM hardware lock, so that we can use the
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.

It also resolves the AIGLX startup deadlock for the sis and the via drivers.
i810, i830 still require that the hardware lock is really taken so the deadlock remains
for those two. I'm not sure about ffb. Anyone familiar with that code?

8 files changed, 145 insertions, 101 deletions

diff --git a/linux-core/drmP.h b/linux-core/drmP.h
index 9c748e6e..edc15575 100644
--- a/linux-core/drmP.h
+++ b/linux-core/drmP.h
@@ -458,6 +458,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;
 
 /**
@@ -712,6 +716,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);
@@ -1193,12 +1199,14 @@ 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. 
+ * DMA quiscent + idle. DMA quiescent usually requires the hardware lock.
  */
 
 extern int drm_i_have_hw_lock(struct file *filp);
diff --git a/linux-core/drm_fops.c b/linux-core/drm_fops.c
index 84e06c87..faf76726 100644
--- a/linux-core/drm_fops.c
+++ b/linux-core/drm_fops.c
@@ -427,38 +427,51 @@ int drm_release(struct inode *inode, struct file *filp)
 		  dev->open_count);
 
 	if (dev->driver->reclaim_buffers_locked && dev->lock.hw_lock) {
-	        unsigned long _end = jiffies + DRM_HZ*3;
-
-		do {
-			retcode = drm_kernel_take_hw_lock(filp);
-		} while(retcode && !time_after_eq(jiffies,_end));
-
-		if (!retcode) {
+		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));
 		} else {
+			unsigned long _end=jiffies + 3*DRM_HZ;
+			int locked = 0;
+
+			drm_idlelock_take(&dev->lock);
 
 			/*
-			 * FIXME: This is not a good solution. We should perhaps associate the
-			 * DRM lock with a process context, and check whether the current process
-			 * holds the lock. Then we can run reclaim buffers locked anyway.
+			 * Wait for a while.
 			 */
 
-			DRM_ERROR("Reclaim buffers locked deadlock.\n"
-				  "\tThis is probably a single thread having multiple\n"
-				  "\tDRM file descriptors open either dying or"
-				  " closing file descriptors\n"
-				  "\twhile having the lock. I will not reclaim buffers.\n"
-				  "\tLocking context is 0x%08x\n",
-				  _DRM_LOCKING_CONTEXT(dev->lock.hw_lock->lock));
+			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");
+			}
+
+			dev->driver->reclaim_buffers_locked(dev, filp);
+			drm_idlelock_release(&dev->lock);
 		}
-	} else if (drm_i_have_hw_lock(filp)) {
+	}
+
+	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, &dev->lock.hw_lock->lock,
+		drm_lock_free(&dev->lock,
 			      _DRM_LOCKING_CONTEXT(dev->lock.hw_lock->lock));
 	}
 
diff --git a/linux-core/drm_irq.c b/linux-core/drm_irq.c
index c365c08e..92cf7f5c 100644
--- a/linux-core/drm_irq.c
+++ b/linux-core/drm_irq.c
@@ -422,7 +422,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;
@@ -433,7 +433,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/linux-core/drm_lock.c b/linux-core/drm_lock.c
index d11c570e..a4b1a63f 100644
--- a/linux-core/drm_lock.c
+++ b/linux-core/drm_lock.c
@@ -35,12 +35,6 @@
 
 #include "drmP.h"
 
-#if 0
-static int drm_lock_transfer(drm_device_t * dev,
-			     __volatile__ unsigned int *lock,
-			     unsigned int context);
-#endif
-
 static int drm_notifier(void *priv);
 
 /**
@@ -83,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) {
@@ -90,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]);
@@ -104,6 +101,9 @@ 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);
 
@@ -184,8 +184,7 @@ int drm_unlock(struct inode *inode, struct file *filp,
 	if (dev->driver->kernel_context_switch_unlock)
 		dev->driver->kernel_context_switch_unlock(dev);
 	else {
-		if (drm_lock_free(dev, &dev->lock.hw_lock->lock,
-				  lock.context)) {
+		if (drm_lock_free(&dev->lock,lock.context)) {
 			/* FIXME: Should really bail out here. */
 		}
 	}
@@ -203,20 +202,29 @@ 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 | _DRM_LOCK_CONT;
+		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) {
+			spin_unlock(&lock_data->spinlock);
 			if (context != DRM_KERNEL_CONTEXT) {
 				DRM_ERROR("%d holds heavyweight lock\n",
 					  context);
@@ -224,14 +232,17 @@ int drm_lock_take(__volatile__ unsigned int *lock, unsigned int context)
 			return 0;
 		}
 	}
-	if (new == (context | _DRM_LOCK_HELD | _DRM_LOCK_CONT)) {
+
+	if ((_DRM_LOCKING_CONTEXT(new)) == context && (new & _DRM_LOCK_HELD)) {
 		/* Have lock */
+
+		spin_unlock(&lock_data->spinlock);
 		return 1;
 	}
+	spin_unlock(&lock_data->spinlock);
 	return 0;
 }
 
-#if 0
 /**
  * This takes a lock forcibly and hands it to context.	Should ONLY be used
  * inside *_unlock to give lock to kernel before calling *_dma_schedule.
@@ -244,13 +255,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;
@@ -258,7 +269,6 @@ static int drm_lock_transfer(drm_device_t * dev,
 	} while (prev != old);
 	return 1;
 }
-#endif
 
 /**
  * Free lock.
@@ -271,10 +281,19 @@ 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);
 
 	do {
 		old = *lock;
@@ -287,7 +306,7 @@ int drm_lock_free(drm_device_t * dev,
 			  context, _DRM_LOCKING_CONTEXT(old));
 		return 1;
 	}
-	wake_up_interruptible(&dev->lock.lock_queue);
+	wake_up_interruptible(&lock_data->lock_queue);
 	return 0;
 }
 
@@ -322,65 +341,66 @@ static int drm_notifier(void *priv)
 	return 0;
 }
 
-/*
- * Can be used by drivers to take the hardware lock if necessary.
- * (Waiting for idle before reclaiming buffers etc.)
+/**
+ * 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.
  */
 
-int drm_i_have_hw_lock(struct file *filp)
+void drm_idlelock_take(drm_lock_data_t *lock_data)
 {
-	DRM_DEVICE;
-
-	return (priv->lock_count && dev->lock.hw_lock &&
-		_DRM_LOCK_IS_HELD(dev->lock.hw_lock->lock) &&
-		dev->lock.filp == filp);
-}
+	int ret = 0;
 
-EXPORT_SYMBOL(drm_i_have_hw_lock);
+	spin_lock(&lock_data->spinlock);
+	lock_data->kernel_waiters++;
+	if (!lock_data->idle_has_lock) {
 
-int drm_kernel_take_hw_lock(struct file *filp)
-{
-	DRM_DEVICE;
+		spin_unlock(&lock_data->spinlock);
+		ret = drm_lock_take(lock_data, DRM_KERNEL_CONTEXT);
+		spin_lock(&lock_data->spinlock);
 
-	int ret = 0; 
-	unsigned long _end = jiffies + 3*DRM_HZ;
-	
-	if (!drm_i_have_hw_lock(filp)) {
-	
-		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 */
-				ret = -EINTR;
-				break;
-			}
-			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 */
-			if (time_after_eq(jiffies,_end)) {
-			        ret = -EBUSY;
-				break;
-			}
+		if (ret == 1)
+			lock_data->idle_has_lock = 1;
+	}
+	spin_unlock(&lock_data->spinlock);
+}
+EXPORT_SYMBOL(drm_idlelock_take);
 
-			schedule_timeout(1);
-			if (signal_pending(current)) {
-				ret = -ERESTARTSYS;
-				break;
-			}
+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;
 		}
-		__set_current_state(TASK_RUNNING);
-		remove_wait_queue(&dev->lock.lock_queue, &entry);
 	}
-	return ret;
+	spin_unlock(&lock_data->spinlock);
 }
+EXPORT_SYMBOL(drm_idlelock_release);
 
-EXPORT_SYMBOL(drm_kernel_take_hw_lock);
 
+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/linux-core/drm_stub.c b/linux-core/drm_stub.c
index 60123cdc..2dc95aa2 100644
--- a/linux-core/drm_stub.c
+++ b/linux-core/drm_stub.c
@@ -63,6 +63,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/linux-core/sis_drv.c b/linux-core/sis_drv.c
index 9b0b9830..114ec8f9 100644
--- a/linux-core/sis_drv.c
+++ b/linux-core/sis_drv.c
@@ -74,7 +74,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/linux-core/via_mm.c b/linux-core/via_mm.c
index d97269f5..67d212d7 100644
--- a/linux-core/via_mm.c
+++ b/linux-core/via_mm.c
@@ -204,6 +204,7 @@ void via_reclaim_buffers_locked(drm_device_t * dev, struct file *filp)
 	if (dev->driver->dma_quiescent) {
 		dev->driver->dma_quiescent(dev);
 	}
+	DRM_ERROR("Lock status is %d\n", dev->lock.idle_has_lock);
 
 	drm_sman_owner_cleanup(&dev_priv->sman, (unsigned long)priv);
 	mutex_unlock(&dev->struct_mutex);
diff --git a/shared-core/via_drv.c b/shared-core/via_drv.c
index 33b0a42d..1446af2c 100644
--- a/shared-core/via_drv.c
+++ b/shared-core/via_drv.c
@@ -57,8 +57,9 @@ 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 = NULL,
 #ifdef VIA_HAVE_CORE_MM
-	.reclaim_buffers_locked = via_reclaim_buffers_locked,
+	.reclaim_buffers_idlelocked = via_reclaim_buffers_locked,
 	.lastclose = via_lastclose,
 #endif
 	.get_map_ofs = drm_core_get_map_ofs,