From 1a75a222f5ca1063249b5c92972d32dcc3c8966e Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Tue, 14 Jun 2016 20:50:57 +0200 Subject: [PATCH] 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 Reviewed-by: Emil Velikov Signed-off-by: Daniel Vetter Link: http://patchwork.freedesktop.org/patch/msgid/1465930269-7883-3-git-send-email-daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_fops.c | 17 +-- drivers/gpu/drm/drm_legacy.h | 8 +- drivers/gpu/drm/drm_lock.c | 238 ++++++++++++++++++----------------- 3 files changed, 126 insertions(+), 137 deletions(-) diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index a27bc7cda975..2fd4f42b907a 100644 --- 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 d3b6ee357a2b..c6f422e879dd 100644 --- 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 daa2ff12101b..0fb8f9e73486 100644 --- 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)); + } +} -- 2.20.1