Commit 040ac320 authored by Thomas Hellstrom's avatar Thomas Hellstrom Committed by Dave Airlie
Browse files

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: default avatarDave Airlie <airlied@linux.ie>
parent 4b560fde

drivers/char/drm/drmP.h

+18 −3
Original line number Diff line number Diff line
@@ -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);

drivers/char/drm/drm_fops.c

+44 −46
Original line number Diff line number Diff line
@@ -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);

		} else {

			unsigned long _end=jiffies + 3*DRM_HZ;

			int locked = 0;



		drm_lock_free(dev, &dev->lock.hw_lock->lock,

			      _DRM_LOCKING_CONTEXT(dev->lock.hw_lock->lock));

			drm_idlelock_take(&dev->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;

			/*

			 * Wait for a while.

			 */



			do{

				spin_lock(&dev->lock.spinlock);

				locked = dev->lock.idle_has_lock;

				spin_unlock(&dev->lock.spinlock);

				if (locked)

					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 */

				schedule();

			if (signal_pending(current)) {

				retcode = -ERESTARTSYS;

				break;

			}

			} 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");

			}

		__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);

drivers/char/drm/drm_irq.c

+2 −2
Original line number Diff line number Diff line
@@ -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;

drivers/char/drm/drm_lock.c

+106 −28
Original line number Diff line number Diff line
@@ -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;

	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);

drivers/char/drm/drm_stub.c

+1 −0
Original line number Diff line number Diff line
@@ -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);

CRA Git | Maintained and supported by SUSTech CRA and CCSE