[PATCH 1/7] drm/i915/gtt: Defer address space cleanup to an RCU worker

[PATCH 1/7] drm/i915/gtt: Defer address space cleanup to an RCU worker

[Chris Wilson]

Enable RCU protection of i915_address_space and its ppgtt superclasses,
and defer its cleanup into a worker executed after an RCU grace period.

In the future we will be able to use the RCU protection to reduce the
locking around VM lookups, but the immediate benefit is being able to
defer the release into a kworker (process context). This is required as
we may need to sleep to reap the WC pages stashed away inside the ppgtt.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110934
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/Makefile.header-test     |   1 +
 drivers/gpu/drm/i915/i915_gem_gtt.c           | 109 ++++++++++--------
 drivers/gpu/drm/i915/i915_gem_gtt.h           |   5 +
 drivers/gpu/drm/i915/selftests/i915_gem_gtt.c |   2 -
 4 files changed, 66 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/i915/Makefile.header-test b/drivers/gpu/drm/i915/Makefile.header-test
index e6ba66f787f9..cb74242f9c3b 100644
--- a/drivers/gpu/drm/i915/Makefile.header-test
+++ b/drivers/gpu/drm/i915/Makefile.header-test
@@ -6,6 +6,7 @@ header_test := \
 	i915_active_types.h \
 	i915_debugfs.h \
 	i915_drv.h \
+	i915_gem_gtt.h \
 	i915_irq.h \
 	i915_params.h \
 	i915_priolist_types.h \
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 8ab820145ea6..d9eddbd79670 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -482,9 +482,69 @@ static void vm_free_page(struct i915_address_space *vm, struct page *page)
 	spin_unlock(&vm->free_pages.lock);
 }
 
+static void i915_address_space_fini(struct i915_address_space *vm)
+{
+	spin_lock(&vm->free_pages.lock);
+	if (pagevec_count(&vm->free_pages.pvec))
+		vm_free_pages_release(vm, true);
+	GEM_BUG_ON(pagevec_count(&vm->free_pages.pvec));
+	spin_unlock(&vm->free_pages.lock);
+
+	drm_mm_takedown(&vm->mm);
+
+	mutex_destroy(&vm->mutex);
+}
+
+static void ppgtt_destroy_vma(struct i915_address_space *vm)
+{
+	struct list_head *phases[] = {
+		&vm->bound_list,
+		&vm->unbound_list,
+		NULL,
+	}, **phase;
+
+	mutex_lock(&vm->i915->drm.struct_mutex);
+	for (phase = phases; *phase; phase++) {
+		struct i915_vma *vma, *vn;
+
+		list_for_each_entry_safe(vma, vn, *phase, vm_link)
+			i915_vma_destroy(vma);
+	}
+	mutex_unlock(&vm->i915->drm.struct_mutex);
+}
+
+static void __i915_vm_release(struct work_struct *work)
+{
+	struct i915_address_space *vm =
+		container_of(work, struct i915_address_space, rcu.work);
+
+	ppgtt_destroy_vma(vm);
+
+	GEM_BUG_ON(!list_empty(&vm->bound_list));
+	GEM_BUG_ON(!list_empty(&vm->unbound_list));
+
+	vm->cleanup(vm);
+	i915_address_space_fini(vm);
+
+	kfree(vm);
+}
+
+void i915_vm_release(struct kref *kref)
+{
+	struct i915_address_space *vm =
+		container_of(kref, struct i915_address_space, ref);
+
+	GEM_BUG_ON(i915_is_ggtt(vm));
+	trace_i915_ppgtt_release(vm);
+
+	vm->closed = true;
+	queue_rcu_work(system_unbound_wq, &vm->rcu);
+}
+
 static void i915_address_space_init(struct i915_address_space *vm, int subclass)
 {
 	kref_init(&vm->ref);
+	INIT_RCU_WORK(&vm->rcu, __i915_vm_release);
 
 	/*
 	 * The vm->mutex must be reclaim safe (for use in the shrinker).
@@ -505,19 +565,6 @@ static void i915_address_space_init(struct i915_address_space *vm, int subclass)
 	INIT_LIST_HEAD(&vm->bound_list);
 }
 
-static void i915_address_space_fini(struct i915_address_space *vm)
-{
-	spin_lock(&vm->free_pages.lock);
-	if (pagevec_count(&vm->free_pages.pvec))
-		vm_free_pages_release(vm, true);
-	GEM_BUG_ON(pagevec_count(&vm->free_pages.pvec));
-	spin_unlock(&vm->free_pages.lock);
-
-	drm_mm_takedown(&vm->mm);
-
-	mutex_destroy(&vm->mutex);
-}
-
 static int __setup_page_dma(struct i915_address_space *vm,
 			    struct i915_page_dma *p,
 			    gfp_t gfp)
@@ -2250,42 +2297,6 @@ i915_ppgtt_create(struct drm_i915_private *i915)
 	return ppgtt;
 }
 
-static void ppgtt_destroy_vma(struct i915_address_space *vm)
-{
-	struct list_head *phases[] = {
-		&vm->bound_list,
-		&vm->unbound_list,
-		NULL,
-	}, **phase;
-
-	vm->closed = true;
-	for (phase = phases; *phase; phase++) {
-		struct i915_vma *vma, *vn;
-
-		list_for_each_entry_safe(vma, vn, *phase, vm_link)
-			i915_vma_destroy(vma);
-	}
-}
-
-void i915_vm_release(struct kref *kref)
-{
-	struct i915_address_space *vm =
-		container_of(kref, struct i915_address_space, ref);
-
-	GEM_BUG_ON(i915_is_ggtt(vm));
-	trace_i915_ppgtt_release(vm);
-
-	ppgtt_destroy_vma(vm);
-
-	GEM_BUG_ON(!list_empty(&vm->bound_list));
-	GEM_BUG_ON(!list_empty(&vm->unbound_list));
-
-	vm->cleanup(vm);
-	i915_address_space_fini(vm);
-
-	kfree(vm);
-}
-
 /* Certain Gen5 chipsets require require idling the GPU before
  * unmapping anything from the GTT when VT-d is enabled.
  */
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 812717ccc69b..8de57f67a911 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -35,8 +35,12 @@
 #define __I915_GEM_GTT_H__
 
 #include <linux/io-mapping.h>
+#include <linux/kref.h>
 #include <linux/mm.h>
 #include <linux/pagevec.h>
+#include <linux/workqueue.h>
+
+#include <drm/drm_mm.h>
 
 #include "gt/intel_reset.h"
 #include "i915_gem_fence_reg.h"
@@ -280,6 +284,7 @@ struct pagestash {
 
 struct i915_address_space {
 	struct kref ref;
+	struct rcu_work rcu;
 
 	struct drm_mm mm;
 	struct drm_i915_private *i915;
diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
index 1a60b9fe8221..0c47276ed5df 100644
--- a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
@@ -208,9 +208,7 @@ static int igt_ppgtt_alloc(void *arg)
 	}
 
 err_ppgtt_cleanup:
-	mutex_lock(&dev_priv->drm.struct_mutex);
 	i915_vm_put(&ppgtt->vm);
-	mutex_unlock(&dev_priv->drm.struct_mutex);
 	return err;
 }
 
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/7] drm/i915: Track i915_active using debugobjects

[Chris Wilson]

Provide runtime asserts and tracking of i915_active via debugobjects.
For example, this should allow us to check that the i915_active is only
active when we expect it to be and is never freed too early.

One consequence is that, for simplicity, we no longer allow i915_active
to be on-stack which only affected the selftests.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_active.c           | 80 +++++++++++++++++++-
 drivers/gpu/drm/i915/selftests/i915_active.c | 78 ++++++++++++++-----
 2 files changed, 137 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
index 293e5bcc4b6c..807ea146e1e6 100644
--- a/drivers/gpu/drm/i915/i915_active.c
+++ b/drivers/gpu/drm/i915/i915_active.c
@@ -4,6 +4,8 @@
  * Copyright © 2019 Intel Corporation
  */
 
+#include <linux/debugobjects.h>
+
 #include "gt/intel_engine_pm.h"
 
 #include "i915_drv.h"
@@ -31,6 +33,69 @@ struct active_node {
 	u64 timeline;
 };
 
