Commit 6ed0897c authored by Rob Clark's avatar Rob Clark
Browse files

drm/msm: Fix debugfs deadlock 



In normal cases the gem obj lock is acquired first before mm_lock.  The
exception is iterating the various object lists.  In the shrinker path,
deadlock is avoided by using msm_gem_trylock() and skipping over objects
that cannot be locked.  But for debugfs the straightforward thing is to
split things out into a separate list of all objects protected by it's
own lock.

Fixes: d984457b ("drm/msm: Add priv->mm_lock to protect active/inactive lists")
Signed-off-by: default avatarRob Clark <robdclark@chromium.org>
Tested-by: default avatarDouglas Anderson <dianders@chromium.org>
Reviewed-by: default avatarDouglas Anderson <dianders@chromium.org>
Link: https://lore.kernel.org/r/20210401012722.527712-4-robdclark@gmail.com


Signed-off-by: default avatarRob Clark <robdclark@chromium.org>
parent cc8a4d5a

drivers/gpu/drm/msm/msm_debugfs.c

+3 −11
Original line number Diff line number Diff line
@@ -111,23 +111,15 @@ static const struct file_operations msm_gpu_fops = { 
static int msm_gem_show(struct drm_device *dev, struct seq_file *m)

{

	struct msm_drm_private *priv = dev->dev_private;

	struct msm_gpu *gpu = priv->gpu;

	int ret;



	ret = mutex_lock_interruptible(&priv->mm_lock);

	ret = mutex_lock_interruptible(&priv->obj_lock);

	if (ret)

		return ret;



	if (gpu) {

		seq_printf(m, "Active Objects (%s):\n", gpu->name);

		msm_gem_describe_objects(&gpu->active_list, m);

	}



	seq_printf(m, "Inactive Objects:\n");

	msm_gem_describe_objects(&priv->inactive_dontneed, m);

	msm_gem_describe_objects(&priv->inactive_willneed, m);

	msm_gem_describe_objects(&priv->objects, m);



	mutex_unlock(&priv->mm_lock);

	mutex_unlock(&priv->obj_lock);



	return 0;

}

drivers/gpu/drm/msm/msm_drv.c

+3 −0
Original line number Diff line number Diff line
@@ -446,6 +446,9 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv) 


	priv->wq = alloc_ordered_workqueue("msm", 0);



	INIT_LIST_HEAD(&priv->objects);

	mutex_init(&priv->obj_lock);



	INIT_LIST_HEAD(&priv->inactive_willneed);

	INIT_LIST_HEAD(&priv->inactive_dontneed);

	INIT_LIST_HEAD(&priv->inactive_purged);

drivers/gpu/drm/msm/msm_drv.h

+8 −1
Original line number Diff line number Diff line
@@ -174,7 +174,14 @@ struct msm_drm_private { 
	struct msm_rd_state *hangrd;   /* debugfs to dump hanging submits */

	struct msm_perf_state *perf;



	/*

	/**

	 * List of all GEM objects (mainly for debugfs, protected by obj_lock

	 * (acquire before per GEM object lock)

	 */

	struct list_head objects;

	struct mutex obj_lock;



	/**

	 * Lists of inactive GEM objects.  Every bo is either in one of the

	 * inactive lists (depending on whether or not it is shrinkable) or

	 * gpu->active_list (for the gpu it is active on[1])

drivers/gpu/drm/msm/msm_gem.c

+13 −1
Original line number Diff line number Diff line
@@ -961,7 +961,7 @@ void msm_gem_describe_objects(struct list_head *list, struct seq_file *m) 
	size_t size = 0;



	seq_puts(m, "   flags       id ref  offset   kaddr            size     madv      name\n");

	list_for_each_entry(msm_obj, list, mm_list) {

	list_for_each_entry(msm_obj, list, node) {

		struct drm_gem_object *obj = &msm_obj->base;

		seq_puts(m, "   ");

		msm_gem_describe(obj, m);
@@ -980,6 +980,10 @@ void msm_gem_free_object(struct drm_gem_object *obj) 
	struct drm_device *dev = obj->dev;

	struct msm_drm_private *priv = dev->dev_private;



	mutex_lock(&priv->obj_lock);

	list_del(&msm_obj->node);

	mutex_unlock(&priv->obj_lock);



	mutex_lock(&priv->mm_lock);

	if (msm_obj->dontneed)

		mark_unpurgable(msm_obj);
@@ -1169,6 +1173,10 @@ static struct drm_gem_object *_msm_gem_new(struct drm_device *dev, 
	list_add_tail(&msm_obj->mm_list, &priv->inactive_willneed);

	mutex_unlock(&priv->mm_lock);



	mutex_lock(&priv->obj_lock);

	list_add_tail(&msm_obj->node, &priv->objects);

	mutex_unlock(&priv->obj_lock);



	return obj;



fail:
@@ -1239,6 +1247,10 @@ struct drm_gem_object *msm_gem_import(struct drm_device *dev, 
	list_add_tail(&msm_obj->mm_list, &priv->inactive_willneed);

	mutex_unlock(&priv->mm_lock);



	mutex_lock(&priv->obj_lock);

	list_add_tail(&msm_obj->node, &priv->objects);

	mutex_unlock(&priv->obj_lock);



	return obj;



fail:

drivers/gpu/drm/msm/msm_gem.h

+8 −2
Original line number Diff line number Diff line
@@ -60,10 +60,16 @@ struct msm_gem_object { 
	 */

	uint8_t vmap_count;



	/**

	 * Node in list of all objects (mainly for debugfs, protected by

	 * priv->obj_lock

	 */

	struct list_head node;



	/**

	 * An object is either:

	 *  inactive - on priv->inactive_dontneed/willneed/purged depending

	 *     on status

	 *  inactive - on priv->inactive_dontneed or priv->inactive_willneed

	 *     (depending on purgability status)

	 *  active   - on one one of the gpu's active_list..  well, at

	 *     least for now we don't have (I don't think) hw sync between

	 *     2d and 3d one devices which have both, meaning we need to