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

5 files changed, 115 insertions, 113 deletions

diff --git a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
index 871fe7bda0e0..1bb766c79dcb 100644
--- a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
+++ b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
@@ -322,7 +322,7 @@ int gen6_ppgtt_pin(struct i915_ppgtt *base, struct i915_gem_ww_ctx *ww)
 	struct gen6_ppgtt *ppgtt = to_gen6_ppgtt(base);
 	int err;
 
-	GEM_BUG_ON(!atomic_read(&ppgtt->base.vm.open));
+	GEM_BUG_ON(!kref_read(&ppgtt->base.vm.ref));
 
 	/*
 	 * Workaround the limited maximum vma->pin_count and the aliasing_ppgtt
diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c
index 8850d4e0f9cc..04191fe2ee34 100644
--- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
@@ -126,7 +126,7 @@ static bool needs_idle_maps(struct drm_i915_private *i915)
 void i915_ggtt_suspend_vm(struct i915_address_space *vm)
 {
 	struct i915_vma *vma, *vn;
-	int open;
+	int save_skip_rewrite;
 
 	drm_WARN_ON(&vm->i915->drm, !vm->is_ggtt && !vm->is_dpt);
 
@@ -135,8 +135,12 @@ retry:
 
 	mutex_lock(&vm->mutex);
 
-	/* Skip rewriting PTE on VMA unbind. */
-	open = atomic_xchg(&vm->open, 0);
+	/*
+	 * Skip rewriting PTE on VMA unbind.
+	 * FIXME: Use an argument to i915_vma_unbind() instead?
+	 */
+	save_skip_rewrite = vm->skip_pte_rewrite;
+	vm->skip_pte_rewrite = true;
 
 	list_for_each_entry_safe(vma, vn, &vm->bound_list, vm_link) {
 		struct drm_i915_gem_object *obj = vma->obj;
@@ -154,16 +158,14 @@ retry:
 			 */
 			i915_gem_object_get(obj);
 
-			atomic_set(&vm->open, open);
 			mutex_unlock(&vm->mutex);
 
 			i915_gem_object_lock(obj, NULL);
-			open = i915_vma_unbind(vma);
+			GEM_WARN_ON(i915_vma_unbind(vma));
 			i915_gem_object_unlock(obj);
-
-			GEM_WARN_ON(open);
-
 			i915_gem_object_put(obj);
+
+			vm->skip_pte_rewrite = save_skip_rewrite;
 			goto retry;
 		}
 
@@ -179,7 +181,7 @@ retry:
 
 	vm->clear_range(vm, 0, vm->total);
 
-	atomic_set(&vm->open, open);
+	vm->skip_pte_rewrite = save_skip_rewrite;
 
 	mutex_unlock(&vm->mutex);
 }
@@ -773,13 +775,13 @@ static void ggtt_cleanup_hw(struct i915_ggtt *ggtt)
 {
 	struct i915_vma *vma, *vn;
 
-	atomic_set(&ggtt->vm.open, 0);
-
 	flush_workqueue(ggtt->vm.i915->wq);
 	i915_gem_drain_freed_objects(ggtt->vm.i915);
 
 	mutex_lock(&ggtt->vm.mutex);
 
+	ggtt->vm.skip_pte_rewrite = true;
+
 	list_for_each_entry_safe(vma, vn, &ggtt->vm.bound_list, vm_link) {
 		struct drm_i915_gem_object *obj = vma->obj;
 		bool trylock;
@@ -1307,16 +1309,12 @@ bool i915_ggtt_resume_vm(struct i915_address_space *vm)
 {
 	struct i915_vma *vma;
 	bool write_domain_objs = false;
-	int open;
 
 	drm_WARN_ON(&vm->i915->drm, !vm->is_ggtt && !vm->is_dpt);
 
 	/* First fill our portion of the GTT with scratch pages */
 	vm->clear_range(vm, 0, vm->total);
 
-	/* Skip rewriting PTE on VMA unbind. */
-	open = atomic_xchg(&vm->open, 0);
-
 	/* clflush objects bound into the GGTT and rebind them. */
 	list_for_each_entry(vma, &vm->bound_list, vm_link) {
 		struct drm_i915_gem_object *obj = vma->obj;
@@ -1333,8 +1331,6 @@ bool i915_ggtt_resume_vm(struct i915_address_space *vm)
 		}
 	}
 
-	atomic_set(&vm->open, open);
-
 	return write_domain_objs;
 }
 
diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c b/drivers/gpu/drm/i915/gt/intel_gtt.c
index 4bcdfcab3642..4f70d512a000 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
@@ -97,32 +97,52 @@ int map_pt_dma_locked(struct i915_address_space *vm, struct drm_i915_gem_object
 	return 0;
 }
 
-void __i915_vm_close(struct i915_address_space *vm)
+static void clear_vm_list(struct list_head *list)
 {
 	struct i915_vma *vma, *vn;
 
-	if (!atomic_dec_and_mutex_lock(&vm->open, &vm->mutex))
-		return;
-
-	list_for_each_entry_safe(vma, vn, &vm->bound_list, vm_link) {
+	list_for_each_entry_safe(vma, vn, list, vm_link) {
 		struct drm_i915_gem_object *obj = vma->obj;
 
-		if (!kref_get_unless_zero(&obj->base.refcount)) {
+		if (!i915_gem_object_get_rcu(obj)) {
 			/*
-			 * Unbind the dying vma to ensure the bound_list
+			 * Object is dying, but has not yet cleared its
+			 * vma list.
+			 * Unbind the dying vma to ensure our list
 			 * is completely drained. We leave the destruction to
-			 * the object destructor.
+			 * the object destructor to avoid the vma
+			 * disappearing under it.
 			 */
 			atomic_and(~I915_VMA_PIN_MASK, &vma->flags);
 			WARN_ON(__i915_vma_unbind(vma));
-			continue;
+
+			/* Remove from the unbound list */
+			list_del_init(&vma->vm_link);
+
+			/*
+			 * Delay the vm and vm mutex freeing until the
+			 * object is done with destruction.
+			 */
+			i915_vm_resv_get(vma->vm);
+			vma->vm_ddestroy = true;
+		} else {
+			i915_vma_destroy_locked(vma);
+			i915_gem_object_put(obj);
 		}
 
-		/* Keep the obj (and hence the vma) alive as _we_ destroy it */
-		i915_vma_destroy_locked(vma);
-		i915_gem_object_put(obj);
 	}
+}
+
+static void __i915_vm_close(struct i915_address_space *vm)
+{
+	mutex_lock(&vm->mutex);
+
+	clear_vm_list(&vm->bound_list);
+	clear_vm_list(&vm->unbound_list);
+
+	/* Check for must-fix unanticipated side-effects */
 	GEM_BUG_ON(!list_empty(&vm->bound_list));
+	GEM_BUG_ON(!list_empty(&vm->unbound_list));
 
 	mutex_unlock(&vm->mutex);
 }
@@ -144,7 +164,6 @@ int i915_vm_lock_objects(struct i915_address_space *vm,
 void i915_address_space_fini(struct i915_address_space *vm)
 {
 	drm_mm_takedown(&vm->mm);
-	mutex_destroy(&vm->mutex);
 }
 
 /**
@@ -152,7 +171,8 @@ void i915_address_space_fini(struct i915_address_space *vm)
  * @kref: Pointer to the &i915_address_space.resv_ref member.
  *
  * This function is called when the last lock sharer no longer shares the
- * &i915_address_space._resv lock.
+ * &i915_address_space._resv lock, and also if we raced when
+ * destroying a vma by the vma destruction
  */
 void i915_vm_resv_release(struct kref *kref)
 {
@@ -160,6 +180,8 @@ void i915_vm_resv_release(struct kref *kref)
 		container_of(kref, typeof(*vm), resv_ref);
 
 	dma_resv_fini(&vm->_resv);
+	mutex_destroy(&vm->mutex);
+
 	kfree(vm);
 }
 
@@ -168,6 +190,8 @@ static void __i915_vm_release(struct work_struct *work)
 	struct i915_address_space *vm =
 		container_of(work, struct i915_address_space, release_work);
 
+	__i915_vm_close(vm);
+
 	/* Synchronize async unbinds. */
 	i915_vma_resource_bind_dep_sync_all(vm);
 
@@ -201,7 +225,6 @@ void i915_address_space_init(struct i915_address_space *vm, int subclass)
 
 	vm->pending_unbind = RB_ROOT_CACHED;
 	INIT_WORK(&vm->release_work, __i915_vm_release);
-	atomic_set(&vm->open, 1);
 
 	/*
 	 * The vm->mutex must be reclaim safe (for use in the shrinker).
@@ -245,6 +268,7 @@ void i915_address_space_init(struct i915_address_space *vm, int subclass)
 	vm->mm.head_node.color = I915_COLOR_UNEVICTABLE;
 
 	INIT_LIST_HEAD(&vm->bound_list);
+	INIT_LIST_HEAD(&vm->unbound_list);
 }
 
 void *__px_vaddr(struct drm_i915_gem_object *p)
diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h b/drivers/gpu/drm/i915/gt/intel_gtt.h
index 9d83c2d3959c..4529b5e9f6e6 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.h
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
@@ -240,15 +240,6 @@ struct i915_address_space {
 
 	unsigned int bind_async_flags;
 
-	/*
-	 * Each active user context has its own address space (in full-ppgtt).
-	 * Since the vm may be shared between multiple contexts, we count how
-	 * many contexts keep us "open". Once open hits zero, we are closed
-	 * and do not allow any new attachments, and proceed to shutdown our
-	 * vma and page directories.
-	 */
-	atomic_t open;
-
 	struct mutex mutex; /* protects vma and our lists */
 
 	struct kref resv_ref; /* kref to keep the reservation lock alive. */
@@ -263,6 +254,11 @@ struct i915_address_space {
 	 */
 	struct list_head bound_list;
 
+	/**
+	 * List of vmas not yet bound or evicted.
+	 */
+	struct list_head unbound_list;
+
 	/* Global GTT */
 	bool is_ggtt:1;
 
@@ -272,6 +268,9 @@ struct i915_address_space {
 	/* Some systems support read-only mappings for GGTT and/or PPGTT */
 	bool has_read_only:1;
 
+	/* Skip pte rewrite on unbind for suspend. Protected by @mutex */
+	bool skip_pte_rewrite:1;
+
 	u8 top;
 	u8 pd_shift;
 	u8 scratch_order;
@@ -446,6 +445,17 @@ i915_vm_get(struct i915_address_space *vm)
 	return vm;
 }
 
+static inline struct i915_address_space *
+i915_vm_tryget(struct i915_address_space *vm)
+{
+	return kref_get_unless_zero(&vm->ref) ? vm : NULL;
+}
+
+static inline void assert_vm_alive(struct i915_address_space *vm)
+{
+	GEM_BUG_ON(!kref_read(&vm->ref));
+}
+
 /**
  * i915_vm_resv_get - Obtain a reference on the vm's reservation lock
  * @vm: The vm whose reservation lock we want to share.
@@ -476,34 +486,6 @@ static inline void i915_vm_resv_put(struct i915_address_space *vm)
 	kref_put(&vm->resv_ref, i915_vm_resv_release);
 }
 
-static inline struct i915_address_space *
-i915_vm_open(struct i915_address_space *vm)
-{
-	GEM_BUG_ON(!atomic_read(&vm->open));
-	atomic_inc(&vm->open);
-	return i915_vm_get(vm);
-}
-
-static inline bool
-i915_vm_tryopen(struct i915_address_space *vm)
-{
-	if (atomic_add_unless(&vm->open, 1, 0))
-		return i915_vm_get(vm);
-
-	return false;
-}
-
-void __i915_vm_close(struct i915_address_space *vm);
-
-static inline void
-i915_vm_close(struct i915_address_space *vm)
-{
-	GEM_BUG_ON(!atomic_read(&vm->open));
-	__i915_vm_close(vm);
-
-	i915_vm_put(vm);
-}
-
 void i915_address_space_init(struct i915_address_space *vm, int subclass);
 void i915_address_space_fini(struct i915_address_space *vm);
 
diff --git a/drivers/gpu/drm/i915/gt/selftest_execlists.c b/drivers/gpu/drm/i915/gt/selftest_execlists.c
index 72d5faab8f9a..09f8cd2d0e2c 100644
--- a/drivers/gpu/drm/i915/gt/selftest_execlists.c
+++ b/drivers/gpu/drm/i915/gt/selftest_execlists.c
@@ -1736,15 +1736,9 @@ static int live_preempt(void *arg)
 	enum intel_engine_id id;
 	int err = -ENOMEM;
 
-	if (igt_spinner_init(&spin_hi, gt))
-		return -ENOMEM;
-
-	if (igt_spinner_init(&spin_lo, gt))
-		goto err_spin_hi;
-
 	ctx_hi = kernel_context(gt->i915, NULL);
 	if (!ctx_hi)
-		goto err_spin_lo;
+		return -ENOMEM;
 	ctx_hi->sched.priority = I915_CONTEXT_MAX_USER_PRIORITY;
 
 	ctx_lo = kernel_context(gt->i915, NULL);
@@ -1752,6 +1746,12 @@ static int live_preempt(void *arg)
 		goto err_ctx_hi;
 	ctx_lo->sched.priority = I915_CONTEXT_MIN_USER_PRIORITY;
 
+	if (igt_spinner_init(&spin_hi, gt))
+		goto err_ctx_lo;
+
+	if (igt_spinner_init(&spin_lo, gt))
+		goto err_spin_hi;
+
 	for_each_engine(engine, gt, id) {
 		struct igt_live_test t;
 		struct i915_request *rq;
@@ -1761,14 +1761,14 @@ static int live_preempt(void *arg)
 
 		if (igt_live_test_begin(&t, gt->i915, __func__, engine->name)) {
 			err = -EIO;
-			goto err_ctx_lo;
+			goto err_spin_lo;
 		}
 
 		rq = spinner_create_request(&spin_lo, ctx_lo, engine,
 					    MI_ARB_CHECK);
 		if (IS_ERR(rq)) {
 			err = PTR_ERR(rq);
-			goto err_ctx_lo;
+			goto err_spin_lo;
 		}
 
 		i915_request_add(rq);
@@ -1777,7 +1777,7 @@ static int live_preempt(void *arg)
 			GEM_TRACE_DUMP();
 			intel_gt_set_wedged(gt);
 			err = -EIO;
-			goto err_ctx_lo;
+			goto err_spin_lo;
 		}
 
 		rq = spinner_create_request(&spin_hi, ctx_hi, engine,
@@ -1785,7 +1785,7 @@ static int live_preempt(void *arg)
 		if (IS_ERR(rq)) {
 			igt_spinner_end(&spin_lo);
 			err = PTR_ERR(rq);
-			goto err_ctx_lo;
+			goto err_spin_lo;
 		}
 
 		i915_request_add(rq);
@@ -1794,7 +1794,7 @@ static int live_preempt(void *arg)
 			GEM_TRACE_DUMP();
 			intel_gt_set_wedged(gt);
 			err = -EIO;
-			goto err_ctx_lo;
+			goto err_spin_lo;
 		}
 
 		igt_spinner_end(&spin_hi);
@@ -1802,19 +1802,19 @@ static int live_preempt(void *arg)
 
 		if (igt_live_test_end(&t)) {
 			err = -EIO;
-			goto err_ctx_lo;
+			goto err_spin_lo;
 		}
 	}
 
 	err = 0;
-err_ctx_lo:
-	kernel_context_close(ctx_lo);
-err_ctx_hi:
-	kernel_context_close(ctx_hi);
 err_spin_lo:
 	igt_spinner_fini(&spin_lo);
 err_spin_hi:
 	igt_spinner_fini(&spin_hi);
+err_ctx_lo:
+	kernel_context_close(ctx_lo);
+err_ctx_hi:
+	kernel_context_close(ctx_hi);
 	return err;
 }
 
@@ -1828,20 +1828,20 @@ static int live_late_preempt(void *arg)
 	enum intel_engine_id id;
 	int err = -ENOMEM;
 
-	if (igt_spinner_init(&spin_hi, gt))
-		return -ENOMEM;
-
-	if (igt_spinner_init(&spin_lo, gt))
-		goto err_spin_hi;
-
 	ctx_hi = kernel_context(gt->i915, NULL);
 	if (!ctx_hi)
-		goto err_spin_lo;
+		return -ENOMEM;
 
 	ctx_lo = kernel_context(gt->i915, NULL);
 	if (!ctx_lo)
 		goto err_ctx_hi;
 
+	if (igt_spinner_init(&spin_hi, gt))
+		goto err_ctx_lo;
+
+	if (igt_spinner_init(&spin_lo, gt))
+		goto err_spin_hi;
+
 	/* Make sure ctx_lo stays before ctx_hi until we trigger preemption. */
 	ctx_lo->sched.priority = 1;
 
@@ -1854,14 +1854,14 @@ static int live_late_preempt(void *arg)
 
 		if (igt_live_test_begin(&t, gt->i915, __func__, engine->name)) {
 			err = -EIO;
-			goto err_ctx_lo;
+			goto err_spin_lo;
 		}
 
 		rq = spinner_create_request(&spin_lo, ctx_lo, engine,
 					    MI_ARB_CHECK);
 		if (IS_ERR(rq)) {
 			err = PTR_ERR(rq);
-			goto err_ctx_lo;
+			goto err_spin_lo;
 		}
 
 		i915_request_add(rq);
@@ -1875,7 +1875,7 @@ static int live_late_preempt(void *arg)
 		if (IS_ERR(rq)) {
 			igt_spinner_end(&spin_lo);
 			err = PTR_ERR(rq);
-			goto err_ctx_lo;
+			goto err_spin_lo;
 		}
 
 		i915_request_add(rq);
@@ -1898,19 +1898,19 @@ static int live_late_preempt(void *arg)
 
 		if (igt_live_test_end(&t)) {
 			err = -EIO;
-			goto err_ctx_lo;
+			goto err_spin_lo;
 		}
 	}
 
 	err = 0;
-err_ctx_lo:
-	kernel_context_close(ctx_lo);
-err_ctx_hi:
-	kernel_context_close(ctx_hi);
 err_spin_lo:
 	igt_spinner_fini(&spin_lo);
 err_spin_hi:
 	igt_spinner_fini(&spin_hi);
+err_ctx_lo:
+	kernel_context_close(ctx_lo);
+err_ctx_hi:
+	kernel_context_close(ctx_hi);
 	return err;
 
 err_wedged:
@@ -1918,7 +1918,7 @@ err_wedged:
 	igt_spinner_end(&spin_lo);
 	intel_gt_set_wedged(gt);
 	err = -EIO;
-	goto err_ctx_lo;
+	goto err_spin_lo;
 }
 
 struct preempt_client {
@@ -3382,12 +3382,9 @@ static int live_preempt_timeout(void *arg)
 	if (!intel_has_reset_engine(gt))
 		return 0;
 
-	if (igt_spinner_init(&spin_lo, gt))
-		return -ENOMEM;
-
 	ctx_hi = kernel_context(gt->i915, NULL);
 	if (!ctx_hi)
-		goto err_spin_lo;
+		return -ENOMEM;
 	ctx_hi->sched.priority = I915_CONTEXT_MAX_USER_PRIORITY;
 
 	ctx_lo = kernel_context(gt->i915, NULL);
@@ -3395,6 +3392,9 @@ static int live_preempt_timeout(void *arg)
 		goto err_ctx_hi;
 	ctx_lo->sched.priority = I915_CONTEXT_MIN_USER_PRIORITY;
 
+	if (igt_spinner_init(&spin_lo, gt))
+		goto err_ctx_lo;
+
 	for_each_engine(engine, gt, id) {
 		unsigned long saved_timeout;
 		struct i915_request *rq;
@@ -3406,21 +3406,21 @@ static int live_preempt_timeout(void *arg)
 					    MI_NOOP); /* preemption disabled */
 		if (IS_ERR(rq)) {
 			err = PTR_ERR(rq);
-			goto err_ctx_lo;
+			goto err_spin_lo;
 		}
 
 		i915_request_add(rq);
 		if (!igt_wait_for_spinner(&spin_lo, rq)) {
 			intel_gt_set_wedged(gt);
 			err = -EIO;
-			goto err_ctx_lo;
+			goto err_spin_lo;
 		}
 
 		rq = igt_request_alloc(ctx_hi, engine);
 		if (IS_ERR(rq)) {
 			igt_spinner_end(&spin_lo);
 			err = PTR_ERR(rq);
-			goto err_ctx_lo;
+			goto err_spin_lo;
 		}
 
 		/* Flush the previous CS ack before changing timeouts */
@@ -3440,7 +3440,7 @@ static int live_preempt_timeout(void *arg)
 			intel_gt_set_wedged(gt);
 			i915_request_put(rq);
 			err = -ETIME;
-			goto err_ctx_lo;
+			goto err_spin_lo;
 		}
 
 		igt_spinner_end(&spin_lo);
@@ -3448,12 +3448,12 @@ static int live_preempt_timeout(void *arg)
 	}
 
 	err = 0;
+err_spin_lo:
+	igt_spinner_fini(&spin_lo);
 err_ctx_lo:
 	kernel_context_close(ctx_lo);
 err_ctx_hi:
 	kernel_context_close(ctx_hi);
-err_spin_lo:
-	igt_spinner_fini(&spin_lo);
 	return err;
 }