drm: Hide hw.lock cleanup in filp->release better

A few things:
- Rename the cleanup function from drm_master_release to
  drm_legacy_lock_release. It doesn't relase any master stuff, but
  just the legacy hw lock.
- Hide it in drm_lock.c, which allows us to make a few more functions
  static in there. To avoid forward decl we need to shuffle the code a
  bit though.
- Push the check for ->master into the function itself.
- Only call this for !DRIVER_MODESET.

End result: Another place that takes struct_mutex gone for good for
modern drivers.

v2: Remove leftover comment.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1465930269-7883-3-git-send-email-daniel.vetter@ffwll.ch

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index a27bc7cda975f14309fc00ddcf4f875950a6f9cd..2fd4f42b907a87d3d1e836e15392a1253a932994 100644 (file)
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -338,18 +338,6 @@ out_prime_destroy:
        return ret;
 }
 
-static void drm_master_release(struct drm_device *dev, struct file *filp)
-{
-       struct drm_file *file_priv = filp->private_data;
-
-       if (drm_legacy_i_have_hw_lock(dev, file_priv)) {
-               DRM_DEBUG("File %p released, freeing lock for context %d\n",
-                         filp, _DRM_LOCKING_CONTEXT(file_priv->master->lock.hw_lock->lock));
-               drm_legacy_lock_free(&file_priv->master->lock,
-                                    _DRM_LOCKING_CONTEXT(file_priv->master->lock.hw_lock->lock));
-       }
-}
-
 static void drm_events_release(struct drm_file *file_priv)
 {
        struct drm_device *dev = file_priv->minor->dev;
@@ -468,9 +456,8 @@ int drm_release(struct inode *inode, struct file *filp)
                  (long)old_encode_dev(file_priv->minor->kdev->devt),
                  dev->open_count);
 
-       /* if the master has gone away we can't do anything with the lock */
-       if (file_priv->minor->master)
-               drm_master_release(dev, filp);
+       if (!drm_core_check_feature(dev, DRIVER_MODESET))
+               drm_legacy_lock_release(dev, filp);
 
        if (drm_core_check_feature(dev, DRIVER_HAVE_DMA))
                drm_legacy_reclaim_buffers(dev, file_priv);

diff --git a/drivers/gpu/drm/drm_legacy.h b/drivers/gpu/drm/drm_legacy.h
index d3b6ee357a2b432aee09fb5b3780c6df84c7a577..c6f422e879dd6986f372bb2dfb6a5c764fcc045d 100644 (file)
--- a/drivers/gpu/drm/drm_legacy.h
+++ b/drivers/gpu/drm/drm_legacy.h
@@ -88,14 +88,10 @@ struct drm_agp_mem {
        struct list_head head;
 };
 
-/*
- * Generic Userspace Locking-API
- */
-
-int drm_legacy_i_have_hw_lock(struct drm_device *d, struct drm_file *f);
+/* drm_lock.c */
 int drm_legacy_lock(struct drm_device *d, void *v, struct drm_file *f);
 int drm_legacy_unlock(struct drm_device *d, void *v, struct drm_file *f);
-int drm_legacy_lock_free(struct drm_lock_data *lock, unsigned int ctx);
+void drm_legacy_lock_release(struct drm_device *dev, struct file *filp);
 
 /* DMA support */
 int drm_legacy_dma_setup(struct drm_device *dev);

diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
index daa2ff12101ba366d82be5b550ba5cf5738a6ff2..0fb8f9e734861dc5f7d464129e811a5397a217b1 100644 (file)
--- a/drivers/gpu/drm/drm_lock.c
+++ b/drivers/gpu/drm/drm_lock.c
@@ -40,6 +40,110 @@
 
 static int drm_lock_take(struct drm_lock_data *lock_data, unsigned int context);
 
