drm/i915: Integrate i915_sw_fence with debugobjects

Add the tracking required to enable debugobjects for fences to improve
error detection in BAT. The debugobject interface lets us track the
lifetime and phases of the fences even while being embedded into larger
structs, i.e. to check they are not used after they have been released.

v2: Don't populate the stubs, debugobjects checks for a NULL pointer and
treats it equivalently.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/20161125131718.20978-4-chris@chris-wilson.co.uk

4 files changed, 161 insertions, 7 deletions

diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug
index 51ba630a134b..a6c69b8cb1d2 100644
--- a/drivers/gpu/drm/i915/Kconfig.debug
+++ b/drivers/gpu/drm/i915/Kconfig.debug
@@ -22,6 +22,7 @@ config DRM_I915_DEBUG
         select X86_MSR # used by igt/pm_rpm
         select DRM_VGEM # used by igt/prime_vgem (dmabuf interop checks)
         select DRM_DEBUG_MM if DRM=y
+        select DRM_I915_SW_FENCE_DEBUG_OBJECTS if DRM_I915=y
         default n
         help
           Choose this option to turn on extra driver debugging that may affect
@@ -43,3 +44,15 @@ config DRM_I915_DEBUG_GEM
 
           If in doubt, say "N".
 
+config DRM_I915_SW_FENCE_DEBUG_OBJECTS
+        bool "Enable additional driver debugging for fence objects"
+        depends on DRM_I915=y
+        select DEBUG_OBJECTS
+        default n
+        help
+          Choose this option to turn on extra driver debugging that may affect
+          performance but will catch some internal issues.
+
+          Recommended for driver developers only.
+
+          If in doubt, say "N".
diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
index 94d71b639f12..fcf22b0e2967 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -62,6 +62,15 @@ static void i915_fence_release(struct dma_fence *fence)
 {
 	struct drm_i915_gem_request *req = to_request(fence);
 
+	/* The request is put onto a RCU freelist (i.e. the address
+	 * is immediately reused), mark the fences as being freed now.
+	 * Otherwise the debugobjects for the fences are only marked as
+	 * freed when the slab cache itself is freed, and so we would get
+	 * caught trying to reuse dead objects.
+	 */
+	i915_sw_fence_fini(&req->submit);
+	i915_sw_fence_fini(&req->execute);
+
 	kmem_cache_free(req->i915->requests, req);
 }
 
diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c
index 147420ccf49c..f5a88092dacf 100644
--- a/drivers/gpu/drm/i915/i915_sw_fence.c
+++ b/drivers/gpu/drm/i915/i915_sw_fence.c
@@ -17,6 +17,92 @@
 
 static DEFINE_SPINLOCK(i915_sw_fence_lock);
 