+#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM) && IS_ENABLED(CONFIG_DEBUG_OBJECTS)
+
+static void *active_debug_hint(void *addr)
+{
+	struct i915_active *ref = addr;
+
+	return (void *)ref->retire ?: (void *)ref;
+}
+
+static struct debug_obj_descr active_debug_desc = {
+	.name = "i915_active",
+	.debug_hint = active_debug_hint,
+};
+
+static void debug_active_init(struct i915_active *ref)
+{
+	debug_object_init(ref, &active_debug_desc);
+}
+
+static void debug_active_activate(struct i915_active *ref)
+{
+	debug_object_activate(ref, &active_debug_desc);
+}
+
+static void debug_active_deactivate(struct i915_active *ref)
+{
+	debug_object_deactivate(ref, &active_debug_desc);
+}
+
+static void debug_active_fini(struct i915_active *ref)
+{
+	debug_object_free(ref, &active_debug_desc);
+}
+
+static void debug_active_assert(struct i915_active *ref)
+{
+	debug_object_assert_init(ref, &active_debug_desc);
+}
+
+#else
+
+static void debug_active_init(struct i915_active *ref)
+{
+}
+
+static void debug_active_activate(struct i915_active *ref)
+{
+}
+
+static void debug_active_deactivate(struct i915_active *ref)
+{
+}
+
+static void debug_active_fini(struct i915_active *ref)
+{
+}
+
+static void debug_active_assert(struct i915_active *ref)
+{
+}
+
+#endif
+
 static void
 __active_park(struct i915_active *ref)
 {
@@ -50,6 +115,8 @@ __active_retire(struct i915_active *ref)
 	if (--ref->count)
 		return;
 
+	debug_active_deactivate(ref);
+
 	/* return the unused nodes to our slabcache */
 	__active_park(ref);
 
@@ -155,6 +222,8 @@ void i915_active_init(struct drm_i915_private *i915,
 		      struct i915_active *ref,
 		      void (*retire)(struct i915_active *ref))
 {
+	debug_active_init(ref);
+
 	ref->i915 = i915;
 	ref->retire = retire;
 	ref->tree = RB_ROOT;
@@ -191,13 +260,21 @@ int i915_active_ref(struct i915_active *ref,
 
 bool i915_active_acquire(struct i915_active *ref)
 {
+	debug_active_assert(ref);
 	lockdep_assert_held(BKL(ref));
-	return !ref->count++;
+
+	if (ref->count++)
+		return false;
+
+	debug_active_activate(ref);
+	return true;
 }
 
 void i915_active_release(struct i915_active *ref)
 {
+	debug_active_assert(ref);
 	lockdep_assert_held(BKL(ref));
+
 	__active_retire(ref);
 }
 
@@ -260,6 +337,7 @@ int i915_request_await_active(struct i915_request *rq, struct i915_active *ref)
 #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
 void i915_active_fini(struct i915_active *ref)
 {
+	debug_active_fini(ref);
 	GEM_BUG_ON(i915_active_request_isset(&ref->last));
 	GEM_BUG_ON(!RB_EMPTY_ROOT(&ref->tree));
 	GEM_BUG_ON(ref->count);
diff --git a/drivers/gpu/drm/i915/selftests/i915_active.c b/drivers/gpu/drm/i915/selftests/i915_active.c
index c0b3537a5fa6..98493bcc91f2 100644
--- a/drivers/gpu/drm/i915/selftests/i915_active.c
+++ b/drivers/gpu/drm/i915/selftests/i915_active.c
@@ -16,28 +16,51 @@ struct live_active {
 	bool retired;
 };
 
-static void __live_active_retire(struct i915_active *base)
+static void __live_free(struct live_active *active)
+{
+	i915_active_fini(&active->base);
+	kfree(active);
+}
+
+static void __live_retire(struct i915_active *base)
 {
 	struct live_active *active = container_of(base, typeof(*active), base);
 
 	active->retired = true;
 }
 
-static int __live_active_setup(struct drm_i915_private *i915,
-			       struct live_active *active)
+static struct live_active *__live_alloc(struct drm_i915_private *i915)
+{
+	struct live_active *active;
+
+	active = kzalloc(sizeof(*active), GFP_KERNEL);
+	if (!active)
+		return NULL;
+
+	i915_active_init(i915, &active->base, __live_retire);
+
+	return active;
+}
+
+static struct live_active *
+__live_active_setup(struct drm_i915_private *i915)
 {
 	struct intel_engine_cs *engine;
 	struct i915_sw_fence *submit;
+	struct live_active *active;
 	enum intel_engine_id id;
 	unsigned int count = 0;
 	int err = 0;
 
-	submit = heap_fence_create(GFP_KERNEL);
-	if (!submit)
-		return -ENOMEM;
+	active = __live_alloc(i915);
+	if (!active)
+		return ERR_PTR(-ENOMEM);
 
-	i915_active_init(i915, &active->base, __live_active_retire);
-	active->retired = false;
+	submit = heap_fence_create(GFP_KERNEL);
+	if (!submit) {
+		kfree(active);
+		return ERR_PTR(-ENOMEM);
+	}
 
 	if (!i915_active_acquire(&active->base)) {
 		pr_err("First i915_active_acquire should report being idle\n");
@@ -84,64 +107,79 @@ static int __live_active_setup(struct drm_i915_private *i915,
 	i915_sw_fence_commit(submit);
 	heap_fence_put(submit);
 
-	return err;
+	/* XXX leaks live_active on error */
+	return err ? ERR_PTR(err) : active;
 }
 
 static int live_active_wait(void *arg)
 {
 	struct drm_i915_private *i915 = arg;
-	struct live_active active;
+	struct live_active *active;
 	intel_wakeref_t wakeref;
-	int err;
+	int err = 0;
 
 	/* Check that we get a callback when requests retire upon waiting */
 
 	mutex_lock(&i915->drm.struct_mutex);
 	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
 
-	err = __live_active_setup(i915, &active);
+	active = __live_active_setup(i915);
+	if (IS_ERR(active)) {
+		err = PTR_ERR(active);
+		goto err;
+	}
 
-	i915_active_wait(&active.base);
-	if (!active.retired) {
+	i915_active_wait(&active->base);
+	if (!active->retired) {
 		pr_err("i915_active not retired after waiting!\n");
 		err = -EINVAL;
 	}
 
-	i915_active_fini(&active.base);
+	__live_free(active);
+
 	if (igt_flush_test(i915, I915_WAIT_LOCKED))
 		err = -EIO;
 
+err:
 	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
 	mutex_unlock(&i915->drm.struct_mutex);
+
 	return err;
 }
 
 static int live_active_retire(void *arg)
 {
 	struct drm_i915_private *i915 = arg;
-	struct live_active active;
+	struct live_active *active;
 	intel_wakeref_t wakeref;
-	int err;
+	int err = 0;
 
 	/* Check that we get a callback when requests are indirectly retired */
 
 	mutex_lock(&i915->drm.struct_mutex);
 	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
 
-	err = __live_active_setup(i915, &active);
+	active = __live_active_setup(i915);
+	if (IS_ERR(active)) {
+		err = PTR_ERR(active);
+		goto err;
+	}
 
 	/* waits for & retires all requests */
 	if (igt_flush_test(i915, I915_WAIT_LOCKED))
 		err = -EIO;
 
-	if (!active.retired) {
+	if (!active->retired) {
 		pr_err("i915_active not retired after flushing!\n");
 		err = -EINVAL;
 	}
 
-	i915_active_fini(&active.base);
+	__live_free(active);
+
+err:
 	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
 	mutex_unlock(&i915->drm.struct_mutex);
+
 	return err;
 }
 
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/7] drm/i915: Signal fence completion from i915_request_wait

[Chris Wilson]

With the upcoming change to automanaged i915_active, the intent is that
whenever we wait on the set of active fences, they are signaled and
collected.  The requirement is that all successful returns from
i915_request_wait() signal the fence, so fixup the one remaining path
where we may return before the interrupt has been run.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_request.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 0c2b53b8a3d1..8d59c35bd22f 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -1451,8 +1451,10 @@ long i915_request_wait(struct i915_request *rq,
 	for (;;) {
 		set_current_state(state);
 
-		if (i915_request_completed(rq))
+		if (i915_request_completed(rq)) {
+			dma_fence_signal(&rq->fence);
 			break;
+		}
 
 		if (signal_pending_state(state, current)) {
 			timeout = -ERESTARTSYS;
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/7] drm/i915: Remove waiting & retiring from shrinker paths

[Chris Wilson]

i915_gem_wait_for_idle() and i915_retire_requests() introduce a
dependency on the timeline->mutex. This is problematic as we want to
later perform allocations underneath i915_active.mutex, forming a link
between the shrinker, the timeline and active mutexes. Nip this cycle in
the bud by removing the acquisition of the timeline mutex (i.e.
retiring) from inside the shrinker.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gem/i915_gem_shrinker.c | 10 ----------
 drivers/gpu/drm/i915/i915_timeline.c         |  3 ---
 2 files changed, 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
index 3a926a8755c6..1bbc690494c7 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
@@ -169,7 +169,6 @@ i915_gem_shrink(struct drm_i915_private *i915,
 	 */
 
 	trace_i915_gem_shrink(i915, target, shrink);
-	i915_retire_requests(i915);
 
 	/*
 	 * Unbinding of objects will require HW access; Let us not wake the
@@ -269,8 +268,6 @@ i915_gem_shrink(struct drm_i915_private *i915,
 	if (shrink & I915_SHRINK_BOUND)
 		intel_runtime_pm_put(&i915->runtime_pm, wakeref);
 
-	i915_retire_requests(i915);
-
 	shrinker_unlock(i915, unlock);
 
 	if (nr_scanned)
@@ -427,12 +424,6 @@ i915_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr
 	if (!shrinker_lock(i915, 0, &unlock))
 		return NOTIFY_DONE;
 
-	/* Force everything onto the inactive lists */
-	if (i915_gem_wait_for_idle(i915,
-				   I915_WAIT_LOCKED,
-				   MAX_SCHEDULE_TIMEOUT))
-		goto out;
-
 	with_intel_runtime_pm(&i915->runtime_pm, wakeref)
 		freed_pages += i915_gem_shrink(i915, -1UL, NULL,
 					       I915_SHRINK_BOUND |
@@ -455,7 +446,6 @@ i915_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr
 	}
 	mutex_unlock(&i915->ggtt.vm.mutex);
 
-out:
 	shrinker_unlock(i915, unlock);
 
 	*(unsigned long *)ptr += freed_pages;
diff --git a/drivers/gpu/drm/i915/i915_timeline.c b/drivers/gpu/drm/i915/i915_timeline.c
index c311ce9c6f9d..f3ee264d7dd5 100644
--- a/drivers/gpu/drm/i915/i915_timeline.c
+++ b/drivers/gpu/drm/i915/i915_timeline.c
@@ -270,9 +270,6 @@ void i915_timelines_init(struct drm_i915_private *i915)
 
 	spin_lock_init(&gt->hwsp_lock);
 	INIT_LIST_HEAD(&gt->hwsp_free_list);
-
-	/* via i915_gem_wait_for_idle() */
-	i915_gem_shrinker_taints_mutex(i915, &gt->mutex);
 }
 
 static void timeline_add_to_active(struct i915_timeline *tl)
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/7] drm/i915: Throw away the active object retirement complexity

[Chris Wilson]

Remove the accumulated optimisations that we have for i915_vma_retire
and reduce it to the bare essential of tracking the active object
reference. This allows us to only use atomic operations, and so will be
able to avoid the struct_mutex requirement.

The principal loss here is the shrinker MRU bumping, so now if we have
to shrink, we will do so in much more random order and more likely to
try and shrink recently used objects. That is a nuisance, but shrinking
active objects is a second step we try to avoid and will always be a
system-wide performance issue.

The other loss is here is in the automatic pruning of the
reservation_object when idling. This is not as large an issue as upon
reservation_object introduction as now adding new fences into the object
replaces already signaled fences, keeping the array compact. But we do
lose the auto-expiration of stale fences and unused arrays. That may be
a noticeable problem for which we need to re-implement autopruning.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_object.c    |  1 -
 drivers/gpu/drm/i915/gem/i915_gem_object.h    |  6 ---
 .../gpu/drm/i915/gem/i915_gem_object_types.h  |  1 -
 drivers/gpu/drm/i915/gem/i915_gem_shrinker.c  |  5 +-
 .../drm/i915/gem/selftests/i915_gem_mman.c    |  9 ----
 drivers/gpu/drm/i915/gt/intel_lrc.c           |  4 +-
 drivers/gpu/drm/i915/gt/intel_ringbuffer.c    |  1 -
 drivers/gpu/drm/i915/gt/selftest_hangcheck.c  | 32 +++++------
 drivers/gpu/drm/i915/i915_debugfs.c           |  8 +--
 drivers/gpu/drm/i915/i915_gem_batch_pool.c    | 42 ++++++---------
 drivers/gpu/drm/i915/i915_vma.c               | 54 ++++---------------
 11 files changed, 47 insertions(+), 116 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index be6caccce0c5..05aead338112 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -160,7 +160,6 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915,
 
 		mutex_lock(&i915->drm.struct_mutex);
 
-		GEM_BUG_ON(i915_gem_object_is_active(obj));
 		list_for_each_entry_safe(vma, vn, &obj->vma.list, obj_link) {
 			GEM_BUG_ON(i915_vma_is_active(vma));
 			vma->flags &= ~I915_VMA_PIN_MASK;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index dfebd5706f16..20754c15412a 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -158,12 +158,6 @@ i915_gem_object_needs_async_cancel(const struct drm_i915_gem_object *obj)
 	return obj->ops->flags & I915_GEM_OBJECT_ASYNC_CANCEL;
 }
 
-static inline bool
-i915_gem_object_is_active(const struct drm_i915_gem_object *obj)
-{
-	return READ_ONCE(obj->active_count);
-}
-
 static inline bool
 i915_gem_object_is_framebuffer(const struct drm_i915_gem_object *obj)
 {
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
index 18bf4f8d6d80..34b51fad02de 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
@@ -154,7 +154,6 @@ struct drm_i915_gem_object {
 
 	/** Count of VMA actually bound by this object */
 	atomic_t bind_count;
-	unsigned int active_count;
 	/** Count of how many global VMA are currently pinned for use by HW */
 	unsigned int pin_global;
 
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
index 1bbc690494c7..d99f1a600b96 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
@@ -229,8 +229,9 @@ i915_gem_shrink(struct drm_i915_private *i915,
 				continue;
 
 			if (!(shrink & I915_SHRINK_ACTIVE) &&
-			    (i915_gem_object_is_active(obj) ||
-			     i915_gem_object_is_framebuffer(obj)))
+			    (i915_gem_object_is_framebuffer(obj) ||
+			     !reservation_object_test_signaled_rcu(obj->base.resv,
+								   true)))
 				continue;
 
 			if (!(shrink & I915_SHRINK_BOUND) &&
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
index 5c81f4b4813a..2053194a8b70 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
@@ -474,15 +474,6 @@ static int igt_mmap_offset_exhaustion(void *arg)
 			pr_err("[loop %d] Failed to busy the object\n", loop);
 			goto err_obj;
 		}
-
-		/* NB we rely on the _active_ reference to access obj now */
-		GEM_BUG_ON(!i915_gem_object_is_active(obj));
-		err = create_mmap_offset(obj);
-		if (err) {
-			pr_err("[loop %d] create_mmap_offset failed with err=%d\n",
-			       loop, err);
-			goto out;
-		}
 	}
 
 out:
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index df58d1d838f7..e2f978cbca0c 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1404,9 +1404,7 @@ static void execlists_submit_request(struct i915_request *request)
 static void __execlists_context_fini(struct intel_context *ce)
 {
 	intel_ring_put(ce->ring);
-
-	GEM_BUG_ON(i915_gem_object_is_active(ce->state->obj));
-	i915_gem_object_put(ce->state->obj);
+	i915_vma_put(ce->state);
 }
 
 static void execlists_context_destroy(struct kref *kref)
diff --git a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
index bd35ad202459..1dee567f30ec 100644
--- a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
@@ -1309,7 +1309,6 @@ void intel_ring_free(struct kref *ref)
 
 static void __ring_context_fini(struct intel_context *ce)
 {
-	GEM_BUG_ON(i915_gem_object_is_active(ce->state->obj));
 	i915_gem_object_put(ce->state->obj);
 }
 
diff --git a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
index b0b2998e56b8..be2190922334 100644
--- a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
+++ b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
@@ -129,33 +129,29 @@ hang_create_request(struct hang *h, struct intel_engine_cs *engine)
 {
 	struct drm_i915_private *i915 = h->i915;
 	struct i915_address_space *vm = h->ctx->vm ?: &i915->ggtt.vm;
+	struct drm_i915_gem_object *obj;
 	struct i915_request *rq = NULL;
 	struct i915_vma *hws, *vma;
 	unsigned int flags;
+	void *vaddr;
 	u32 *batch;
 	int err;
 
-	if (i915_gem_object_is_active(h->obj)) {
-		struct drm_i915_gem_object *obj;
-		void *vaddr;
-
-		obj = i915_gem_object_create_internal(h->i915, PAGE_SIZE);
-		if (IS_ERR(obj))
-			return ERR_CAST(obj);
+	obj = i915_gem_object_create_internal(h->i915, PAGE_SIZE);
+	if (IS_ERR(obj))
+		return ERR_CAST(obj);
 
-		vaddr = i915_gem_object_pin_map(obj,
-						i915_coherent_map_type(h->i915));
-		if (IS_ERR(vaddr)) {
-			i915_gem_object_put(obj);
-			return ERR_CAST(vaddr);
-		}
+	vaddr = i915_gem_object_pin_map(obj, i915_coherent_map_type(h->i915));
+	if (IS_ERR(vaddr)) {
+		i915_gem_object_put(obj);
+		return ERR_CAST(vaddr);
+	}
 
-		i915_gem_object_unpin_map(h->obj);
-		i915_gem_object_put(h->obj);
+	i915_gem_object_unpin_map(h->obj);
+	i915_gem_object_put(h->obj);
 
-		h->obj = obj;
-		h->batch = vaddr;
-	}
+	h->obj = obj;
+	h->batch = vaddr;
 
 	vma = i915_vma_instance(h->obj, vm, NULL);
 	if (IS_ERR(vma))
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 62cf34db9280..eeecdad0e3ca 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -75,11 +75,6 @@ static int i915_capabilities(struct seq_file *m, void *data)
 	return 0;
 }
 
-static char get_active_flag(struct drm_i915_gem_object *obj)
-{
-	return i915_gem_object_is_active(obj) ? '*' : ' ';
-}
-
 static char get_pin_flag(struct drm_i915_gem_object *obj)
 {
 	return obj->pin_global ? 'p' : ' ';
@@ -144,9 +139,8 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
 	unsigned int frontbuffer_bits;
 	int pin_count = 0;
 
-	seq_printf(m, "%pK: %c%c%c%c%c %8zdKiB %02x %02x %s%s%s",
+	seq_printf(m, "%pK: %c%c%c%c %8zdKiB %02x %02x %s%s%s",
 		   &obj->base,
-		   get_active_flag(obj),
 		   get_pin_flag(obj),
 		   get_tiling_flag(obj),
 		   get_global_flag(obj),
diff --git a/drivers/gpu/drm/i915/i915_gem_batch_pool.c b/drivers/gpu/drm/i915/i915_gem_batch_pool.c
index 25a3e4d09a2f..b17f23991253 100644
--- a/drivers/gpu/drm/i915/i915_gem_batch_pool.c
+++ b/drivers/gpu/drm/i915/i915_gem_batch_pool.c
@@ -94,34 +94,26 @@ i915_gem_batch_pool_get(struct i915_gem_batch_pool *pool,
 	list = &pool->cache_list[n];
 
 	list_for_each_entry(obj, list, batch_pool_link) {
+		struct reservation_object *resv = obj->base.resv;
+
 		/* The batches are strictly LRU ordered */
-		if (i915_gem_object_is_active(obj)) {
-			struct reservation_object *resv = obj->base.resv;
-
-			if (!reservation_object_test_signaled_rcu(resv, true))
-				break;
-
-			i915_retire_requests(pool->engine->i915);
-			GEM_BUG_ON(i915_gem_object_is_active(obj));
-
-			/*
-			 * The object is now idle, clear the array of shared
-			 * fences before we add a new request. Although, we
-			 * remain on the same engine, we may be on a different
-			 * timeline and so may continually grow the array,
-			 * trapping a reference to all the old fences, rather
-			 * than replace the existing fence.
-			 */
-			if (rcu_access_pointer(resv->fence)) {
-				reservation_object_lock(resv, NULL);
-				reservation_object_add_excl_fence(resv, NULL);
-				reservation_object_unlock(resv);
-			}
+		if (!reservation_object_test_signaled_rcu(resv, true))
+			break;
+
+		/*
+		 * The object is now idle, clear the array of shared
+		 * fences before we add a new request. Although, we
+		 * remain on the same engine, we may be on a different
+		 * timeline and so may continually grow the array,
+		 * trapping a reference to all the old fences, rather
+		 * than replace the existing fence.
+		 */
+		if (rcu_access_pointer(resv->fence)) {
+			reservation_object_lock(resv, NULL);
+			reservation_object_add_excl_fence(resv, NULL);
+			reservation_object_unlock(resv);
 		}
 
-		GEM_BUG_ON(!reservation_object_test_signaled_rcu(obj->base.resv,
-								 true));
-
 		if (obj->base.size >= size)
 			goto found;
 	}
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index a57729be8312..b60a69235bad 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -77,43 +77,11 @@ static void vma_print_allocator(struct i915_vma *vma, const char *reason)
 
 #endif
 
-static void obj_bump_mru(struct drm_i915_gem_object *obj)
-{
-	struct drm_i915_private *i915 = to_i915(obj->base.dev);
-	unsigned long flags;
-
-	spin_lock_irqsave(&i915->mm.obj_lock, flags);
-	list_move_tail(&obj->mm.link, &i915->mm.shrink_list);
-	spin_unlock_irqrestore(&i915->mm.obj_lock, flags);
-
-	obj->mm.dirty = true; /* be paranoid  */
-}
-
 static void __i915_vma_retire(struct i915_active *ref)
 {
 	struct i915_vma *vma = container_of(ref, typeof(*vma), active);
-	struct drm_i915_gem_object *obj = vma->obj;
-
-	GEM_BUG_ON(!i915_gem_object_is_active(obj));
-	if (--obj->active_count)
-		return;
-
-	/* Prune the shared fence arrays iff completely idle (inc. external) */
-	if (reservation_object_trylock(obj->base.resv)) {
-		if (reservation_object_test_signaled_rcu(obj->base.resv, true))
-			reservation_object_add_excl_fence(obj->base.resv, NULL);
-		reservation_object_unlock(obj->base.resv);
-	}
 
-	/*
-	 * Bump our place on the bound list to keep it roughly in LRU order
-	 * so that we don't steal from recently used but inactive objects
-	 * (unless we are forced to ofc!)
-	 */
-	if (i915_gem_object_is_shrinkable(obj))
-		obj_bump_mru(obj);
-
-	i915_gem_object_put(obj); /* and drop the active reference */
+	i915_vma_put(vma);
 }
 
 static struct i915_vma *
@@ -921,6 +889,7 @@ int i915_vma_move_to_active(struct i915_vma *vma,
 			    unsigned int flags)
 {
 	struct drm_i915_gem_object *obj = vma->obj;
+	int err;
 
 	assert_vma_held(vma);
 	assert_object_held(obj);
@@ -934,17 +903,13 @@ int i915_vma_move_to_active(struct i915_vma *vma,
 	 * add the active reference first and queue for it to be dropped
 	 * *last*.
 	 */
-	if (!vma->active.count && !obj->active_count++)
-		i915_gem_object_get(obj); /* once more for the active ref */
-
-	if (unlikely(i915_active_ref(&vma->active, rq->fence.context, rq))) {
-		if (!vma->active.count && !--obj->active_count)
-			i915_gem_object_put(obj);
-		return -ENOMEM;
-	}
+	if (i915_active_acquire(&vma->active))
+		i915_vma_get(vma);
 
-	GEM_BUG_ON(!i915_vma_is_active(vma));
-	GEM_BUG_ON(!obj->active_count);
+	err = i915_active_ref(&vma->active, rq->fence.context, rq);
+	i915_active_release(&vma->active);
+	if (unlikely(err))
+		return err;
 
 	obj->write_domain = 0;
 	if (flags & EXEC_OBJECT_WRITE) {
@@ -956,11 +921,14 @@ int i915_vma_move_to_active(struct i915_vma *vma,
 		obj->read_domains = 0;
 	}
 	obj->read_domains |= I915_GEM_GPU_DOMAINS;
+	obj->mm.dirty = true;
 
 	if (flags & EXEC_OBJECT_NEEDS_FENCE)
 		__i915_active_request_set(&vma->last_fence, rq);
 
 	export_fence(vma, rq, flags);
+
+	GEM_BUG_ON(!i915_vma_is_active(vma));
 	return 0;
 }
 
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 6/7] drm/i915: Provide an i915_active.acquire callback

[Chris Wilson]

If we introduce a callback for i915_active that is only called the first
time we use the i915_active and is symmetrically paired with the
i915_active.retire callback, we can replace the open-coded and
non-atomic implementations -- which will be very fragile (i.e. broken)
upon removing the struct_mutex serialisation.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_context.c  |   8 +-
 drivers/gpu/drm/i915/gt/intel_context.c      |  83 ++++---
 drivers/gpu/drm/i915/gt/intel_context.h      |  14 +-
 drivers/gpu/drm/i915/gt/intel_lrc.c          |   6 +-
 drivers/gpu/drm/i915/gt/intel_ringbuffer.c   |   2 +-
 drivers/gpu/drm/i915/gt/mock_engine.c        |   2 +-
 drivers/gpu/drm/i915/i915_active.c           | 226 +++++++++----------
 drivers/gpu/drm/i915/i915_active.h           |  25 +-
 drivers/gpu/drm/i915/i915_active_types.h     |  10 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c          |   2 +-
 drivers/gpu/drm/i915/i915_timeline.c         |  16 +-
 drivers/gpu/drm/i915/i915_vma.c              |  22 +-
 drivers/gpu/drm/i915/selftests/i915_active.c |  53 ++++-
 13 files changed, 265 insertions(+), 204 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 0f2c22a3bcb6..9a688e0bf3b2 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -923,8 +923,12 @@ static int context_barrier_task(struct i915_gem_context *ctx,
 	if (!cb)
 		return -ENOMEM;
 
-	i915_active_init(i915, &cb->base, cb_retire);
-	i915_active_acquire(&cb->base);
+	i915_active_init(i915, &cb->base, NULL, cb_retire);
+	err = i915_active_acquire(&cb->base);
+	if (err) {
+		kfree(cb);
+		return err;
+	}
 
 	for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) {
 		struct i915_request *rq;
diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
index 2c454f227c2e..20c708ae6dc0 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -95,11 +95,15 @@ void intel_context_unpin(struct intel_context *ce)
 	intel_context_put(ce);
 }
 
-static int __context_pin_state(struct i915_vma *vma, unsigned long flags)
+static int __context_pin_state(struct i915_vma *vma)
 {
+	u64 flags;
 	int err;
 
-	err = i915_vma_pin(vma, 0, 0, flags | PIN_GLOBAL);
+	flags = i915_ggtt_pin_bias(vma) | PIN_OFFSET_BIAS;
+	flags |= PIN_HIGH | PIN_GLOBAL;
+
+	err = i915_vma_pin(vma, 0, 0, flags);
 	if (err)
 		return err;
 
@@ -119,7 +123,7 @@ static void __context_unpin_state(struct i915_vma *vma)
 	__i915_vma_unpin(vma);
 }
 
-static void intel_context_retire(struct i915_active *active)
+static void __intel_context_retire(struct i915_active *active)
 {
 	struct intel_context *ce = container_of(active, typeof(*ce), active);
 
@@ -129,65 +133,58 @@ static void intel_context_retire(struct i915_active *active)
 	intel_context_put(ce);
 }
 
-void
-intel_context_init(struct intel_context *ce,
-		   struct i915_gem_context *ctx,
-		   struct intel_engine_cs *engine)
-{
-	GEM_BUG_ON(!engine->cops);
-
-	kref_init(&ce->ref);
-
-	ce->gem_context = ctx;
-	ce->engine = engine;
-	ce->ops = engine->cops;
-	ce->sseu = engine->sseu;
-
-	INIT_LIST_HEAD(&ce->signal_link);
-	INIT_LIST_HEAD(&ce->signals);
-
-	mutex_init(&ce->pin_mutex);
-
-	i915_active_init(ctx->i915, &ce->active, intel_context_retire);
-}
-
-int intel_context_active_acquire(struct intel_context *ce, unsigned long flags)
+static int __intel_context_active(struct i915_active *active)
 {
+	struct intel_context *ce = container_of(active, typeof(*ce), active);
 	int err;
 
-	if (!i915_active_acquire(&ce->active))
-		return 0;
-
 	intel_context_get(ce);
 
 	if (!ce->state)
 		return 0;
 
-	err = __context_pin_state(ce->state, flags);
-	if (err) {
-		i915_active_cancel(&ce->active);
-		intel_context_put(ce);
-		return err;
-	}
+	err = __context_pin_state(ce->state);
+	if (err)
+		goto err_put;
 
 	/* Preallocate tracking nodes */
 	if (!i915_gem_context_is_kernel(ce->gem_context)) {
 		err = i915_active_acquire_preallocate_barrier(&ce->active,
 							      ce->engine);
-		if (err) {
-			i915_active_release(&ce->active);
-			return err;
-		}
+		if (err)
+			goto err_unpin;
 	}
 
 	return 0;
+
+err_unpin:
+	__context_unpin_state(ce->state);
+err_put:
+	intel_context_put(ce);
+	return err;
 }
 
-void intel_context_active_release(struct intel_context *ce)
+void
+intel_context_init(struct intel_context *ce,
+		   struct i915_gem_context *ctx,
+		   struct intel_engine_cs *engine)
 {
-	/* Nodes preallocated in intel_context_active() */
-	i915_active_acquire_barrier(&ce->active);
-	i915_active_release(&ce->active);
+	GEM_BUG_ON(!engine->cops);
+
+	kref_init(&ce->ref);
+
+	ce->gem_context = ctx;
+	ce->engine = engine;
+	ce->ops = engine->cops;
+	ce->sseu = engine->sseu;
+
+	INIT_LIST_HEAD(&ce->signal_link);
+	INIT_LIST_HEAD(&ce->signals);
+
+	mutex_init(&ce->pin_mutex);
+
+	i915_active_init(ctx->i915, &ce->active,
+			 __intel_context_active, __intel_context_retire);
 }
 
 static void i915_global_context_shrink(void)
diff --git a/drivers/gpu/drm/i915/gt/intel_context.h b/drivers/gpu/drm/i915/gt/intel_context.h
index a47275bc4f01..40cd8320fcc3 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.h
+++ b/drivers/gpu/drm/i915/gt/intel_context.h
@@ -9,6 +9,7 @@
 
 #include <linux/lockdep.h>
 
+#include "i915_active.h"
 #include "intel_context_types.h"
 #include "intel_engine_types.h"
 
@@ -102,8 +103,17 @@ static inline void intel_context_exit(struct intel_context *ce)
 		ce->ops->exit(ce);
 }
 
-int intel_context_active_acquire(struct intel_context *ce, unsigned long flags);
-void intel_context_active_release(struct intel_context *ce);
+static inline int intel_context_active_acquire(struct intel_context *ce)
+{
+	return i915_active_acquire(&ce->active);
+}
+
+static inline void intel_context_active_release(struct intel_context *ce)
+{
+	/* Nodes preallocated in intel_context_active() */
+	i915_active_acquire_barrier(&ce->active);
+	i915_active_release(&ce->active);
+}
 
 static inline struct intel_context *intel_context_get(struct intel_context *ce)
 {
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index e2f978cbca0c..fa84ad121a08 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1460,12 +1460,10 @@ __execlists_context_pin(struct intel_context *ce,
 		goto err;
 	GEM_BUG_ON(!ce->state);
 
-	ret = intel_context_active_acquire(ce,
-					   engine->i915->ggtt.pin_bias |
-					   PIN_OFFSET_BIAS |
-					   PIN_HIGH);
+	ret = intel_context_active_acquire(ce);
 	if (ret)
 		goto err;
+	GEM_BUG_ON(!i915_vma_is_pinned(ce->state));
 
 	vaddr = i915_gem_object_pin_map(ce->state->obj,
 					i915_coherent_map_type(engine->i915) |
diff --git a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
index 1dee567f30ec..9cbab56ffd7d 100644
--- a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
@@ -1437,7 +1437,7 @@ static int ring_context_pin(struct intel_context *ce)
 		ce->state = vma;
 	}
 
-	err = intel_context_active_acquire(ce, PIN_HIGH);
+	err = intel_context_active_acquire(ce);
 	if (err)
 		return err;
 
diff --git a/drivers/gpu/drm/i915/gt/mock_engine.c b/drivers/gpu/drm/i915/gt/mock_engine.c
index 086801b51441..b9c2764beca3 100644
--- a/drivers/gpu/drm/i915/gt/mock_engine.c
+++ b/drivers/gpu/drm/i915/gt/mock_engine.c
@@ -154,7 +154,7 @@ static int mock_context_pin(struct intel_context *ce)
 			return -ENOMEM;
 	}
 
-	ret = intel_context_active_acquire(ce, PIN_HIGH);
+	ret = intel_context_active_acquire(ce);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
index 807ea146e1e6..6eec9110d749 100644
--- a/drivers/gpu/drm/i915/i915_active.c
+++ b/drivers/gpu/drm/i915/i915_active.c
@@ -39,7 +39,7 @@ static void *active_debug_hint(void *addr)
 {
 	struct i915_active *ref = addr;
 
-	return (void *)ref->retire ?: (void *)ref;
+	return (void *)ref->active ?: (void *)ref->retire ?: (void *)ref;
 }
 
 static struct debug_obj_descr active_debug_desc = {
@@ -97,50 +97,51 @@ static void debug_active_assert(struct i915_active *ref)
 #endif
 
 static void
-__active_park(struct i915_active *ref)
+active_retire(struct i915_active *ref)
 {
 	struct active_node *it, *n;
+	struct rb_root root;
+	bool retire = false;
 
-	rbtree_postorder_for_each_entry_safe(it, n, &ref->tree, node) {
-		GEM_BUG_ON(i915_active_request_isset(&it->base));
-		kmem_cache_free(global.slab_cache, it);
-	}
-	ref->tree = RB_ROOT;
-}
-
-static void
-__active_retire(struct i915_active *ref)
-{
-	GEM_BUG_ON(!ref->count);
-	if (--ref->count)
+	GEM_BUG_ON(!atomic_read(&ref->count));
+	if (atomic_add_unless(&ref->count, -1, 1))
 		return;
 
-	debug_active_deactivate(ref);
+	/* One active may be flushed from inside the acquire of another */
+	mutex_lock_nested(&ref->mutex, SINGLE_DEPTH_NESTING);
+
+	/* return the unused nodes to our slabcache -- flushing the allocator */
+	if (atomic_dec_and_test(&ref->count)) {
+		debug_active_deactivate(ref);
+		root = ref->tree;
+		ref->tree = RB_ROOT;
+		ref->cache = NULL;
+		retire = true;
+	}
 
-	/* return the unused nodes to our slabcache */
-	__active_park(ref);
+	mutex_unlock(&ref->mutex);
+	if (!retire)
+		return;
 
 	ref->retire(ref);
-}
 
-static void
-node_retire(struct i915_active_request *base, struct i915_request *rq)
-{
-	__active_retire(container_of(base, struct active_node, base)->ref);
+	rbtree_postorder_for_each_entry_safe(it, n, &root, node) {
+		GEM_BUG_ON(i915_active_request_isset(&it->base));
+		kmem_cache_free(global.slab_cache, it);
+	}
 }
 
 static void
-last_retire(struct i915_active_request *base, struct i915_request *rq)
+node_retire(struct i915_active_request *base, struct i915_request *rq)
 {
-	__active_retire(container_of(base, struct i915_active, last));
+	active_retire(container_of(base, struct active_node, base)->ref);
 }
 
 static struct i915_active_request *
 active_instance(struct i915_active *ref, u64 idx)
 {
-	struct active_node *node;
+	struct active_node *node, *prealloc;
 	struct rb_node **p, *parent;
-	struct i915_request *old;
 
 	/*
 	 * We track the most recently used timeline to skip a rbtree search
@@ -148,20 +149,18 @@ active_instance(struct i915_active *ref, u64 idx)
 	 * at all. We can reuse the last slot if it is empty, that is
 	 * after the previous activity has been retired, or if it matches the
 	 * current timeline.
-	 *
-	 * Note that we allow the timeline to be active simultaneously in
-	 * the rbtree and the last cache. We do this to avoid having
-	 * to search and replace the rbtree element for a new timeline, with
-	 * the cost being that we must be aware that the ref may be retired
-	 * twice for the same timeline (as the older rbtree element will be
-	 * retired before the new request added to last).
 	 */
-	old = i915_active_request_raw(&ref->last, BKL(ref));
-	if (!old || old->fence.context == idx)
-		goto out;
+	node = READ_ONCE(ref->cache);
+	if (node && node->timeline == idx)
+		return &node->base;
+
+	/* Preallocate a replacement, just in case */
+	prealloc = kmem_cache_alloc(global.slab_cache, GFP_KERNEL);
+	if (!prealloc)
+		return NULL;
 
-	/* Move the currently active fence into the rbtree */
-	idx = old->fence.context;
+	mutex_lock(&ref->mutex);
+	GEM_BUG_ON(i915_active_is_idle(ref));
 
 	parent = NULL;
 	p = &ref->tree.rb_node;
@@ -169,8 +168,10 @@ active_instance(struct i915_active *ref, u64 idx)
 		parent = *p;
 
 		node = rb_entry(parent, struct active_node, node);
-		if (node->timeline == idx)
-			goto replace;
+		if (node->timeline == idx) {
+			kmem_cache_free(global.slab_cache, prealloc);
+			goto out;
+		}
 
 		if (node->timeline < idx)
 			p = &parent->rb_right;
@@ -178,17 +179,7 @@ active_instance(struct i915_active *ref, u64 idx)
 			p = &parent->rb_left;
 	}
 
-	node = kmem_cache_alloc(global.slab_cache, GFP_KERNEL);
-
-	/* kmalloc may retire the ref->last (thanks shrinker)! */
-	if (unlikely(!i915_active_request_raw(&ref->last, BKL(ref)))) {
-		kmem_cache_free(global.slab_cache, node);
-		goto out;
-	}
-
-	if (unlikely(!node))
-		return ERR_PTR(-ENOMEM);
-
+	node = prealloc;
 	i915_active_request_init(&node->base, NULL, node_retire);
 	node->ref = ref;
 	node->timeline = idx;
@@ -196,40 +187,29 @@ active_instance(struct i915_active *ref, u64 idx)
 	rb_link_node(&node->node, parent, p);
 	rb_insert_color(&node->node, &ref->tree);
 
-replace:
-	/*
-	 * Overwrite the previous active slot in the rbtree with last,
-	 * leaving last zeroed. If the previous slot is still active,
-	 * we must be careful as we now only expect to receive one retire
-	 * callback not two, and so much undo the active counting for the
-	 * overwritten slot.
-	 */
-	if (i915_active_request_isset(&node->base)) {
-		/* Retire ourselves from the old rq->active_list */
-		__list_del_entry(&node->base.link);
-		ref->count--;
-		GEM_BUG_ON(!ref->count);
-	}
-	GEM_BUG_ON(list_empty(&ref->last.link));
-	list_replace_init(&ref->last.link, &node->base.link);
-	node->base.request = fetch_and_zero(&ref->last.request);
-
 out:
-	return &ref->last;
+	ref->cache = node;
+	mutex_unlock(&ref->mutex);
+
+	return &node->base;
 }
 
-void i915_active_init(struct drm_i915_private *i915,
-		      struct i915_active *ref,
-		      void (*retire)(struct i915_active *ref))
+void __i915_active_init(struct drm_i915_private *i915,
+			struct i915_active *ref,
+			int (*active)(struct i915_active *ref),
+			void (*retire)(struct i915_active *ref),
+			struct lock_class_key *key)
 {
 	debug_active_init(ref);
 
 	ref->i915 = i915;
+	ref->active = active;
 	ref->retire = retire;
 	ref->tree = RB_ROOT;
-	i915_active_request_init(&ref->last, NULL, last_retire);
+	ref->cache = NULL;
 	init_llist_head(&ref->barriers);
-	ref->count = 0;
+	atomic_set(&ref->count, 0);
+	__mutex_init(&ref->mutex, "i915_active", key);
 }
 
 int i915_active_ref(struct i915_active *ref,
@@ -237,68 +217,84 @@ int i915_active_ref(struct i915_active *ref,
 		    struct i915_request *rq)
 {
 	struct i915_active_request *active;
-	int err = 0;
+	int err;
 
 	/* Prevent reaping in case we malloc/wait while building the tree */
-	i915_active_acquire(ref);
+	err = i915_active_acquire(ref);
+	if (err)
+		return err;
 
 	active = active_instance(ref, timeline);
-	if (IS_ERR(active)) {
-		err = PTR_ERR(active);
+	if (!active) {
+		err = -ENOMEM;
 		goto out;
 	}
 
 	if (!i915_active_request_isset(active))
-		ref->count++;
+		atomic_inc(&ref->count);
 	__i915_active_request_set(active, rq);
 
-	GEM_BUG_ON(!ref->count);
 out:
 	i915_active_release(ref);
 	return err;
 }
 
-bool i915_active_acquire(struct i915_active *ref)
+int i915_active_acquire(struct i915_active *ref)
 {
+	int err;
+
 	debug_active_assert(ref);
-	lockdep_assert_held(BKL(ref));
+	if (atomic_add_unless(&ref->count, 1, 0))
+		return 0;
 
-	if (ref->count++)
-		return false;
+	err = mutex_lock_interruptible(&ref->mutex);
+	if (err)
+		return err;
+
+	if (!atomic_read(&ref->count) && ref->active)
+		err = ref->active(ref);
+	if (!err) {
+		debug_active_activate(ref);
+		atomic_inc(&ref->count);
+	}
 
-	debug_active_activate(ref);
-	return true;
+	mutex_unlock(&ref->mutex);
+
+	return err;
 }
 
 void i915_active_release(struct i915_active *ref)
 {
 	debug_active_assert(ref);
-	lockdep_assert_held(BKL(ref));
-
-	__active_retire(ref);
+	active_retire(ref);
 }
 
 int i915_active_wait(struct i915_active *ref)
 {
 	struct active_node *it, *n;
-	int ret = 0;
+	int err;
 
-	if (i915_active_acquire(ref))
-		goto out_release;
+	might_sleep();
+	if (RB_EMPTY_ROOT(&ref->tree))
+		return 0;
 
-	ret = i915_active_request_retire(&ref->last, BKL(ref));
-	if (ret)
-		goto out_release;
+	err = mutex_lock_interruptible(&ref->mutex);
+	if (err)
+		return err;
+
+	if (!atomic_add_unless(&ref->count, 1, 0))
+		goto unlock;
 
 	rbtree_postorder_for_each_entry_safe(it, n, &ref->tree, node) {
-		ret = i915_active_request_retire(&it->base, BKL(ref));
-		if (ret)
+		err = i915_active_request_retire(&it->base, BKL(ref));
+		if (err)
 			break;
 	}
 
-out_release:
-	i915_active_release(ref);
-	return ret;
+	active_retire(ref);
+unlock:
+	mutex_unlock(&ref->mutex);
+	return err;
 }
 
 int i915_request_await_active_request(struct i915_request *rq,
@@ -313,23 +309,24 @@ int i915_request_await_active_request(struct i915_request *rq,
 int i915_request_await_active(struct i915_request *rq, struct i915_active *ref)
 {
 	struct active_node *it, *n;
-	int err = 0;
+	int err;
 
-	/* await allocates and so we need to avoid hitting the shrinker */
-	if (i915_active_acquire(ref))
-		goto out; /* was idle */
+	if (RB_EMPTY_ROOT(&ref->tree))
+		return 0;
 
-	err = i915_request_await_active_request(rq, &ref->last);
+	/* await allocates and so we need to avoid hitting the shrinker */
+	err = i915_active_acquire(ref);
 	if (err)
-		goto out;
+		return err;
 
+	mutex_lock(&ref->mutex);
 	rbtree_postorder_for_each_entry_safe(it, n, &ref->tree, node) {
 		err = i915_request_await_active_request(rq, &it->base);
 		if (err)
-			goto out;
+			break;
 	}
+	mutex_unlock(&ref->mutex);
 
-out:
 	i915_active_release(ref);
 	return err;
 }
@@ -338,9 +335,9 @@ int i915_request_await_active(struct i915_request *rq, struct i915_active *ref)
 void i915_active_fini(struct i915_active *ref)
 {
 	debug_active_fini(ref);
-	GEM_BUG_ON(i915_active_request_isset(&ref->last));
 	GEM_BUG_ON(!RB_EMPTY_ROOT(&ref->tree));
-	GEM_BUG_ON(ref->count);
+	GEM_BUG_ON(atomic_read(&ref->count));
+	mutex_destroy(&ref->mutex);
 }
 #endif
 
@@ -367,7 +364,7 @@ int i915_active_acquire_preallocate_barrier(struct i915_active *ref,
 					 (void *)engine, node_retire);
 		node->timeline = kctx->ring->timeline->fence_context;
 		node->ref = ref;
-		ref->count++;
+		atomic_inc(&ref->count);
 
 		intel_engine_pm_get(engine);
 		llist_add((struct llist_node *)&node->base.link,
@@ -394,8 +391,9 @@ void i915_active_acquire_barrier(struct i915_active *ref)
 {
 	struct llist_node *pos, *next;
 
-	i915_active_acquire(ref);
+	GEM_BUG_ON(i915_active_is_idle(ref));
 
+	mutex_lock_nested(&ref->mutex, SINGLE_DEPTH_NESTING);
 	llist_for_each_safe(pos, next, llist_del_all(&ref->barriers)) {
 		struct intel_engine_cs *engine;
 		struct active_node *node;
@@ -425,7 +423,7 @@ void i915_active_acquire_barrier(struct i915_active *ref)
 			  &engine->barrier_tasks);
 		intel_engine_pm_put(engine);
 	}
-	i915_active_release(ref);
+	mutex_unlock(&ref->mutex);
 }
 
 void i915_request_add_barriers(struct i915_request *rq)
diff --git a/drivers/gpu/drm/i915/i915_active.h b/drivers/gpu/drm/i915/i915_active.h
index d55d37673944..6fa9263ca407 100644
--- a/drivers/gpu/drm/i915/i915_active.h
+++ b/drivers/gpu/drm/i915/i915_active.h
@@ -369,9 +369,16 @@ i915_active_request_retire(struct i915_active_request *active,
  * synchronisation.
  */
 
-void i915_active_init(struct drm_i915_private *i915,
-		      struct i915_active *ref,
-		      void (*retire)(struct i915_active *ref));
+void __i915_active_init(struct drm_i915_private *i915,
+			struct i915_active *ref,
+			int (*active)(struct i915_active *ref),
+			void (*retire)(struct i915_active *ref),
+			struct lock_class_key *key);
+#define i915_active_init(i915, ref, active, retire) do {		\
+	static struct lock_class_key __key;				\
+									\
+	__i915_active_init(i915, ref, active, retire, &__key);		\
+} while (0)
 
 int i915_active_ref(struct i915_active *ref,
 		    u64 timeline,
@@ -384,20 +391,14 @@ int i915_request_await_active(struct i915_request *rq,
 int i915_request_await_active_request(struct i915_request *rq,
 				      struct i915_active_request *active);
 
-bool i915_active_acquire(struct i915_active *ref);
-
-static inline void i915_active_cancel(struct i915_active *ref)
-{
-	GEM_BUG_ON(ref->count != 1);
-	ref->count = 0;
-}
-
+int i915_active_acquire(struct i915_active *ref);
 void i915_active_release(struct i915_active *ref);
+void __i915_active_release_nested(struct i915_active *ref, int subclass);
 
 static inline bool
 i915_active_is_idle(const struct i915_active *ref)
 {
-	return !ref->count;
+	return !atomic_read(&ref->count);
 }
 
 #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
diff --git a/drivers/gpu/drm/i915/i915_active_types.h b/drivers/gpu/drm/i915/i915_active_types.h
index c025991b9233..5b0a3024ce24 100644
--- a/drivers/gpu/drm/i915/i915_active_types.h
+++ b/drivers/gpu/drm/i915/i915_active_types.h
@@ -7,7 +7,9 @@
 #ifndef _I915_ACTIVE_TYPES_H_
 #define _I915_ACTIVE_TYPES_H_
 
+#include <linux/atomic.h>
 #include <linux/llist.h>
+#include <linux/mutex.h>
 #include <linux/rbtree.h>
 #include <linux/rcupdate.h>
 
@@ -24,13 +26,17 @@ struct i915_active_request {
 	i915_active_retire_fn retire;
 };
 
+struct active_node;
+
 struct i915_active {
 	struct drm_i915_private *i915;
 
+	struct active_node *cache;
 	struct rb_root tree;
-	struct i915_active_request last;
-	unsigned int count;
+	struct mutex mutex;
+	atomic_t count;
 
+	int (*active)(struct i915_active *ref);
 	void (*retire)(struct i915_active *ref);
 
 	struct llist_head barriers;
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index d9eddbd79670..db9068c529fc 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2096,7 +2096,7 @@ static struct i915_vma *pd_vma_create(struct gen6_ppgtt *ppgtt, int size)
 	if (!vma)
 		return ERR_PTR(-ENOMEM);
 
-	i915_active_init(i915, &vma->active, NULL);
+	i915_active_init(i915, &vma->active, NULL, NULL);
 	INIT_ACTIVE_REQUEST(&vma->last_fence);
 
 	vma->vm = &ggtt->vm;
diff --git a/drivers/gpu/drm/i915/i915_timeline.c b/drivers/gpu/drm/i915/i915_timeline.c
index f3ee264d7dd5..3ea1b881a4ae 100644
--- a/drivers/gpu/drm/i915/i915_timeline.c
+++ b/drivers/gpu/drm/i915/i915_timeline.c
@@ -148,6 +148,15 @@ static void __cacheline_retire(struct i915_active *active)
 		__idle_cacheline_free(cl);
 }
 
+static int __cacheline_active(struct i915_active *active)
+{
+	struct i915_timeline_cacheline *cl =
+		container_of(active, typeof(*cl), active);
+
+	__i915_vma_pin(cl->hwsp->vma);
+	return 0;
+}
+
 static struct i915_timeline_cacheline *
 cacheline_alloc(struct i915_timeline_hwsp *hwsp, unsigned int cacheline)
 {
@@ -170,15 +179,16 @@ cacheline_alloc(struct i915_timeline_hwsp *hwsp, unsigned int cacheline)
 	cl->hwsp = hwsp;
 	cl->vaddr = page_pack_bits(vaddr, cacheline);
 
-	i915_active_init(hwsp_to_i915(hwsp), &cl->active, __cacheline_retire);
+	i915_active_init(hwsp_to_i915(hwsp), &cl->active,
+			 __cacheline_active, __cacheline_retire);
 
 	return cl;
 }
 
 static void cacheline_acquire(struct i915_timeline_cacheline *cl)
 {
-	if (cl && i915_active_acquire(&cl->active))
-		__i915_vma_pin(cl->hwsp->vma);
+	if (cl)
+		i915_active_acquire(&cl->active);
 }
 
 static void cacheline_release(struct i915_timeline_cacheline *cl)
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index b60a69235bad..8863b504d996 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -77,11 +77,20 @@ static void vma_print_allocator(struct i915_vma *vma, const char *reason)
 
 #endif
 
-static void __i915_vma_retire(struct i915_active *ref)
+static inline struct i915_vma *active_to_vma(struct i915_active *ref)
 {
-	struct i915_vma *vma = container_of(ref, typeof(*vma), active);
+	return container_of(ref, typeof(struct i915_vma), active);
+}
 
-	i915_vma_put(vma);
+static int __i915_vma_active(struct i915_active *ref)
+{
+	i915_vma_get(active_to_vma(ref));
+	return 0;
+}
+
+static void __i915_vma_retire(struct i915_active *ref)
+{
+	i915_vma_put(active_to_vma(ref));
 }
 
 static struct i915_vma *
@@ -106,7 +115,8 @@ vma_create(struct drm_i915_gem_object *obj,
 	vma->size = obj->base.size;
 	vma->display_alignment = I915_GTT_MIN_ALIGNMENT;
 
-	i915_active_init(vm->i915, &vma->active, __i915_vma_retire);
+	i915_active_init(vm->i915, &vma->active,
+			 __i915_vma_active, __i915_vma_retire);
 	INIT_ACTIVE_REQUEST(&vma->last_fence);
 
 	INIT_LIST_HEAD(&vma->closed_link);
@@ -903,11 +913,7 @@ int i915_vma_move_to_active(struct i915_vma *vma,
 	 * add the active reference first and queue for it to be dropped
 	 * *last*.
 	 */
-	if (i915_active_acquire(&vma->active))
-		i915_vma_get(vma);
-
 	err = i915_active_ref(&vma->active, rq->fence.context, rq);
-	i915_active_release(&vma->active);
 	if (unlikely(err))
 		return err;
 
diff --git a/drivers/gpu/drm/i915/selftests/i915_active.c b/drivers/gpu/drm/i915/selftests/i915_active.c
index 98493bcc91f2..84fce379c0de 100644
--- a/drivers/gpu/drm/i915/selftests/i915_active.c
+++ b/drivers/gpu/drm/i915/selftests/i915_active.c
@@ -4,6 +4,8 @@
  * Copyright © 2018 Intel Corporation
  */
 
+#include <linux/kref.h>
+
 #include "gem/i915_gem_pm.h"
 
 #include "i915_selftest.h"
@@ -13,20 +15,47 @@
 
 struct live_active {
 	struct i915_active base;
+	struct kref ref;
 	bool retired;
 };
 
+static void __live_get(struct live_active *active)
+{
+	kref_get(&active->ref);
+}
+
 static void __live_free(struct live_active *active)
 {
 	i915_active_fini(&active->base);
 	kfree(active);
 }
 
+static void __live_release(struct kref *ref)
+{
+	struct live_active *active = container_of(ref, typeof(*active), ref);
+
+	__live_free(active);
+}
+
+static void __live_put(struct live_active *active)
+{
+	kref_put(&active->ref, __live_release);
+}
+
+static int __live_active(struct i915_active *base)
+{
+	struct live_active *active = container_of(base, typeof(*active), base);
+
+	__live_get(active);
+	return 0;
+}
+
 static void __live_retire(struct i915_active *base)
 {
 	struct live_active *active = container_of(base, typeof(*active), base);
 
 	active->retired = true;
+	__live_put(active);
 }
 
 static struct live_active *__live_alloc(struct drm_i915_private *i915)
@@ -37,7 +66,8 @@ static struct live_active *__live_alloc(struct drm_i915_private *i915)
 	if (!active)
 		return NULL;
 
-	i915_active_init(i915, &active->base, __live_retire);
+	kref_init(&active->ref);
+	i915_active_init(i915, &active->base, __live_active, __live_retire);
 
 	return active;
 }
@@ -62,11 +92,9 @@ __live_active_setup(struct drm_i915_private *i915)
 		return ERR_PTR(-ENOMEM);
 	}
 
-	if (!i915_active_acquire(&active->base)) {
-		pr_err("First i915_active_acquire should report being idle\n");
-		err = -EINVAL;
+	err = i915_active_acquire(&active->base);
+	if (err)
 		goto out;
-	}
 
 	for_each_engine(engine, i915, id) {
 		struct i915_request *rq;
@@ -97,18 +125,21 @@ __live_active_setup(struct drm_i915_private *i915)
 		pr_err("i915_active retired before submission!\n");
 		err = -EINVAL;
 	}
-	if (active->base.count != count) {
+	if (atomic_read(&active->base.count) != count) {
 		pr_err("i915_active not tracking all requests, found %d, expected %d\n",
-		       active->base.count, count);
+		       atomic_read(&active->base.count), count);
 		err = -EINVAL;
 	}
 
 out:
 	i915_sw_fence_commit(submit);
 	heap_fence_put(submit);
+	if (err) {
+		__live_put(active);
+		active = ERR_PTR(err);
+	}
 
-	/* XXX leaks live_active on error */
-	return err ? ERR_PTR(err) : active;
+	return active;
 }
 
 static int live_active_wait(void *arg)
@@ -135,7 +166,7 @@ static int live_active_wait(void *arg)
 		err = -EINVAL;
 	}
 
-	__live_free(active);
+	__live_put(active);
 
 	if (igt_flush_test(i915, I915_WAIT_LOCKED))
 		err = -EIO;
@@ -174,7 +205,7 @@ static int live_active_retire(void *arg)
 		err = -EINVAL;
 	}
 
-	__live_free(active);
+	__live_put(active);
 
 err:
 	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 7/7] drm/i915: Push the i915_active.retire into a worker

[Chris Wilson]

As we need to use a mutex to serialise i915_active activation
(because we want to allow the callback to sleep), we need to push the
i915_active.retire into a worker callback in case we get need to retire
from an atomic context.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_context.c |  1 +
 drivers/gpu/drm/i915/gt/intel_context.c     |  1 +
 drivers/gpu/drm/i915/i915_active.c          | 72 ++++++++++++++++-----
 drivers/gpu/drm/i915/i915_active_types.h    | 12 ++++
 drivers/gpu/drm/i915/i915_timeline.c        |  1 +
 drivers/gpu/drm/i915/i915_vma.c             |  3 +-
 6 files changed, 74 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 9a688e0bf3b2..ba841af166df 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -891,6 +891,7 @@ struct context_barrier_task {
 	void *data;
 };
 
+__i915_active_call
 static void cb_retire(struct i915_active *base)
 {
 	struct context_barrier_task *cb = container_of(base, typeof(*cb), base);
diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
index 20c708ae6dc0..70d3b2568227 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -123,6 +123,7 @@ static void __context_unpin_state(struct i915_vma *vma)
 	__i915_vma_unpin(vma);
 }
 
+__i915_active_call
 static void __intel_context_retire(struct i915_active *active)
 {
 	struct intel_context *ce = container_of(active, typeof(*ce), active);
diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
index 6eec9110d749..b48ac43fc857 100644
--- a/drivers/gpu/drm/i915/i915_active.c
+++ b/drivers/gpu/drm/i915/i915_active.c
@@ -97,18 +97,14 @@ static void debug_active_assert(struct i915_active *ref)
 #endif
 
 static void
-active_retire(struct i915_active *ref)
+__active_retire(struct i915_active *ref)
 {
 	struct active_node *it, *n;
 	struct rb_root root;
 	bool retire = false;
 
-	GEM_BUG_ON(!atomic_read(&ref->count));
-	if (atomic_add_unless(&ref->count, -1, 1))
-		return;
-
-	/* One active may be flushed from inside the acquire of another */
-	mutex_lock_nested(&ref->mutex, SINGLE_DEPTH_NESTING);
+	lockdep_assert_held(&ref->mutex);
+	GEM_BUG_ON(i915_active_is_idle(ref));
 
 	/* return the unused nodes to our slabcache -- flushing the allocator */
 	if (atomic_dec_and_test(&ref->count)) {
@@ -131,6 +127,36 @@ active_retire(struct i915_active *ref)
 	}
 }
 
+static void
+active_work(struct work_struct *wrk)
+{
+	struct i915_active *ref = container_of(wrk, typeof(*ref), work);
+
+	GEM_BUG_ON(!atomic_read(&ref->count));
+	if (atomic_add_unless(&ref->count, -1, 1))
+		return;
+
+	mutex_lock(&ref->mutex);
+	__active_retire(ref);
+}
+
+static void
+active_retire(struct i915_active *ref)
+{
+	GEM_BUG_ON(!atomic_read(&ref->count));
+	if (atomic_add_unless(&ref->count, -1, 1))
+		return;
+
+	/* If we are inside interrupt context (fence signaling), defer */
+	if (ref->flags & I915_ACTIVE_RETIRE_SLEEPS ||
+	    !mutex_trylock(&ref->mutex)) {
+		queue_work(system_unbound_wq, &ref->work);
+		return;
+	}
+
+	__active_retire(ref);
+}
+
 static void
 node_retire(struct i915_active_request *base, struct i915_request *rq)
 {
@@ -200,16 +226,24 @@ void __i915_active_init(struct drm_i915_private *i915,
 			void (*retire)(struct i915_active *ref),
 			struct lock_class_key *key)
 {
+	unsigned long bits;
+
 	debug_active_init(ref);
 
 	ref->i915 = i915;
+
+	ref->flags = 0;
 	ref->active = active;
-	ref->retire = retire;
+	ref->retire = ptr_unpack_bits(retire, &bits, 2);
+	if (bits & I915_ACTIVE_MAY_SLEEP)
+		ref->flags |= I915_ACTIVE_RETIRE_SLEEPS;
+
 	ref->tree = RB_ROOT;
 	ref->cache = NULL;
 	init_llist_head(&ref->barriers);
 	atomic_set(&ref->count, 0);
 	__mutex_init(&ref->mutex, "i915_active", key);
+	INIT_WORK(&ref->work, active_work);
 }
 
 int i915_active_ref(struct i915_active *ref,
@@ -282,8 +316,10 @@ int i915_active_wait(struct i915_active *ref)
 	if (err)
 		return err;
 
-	if (!atomic_add_unless(&ref->count, 1, 0))
-		goto unlock;
+	if (!atomic_add_unless(&ref->count, 1, 0)) {
+		mutex_unlock(&ref->mutex);
+		return 0;
+	}
 
 	rbtree_postorder_for_each_entry_safe(it, n, &ref->tree, node) {
 		err = i915_active_request_retire(&it->base, BKL(ref));
@@ -291,10 +327,15 @@ int i915_active_wait(struct i915_active *ref)
 			break;
 	}
 
-	active_retire(ref);
-unlock:
-	mutex_unlock(&ref->mutex);
-	return err;
+	__active_retire(ref);
+	if (err)
+		return err;
+
+	flush_work(&ref->work);
+	if (!i915_active_is_idle(ref))
+		return -EBUSY;
+
+	return 0;
 }
 
 int i915_request_await_active_request(struct i915_request *rq,
@@ -335,8 +376,9 @@ int i915_request_await_active(struct i915_request *rq, struct i915_active *ref)
 void i915_active_fini(struct i915_active *ref)
 {
 	debug_active_fini(ref);
-	GEM_BUG_ON(!RB_EMPTY_ROOT(&ref->tree));
 	GEM_BUG_ON(atomic_read(&ref->count));
+	GEM_BUG_ON(work_pending(&ref->work));
+	GEM_BUG_ON(!RB_EMPTY_ROOT(&ref->tree));
 	mutex_destroy(&ref->mutex);
 }
 #endif
diff --git a/drivers/gpu/drm/i915/i915_active_types.h b/drivers/gpu/drm/i915/i915_active_types.h
index 5b0a3024ce24..a3a5ec3e4163 100644
--- a/drivers/gpu/drm/i915/i915_active_types.h
+++ b/drivers/gpu/drm/i915/i915_active_types.h
@@ -12,6 +12,9 @@
 #include <linux/mutex.h>
 #include <linux/rbtree.h>
 #include <linux/rcupdate.h>
+#include <linux/workqueue.h>
+
+#include "i915_utils.h"
 
 struct drm_i915_private;
 struct i915_active_request;
@@ -28,6 +31,11 @@ struct i915_active_request {
 
 struct active_node;
 
+#define I915_ACTIVE_MAY_SLEEP BIT(0)
+
+#define __i915_active_call __aligned(4)
+#define i915_active_may_sleep(fn) ptr_pack_bits(&(fn), I915_ACTIVE_MAY_SLEEP, 2)
+
 struct i915_active {
 	struct drm_i915_private *i915;
 
@@ -38,6 +46,10 @@ struct i915_active {
 
 	int (*active)(struct i915_active *ref);
 	void (*retire)(struct i915_active *ref);
+	unsigned long flags;
+#define I915_ACTIVE_RETIRE_SLEEPS BIT(0)
+
+	struct work_struct work;
 
 	struct llist_head barriers;
 };
diff --git a/drivers/gpu/drm/i915/i915_timeline.c b/drivers/gpu/drm/i915/i915_timeline.c
index 3ea1b881a4ae..0f4ca9478ae5 100644
--- a/drivers/gpu/drm/i915/i915_timeline.c
+++ b/drivers/gpu/drm/i915/i915_timeline.c
@@ -138,6 +138,7 @@ static void __idle_cacheline_free(struct i915_timeline_cacheline *cl)
 	kfree(cl);
 }
 
+__i915_active_call
 static void __cacheline_retire(struct i915_active *active)
 {
 	struct i915_timeline_cacheline *cl =
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 8863b504d996..aa0df8332e25 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -88,6 +88,7 @@ static int __i915_vma_active(struct i915_active *ref)
 	return 0;
 }
 
+__i915_active_call
 static void __i915_vma_retire(struct i915_active *ref)
 {
 	i915_vma_put(active_to_vma(ref));
@@ -976,13 +977,13 @@ int i915_vma_unbind(struct i915_vma *vma)
 		if (ret)
 			return ret;
 	}
-	GEM_BUG_ON(i915_vma_is_active(vma));
 
 	if (i915_vma_is_pinned(vma)) {
 		vma_print_allocator(vma, "is pinned");
 		return -EBUSY;
 	}
 
+	GEM_BUG_ON(i915_vma_is_active(vma));
 	if (!drm_mm_node_allocated(&vma->node))
 		return 0;
 
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/7] drm/i915/gtt: Defer address space cleanup to an RCU worker

[Patchwork]

== Series Details ==

Series: series starting with [1/7] drm/i915/gtt: Defer address space cleanup to an RCU worker
URL   : https://patchwork.freedesktop.org/series/62382/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
3e218a90e0bb drm/i915/gtt: Defer address space cleanup to an RCU worker
5fe199a684ea drm/i915: Track i915_active using debugobjects
91b44795c947 drm/i915: Signal fence completion from i915_request_wait
e6386a9a70d1 drm/i915: Remove waiting & retiring from shrinker paths
0b718dbee0a0 drm/i915: Throw away the active object retirement complexity
9185cf9f56f7 drm/i915: Provide an i915_active.acquire callback
-:708: CHECK:UNCOMMENTED_DEFINITION: struct mutex definition without comment
#708: FILE: drivers/gpu/drm/i915/i915_active_types.h:36:
+	struct mutex mutex;

total: 0 errors, 0 warnings, 1 checks, 842 lines checked
4b58a7fe5a7f drm/i915: Push the i915_active.retire into a worker

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.SPARSE: warning for series starting with [1/7] drm/i915/gtt: Defer address space cleanup to an RCU worker

[Patchwork]

== Series Details ==

Series: series starting with [1/7] drm/i915/gtt: Defer address space cleanup to an RCU worker
URL   : https://patchwork.freedesktop.org/series/62382/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915/gtt: Defer address space cleanup to an RCU worker
+./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)

Commit: drm/i915: Track i915_active using debugobjects
Okay!

Commit: drm/i915: Signal fence completion from i915_request_wait
Okay!

Commit: drm/i915: Remove waiting & retiring from shrinker paths
Okay!

Commit: drm/i915: Throw away the active object retirement complexity
Okay!

Commit: drm/i915: Provide an i915_active.acquire callback
Okay!

Commit: drm/i915: Push the i915_active.retire into a worker
Okay!

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [1/7] drm/i915/gtt: Defer address space cleanup to an RCU worker

[Patchwork]

== Series Details ==

Series: series starting with [1/7] drm/i915/gtt: Defer address space cleanup to an RCU worker
URL   : https://patchwork.freedesktop.org/series/62382/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_6304 -> Patchwork_13345
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_13345 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_13345, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13345/

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_13345:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live_hangcheck:
    - fi-bsw-n3050:       [PASS][1] -> [DMESG-FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6304/fi-bsw-n3050/igt@i915_selftest@live_hangcheck.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13345/fi-bsw-n3050/igt@i915_selftest@live_hangcheck.html
    - fi-hsw-peppy:       [PASS][3] -> [DMESG-FAIL][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6304/fi-hsw-peppy/igt@i915_selftest@live_hangcheck.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13345/fi-hsw-peppy/igt@i915_selftest@live_hangcheck.html

  
Known issues
------------

  Here are the changes found in Patchwork_13345 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_linear_blits@basic:
    - fi-icl-dsi:         [PASS][5] -> [DMESG-WARN][6] ([fdo#106107])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6304/fi-icl-dsi/igt@gem_linear_blits@basic.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13345/fi-icl-dsi/igt@gem_linear_blits@basic.html

  * igt@i915_module_load@reload-with-fault-injection:
    - fi-snb-2600:        [PASS][7] -> [DMESG-WARN][8] ([fdo#110343])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6304/fi-snb-2600/igt@i915_module_load@reload-with-fault-injection.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13345/fi-snb-2600/igt@i915_module_load@reload-with-fault-injection.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [PASS][9] -> [FAIL][10] ([fdo#109485])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6304/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13345/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  
#### Possible fixes ####

  * igt@gem_ctx_create@basic-files:
    - fi-icl-guc:         [INCOMPLETE][11] ([fdo#107713] / [fdo#109100]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6304/fi-icl-guc/igt@gem_ctx_create@basic-files.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13345/fi-icl-guc/igt@gem_ctx_create@basic-files.html

  * igt@gem_exec_basic@basic-all:
    - fi-icl-u2:          [INCOMPLETE][13] ([fdo#107713]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6304/fi-icl-u2/igt@gem_exec_basic@basic-all.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13345/fi-icl-u2/igt@gem_exec_basic@basic-all.html

  * igt@gem_exec_parallel@basic:
    - fi-icl-dsi:         [FAIL][15] -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6304/fi-icl-dsi/igt@gem_exec_parallel@basic.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13345/fi-icl-dsi/igt@gem_exec_parallel@basic.html

  * igt@i915_selftest@live_contexts:
    - fi-bdw-gvtdvm:      [DMESG-FAIL][17] ([fdo#110235]) -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6304/fi-bdw-gvtdvm/igt@i915_selftest@live_contexts.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13345/fi-bdw-gvtdvm/igt@i915_selftest@live_contexts.html
    - fi-skl-gvtdvm:      [DMESG-FAIL][19] ([fdo#110235]) -> [PASS][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6304/fi-skl-gvtdvm/igt@i915_selftest@live_contexts.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13345/fi-skl-gvtdvm/igt@i915_selftest@live_contexts.html

  * igt@i915_selftest@live_evict:
    - fi-kbl-r:           [INCOMPLETE][21] ([fdo#110938]) -> [PASS][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6304/fi-kbl-r/igt@i915_selftest@live_evict.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13345/fi-kbl-r/igt@i915_selftest@live_evict.html

  
  [fdo#106107]: https://bugs.freedesktop.org/show_bug.cgi?id=106107
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100
  [fdo#109485]: https://bugs.freedesktop.org/show_bug.cgi?id=109485
  [fdo#110235]: https://bugs.freedesktop.org/show_bug.cgi?id=110235
  [fdo#110343]: https://bugs.freedesktop.org/show_bug.cgi?id=110343
  [fdo#110938]: https://bugs.freedesktop.org/show_bug.cgi?id=110938


Participating hosts (53 -> 43)
------------------------------

  Additional (1): fi-pnv-d510 
  Missing    (11): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-skl-guc fi-byt-squawks fi-bsw-cyan fi-snb-2520m fi-icl-y fi-byt-clapper fi-bdw-samus fi-cml-u 


Build changes
-------------

  * Linux: CI_DRM_6304 -> Patchwork_13345

  CI_DRM_6304: 162de299f0bd2ce05e11b77bcd55025343009bf7 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5061: c88ced79a7b71aec58f1d9c5c599ac2f431bcf7a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13345: 4b58a7fe5a7ff3be7cd48abb37399519e731f101 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

4b58a7fe5a7f drm/i915: Push the i915_active.retire into a worker
9185cf9f56f7 drm/i915: Provide an i915_active.acquire callback
0b718dbee0a0 drm/i915: Throw away the active object retirement complexity
e6386a9a70d1 drm/i915: Remove waiting & retiring from shrinker paths
91b44795c947 drm/i915: Signal fence completion from i915_request_wait
5fe199a684ea drm/i915: Track i915_active using debugobjects
3e218a90e0bb drm/i915/gtt: Defer address space cleanup to an RCU worker

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13345/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/7] drm/i915: Signal fence completion from i915_request_wait

[Tvrtko Ursulin]

On 19/06/2019 12:23, Chris Wilson wrote:
> With the upcoming change to automanaged i915_active, the intent is that
> whenever we wait on the set of active fences, they are signaled and
> collected.  The requirement is that all successful returns from
> i915_request_wait() signal the fence, so fixup the one remaining path
> where we may return before the interrupt has been run.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_request.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 0c2b53b8a3d1..8d59c35bd22f 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -1451,8 +1451,10 @@ long i915_request_wait(struct i915_request *rq,
>   	for (;;) {
>   		set_current_state(state);
>   
> -		if (i915_request_completed(rq))
> +		if (i915_request_completed(rq)) {
> +			dma_fence_signal(&rq->fence);
>   			break;
> +		}
>   
>   		if (signal_pending_state(state, current)) {
>   			timeout = -ERESTARTSYS;
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Patch is fine, but I have to say commit message scares me a bit. Where 
are we adding such subtle fragility that wait has to exit with fence 
signaled? Is it just some specific i915_request_wait call site or all of 
them?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/7] drm/i915: Signal fence completion from i915_request_wait

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-06-19 15:27:21)
> 
> On 19/06/2019 12:23, Chris Wilson wrote:
> > With the upcoming change to automanaged i915_active, the intent is that
> > whenever we wait on the set of active fences, they are signaled and
> > collected.  The requirement is that all successful returns from
> > i915_request_wait() signal the fence, so fixup the one remaining path
> > where we may return before the interrupt has been run.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_request.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> > index 0c2b53b8a3d1..8d59c35bd22f 100644
> > --- a/drivers/gpu/drm/i915/i915_request.c
> > +++ b/drivers/gpu/drm/i915/i915_request.c
> > @@ -1451,8 +1451,10 @@ long i915_request_wait(struct i915_request *rq,
> >       for (;;) {
> >               set_current_state(state);
> >   
> > -             if (i915_request_completed(rq))
> > +             if (i915_request_completed(rq)) {
> > +                     dma_fence_signal(&rq->fence);
> >                       break;
> > +             }
> >   
> >               if (signal_pending_state(state, current)) {
> >                       timeout = -ERESTARTSYS;
> > 
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Patch is fine, but I have to say commit message scares me a bit. Where 
> are we adding such subtle fragility that wait has to exit with fence 
> signaled? Is it just some specific i915_request_wait call site or all of 
> them?

dma_fence_wait_default already implies that the fence is signaled prior
to its return, so in terms of the bigger picture I can argue this bring
us into line.

In the scenarios I've been fixing it's basically making sure that
i915_gem_wait_for_idle() does guarantee that we idle, which involves
some assurance that the request are retired, and uses
i915_request_wait() directly for the most part.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/7] drm/i915/gtt: Defer address space cleanup to an RCU worker

[Tvrtko Ursulin]

On 19/06/2019 12:23, Chris Wilson wrote:
> Enable RCU protection of i915_address_space and its ppgtt superclasses,
> and defer its cleanup into a worker executed after an RCU grace period.
> 
> In the future we will be able to use the RCU protection to reduce the
> locking around VM lookups, but the immediate benefit is being able to
> defer the release into a kworker (process context). This is required as
> we may need to sleep to reap the WC pages stashed away inside the ppgtt.

I can't see that it adds RCU protection apart from using queue_rcu_work 
for some reason. It _seems_ like it could just as well use normal 
deferred worker? RCU may have a benefit of being relaxed in timing ie we 
don't care it happens immediately, but also don't want to put some made 
up time period against it. So it's all about natural batching only at 
this point?

> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110934
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/Makefile.header-test     |   1 +
>   drivers/gpu/drm/i915/i915_gem_gtt.c           | 109 ++++++++++--------
>   drivers/gpu/drm/i915/i915_gem_gtt.h           |   5 +
>   drivers/gpu/drm/i915/selftests/i915_gem_gtt.c |   2 -
>   4 files changed, 66 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/Makefile.header-test b/drivers/gpu/drm/i915/Makefile.header-test
> index e6ba66f787f9..cb74242f9c3b 100644
> --- a/drivers/gpu/drm/i915/Makefile.header-test
> +++ b/drivers/gpu/drm/i915/Makefile.header-test
> @@ -6,6 +6,7 @@ header_test := \
>   	i915_active_types.h \
>   	i915_debugfs.h \
>   	i915_drv.h \
> +	i915_gem_gtt.h \
>   	i915_irq.h \
>   	i915_params.h \
>   	i915_priolist_types.h \
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 8ab820145ea6..d9eddbd79670 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -482,9 +482,69 @@ static void vm_free_page(struct i915_address_space *vm, struct page *page)
>   	spin_unlock(&vm->free_pages.lock);
>   }
>   
> +static void i915_address_space_fini(struct i915_address_space *vm)
> +{
> +	spin_lock(&vm->free_pages.lock);
> +	if (pagevec_count(&vm->free_pages.pvec))
> +		vm_free_pages_release(vm, true);
> +	GEM_BUG_ON(pagevec_count(&vm->free_pages.pvec));
> +	spin_unlock(&vm->free_pages.lock);
> +
> +	drm_mm_takedown(&vm->mm);
> +
> +	mutex_destroy(&vm->mutex);
> +}
> +
> +static void ppgtt_destroy_vma(struct i915_address_space *vm)
> +{
> +	struct list_head *phases[] = {
> +		&vm->bound_list,
> +		&vm->unbound_list,
> +		NULL,
> +	}, **phase;
> +
> +	mutex_lock(&vm->i915->drm.struct_mutex);
> +	for (phase = phases; *phase; phase++) {
> +		struct i915_vma *vma, *vn;
> +
> +		list_for_each_entry_safe(vma, vn, *phase, vm_link)

No one can access the lists at this point, right? So apart from 
inefficiency, strictly pedantic thing would be to keep taking and 
releasing struct_mutext for i915_vma_destroy only I think. It's find to 
optimize that, but maybe add a comment above mutex_lock.

> +			i915_vma_destroy(vma);
> +	}
> +	mutex_unlock(&vm->i915->drm.struct_mutex);
> +}
> +
> +static void __i915_vm_release(struct work_struct *work)
> +{
> +	struct i915_address_space *vm =
> +		container_of(work, struct i915_address_space, rcu.work);
> +
> +	ppgtt_destroy_vma(vm);
> +
> +	GEM_BUG_ON(!list_empty(&vm->bound_list));
> +	GEM_BUG_ON(!list_empty(&vm->unbound_list));
> +
> +	vm->cleanup(vm);
> +	i915_address_space_fini(vm);
> +
> +	kfree(vm);
> +}
> +
> +void i915_vm_release(struct kref *kref)
> +{
> +	struct i915_address_space *vm =
> +		container_of(kref, struct i915_address_space, ref);
> +
> +	GEM_BUG_ON(i915_is_ggtt(vm));
> +	trace_i915_ppgtt_release(vm);
> +
> +	vm->closed = true;

This is now outside any locking although I am not sure if it matters. 
Feels weird at least.

> +	queue_rcu_work(system_unbound_wq, &vm->rcu);
> +}
> +
>   static void i915_address_space_init(struct i915_address_space *vm, int subclass)
>   {
>   	kref_init(&vm->ref);
> +	INIT_RCU_WORK(&vm->rcu, __i915_vm_release);
>   
>   	/*
>   	 * The vm->mutex must be reclaim safe (for use in the shrinker).
> @@ -505,19 +565,6 @@ static void i915_address_space_init(struct i915_address_space *vm, int subclass)
>   	INIT_LIST_HEAD(&vm->bound_list);
>   }
>   
> -static void i915_address_space_fini(struct i915_address_space *vm)
> -{
> -	spin_lock(&vm->free_pages.lock);
> -	if (pagevec_count(&vm->free_pages.pvec))
> -		vm_free_pages_release(vm, true);
> -	GEM_BUG_ON(pagevec_count(&vm->free_pages.pvec));
> -	spin_unlock(&vm->free_pages.lock);
> -
> -	drm_mm_takedown(&vm->mm);
> -
> -	mutex_destroy(&vm->mutex);
> -}
> -
>   static int __setup_page_dma(struct i915_address_space *vm,
>   			    struct i915_page_dma *p,
>   			    gfp_t gfp)
> @@ -2250,42 +2297,6 @@ i915_ppgtt_create(struct drm_i915_private *i915)
>   	return ppgtt;
>   }
>   
> -static void ppgtt_destroy_vma(struct i915_address_space *vm)
> -{
> -	struct list_head *phases[] = {
> -		&vm->bound_list,
> -		&vm->unbound_list,
> -		NULL,
> -	}, **phase;
> -
> -	vm->closed = true;
> -	for (phase = phases; *phase; phase++) {
> -		struct i915_vma *vma, *vn;
> -
> -		list_for_each_entry_safe(vma, vn, *phase, vm_link)
> -			i915_vma_destroy(vma);
> -	}
> -}
> -
> -void i915_vm_release(struct kref *kref)
> -{
> -	struct i915_address_space *vm =
> -		container_of(kref, struct i915_address_space, ref);
> -
> -	GEM_BUG_ON(i915_is_ggtt(vm));
> -	trace_i915_ppgtt_release(vm);
> -
> -	ppgtt_destroy_vma(vm);
> -
> -	GEM_BUG_ON(!list_empty(&vm->bound_list));
> -	GEM_BUG_ON(!list_empty(&vm->unbound_list));
> -
> -	vm->cleanup(vm);
> -	i915_address_space_fini(vm);
> -
> -	kfree(vm);
> -}
> -
>   /* Certain Gen5 chipsets require require idling the GPU before
>    * unmapping anything from the GTT when VT-d is enabled.
>    */
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 812717ccc69b..8de57f67a911 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -35,8 +35,12 @@
>   #define __I915_GEM_GTT_H__
>   
>   #include <linux/io-mapping.h>
> +#include <linux/kref.h>
>   #include <linux/mm.h>
>   #include <linux/pagevec.h>
> +#include <linux/workqueue.h>
> +
> +#include <drm/drm_mm.h>
>   
>   #include "gt/intel_reset.h"
>   #include "i915_gem_fence_reg.h"
> @@ -280,6 +284,7 @@ struct pagestash {
>   
>   struct i915_address_space {
>   	struct kref ref;
> +	struct rcu_work rcu;
>   
>   	struct drm_mm mm;
>   	struct drm_i915_private *i915;
> diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
> index 1a60b9fe8221..0c47276ed5df 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
> @@ -208,9 +208,7 @@ static int igt_ppgtt_alloc(void *arg)
>   	}
>   
>   err_ppgtt_cleanup:
> -	mutex_lock(&dev_priv->drm.struct_mutex);
>   	i915_vm_put(&ppgtt->vm);
> -	mutex_unlock(&dev_priv->drm.struct_mutex);
>   	return err;
>   }
>   
> 

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/7] drm/i915/gtt: Defer address space cleanup to an RCU worker

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-06-19 15:51:18)
> 
> On 19/06/2019 12:23, Chris Wilson wrote:
> > Enable RCU protection of i915_address_space and its ppgtt superclasses,
> > and defer its cleanup into a worker executed after an RCU grace period.
> > 
> > In the future we will be able to use the RCU protection to reduce the
> > locking around VM lookups, but the immediate benefit is being able to
> > defer the release into a kworker (process context). This is required as
> > we may need to sleep to reap the WC pages stashed away inside the ppgtt.
> 
> I can't see that it adds RCU protection apart from using queue_rcu_work 
> for some reason.

That provides the RCU safe freeing, yes. My intentional is to use the
rcu read lock around the vm lookup + kref when dropping the struct_mutex
there.

> It _seems_ like it could just as well use normal 
> deferred worker? RCU may have a benefit of being relaxed in timing ie we 
> don't care it happens immediately, but also don't want to put some made 
> up time period against it. So it's all about natural batching only at 
> this point?

At this moment, to fix up the current bug and to allow i915_active to be
struct_mutex-less, we need to defer the i915_vm_release and
gen8_ppgtt_release to process context. Hence using the worker callback
and not the regular softirq-context rcu callback.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/7] drm/i915/gtt: Defer address space cleanup to an RCU worker

[Tvrtko Ursulin]

On 19/06/2019 15:56, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-06-19 15:51:18)
>>
>> On 19/06/2019 12:23, Chris Wilson wrote:
>>> Enable RCU protection of i915_address_space and its ppgtt superclasses,
>>> and defer its cleanup into a worker executed after an RCU grace period.
>>>
>>> In the future we will be able to use the RCU protection to reduce the
>>> locking around VM lookups, but the immediate benefit is being able to
>>> defer the release into a kworker (process context). This is required as
>>> we may need to sleep to reap the WC pages stashed away inside the ppgtt.
>>
>> I can't see that it adds RCU protection apart from using queue_rcu_work
>> for some reason.
> 
> That provides the RCU safe freeing, yes. My intentional is to use the
> rcu read lock around the vm lookup + kref when dropping the struct_mutex
> there.
> 
>> It _seems_ like it could just as well use normal
>> deferred worker? RCU may have a benefit of being relaxed in timing ie we
>> don't care it happens immediately, but also don't want to put some made
>> up time period against it. So it's all about natural batching only at
>> this point?
> 
> At this moment, to fix up the current bug and to allow i915_active to be
> struct_mutex-less, we need to defer the i915_vm_release and
> gen8_ppgtt_release to process context. Hence using the worker callback
> and not the regular softirq-context rcu callback.

It's fine by me, I just wanted for some reassurances about future plans. :)

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx