[PATCH 56/62] drm: Remove drm_minor_lock and idr_preload

From: Matthew Wilcox
Date: Wed Nov 22 2017 - 16:10:54 EST

From: Matthew Wilcox <mawilcox@xxxxxxxxxxxxx>

The IDR now has its own lock; use it to protect the lookup too.

Signed-off-by: Matthew Wilcox <mawilcox@xxxxxxxxxxxxx>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 8 ++++----
drivers/gpu/drm/drm_drv.c | 25 +++----------------------
drivers/gpu/drm/drm_gem.c | 27 ++++-----------------------
drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 6 +++---
drivers/gpu/drm/i915/i915_debugfs.c | 4 ++--
drivers/gpu/drm/msm/msm_gem_submit.c | 10 +++++-----
drivers/gpu/drm/vc4/vc4_gem.c | 4 ++--
include/drm/drm_file.h | 5 +----
8 files changed, 24 insertions(+), 65 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index a418df1b9422..25ee4eef3279 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -89,13 +89,13 @@ void amdgpu_gem_force_release(struct amdgpu_device *adev)
int handle;

WARN_ONCE(1, "Still active user space clients!\n");
- spin_lock(&file->table_lock);
+ idr_lock(&file->object_idr);
idr_for_each_entry(&file->object_idr, gobj, handle) {
WARN_ONCE(1, "And also active allocations!\n");
drm_gem_object_put_unlocked(gobj);
}
+ idr_unlock(&file->object_idr);
idr_destroy(&file->object_idr);
- spin_unlock(&file->table_lock);
}

mutex_unlock(&ddev->filelist_mutex);
@@ -824,9 +824,9 @@ static int amdgpu_debugfs_gem_info(struct seq_file *m, void *data)
task ? task->comm : "<unknown>");
rcu_read_unlock();

- spin_lock(&file->table_lock);
+ idr_lock(&file->object_idr);
idr_for_each(&file->object_idr, amdgpu_debugfs_gem_bo_info, m);
- spin_unlock(&file->table_lock);
+ idr_unlock(&file->object_idr);
}

mutex_unlock(&dev->filelist_mutex);
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index a934fd5e7e55..df51db70ea39 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -61,7 +61,6 @@ MODULE_PARM_DESC(debug, "Enable debug output, where each bit enables a debug cat
"\t\tBit 7 (0x80) will enable LEASE messages (leasing code)");
module_param_named(debug, drm_debug, int, 0600);

-static DEFINE_SPINLOCK(drm_minor_lock);
static struct idr drm_minors_idr;

/*
@@ -153,7 +152,6 @@ static struct drm_minor **drm_minor_get_slot(struct drm_device *dev,
static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
{
struct drm_minor *minor;
- unsigned long flags;
int r;

minor = kzalloc(sizeof(*minor), GFP_KERNEL);
@@ -163,15 +161,11 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
minor->type = type;
minor->dev = dev;

- idr_preload(GFP_KERNEL);
- spin_lock_irqsave(&drm_minor_lock, flags);
r = idr_alloc(&drm_minors_idr,
NULL,
64 * type,
64 * (type + 1),
- GFP_NOWAIT);
- spin_unlock_irqrestore(&drm_minor_lock, flags);
- idr_preload_end();
+ GFP_KERNEL);

if (r < 0)
goto err_free;
@@ -188,9 +182,7 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
return 0;

err_index:
- spin_lock_irqsave(&drm_minor_lock, flags);
idr_remove(&drm_minors_idr, minor->index);
- spin_unlock_irqrestore(&drm_minor_lock, flags);
err_free:
kfree(minor);
return r;
@@ -199,7 +191,6 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
static void drm_minor_free(struct drm_device *dev, unsigned int type)
{
struct drm_minor **slot, *minor;
- unsigned long flags;

slot = drm_minor_get_slot(dev, type);
minor = *slot;
@@ -207,11 +198,7 @@ static void drm_minor_free(struct drm_device *dev, unsigned int type)
return;

put_device(minor->kdev);
-
- spin_lock_irqsave(&drm_minor_lock, flags);
idr_remove(&drm_minors_idr, minor->index);
- spin_unlock_irqrestore(&drm_minor_lock, flags);
-
kfree(minor);
*slot = NULL;
}
@@ -219,7 +206,6 @@ static void drm_minor_free(struct drm_device *dev, unsigned int type)
static int drm_minor_register(struct drm_device *dev, unsigned int type)
{
struct drm_minor *minor;
- unsigned long flags;
int ret;

DRM_DEBUG("\n");
@@ -239,9 +225,7 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type)
goto err_debugfs;

/* replace NULL with @minor so lookups will succeed from now on */
- spin_lock_irqsave(&drm_minor_lock, flags);
idr_replace(&drm_minors_idr, minor, minor->index);
- spin_unlock_irqrestore(&drm_minor_lock, flags);

DRM_DEBUG("new minor registered %d\n", minor->index);
return 0;
@@ -254,16 +238,13 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type)
static void drm_minor_unregister(struct drm_device *dev, unsigned int type)
{
struct drm_minor *minor;
- unsigned long flags;

minor = *drm_minor_get_slot(dev, type);
if (!minor || !device_is_registered(minor->kdev))
return;

/* replace @minor with NULL so lookups will fail from now on */
- spin_lock_irqsave(&drm_minor_lock, flags);
idr_replace(&drm_minors_idr, NULL, minor->index);
- spin_unlock_irqrestore(&drm_minor_lock, flags);

device_del(minor->kdev);
dev_set_drvdata(minor->kdev, NULL); /* safety belt */
@@ -284,11 +265,11 @@ struct drm_minor *drm_minor_acquire(unsigned int minor_id)
struct drm_minor *minor;
unsigned long flags;

- spin_lock_irqsave(&drm_minor_lock, flags);
+ idr_lock_irqsave(&drm_minors_idr, flags);
minor = idr_find(&drm_minors_idr, minor_id);
if (minor)
drm_dev_get(minor->dev);
- spin_unlock_irqrestore(&drm_minor_lock, flags);
+ idr_unlock_irqrestore(&drm_minors_idr, flags);

if (!minor) {
return ERR_PTR(-ENODEV);
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 55d6182555c7..7d45b3ef4dd6 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -282,11 +282,8 @@ drm_gem_handle_delete(struct drm_file *filp, u32 handle)
{
struct drm_gem_object *obj;

- spin_lock(&filp->table_lock);
-
/* Check if we currently have a reference on the object */
obj = idr_replace(&filp->object_idr, NULL, handle);
- spin_unlock(&filp->table_lock);
if (IS_ERR_OR_NULL(obj))
return -EINVAL;

@@ -294,9 +291,7 @@ drm_gem_handle_delete(struct drm_file *filp, u32 handle)
drm_gem_object_release_handle(handle, obj, filp);

/* And finally make the handle available for future allocations. */
- spin_lock(&filp->table_lock);
idr_remove(&filp->object_idr, handle);
- spin_unlock(&filp->table_lock);

return 0;
}
@@ -387,17 +382,8 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
if (obj->handle_count++ == 0)
drm_gem_object_get(obj);

- /*
- * Get the user-visible handle using idr. Preload and perform
- * allocation under our spinlock.
- */
- idr_preload(GFP_KERNEL);
- spin_lock(&file_priv->table_lock);
-
- ret = idr_alloc(&file_priv->object_idr, obj, 1, 0, GFP_NOWAIT);
-
- spin_unlock(&file_priv->table_lock);
- idr_preload_end();
+ /* Get the user-visible handle using idr. */
+ ret = idr_alloc(&file_priv->object_idr, obj, 1, 0, GFP_KERNEL);

mutex_unlock(&dev->object_name_lock);
if (ret < 0)
@@ -421,9 +407,7 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
err_revoke:
drm_vma_node_revoke(&obj->vma_node, file_priv);
err_remove:
- spin_lock(&file_priv->table_lock);
idr_remove(&file_priv->object_idr, handle);
- spin_unlock(&file_priv->table_lock);
err_unref:
drm_gem_object_handle_put_unlocked(obj);
return ret;
@@ -634,14 +618,12 @@ drm_gem_object_lookup(struct drm_file *filp, u32 handle)
{
struct drm_gem_object *obj;

- spin_lock(&filp->table_lock);
-
/* Check if we currently have a reference on the object */
+ idr_lock(&filp->object_idr);
obj = idr_find(&filp->object_idr, handle);
if (obj)
drm_gem_object_get(obj);
-
- spin_unlock(&filp->table_lock);
+ idr_unlock(&filp->object_idr);

return obj;
}
@@ -776,7 +758,6 @@ void
drm_gem_open(struct drm_device *dev, struct drm_file *file_private)
{
idr_init(&file_private->object_idr);
- spin_lock_init(&file_private->table_lock);
}

/**
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
index ff911541a190..5298d9e78523 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
@@ -61,7 +61,7 @@ static int submit_lookup_objects(struct etnaviv_gem_submit *submit,
unsigned i;
int ret = 0;

- spin_lock(&file->table_lock);
+ idr_lock(&file->object_idr);

for (i = 0, bo = submit_bos; i < nr_bos; i++, bo++) {
struct drm_gem_object *obj;
@@ -75,7 +75,7 @@ static int submit_lookup_objects(struct etnaviv_gem_submit *submit,
submit->bos[i].flags = bo->flags;

/* normally use drm_gem_object_lookup(), but for bulk lookup
- * all under single table_lock just hit object_idr directly:
+ * all under single lock just hit object_idr directly:
*/
obj = idr_find(&file->object_idr, bo->handle);
if (!obj) {
@@ -96,7 +96,7 @@ static int submit_lookup_objects(struct etnaviv_gem_submit *submit,

out_unlock:
submit->nr_bos = i;
- spin_unlock(&file->table_lock);
+ idr_unlock(&file->object_idr);

return ret;
}
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index c65e381b85f3..1d88bd0852c3 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -544,9 +544,9 @@ static int i915_gem_object_info(struct seq_file *m, void *data)

memset(&stats, 0, sizeof(stats));
stats.file_priv = file->driver_priv;
- spin_lock(&file->table_lock);
+ idr_lock(&file->object_idr);
idr_for_each(&file->object_idr, per_file_stats, &stats);
- spin_unlock(&file->table_lock);
+ idr_unlock(&file->object_idr);
/*
* Although we have a valid reference on file->pid, that does
* not guarantee that the task_struct who called get_pid() is
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index b8dc8f96caf2..aa8d0d6e40cc 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -88,7 +88,7 @@ static int submit_lookup_objects(struct msm_gem_submit *submit,
unsigned i;
int ret = 0;

- spin_lock(&file->table_lock);
+ idr_lock(&file->object_idr);
pagefault_disable();

for (i = 0; i < args->nr_bos; i++) {
@@ -105,12 +105,12 @@ static int submit_lookup_objects(struct msm_gem_submit *submit,

if (copy_from_user_inatomic(&submit_bo, userptr, sizeof(submit_bo))) {
pagefault_enable();
- spin_unlock(&file->table_lock);
+ idr_unlock(&file->object_idr);
if (copy_from_user(&submit_bo, userptr, sizeof(submit_bo))) {
ret = -EFAULT;
goto out;
}
- spin_lock(&file->table_lock);
+ idr_lock(&file->object_idr);
pagefault_disable();
}

@@ -126,7 +126,7 @@ static int submit_lookup_objects(struct msm_gem_submit *submit,
submit->bos[i].iova = submit_bo.presumed;

/* normally use drm_gem_object_lookup(), but for bulk lookup
- * all under single table_lock just hit object_idr directly:
+ * all under single lock just hit object_idr directly:
*/
obj = idr_find(&file->object_idr, submit_bo.handle);
if (!obj) {
@@ -153,7 +153,7 @@ static int submit_lookup_objects(struct msm_gem_submit *submit,

out_unlock:
pagefault_enable();
- spin_unlock(&file->table_lock);
+ idr_unlock(&file->object_idr);

out:
submit->nr_bos = i;
diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
index e00ac2f3a264..3dfd4fa103c3 100644
--- a/drivers/gpu/drm/vc4/vc4_gem.c
+++ b/drivers/gpu/drm/vc4/vc4_gem.c
@@ -713,7 +713,7 @@ vc4_cl_lookup_bos(struct drm_device *dev,
goto fail;
}

- spin_lock(&file_priv->table_lock);
+ idr_lock(&file_priv->object_idr);
for (i = 0; i < exec->bo_count; i++) {
struct drm_gem_object *bo = idr_find(&file_priv->object_idr,
handles[i]);
@@ -727,7 +727,7 @@ vc4_cl_lookup_bos(struct drm_device *dev,
drm_gem_object_get(bo);
exec->bo[i] = (struct drm_gem_cma_object *)bo;
}
- spin_unlock(&file_priv->table_lock);
+ idr_unlock(&file_priv->object_idr);

if (ret)
goto fail_put_bo;
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index 0e0c868451a5..dd3520d8ce60 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -225,13 +225,10 @@ struct drm_file {
* @object_idr:
*
* Mapping of mm object handles to object pointers. Used by the GEM
- * subsystem. Protected by @table_lock.
+ * subsystem.
*/
struct idr object_idr;

- /** @table_lock: Protects @object_idr. */
- spinlock_t table_lock;
-
/** @syncobj_idr: Mapping of sync object handles to object pointers. */
struct idr syncobj_idr;
/** @syncobj_table_lock: Protects @syncobj_idr. */
--
2.15.0