From: oga <oga@openbsd.org>
Date: Mon, 7 Jul 2008 16:29:57 +0000 (+0000)
Subject: The current drm_locked_task*() code sometimes tries to sleep in an
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=f7ae2e58961a90a99d5f7fbf2ea2c1a817a9deb7;p=openbsd

The current drm_locked_task*() code sometimes tries to sleep in an
interrupt handler.

This is bad and wrong. So change it so that if we can't immediately grab
the hardware lock, to just leave the task flagged so that we can run it
when we release the lock. The linux implementation uses a similar
scheme.

Tested by guenther@, landry@ and bernd@. Also tested by many a while
ago as part of a larger diff.
---

diff --git a/sys/dev/pci/drm/drmP.h b/sys/dev/pci/drm/drmP.h
index 31d73c0403c..3b448aff895 100644
--- a/sys/dev/pci/drm/drmP.h
+++ b/sys/dev/pci/drm/drmP.h
@@ -638,6 +638,7 @@ struct drm_device {
 	DRM_SPINTYPE	  irq_lock;	/* protects irq condition checks */
 	DRM_SPINTYPE	  dev_lock;	/* protects everything else */
 	DRM_SPINTYPE	  drw_lock;
+	DRM_SPINTYPE	  tsk_lock;
 
 				/* Usage Counters */
 	int		  open_count;	/* Outstanding files open	   */
diff --git a/sys/dev/pci/drm/drm_drv.c b/sys/dev/pci/drm/drm_drv.c
index 3175dacaf9d..9f5a107d068 100644
--- a/sys/dev/pci/drm/drm_drv.c
+++ b/sys/dev/pci/drm/drm_drv.c
@@ -169,7 +169,8 @@ drm_attach(struct device *parent, struct device *kdev,
 	dev->pci_slot = pa->pa_device;
 	dev->pci_func = pa->pa_function;
 	DRM_SPININIT(&dev->dev_lock, "drm device");
-	DRM_SPININIT(&dev->drw_lock, "drm drawable lock");
+	mtx_init(&dev->drw_lock, IPL_BIO);
+	mtx_init(&dev->tsk_lock, IPL_BIO);
 
 	id_entry = drm_find_description(PCI_VENDOR(pa->pa_id),
 	    PCI_PRODUCT(pa->pa_id), idlist);
diff --git a/sys/dev/pci/drm/drm_irq.c b/sys/dev/pci/drm/drm_irq.c
index 76477a1a8ff..9c216f59b04 100644
--- a/sys/dev/pci/drm/drm_irq.c
+++ b/sys/dev/pci/drm/drm_irq.c
@@ -519,38 +519,43 @@ drm_locked_task(void *context, void *pending)
 {
 	struct drm_device *dev = context;
 
-	DRM_LOCK();
-	for (;;) {
-		int ret;
-
-		if (drm_lock_take(&dev->lock.hw_lock->lock,
-		    DRM_KERNEL_CONTEXT))
-		{
-			dev->lock.file_priv = NULL; /* kernel owned */
-			dev->lock.lock_time = jiffies;
-			atomic_inc(&dev->counts[_DRM_STAT_LOCKS]);
-			break;  /* Got lock */
-		}
+	DRM_SPINLOCK(&dev->tsk_lock);
 
-		/* Contention */
-		ret = DRM_SLEEPLOCK((void *)&dev->lock.lock_queue, &dev->dev_lock,
-		    PZERO | PCATCH, "drmlk2", 0);
-		if (ret != 0) {
-			DRM_UNLOCK();
-			return;
-		}
+	DRM_LOCK(); /* XXX drm_lock_take() should do its own locking */
+	if (dev->locked_task_call == NULL ||
+	    drm_lock_take(&dev->lock.hw_lock->lock, DRM_KERNEL_CONTEXT) == 0) {
+		DRM_UNLOCK();
+		DRM_SPINUNLOCK(&dev->tsk_lock);
+		return;
 	}
+
+	dev->lock.file_priv = NULL; /* kernel owned */
+	dev->lock.lock_time = jiffies;
+	atomic_inc(&dev->counts[_DRM_STAT_LOCKS]);
+
 	DRM_UNLOCK();
 
 	dev->locked_task_call(dev);
 
 	drm_lock_free(dev, &dev->lock.hw_lock->lock, DRM_KERNEL_CONTEXT);
+
+	dev->locked_task_call = NULL;
+
+	DRM_SPINUNLOCK(&dev->tsk_lock);
 }
 
 void
 drm_locked_tasklet(struct drm_device *dev, void (*tasklet)(struct drm_device *))
 {
+	DRM_SPINLOCK(&dev->tsk_lock);
+	if (dev->locked_task_call != NULL) {
+		DRM_SPINUNLOCK(&dev->tsk_lock);
+		return;
+	}
+
 	dev->locked_task_call = tasklet;
+	DRM_SPINUNLOCK(&dev->tsk_lock);
+
 	if (workq_add_task(NULL, 0, drm_locked_task, dev, NULL) == ENOMEM)
 		DRM_ERROR("error adding task to workq\n");
 }
diff --git a/sys/dev/pci/drm/drm_lock.c b/sys/dev/pci/drm/drm_lock.c
index 69a832387b1..f1640fc2f3b 100644
--- a/sys/dev/pci/drm/drm_lock.c
+++ b/sys/dev/pci/drm/drm_lock.c
@@ -180,6 +180,13 @@ drm_unlock(struct drm_device *dev, void *data, struct drm_file *file_priv)
 	    _DRM_LOCKING_CONTEXT(dev->lock.hw_lock->lock) != lock->context)
 		return EINVAL;
 
+	DRM_SPINLOCK(&dev->tsk_lock);
+	if (dev->locked_task_call != NULL) {
+		dev->locked_task_call(dev);
+		dev->locked_task_call = NULL;
+	}
+	DRM_SPINUNLOCK(&dev->tsk_lock);
+
 	atomic_inc(&dev->counts[_DRM_STAT_UNLOCKS]);
 
 	DRM_LOCK();