Use debugobjects to track and validate the lifecycle of a struct
drm_i915_gem_request.

v2: Mark up allowed NULL request pointers.

Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/Kconfig.debug      |  14 ++++
 drivers/gpu/drm/i915/i915_gem_request.c | 140 ++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/i915_gem_request.h |  37 ++++++++-
 3 files changed, 181 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/Kconfig.debug 
b/drivers/gpu/drm/i915/Kconfig.debug
index 108d21f34777..326be039e39c 100644
--- a/drivers/gpu/drm/i915/Kconfig.debug
+++ b/drivers/gpu/drm/i915/Kconfig.debug
@@ -27,6 +27,7 @@ config DRM_I915_DEBUG
         select DRM_DEBUG_MM if DRM=y
        select DRM_DEBUG_MM_SELFTEST
        select SW_SYNC # signaling validation framework (igt/syncobj*)
+       select DRM_I915_REQUEST_DEBUG_OBJECTS
        select DRM_I915_SW_FENCE_DEBUG_OBJECTS
        select DRM_I915_SELFTEST
         default n
@@ -64,6 +65,19 @@ config DRM_I915_TRACE_GEM
 
          If in doubt, say "N".
 
+config DRM_I915_REQUEST_DEBUG_OBJECTS
+        bool "Enable additional driver debugging for request objects"
+        depends on DRM_I915
+        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".
+
 config DRM_I915_SW_FENCE_DEBUG_OBJECTS
         bool "Enable additional driver debugging for fence objects"
         depends on DRM_I915
diff --git a/drivers/gpu/drm/i915/i915_gem_request.c 
b/drivers/gpu/drm/i915/i915_gem_request.c
index cf2f3895b70e..164a1bb8d655 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -30,6 +30,108 @@
 
 #include "i915_drv.h"
 
+enum {
+       DEBUG_REQUEST_INACTIVE = 0,
+       DEBUG_REQUEST_WAITING,
+       DEBUG_REQUEST_READY,
+       DEBUG_REQUEST_EXECUTING,
+       DEBUG_REQUEST_RETIRED,
+};
+
+#if IS_ENABLED(CONFIG_DRM_I915_REQUEST_DEBUG_OBJECTS)
+
+static struct debug_obj_descr i915_request_debug_descr = {
+       .name = "i915_request",
+};
+
+static inline void debug_request_init(struct drm_i915_gem_request *rq)
+{
+       debug_object_init(rq, &i915_request_debug_descr);
+}
+
+static inline void debug_request_activate(struct drm_i915_gem_request *rq)
+{
+       debug_object_activate(rq, &i915_request_debug_descr);
+}
+
+static inline void debug_request_set_state(struct drm_i915_gem_request *rq,
+                                          int old, int new)
+{
+       debug_object_active_state(rq, &i915_request_debug_descr, old, new);
+}
+
+static inline void debug_request_deactivate(struct drm_i915_gem_request *rq)
+{
+       debug_object_deactivate(rq, &i915_request_debug_descr);
+}
+
+static inline void debug_request_destroy(struct drm_i915_gem_request *rq)
+{
+       debug_object_destroy(rq, &i915_request_debug_descr);
+}
+
+static inline void debug_request_free(struct drm_i915_gem_request *rq)
+{
+       debug_object_free(rq, &i915_request_debug_descr);
+       smp_wmb(); /* flush the change in state before reallocation */
+}
+
+void debug_request_assert(const struct drm_i915_gem_request *rq)
+{
+       debug_object_assert_init((void *)rq, &i915_request_debug_descr);
+}
+
+#else
+
+static inline void debug_request_init(struct drm_i915_gem_request *rq)
+{
+}
+
+static inline void debug_request_activate(struct drm_i915_gem_request *rq)
+{
+}
+
+static inline void debug_request_set_state(struct drm_i915_gem_request *rq,
+                                          int old, int new)
+{
+}
+
+static inline void debug_request_deactivate(struct drm_i915_gem_request *rq)
+{
+}
+
+static inline void debug_request_destroy(struct drm_i915_gem_request *rq)
+{
+}
+
+static inline void debug_request_free(struct drm_i915_gem_request *rq)
+{
+}
+
+#endif
+
+static inline struct drm_i915_gem_request *
+__request_alloc(struct drm_i915_private *i915, gfp_t gfp)
+{
+#if IS_ENABLED(CONFIG_KASAN)
+       return kmalloc(sizeof(struct drm_i915_gem_request), gfp);
+#else
+       return kmem_cache_alloc(i915->requests, gfp);
+#endif
+}
+
+static inline void
+__request_free(struct drm_i915_gem_request *rq)
+{
+       debug_request_free(rq);
+
+#if IS_ENABLED(CONFIG_KASAN)
+       dma_fence_free(&rq->fence);
+#else
+       kmem_cache_free(rq->i915->requests, rq);
+#endif
+}
+
 static const char *i915_fence_get_driver_name(struct dma_fence *fence)
 {
        return "i915";
@@ -37,7 +139,8 @@ static const char *i915_fence_get_driver_name(struct 
dma_fence *fence)
 
 static const char *i915_fence_get_timeline_name(struct dma_fence *fence)
 {
-       /* The timeline struct (as part of the ppgtt underneath a context)
+       /*
+        * The timeline struct (as part of the ppgtt underneath a context)
         * may be freed when the request is no longer in use by the GPU.
         * We could extend the life of a context to beyond that of all
         * fences, possibly keeping the hw resource around indefinitely,
@@ -76,7 +179,12 @@ 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
+       debug_request_set_state(req,
+                               DEBUG_REQUEST_RETIRED, DEBUG_REQUEST_INACTIVE);
+       debug_request_deactivate(req);
+
+       /*
+        * 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
@@ -84,7 +192,7 @@ static void i915_fence_release(struct dma_fence *fence)
         */
        i915_sw_fence_fini(&req->submit);
 
-       kmem_cache_free(req->i915->requests, req);
+       __request_free(req);
 }
 
 const struct dma_fence_ops i915_fence_ops = {
@@ -389,6 +497,8 @@ static void i915_gem_request_retire(struct 
drm_i915_gem_request *request)
        GEM_BUG_ON(!request->i915->gt.active_requests);
 
        trace_i915_gem_request_retire(request);
+       debug_request_set_state(request,
+                               DEBUG_REQUEST_EXECUTING, DEBUG_REQUEST_RETIRED);
 
        spin_lock_irq(&engine->timeline->lock);
        list_del_init(&request->link);
@@ -486,6 +596,8 @@ void __i915_gem_request_submit(struct drm_i915_gem_request 
*request)
        lockdep_assert_held(&engine->timeline->lock);
 
        trace_i915_gem_request_execute(request);
+       debug_request_set_state(request,
+                               DEBUG_REQUEST_READY, DEBUG_REQUEST_EXECUTING);
 
        /* Transfer from per-context onto the global per-engine timeline */
        timeline = engine->timeline;
@@ -534,6 +646,9 @@ void __i915_gem_request_unsubmit(struct 
drm_i915_gem_request *request)
        GEM_BUG_ON(!irqs_disabled());
        lockdep_assert_held(&engine->timeline->lock);
 
+       debug_request_set_state(request,
+                               DEBUG_REQUEST_EXECUTING, DEBUG_REQUEST_READY);
+
        /* Only unwind in reverse order, required so that the per-context list
         * is kept in seqno/ring order.
         */
@@ -588,6 +703,9 @@ submit_notify(struct i915_sw_fence *fence, enum 
i915_sw_fence_notify state)
        switch (state) {
        case FENCE_COMPLETE:
                trace_i915_gem_request_submit(request);
+               debug_request_set_state(request,
+                                       DEBUG_REQUEST_WAITING,
+                                       DEBUG_REQUEST_READY);
                /*
                 * We need to serialize use of the submit_request() callback 
with its
                 * hotplugging performed during an emergency 
i915_gem_set_wedged().
@@ -692,8 +810,8 @@ i915_gem_request_alloc(struct intel_engine_cs *engine,
         *
         * Do not use kmem_cache_zalloc() here!
         */
-       req = kmem_cache_alloc(dev_priv->requests,
-                              GFP_KERNEL | __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
+       req = __request_alloc(dev_priv,
+                             GFP_KERNEL | __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
        if (unlikely(!req)) {
                /* Ratelimit ourselves to prevent oom from malicious clients */
                ret = i915_gem_wait_for_idle(dev_priv,
@@ -713,13 +831,15 @@ i915_gem_request_alloc(struct intel_engine_cs *engine,
                kmem_cache_shrink(dev_priv->requests);
                rcu_barrier(); /* Recover the TYPESAFE_BY_RCU pages */
 
-               req = kmem_cache_alloc(dev_priv->requests, GFP_KERNEL);
+               req = __request_alloc(dev_priv, GFP_KERNEL);
                if (!req) {
                        ret = -ENOMEM;
                        goto err_unreserve;
                }
        }
 
+       debug_request_init(req);
+
        req->timeline = i915_gem_context_lookup_timeline(ctx, engine);
        GEM_BUG_ON(req->timeline == engine->timeline);
 
@@ -788,7 +908,7 @@ i915_gem_request_alloc(struct intel_engine_cs *engine,
        GEM_BUG_ON(!list_empty(&req->priotree.signalers_list));
        GEM_BUG_ON(!list_empty(&req->priotree.waiters_list));
 
-       kmem_cache_free(dev_priv->requests, req);
+       __request_free(req);
 err_unreserve:
        unreserve_engine(engine);
 err_unpin:
@@ -1067,6 +1187,10 @@ void __i915_add_request(struct drm_i915_gem_request 
*request, bool flush_caches)
                engine->schedule(request, request->ctx->priority);
 
        local_bh_disable();
+       debug_request_activate(request);
+       debug_request_set_state(request,
+                               DEBUG_REQUEST_INACTIVE,
+                               DEBUG_REQUEST_WAITING);
        i915_sw_fence_commit(&request->submit);
        local_bh_enable(); /* Kick the execlists tasklet if just scheduled */
 }
@@ -1208,6 +1332,7 @@ long i915_wait_request(struct drm_i915_gem_request *req,
                   !!(flags & I915_WAIT_LOCKED));
 #endif
        GEM_BUG_ON(timeout < 0);
+       debug_request_assert(req);
 
        if (i915_gem_request_completed(req))
                return timeout;
@@ -1323,6 +1448,7 @@ long i915_wait_request(struct drm_i915_gem_request *req,
                remove_wait_queue(errq, &reset);
        remove_wait_queue(&req->execute, &exec);
        trace_i915_gem_request_wait_end(req);
+       debug_request_assert(req);
 
        return timeout;
 }
diff --git a/drivers/gpu/drm/i915/i915_gem_request.h 
b/drivers/gpu/drm/i915/i915_gem_request.h
index 2236e9188c5c..35065b27d9d6 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.h
+++ b/drivers/gpu/drm/i915/i915_gem_request.h
@@ -218,13 +218,33 @@ i915_gem_request_alloc(struct intel_engine_cs *engine,
                       struct i915_gem_context *ctx);
 void i915_gem_request_retire_upto(struct drm_i915_gem_request *req);
 
+#if IS_ENABLED(CONFIG_DRM_I915_REQUEST_DEBUG_OBJECTS)
+void debug_request_assert(const struct drm_i915_gem_request *request);
+#else
+static inline void
+debug_request_assert(const struct drm_i915_gem_request *request) {}
+#endif
+
+static inline void
+debug_request_assert_or_null(const struct drm_i915_gem_request *request)
+{
+       if (request)
+               debug_request_assert(request);
+}
+
 static inline struct drm_i915_gem_request *
 to_request(struct dma_fence *fence)
 {
+       struct drm_i915_gem_request *rq;
+
        /* We assume that NULL fence/request are interoperable */
        BUILD_BUG_ON(offsetof(struct drm_i915_gem_request, fence) != 0);
        GEM_BUG_ON(fence && !dma_fence_is_i915(fence));
-       return container_of(fence, struct drm_i915_gem_request, fence);
+
+       rq = container_of(fence, struct drm_i915_gem_request, fence);
+       debug_request_assert_or_null(rq);
+
+       return rq;
 }
 
 static inline struct drm_i915_gem_request *
@@ -242,6 +262,7 @@ i915_gem_request_get_rcu(struct drm_i915_gem_request *req)
 static inline void
 i915_gem_request_put(struct drm_i915_gem_request *req)
 {
+       debug_request_assert_or_null(req);
        dma_fence_put(&req->fence);
 }
 
@@ -266,6 +287,7 @@ i915_gem_request_put(struct drm_i915_gem_request *req)
 static u32
 i915_gem_request_global_seqno(const struct drm_i915_gem_request *request)
 {
+       debug_request_assert(request);
        return READ_ONCE(request->global_seqno);
 }
 
@@ -322,6 +344,8 @@ i915_gem_request_completed(const struct 
drm_i915_gem_request *req)
 {
        u32 seqno;
 
+       debug_request_assert(req);
+
        seqno = i915_gem_request_global_seqno(req);
        if (!seqno)
                return false;
@@ -334,6 +358,8 @@ i915_gem_request_started(const struct drm_i915_gem_request 
*req)
 {
        u32 seqno;
 
+       debug_request_assert(req);
+
        seqno = i915_gem_request_global_seqno(req);
        if (!seqno)
                return false;
@@ -423,6 +449,7 @@ static inline void
 i915_gem_active_set(struct i915_gem_active *active,
                    struct drm_i915_gem_request *request)
 {
+       debug_request_assert(request);
        list_move(&active->link, &request->active_list);
        rcu_assign_pointer(active->request, request);
 }
@@ -469,8 +496,12 @@ __i915_gem_active_peek(const struct i915_gem_active 
*active)
 static inline struct drm_i915_gem_request *
 i915_gem_active_raw(const struct i915_gem_active *active, struct mutex *mutex)
 {
-       return rcu_dereference_protected(active->request,
-                                        lockdep_is_held(mutex));
+       struct drm_i915_gem_request *rq =
+               rcu_dereference_protected(active->request,
+                                         lockdep_is_held(mutex));
+
+       debug_request_assert_or_null(rq);
+       return rq;
 }
 
 /**
-- 
2.15.1

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

Reply via email to