drm/i915: Remove the vm open count

vms are not getting properly closed. Rather than fixing that,
Remove the vm open count and instead rely on the vm refcount.

The vm open count existed solely to break the strong references the
vmas had on the vms. Now instead make those references weak and
ensure vmas are destroyed when the vm is destroyed.

Unfortunately if the vm destructor and the object destructor both
wants to destroy a vma, that may lead to a race in that the vm
destructor just unbinds the vma and leaves the actual vma destruction
to the object destructor. However in order for the object destructor
to ensure the vma is unbound it needs to grab the vm mutex. In order
to keep the vm mutex alive until the object destructor is done with
it, somewhat hackishly grab a vm_resv refcount that is released late
in the vma destruction process, when the vm mutex is no longer needed.

v2: Address review-comments from Niranjana
- Clarify that the struct i915_address_space::skip_pte_rewrite is a hack
  and should ideally be replaced in an upcoming patch.
- Remove an unneeded continue in clear_vm_list and update comment.

v3:
- Documentation update
- Commit message formatting

Co-developed-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Reviewed-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20220304082641.308069-2-thomas.hellstrom@linux.intel.com

1 files changed, 44 insertions, 19 deletions

diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 94fcdb7bd21d..cfa8a212b4dd 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -47,7 +47,7 @@ static inline void assert_vma_held_evict(const struct i915_vma *vma)
 	 * This is the only exception to the requirement of the object lock
 	 * being held.
 	 */
-	if (atomic_read(&vma->vm->open))
+	if (kref_read(&vma->vm->ref))
 		assert_object_held_shared(vma->obj);
 }
 
@@ -113,6 +113,7 @@ vma_create(struct drm_i915_gem_object *obj,
 	struct i915_vma *pos = ERR_PTR(-E2BIG);
 	struct i915_vma *vma;
 	struct rb_node *rb, **p;
+	int err;
 
 	/* The aliasing_ppgtt should never be used directly! */
 	GEM_BUG_ON(vm == &vm->gt->ggtt->alias->vm);
@@ -122,7 +123,6 @@ vma_create(struct drm_i915_gem_object *obj,
 		return ERR_PTR(-ENOMEM);
 
 	kref_init(&vma->ref);
-	vma->vm = i915_vm_get(vm);
 	vma->ops = &vm->vma_ops;
 	vma->obj = obj;
 	vma->size = obj->base.size;
@@ -138,6 +138,8 @@ vma_create(struct drm_i915_gem_object *obj,
 	}
 
 	INIT_LIST_HEAD(&vma->closed_link);
+	INIT_LIST_HEAD(&vma->obj_link);
+	RB_CLEAR_NODE(&vma->obj_node);
 
 	if (view && view->type != I915_GGTT_VIEW_NORMAL) {
 		vma->ggtt_view = *view;
@@ -163,8 +165,16 @@ vma_create(struct drm_i915_gem_object *obj,
 
 	GEM_BUG_ON(!IS_ALIGNED(vma->size, I915_GTT_PAGE_SIZE));
 
-	spin_lock(&obj->vma.lock);
+	err = mutex_lock_interruptible(&vm->mutex);
+	if (err) {
+		pos = ERR_PTR(err);
+		goto err_vma;
+	}
 
+	vma->vm = vm;
+	list_add_tail(&vma->vm_link, &vm->unbound_list);
+
+	spin_lock(&obj->vma.lock);
 	if (i915_is_ggtt(vm)) {
 		if (unlikely(overflows_type(vma->size, u32)))
 			goto err_unlock;
@@ -222,13 +232,15 @@ vma_create(struct drm_i915_gem_object *obj,
 		list_add_tail(&vma->obj_link, &obj->vma.list);
 
 	spin_unlock(&obj->vma.lock);
+	mutex_unlock(&vm->mutex);
 
 	return vma;
 
 err_unlock:
 	spin_unlock(&obj->vma.lock);
+	list_del_init(&vma->vm_link);
+	mutex_unlock(&vm->mutex);
 err_vma:
-	i915_vm_put(vm);
 	i915_vma_free(vma);
 	return pos;
 }
@@ -279,7 +291,7 @@ i915_vma_instance(struct drm_i915_gem_object *obj,
 	struct i915_vma *vma;
 
 	GEM_BUG_ON(view && !i915_is_ggtt_or_dpt(vm));
-	GEM_BUG_ON(!atomic_read(&vm->open));
+	GEM_BUG_ON(!kref_read(&vm->ref));
 
 	spin_lock(&obj->vma.lock);
 	vma = i915_vma_lookup(obj, vm, view);
@@ -322,7 +334,6 @@ static void __vma_release(struct dma_fence_work *work)
 		i915_gem_object_put(vw->pinned);
 
 	i915_vm_free_pt_stash(vw->vm, &vw->stash);
-	i915_vm_put(vw->vm);
 	if (vw->vma_res)
 		i915_vma_resource_put(vw->vma_res);
 }
@@ -841,7 +852,7 @@ i915_vma_insert(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
 	GEM_BUG_ON(!drm_mm_node_allocated(&vma->node));
 	GEM_BUG_ON(!i915_gem_valid_gtt_space(vma, color));
 
-	list_add_tail(&vma->vm_link, &vma->vm->bound_list);
+	list_move_tail(&vma->vm_link, &vma->vm->bound_list);
 
 	return 0;
 }
@@ -857,7 +868,7 @@ i915_vma_detach(struct i915_vma *vma)
 	 * vma, we can drop its hold on the backing storage and allow
 	 * it to be reaped by the shrinker.
 	 */
-	list_del(&vma->vm_link);
+	list_move_tail(&vma->vm_link, &vma->vm->unbound_list);
 }
 
 static bool try_qad_pin(struct i915_vma *vma, unsigned int flags)
@@ -1373,8 +1384,7 @@ int i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
 			goto err_rpm;
 		}
 
-		work->vm = i915_vm_get(vma->vm);
-
+		work->vm = vma->vm;
 		dma_fence_work_chain(&work->base, moving);
 
 		/* Allocate enough page directories to used PTE */
@@ -1622,7 +1632,6 @@ void i915_vma_release(struct kref *ref)
 {
 	struct i915_vma *vma = container_of(ref, typeof(*vma), ref);
 
-	i915_vm_put(vma->vm);
 	i915_active_fini(&vma->active);
 	GEM_WARN_ON(vma->resource);
 	i915_vma_free(vma);
@@ -1638,7 +1647,7 @@ static void force_unbind(struct i915_vma *vma)
 	GEM_BUG_ON(drm_mm_node_allocated(&vma->node));
 }
 
-static void release_references(struct i915_vma *vma)
+static void release_references(struct i915_vma *vma, bool vm_ddestroy)
 {
 	struct drm_i915_gem_object *obj = vma->obj;
 
@@ -1648,10 +1657,14 @@ static void release_references(struct i915_vma *vma)
 	list_del(&vma->obj_link);
 	if (!RB_EMPTY_NODE(&vma->obj_node))
 		rb_erase(&vma->obj_node, &obj->vma.tree);
+
 	spin_unlock(&obj->vma.lock);
 
 	__i915_vma_remove_closed(vma);
 
+	if (vm_ddestroy)
+		i915_vm_resv_put(vma->vm);
+
 	__i915_vma_put(vma);
 }
 
@@ -1667,8 +1680,12 @@ static void release_references(struct i915_vma *vma)
  * - __i915_gem_object_pages_fini()
  * - __i915_vm_close() - Blocks the above function by taking a reference on
  * the object.
- * - __i915_vma_parked() - Blocks the above functions by taking an open-count on
- * the vm and a reference on the object.
+ * - __i915_vma_parked() - Blocks the above functions by taking a reference
+ * on the vm and a reference on the object. Also takes the object lock so
+ * destruction from __i915_vma_parked() can be blocked by holding the
+ * object lock. Since the object lock is only allowed from within i915 with
+ * an object refcount, holding the object lock also implicitly blocks the
+ * vma freeing from __i915_gem_object_pages_fini().
  *
  * Because of locks taken during destruction, a vma is also guaranteed to
  * stay alive while the following locks are held if it was looked up while
@@ -1685,15 +1702,21 @@ void i915_vma_destroy_locked(struct i915_vma *vma)
 	lockdep_assert_held(&vma->vm->mutex);
 
 	force_unbind(vma);
-	release_references(vma);
+	list_del_init(&vma->vm_link);
+	release_references(vma, false);
 }
 
 void i915_vma_destroy(struct i915_vma *vma)
 {
+	bool vm_ddestroy;
+
 	mutex_lock(&vma->vm->mutex);
 	force_unbind(vma);
+	list_del_init(&vma->vm_link);
+	vm_ddestroy = vma->vm_ddestroy;
+	vma->vm_ddestroy = false;
 	mutex_unlock(&vma->vm->mutex);
-	release_references(vma);
+	release_references(vma, vm_ddestroy);
 }
 
 void i915_vma_parked(struct intel_gt *gt)
@@ -1711,7 +1734,7 @@ void i915_vma_parked(struct intel_gt *gt)
 		if (!kref_get_unless_zero(&obj->base.refcount))
 			continue;
 
-		if (!i915_vm_tryopen(vm)) {
+		if (!i915_vm_tryget(vm)) {
 			i915_gem_object_put(obj);
 			continue;
 		}
@@ -1737,7 +1760,7 @@ void i915_vma_parked(struct intel_gt *gt)
 		}
 
 		i915_gem_object_put(obj);
-		i915_vm_close(vm);
+		i915_vm_put(vm);
 	}
 }
 
@@ -1888,7 +1911,9 @@ struct dma_fence *__i915_vma_evict(struct i915_vma *vma, bool async)
 
 	/* If vm is not open, unbind is a nop. */
 	vma_res->needs_wakeref = i915_vma_is_bound(vma, I915_VMA_GLOBAL_BIND) &&
-		atomic_read(&vma->vm->open);
+		kref_read(&vma->vm->ref);
+	vma_res->skip_pte_rewrite = !kref_read(&vma->vm->ref) ||
+		vma->vm->skip_pte_rewrite;
 	trace_i915_vma_unbind(vma);
 
 	unbind_fence = i915_vma_resource_unbind(vma_res);