+/**
+ * Take the heavyweight lock.
+ *
+ * \param lock lock pointer.
+ * \param context locking context.
+ * \return one if the lock is held, or zero otherwise.
+ *
+ * Attempt to mark the lock as held by the given context, via the \p cmpxchg instruction.
+ */
+static
+int drm_lock_take(struct drm_lock_data *lock_data,
+                 unsigned int context)
+{
+       unsigned int old, new, prev;
+       volatile unsigned int *lock = &lock_data->hw_lock->lock;
+
+       spin_lock_bh(&lock_data->spinlock);
+       do {
+               old = *lock;
+               if (old & _DRM_LOCK_HELD)
+                       new = old | _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_bh(&lock_data->spinlock);
+
+       if (_DRM_LOCKING_CONTEXT(old) == context) {
+               if (old & _DRM_LOCK_HELD) {
+                       if (context != DRM_KERNEL_CONTEXT) {
+                               DRM_ERROR("%d holds heavyweight lock\n",
+                                         context);
+                       }
+                       return 0;
+               }
+       }
+
+       if ((_DRM_LOCKING_CONTEXT(new)) == context && (new & _DRM_LOCK_HELD)) {
+               /* Have lock */
+               return 1;
+       }
+       return 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.
+ *
+ * \param dev DRM device.
+ * \param lock lock pointer.
+ * \param context locking context.
+ * \return always one.
+ *
+ * Resets the lock file pointer.
+ * Marks the lock as held by the given context, via the \p cmpxchg instruction.
+ */
+static int drm_lock_transfer(struct drm_lock_data *lock_data,
+                            unsigned int context)
+{
+       unsigned int old, new, prev;
+       volatile unsigned int *lock = &lock_data->hw_lock->lock;
+
+       lock_data->file_priv = NULL;
+       do {
+               old = *lock;
+               new = context | _DRM_LOCK_HELD;
+               prev = cmpxchg(lock, old, new);
+       } while (prev != old);
+       return 1;
+}
+
+static int drm_legacy_lock_free(struct drm_lock_data *lock_data,
+                               unsigned int context)
+{
+       unsigned int old, new, prev;
+       volatile unsigned int *lock = &lock_data->hw_lock->lock;
+
+       spin_lock_bh(&lock_data->spinlock);
+       if (lock_data->kernel_waiters != 0) {
+               drm_lock_transfer(lock_data, 0);
+               lock_data->idle_has_lock = 1;
+               spin_unlock_bh(&lock_data->spinlock);
+               return 1;
+       }
+       spin_unlock_bh(&lock_data->spinlock);
+
+       do {
+               old = *lock;
+               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(&lock_data->lock_queue);
+       return 0;
+}
+
 /**
  * Lock ioctl.
  *
@@ -164,120 +268,6 @@ int drm_legacy_unlock(struct drm_device *dev, void *data, struct drm_file *file_
        return 0;
 }
 
-/**
- * Take the heavyweight lock.
- *
- * \param lock lock pointer.
- * \param context locking context.
- * \return one if the lock is held, or zero otherwise.
- *
- * Attempt to mark the lock as held by the given context, via the \p cmpxchg instruction.
- */
-static
-int drm_lock_take(struct drm_lock_data *lock_data,
-                 unsigned int context)
-{
-       unsigned int old, new, prev;
-       volatile unsigned int *lock = &lock_data->hw_lock->lock;
-
-       spin_lock_bh(&lock_data->spinlock);
-       do {
-               old = *lock;
-               if (old & _DRM_LOCK_HELD)
-                       new = old | _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_bh(&lock_data->spinlock);
-
-       if (_DRM_LOCKING_CONTEXT(old) == context) {
-               if (old & _DRM_LOCK_HELD) {
-                       if (context != DRM_KERNEL_CONTEXT) {
-                               DRM_ERROR("%d holds heavyweight lock\n",
-                                         context);
-                       }
-                       return 0;
-               }
-       }
-
-       if ((_DRM_LOCKING_CONTEXT(new)) == context && (new & _DRM_LOCK_HELD)) {
-               /* Have lock */
-               return 1;
-       }
-       return 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.
- *
- * \param dev DRM device.
- * \param lock lock pointer.
- * \param context locking context.
- * \return always one.
- *
- * Resets the lock file pointer.
- * Marks the lock as held by the given context, via the \p cmpxchg instruction.
- */
-static int drm_lock_transfer(struct drm_lock_data *lock_data,
-                            unsigned int context)
-{
-       unsigned int old, new, prev;
-       volatile unsigned int *lock = &lock_data->hw_lock->lock;
-
-       lock_data->file_priv = NULL;
-       do {
-               old = *lock;
-               new = context | _DRM_LOCK_HELD;
-               prev = cmpxchg(lock, old, new);
-       } while (prev != old);
-       return 1;
-}
-
-/**
- * Free lock.
- *
- * \param dev DRM device.
- * \param lock lock.
- * \param context context.
- *
- * Resets the lock file pointer.
- * Marks the lock as not held, via the \p cmpxchg instruction. Wakes any task
- * waiting on the lock queue.
- */
-int drm_legacy_lock_free(struct drm_lock_data *lock_data, unsigned int context)
-{
-       unsigned int old, new, prev;
-       volatile unsigned int *lock = &lock_data->hw_lock->lock;
-
-       spin_lock_bh(&lock_data->spinlock);
-       if (lock_data->kernel_waiters != 0) {
-               drm_lock_transfer(lock_data, 0);
-               lock_data->idle_has_lock = 1;
-               spin_unlock_bh(&lock_data->spinlock);
-               return 1;
-       }
-       spin_unlock_bh(&lock_data->spinlock);
-
-       do {
-               old = *lock;
-               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(&lock_data->lock_queue);
-       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
@@ -330,11 +320,27 @@ void drm_legacy_idlelock_release(struct drm_lock_data *lock_data)
 }
 EXPORT_SYMBOL(drm_legacy_idlelock_release);
 
-int drm_legacy_i_have_hw_lock(struct drm_device *dev,
-                             struct drm_file *file_priv)
+static int drm_legacy_i_have_hw_lock(struct drm_device *dev,
+                                    struct drm_file *file_priv)
 {
        struct drm_master *master = file_priv->master;
        return (file_priv->lock_count && master->lock.hw_lock &&
                _DRM_LOCK_IS_HELD(master->lock.hw_lock->lock) &&
                master->lock.file_priv == file_priv);
 }
+
+void drm_legacy_lock_release(struct drm_device *dev, struct file *filp)
+{
+       struct drm_file *file_priv = filp->private_data;
+
+       /* if the master has gone away we can't do anything with the lock */
+       if (!file_priv->minor->master)
+               return;
+
+       if (drm_legacy_i_have_hw_lock(dev, file_priv)) {
+               DRM_DEBUG("File %p released, freeing lock for context %d\n",
+                         filp, _DRM_LOCKING_CONTEXT(file_priv->master->lock.hw_lock->lock));
+               drm_legacy_lock_free(&file_priv->master->lock,
+                                    _DRM_LOCKING_CONTEXT(file_priv->master->lock.hw_lock->lock));
+       }
+}