drm/i915: Provide an i915_active.acquire callback

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>
Link: https://patchwork.freedesktop.org/patch/msgid/20190621183801.23252-4-chris@chris-wilson.co.uk

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;

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);
 
@@ -130,35 +134,11 @@ 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);
 
 	err = intel_ring_pin(ce->ring);
@@ -168,7 +148,7 @@ int intel_context_active_acquire(struct intel_context *ce, unsigned long flags)
 	if (!ce->state)
 		return 0;
 
-	err = __context_pin_state(ce->state, flags);
+	err = __context_pin_state(ce->state);
 	if (err)
 		goto err_ring;
 
@@ -188,15 +168,30 @@ int intel_context_active_acquire(struct intel_context *ce, unsigned long flags)
 	intel_ring_unpin(ce->ring);
 err_put:
 	intel_context_put(ce);
-	i915_active_cancel(&ce->active);
 	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)

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)
 {

drivers/gpu/drm/i915/gt/intel_lrc.c

@@ -1542,12 +1542,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) |

drivers/gpu/drm/i915/gt/intel_ringbuffer.c

@@ -1455,7 +1455,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;
 

drivers/gpu/drm/i915/gt/intel_timeline.c

@@ -146,6 +146,15 @@ static void __cacheline_retire(struct i915_active *active)
 		__idle_cacheline_free(cl);
 }
 
+static int __cacheline_active(struct i915_active *active)
+{
+	struct intel_timeline_cacheline *cl =
+		container_of(active, typeof(*cl), active);
+
+	__i915_vma_pin(cl->hwsp->vma);
+	return 0;
+}
+
 static struct intel_timeline_cacheline *
 cacheline_alloc(struct intel_timeline_hwsp *hwsp, unsigned int cacheline)
 {
@@ -168,15 +177,16 @@ cacheline_alloc(struct intel_timeline_hwsp *hwsp, unsigned int cacheline)
 	cl->hwsp = hwsp;
 	cl->vaddr = page_pack_bits(vaddr, cacheline);
 
-	i915_active_init(hwsp->gt->i915, &cl->active, __cacheline_retire);
+	i915_active_init(hwsp->gt->i915, &cl->active,
+			 __cacheline_active, __cacheline_retire);
 
 	return cl;
 }
 
 static void cacheline_acquire(struct intel_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 intel_timeline_cacheline *cl)

drivers/gpu/drm/i915/gt/mock_engine.c

@@ -155,7 +155,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;
 

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)

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;

drivers/gpu/drm/i915/i915_gem_gtt.c

@@ -2055,7 +2055,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;

drivers/gpu/drm/i915/i915_vma.c

@@ -78,11 +78,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 *
@@ -107,7 +116,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);
@@ -904,11 +914,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;
 

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);