+enum {
+	DEBUG_FENCE_IDLE = 0,
+	DEBUG_FENCE_NOTIFY,
+};
+
+#ifdef CONFIG_DRM_I915_SW_FENCE_DEBUG_OBJECTS
+
+static void *i915_sw_fence_debug_hint(void *addr)
+{
+	return (void *)(((struct i915_sw_fence *)addr)->flags & I915_SW_FENCE_MASK);
+}
+
+static struct debug_obj_descr i915_sw_fence_debug_descr = {
+	.name = "i915_sw_fence",
+	.debug_hint = i915_sw_fence_debug_hint,
+};
+
+static inline void debug_fence_init(struct i915_sw_fence *fence)
+{
+	debug_object_init(fence, &i915_sw_fence_debug_descr);
+}
+
+static inline void debug_fence_activate(struct i915_sw_fence *fence)
+{
+	debug_object_activate(fence, &i915_sw_fence_debug_descr);
+}
+
+static inline void debug_fence_set_state(struct i915_sw_fence *fence,
+					 int old, int new)
+{
+	debug_object_active_state(fence, &i915_sw_fence_debug_descr, old, new);
+}
+
+static inline void debug_fence_deactivate(struct i915_sw_fence *fence)
+{
+	debug_object_deactivate(fence, &i915_sw_fence_debug_descr);
+}
+
+static inline void debug_fence_destroy(struct i915_sw_fence *fence)
+{
+	debug_object_destroy(fence, &i915_sw_fence_debug_descr);
+}
+
+static inline void debug_fence_free(struct i915_sw_fence *fence)
+{
+	debug_object_free(fence, &i915_sw_fence_debug_descr);
+}
+
+static inline void debug_fence_assert(struct i915_sw_fence *fence)
+{
+	debug_object_assert_init(fence, &i915_sw_fence_debug_descr);
+}
+
+#else
+
+static inline void debug_fence_init(struct i915_sw_fence *fence)
+{
+}
+
+static inline void debug_fence_activate(struct i915_sw_fence *fence)
+{
+}
+
+static inline void debug_fence_set_state(struct i915_sw_fence *fence,
+					 int old, int new)
+{
+}
+
+static inline void debug_fence_deactivate(struct i915_sw_fence *fence)
+{
+}
+
+static inline void debug_fence_destroy(struct i915_sw_fence *fence)
+{
+}
+
+static inline void debug_fence_free(struct i915_sw_fence *fence)
+{
+}
+
+static inline void debug_fence_assert(struct i915_sw_fence *fence)
+{
+}
+
+#endif
+
 static int __i915_sw_fence_notify(struct i915_sw_fence *fence,
 				  enum i915_sw_fence_notify state)
 {
@@ -26,25 +112,37 @@ static int __i915_sw_fence_notify(struct i915_sw_fence *fence,
 	return fn(fence, state);
 }
 
-static void i915_sw_fence_free(struct kref *kref)
+#ifdef CONFIG_DRM_I915_SW_FENCE_DEBUG_OBJECTS
+void i915_sw_fence_fini(struct i915_sw_fence *fence)
+{
+	debug_fence_free(fence);
+}
+#endif
+
+static void i915_sw_fence_release(struct kref *kref)
 {
 	struct i915_sw_fence *fence = container_of(kref, typeof(*fence), kref);
 
 	WARN_ON(atomic_read(&fence->pending) > 0);
+	debug_fence_destroy(fence);
 
-	if (fence->flags & I915_SW_FENCE_MASK)
+	if (fence->flags & I915_SW_FENCE_MASK) {
 		__i915_sw_fence_notify(fence, FENCE_FREE);
-	else
+	} else {
+		i915_sw_fence_fini(fence);
 		kfree(fence);
+	}
 }
 
 static void i915_sw_fence_put(struct i915_sw_fence *fence)
 {
-	kref_put(&fence->kref, i915_sw_fence_free);
+	debug_fence_assert(fence);
+	kref_put(&fence->kref, i915_sw_fence_release);
 }
 
 static struct i915_sw_fence *i915_sw_fence_get(struct i915_sw_fence *fence)
 {
+	debug_fence_assert(fence);
 	kref_get(&fence->kref);
 	return fence;
 }
@@ -56,6 +154,7 @@ static void __i915_sw_fence_wake_up_all(struct i915_sw_fence *fence,
 	wait_queue_t *pos, *next;
 	unsigned long flags;
 
+	debug_fence_deactivate(fence);
 	atomic_set_release(&fence->pending, -1); /* 0 -> -1 [done] */
 
 	/*
@@ -88,23 +187,33 @@ static void __i915_sw_fence_wake_up_all(struct i915_sw_fence *fence,
 		} while (1);
 	}
 	spin_unlock_irqrestore(&x->lock, flags);
+
+	debug_fence_assert(fence);
 }
 
 static void __i915_sw_fence_complete(struct i915_sw_fence *fence,
 				     struct list_head *continuation)
 {
+	debug_fence_assert(fence);
+
 	if (!atomic_dec_and_test(&fence->pending))
 		return;
 
+	debug_fence_set_state(fence, DEBUG_FENCE_IDLE, DEBUG_FENCE_NOTIFY);
+
 	if (fence->flags & I915_SW_FENCE_MASK &&
 	    __i915_sw_fence_notify(fence, FENCE_COMPLETE) != NOTIFY_DONE)
 		return;
 
+	debug_fence_set_state(fence, DEBUG_FENCE_NOTIFY, DEBUG_FENCE_IDLE);
+
 	__i915_sw_fence_wake_up_all(fence, continuation);
 }
 
 static void i915_sw_fence_complete(struct i915_sw_fence *fence)
 {
+	debug_fence_assert(fence);
+
 	if (WARN_ON(i915_sw_fence_done(fence)))
 		return;
 
@@ -113,6 +222,7 @@ static void i915_sw_fence_complete(struct i915_sw_fence *fence)
 
 static void i915_sw_fence_await(struct i915_sw_fence *fence)
 {
+	debug_fence_assert(fence);
 	WARN_ON(atomic_inc_return(&fence->pending) <= 1);
 }
 
@@ -123,18 +233,26 @@ void __i915_sw_fence_init(struct i915_sw_fence *fence,
 {
 	BUG_ON((unsigned long)fn & ~I915_SW_FENCE_MASK);
 
+	debug_fence_init(fence);
+
 	__init_waitqueue_head(&fence->wait, name, key);
 	kref_init(&fence->kref);
 	atomic_set(&fence->pending, 1);
 	fence->flags = (unsigned long)fn;
 }
 
-void i915_sw_fence_commit(struct i915_sw_fence *fence)
+static void __i915_sw_fence_commit(struct i915_sw_fence *fence)
 {
 	i915_sw_fence_complete(fence);
 	i915_sw_fence_put(fence);
 }
 
+void i915_sw_fence_commit(struct i915_sw_fence *fence)
+{
+	debug_fence_activate(fence);
+	__i915_sw_fence_commit(fence);
+}
+
 static int i915_sw_fence_wake(wait_queue_t *wq, unsigned mode, int flags, void *key)
 {
 	list_del(&wq->task_list);
@@ -206,9 +324,13 @@ static int __i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence,
 	unsigned long flags;
 	int pending;
 
+	debug_fence_assert(fence);
+
 	if (i915_sw_fence_done(signaler))
 		return 0;
 
+	debug_fence_assert(signaler);
+
 	/* The dependency graph must be acyclic. */
 	if (unlikely(i915_sw_fence_check_if_after(fence, signaler)))
 		return -EINVAL;
@@ -279,7 +401,7 @@ static void timer_i915_sw_fence_wake(unsigned long data)
 	dma_fence_put(cb->dma);
 	cb->dma = NULL;
 
-	i915_sw_fence_commit(cb->fence);
+	__i915_sw_fence_commit(cb->fence);
 	cb->timer.function = NULL;
 }
 
@@ -290,7 +412,7 @@ static void dma_i915_sw_fence_wake(struct dma_fence *dma,
 
 	del_timer_sync(&cb->timer);
 	if (cb->timer.function)
-		i915_sw_fence_commit(cb->fence);
+		__i915_sw_fence_commit(cb->fence);
 	dma_fence_put(cb->dma);
 
 	kfree(cb);
@@ -304,6 +426,8 @@ int i915_sw_fence_await_dma_fence(struct i915_sw_fence *fence,
 	struct i915_sw_dma_fence_cb *cb;
 	int ret;
 
+	debug_fence_assert(fence);
+
 	if (dma_fence_is_signaled(dma))
 		return 0;
 
@@ -349,6 +473,8 @@ int i915_sw_fence_await_reservation(struct i915_sw_fence *fence,
 	struct dma_fence *excl;
 	int ret = 0, pending;
 
+	debug_fence_assert(fence);
+
 	if (write) {
 		struct dma_fence **shared;
 		unsigned int count, i;
diff --git a/drivers/gpu/drm/i915/i915_sw_fence.h b/drivers/gpu/drm/i915/i915_sw_fence.h
index 0f3185ef7f4e..d31cefbbcc04 100644
--- a/drivers/gpu/drm/i915/i915_sw_fence.h
+++ b/drivers/gpu/drm/i915/i915_sw_fence.h
@@ -56,6 +56,12 @@ do {								\
 	__i915_sw_fence_init((fence), (fn), NULL, NULL)
 #endif
 
+#ifdef CONFIG_DRM_I915_SW_FENCE_DEBUG_OBJECTS
+void i915_sw_fence_fini(struct i915_sw_fence *fence);
+#else
+static inline void i915_sw_fence_fini(struct i915_sw_fence *fence) {}
+#endif
+
 void i915_sw_fence_commit(struct i915_sw_fence *fence);
 
 int i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence,