[PATCH 3/6] drm/i915: Require the object lock for vma destruction

[Thomas Hellström]

The only place we can destroy a vma with the object alive without
holding the object lock is __i915_vm_close(). Fix that so that we
can rely on vmas staying alive while we hold the object lock and
thus be ok to remove the vma refcount.

Since i915_gem_object_unbind() might race with i915_vm_close() in which case
i915_gem_object_unbind() skips the unbind and returns -EAGAIN, hoping that
i915_vm_close() will make progress. Now It won't since we now hold the object
lock, but we can now instead allow i915_gem_object_unbind() to proceed.

Signed-off-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gtt.c | 16 ++++++++++++++++
 drivers/gpu/drm/i915/i915_gem.c     | 13 ++++++-------
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c b/drivers/gpu/drm/i915/gt/intel_gtt.c
index c9a24b021a7e..c115dc82de3c 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
@@ -102,6 +102,7 @@ void __i915_vm_close(struct i915_address_space *vm)
 	if (!atomic_dec_and_mutex_lock(&vm->open, &vm->mutex))
 		return;
 
+retry:
 	list_for_each_entry_safe(vma, vn, &vm->bound_list, vm_link) {
 		struct drm_i915_gem_object *obj = vma->obj;
 
@@ -117,7 +118,22 @@ void __i915_vm_close(struct i915_address_space *vm)
 		}
 
 		/* Keep the obj (and hence the vma) alive as _we_ destroy it */
+		if (!i915_gem_object_trylock(obj, NULL)) {
+			/*
+			 * We need to unlock the vm->mutex to maintain locking
+			 * order wrt object lock, and then restart the loop.
+			 */
+			mutex_unlock(&vm->mutex);
+			i915_gem_object_lock(obj, NULL);
+			mutex_lock(&vm->mutex);
+			i915_vma_destroy_locked(vma);
+			i915_gem_object_unlock(obj);
+			i915_gem_object_put(obj);
+			goto retry;
+		}
+
 		i915_vma_destroy_locked(vma);
+		i915_gem_object_unlock(obj);
 		i915_gem_object_put(obj);
 	}
 	GEM_BUG_ON(!list_empty(&vm->bound_list));
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index e3a2c2a0e156..0129ed8ee09f 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -139,8 +139,6 @@ int i915_gem_object_unbind(struct drm_i915_gem_object *obj,
 	while (!ret && (vma = list_first_entry_or_null(&obj->vma.list,
 						       struct i915_vma,
 						       obj_link))) {
-		struct i915_address_space *vm = vma->vm;
-
 		list_move_tail(&vma->obj_link, &still_in_list);
 		if (!i915_vma_is_bound(vma, I915_VMA_BIND_MASK))
 			continue;
@@ -150,14 +148,16 @@ int i915_gem_object_unbind(struct drm_i915_gem_object *obj,
 			break;
 		}
 
-		ret = -EAGAIN;
-		if (!i915_vm_tryopen(vm))
-			break;
+		/*
+		 * We might race with a i915_vm_close() or a i915_vma_close,
+		 * but since we hold the object lock, they will block on this
+		 * object. Hence, the vma will stay alive, and since the vma
+		 * has a reference on the vm, so will the vm.
+		 */
 
 		/* Prevent vma being freed by i915_vma_parked as we unbind */
 		vma = __i915_vma_get(vma);
 		spin_unlock(&obj->vma.lock);
-
 		if (vma) {
 			bool vm_trylock = !!(flags & I915_GEM_OBJECT_UNBIND_VM_TRYLOCK);
 			ret = -EBUSY;
@@ -183,7 +183,6 @@ int i915_gem_object_unbind(struct drm_i915_gem_object *obj,
 			__i915_vma_put(vma);
 		}
 
-		i915_vm_close(vm);
 		spin_lock(&obj->vma.lock);
 	}
 	list_splice_init(&still_in_list, &obj->vma.list);
-- 
2.34.1