drm/i915: Use DRIVER_NAME for tracing unattached requests

From: Chris Wilson <[email protected]>

The first tracepoint for a request is trace_dma_fence_init called before we have associated the request with a device. The tracepoint uses fence->ops->get_driver_name() as a pretty name, and as we try to report the device name this oopses as it is then NULL. Support the early tracepoint by reporting the DRIVER_NAME instead of the actual device name.

Note that rq->engine remains during the course of request recycling (SLAB_TYPESAFE_BY_RCU). For the physical engines, the pointer remains valid, however a virtual engine may be destroyed after the request is retired. If we process a preempt-to-busy completed request along the virtual engine, we should make sure we mark the request as no longer belonging to the virtual engine to remove the dangling pointers from the tracepoint.

Fixes 855e39e65cfc ("drm/i915: Initialise basic fence before acquiring seqno")

Signed-off-by Chris Wilson <[email protected]>

Cc Tvrtko Ursulin <[email protected]>

Cc Chintan M Patel <[email protected]>

Cc Andi Shyti <[email protected]>

Cc <[email protected]> # v5.7+

Signed-off-by Matthew Auld <[email protected]>

---
 .../drm/i915/gt/intel_execlists_submission.c  | 20 ++++++++++++++-----
 drivers/gpu/drm/i915/i915_request.c           |  7 ++++++-
 2 files changed, 21 insertions(+), 6 deletions(-)

drivers/gpu/drm/i915/gt/intel_execlists_submission.c +15 -5

index de124870af44..75604e927d34 100644
@@ -3249,6 +3249,18 @@ static struct list_head *virtual_queue(struct virtual_engine *ve)
 	return &ve->base.execlists.default_priolist.requests;
 }
 
+static void
+virtual_submit_completed(struct virtual_engine *ve, struct i915_request *rq)
+{
+	GEM_BUG_ON(!__i915_request_is_complete(rq));
+	GEM_BUG_ON(rq->engine != &ve->base);
+
+	__i915_request_submit(rq);
+
+	/* Remove the dangling pointer to the stale virtual engine */
+	WRITE_ONCE(rq->engine, ve->siblings[0]);
+}
+
 static void rcu_virtual_context_destroy(struct work_struct *wrk)
 {
 	struct virtual_engine *ve =

@@ -3265,8 +3277,7 @@ static void rcu_virtual_context_destroy(struct work_struct *wrk)
 
 		old = fetch_and_zero(&ve->request);
 		if (old) {
-			GEM_BUG_ON(!__i915_request_is_complete(old));
-			__i915_request_submit(old);
+			virtual_submit_completed(ve, old);
 			i915_request_put(old);
 		}
 

@@ -3538,13 +3549,12 @@ static void virtual_submit_request(struct i915_request *rq)
 
 	/* By the time we resubmit a request, it may be completed */
 	if (__i915_request_is_complete(rq)) {
-		__i915_request_submit(rq);
+		virtual_submit_completed(ve, rq);
 		goto unlock;
 	}
 
 	if (ve->request) { /* background completion from preempt-to-busy */
-		GEM_BUG_ON(!__i915_request_is_complete(ve->request));
-		__i915_request_submit(ve->request);
+		virtual_submit_completed(ve, ve->request);
 		i915_request_put(ve->request);
 	}
 

drivers/gpu/drm/i915/i915_request.c +6 -1

index 970d8f4986bb..aa124adb1051 100644
@@ -61,7 +61,12 @@ static struct i915_global_request {
 
 static const char *i915_fence_get_driver_name(struct dma_fence *fence)
 {
-	return dev_name(to_request(fence)->engine->i915->drm.dev);
+	struct i915_request *rq = to_request(fence);
+
+	if (unlikely(!rq->engine)) /* not yet attached to any device */
+		return DRIVER_NAME;
+
+	return dev_name(rq->engine->i915->drm.dev);
 }
 
 static const char *i915_fence_get_timeline_name(struct dma_fence *fence)

Signature · 1 line

2.26.3

Quoted 9 lines · Matthew Auld

From: Chris Wilson <[email protected]>

The first tracepoint for a request is trace_dma_fence_init called before we have associated the request with a device. The tracepoint uses fence->ops->get_driver_name() as a pretty name, and as we try to report the device name this oopses as it is then NULL. Support the early tracepoint by reporting the DRIVER_NAME instead of the actual device name.

Quoted 7 lines

Note that rq->engine remains during the course of request recycling (SLAB_TYPESAFE_BY_RCU). For the physical engines, the pointer remains valid, however a virtual engine may be destroyed after the request is retired. If we process a preempt-to-busy completed request along the virtual engine, we should make sure we mark the request as no longer belonging to the virtual engine to remove the dangling pointers from the tracepoint.

Why can't we assign the request beforehand? The idea behind these tracepoints is that they actually match up, if trace_dma_fence_init is different, then we're breaking that.

-Daniel

Quoted 81 lines

Fixes 855e39e65cfc ("drm/i915: Initialise basic fence before acquiring seqno")

Signed-off-by Chris Wilson <[email protected]>

Cc Tvrtko Ursulin <[email protected]>

Cc Chintan M Patel <[email protected]>

Cc Andi Shyti <[email protected]>

Cc <[email protected]> # v5.7+

Signed-off-by Matthew Auld <[email protected]>

---
 .../drm/i915/gt/intel_execlists_submission.c  | 20 ++++++++++++++-----
 drivers/gpu/drm/i915/i915_request.c           |  7 ++++++-
 2 files changed, 21 insertions(+), 6 deletions(-)

drivers/gpu/drm/i915/gt/intel_execlists_submission.c +15 -5

index de124870af44..75604e927d34 100644
@@ -3249,6 +3249,18 @@ static struct list_head *virtual_queue(struct virtual_engine *ve)
      return &ve->base.execlists.default_priolist.requests;
 }
 
+static void
+virtual_submit_completed(struct virtual_engine *ve, struct i915_request *rq)
+{
+     GEM_BUG_ON(!__i915_request_is_complete(rq));
+     GEM_BUG_ON(rq->engine != &ve->base);
+
+     __i915_request_submit(rq);
+
+     /* Remove the dangling pointer to the stale virtual engine */
+     WRITE_ONCE(rq->engine, ve->siblings[0]);
+}
+
 static void rcu_virtual_context_destroy(struct work_struct *wrk)
 {
      struct virtual_engine *ve =

@@ -3265,8 +3277,7 @@ static void rcu_virtual_context_destroy(struct work_struct *wrk)
 
              old = fetch_and_zero(&ve->request);
              if (old) {
-                     GEM_BUG_ON(!__i915_request_is_complete(old));
-                     __i915_request_submit(old);
+                     virtual_submit_completed(ve, old);
                      i915_request_put(old);
              }
 

@@ -3538,13 +3549,12 @@ static void virtual_submit_request(struct i915_request *rq)
 
      /* By the time we resubmit a request, it may be completed */
      if (__i915_request_is_complete(rq)) {
-             __i915_request_submit(rq);
+             virtual_submit_completed(ve, rq);
              goto unlock;
      }
 
      if (ve->request) { /* background completion from preempt-to-busy */
-             GEM_BUG_ON(!__i915_request_is_complete(ve->request));
-             __i915_request_submit(ve->request);
+             virtual_submit_completed(ve, ve->request);
              i915_request_put(ve->request);
      }
 

drivers/gpu/drm/i915/i915_request.c +6 -2

index 970d8f4986bb..aa124adb1051 100644
@@ -61,7 +61,12 @@ static struct i915_global_request {
 
 static const char *i915_fence_get_driver_name(struct dma_fence *fence)
 {
-     return dev_name(to_request(fence)->engine->i915->drm.dev);
+     struct i915_request *rq = to_request(fence);
+
+     if (unlikely(!rq->engine)) /* not yet attached to any device */
+             return DRIVER_NAME;
+
+     return dev_name(rq->engine->i915->drm.dev);
 }
 
 static const char *i915_fence_get_timeline_name(struct dma_fence *fence)
--

Quoted 2 lines

2.26.3

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Quoted 19 lines · Daniel Vetter

Quoted 9 lines · Matthew Auld

From: Chris Wilson <[email protected]>

The first tracepoint for a request is trace_dma_fence_init called before we have associated the request with a device. The tracepoint uses fence->ops->get_driver_name() as a pretty name, and as we try to report the device name this oopses as it is then NULL. Support the early tracepoint by reporting the DRIVER_NAME instead of the actual device name.

Quoted 7 lines

Note that rq->engine remains during the course of request recycling (SLAB_TYPESAFE_BY_RCU). For the physical engines, the pointer remains valid, however a virtual engine may be destroyed after the request is retired. If we process a preempt-to-busy completed request along the virtual engine, we should make sure we mark the request as no longer belonging to the virtual engine to remove the dangling pointers from the tracepoint.

Quoted 3 lines

Why can't we assign the request beforehand? The idea behind these tracepoints is that they actually match up, if trace_dma_fence_init is different, then we're breaking that.

Ok I looked a bit more and pondered this a bit, and the initial tracepoint is called from dma_fence_init, where we haven't yet set up rq->engine properly. So that part makes sense, but should have a bigger comment that explains this a bit more and why we can't solve this in a neater way. Probably should also drop the unlikely(), this isn't a performance critical path, ever.

The other changes thgouh feel like they should be split out into a separate path, since they solve a conceptually totally different
issue: SLAB_TYPESAFE_BY_RCU recycling.

And I'm honestly not sure about
that one whether it's even correct, there's another patch floating around that sprinkles rcu_read_lock around some of these accesssors, and that would be a breakage of dma_fence interaces where outside of i915 rcu isn't required for this stuff. So imo should be split out, and come with a wider analysis of what's going on there and why and how exactly i915 works.

In generally SLAB_TYPESAFE_BY_RCU is extremely dangerous and I'm frankly not sure we have the perf data (outside of contrived microbenchmarks) showing that it's needed and justifies all the costs it's encurring.

-Daniel

Quoted 90 lines

-Daniel

Quoted 81 lines

Fixes 855e39e65cfc ("drm/i915: Initialise basic fence before acquiring seqno")

Signed-off-by Chris Wilson <[email protected]>

Cc Tvrtko Ursulin <[email protected]>

Cc Chintan M Patel <[email protected]>

Cc Andi Shyti <[email protected]>

Cc <[email protected]> # v5.7+

Signed-off-by Matthew Auld <[email protected]>

---
  .../drm/i915/gt/intel_execlists_submission.c  | 20 ++++++++++++++-----
  drivers/gpu/drm/i915/i915_request.c           |  7 ++++++-
  2 files changed, 21 insertions(+), 6 deletions(-)

drivers/gpu/drm/i915/gt/intel_execlists_submission.c +15 -5

index de124870af44..75604e927d34 100644
@@ -3249,6 +3249,18 @@ static struct list_head *virtual_queue(struct virtual_engine *ve)
       return &ve->base.execlists.default_priolist.requests;
  }
 
+static void
+virtual_submit_completed(struct virtual_engine *ve, struct i915_request *rq)
+{
+     GEM_BUG_ON(!__i915_request_is_complete(rq));
+     GEM_BUG_ON(rq->engine != &ve->base);
+
+     __i915_request_submit(rq);
+
+     /* Remove the dangling pointer to the stale virtual engine */
+     WRITE_ONCE(rq->engine, ve->siblings[0]);
+}
+
  static void rcu_virtual_context_destroy(struct work_struct *wrk)
  {
       struct virtual_engine *ve =

@@ -3265,8 +3277,7 @@ static void rcu_virtual_context_destroy(struct work_struct *wrk)
 
               old = fetch_and_zero(&ve->request);
               if (old) {
-                     GEM_BUG_ON(!__i915_request_is_complete(old));
-                     __i915_request_submit(old);
+                     virtual_submit_completed(ve, old);
                       i915_request_put(old);
               }
 

@@ -3538,13 +3549,12 @@ static void virtual_submit_request(struct i915_request *rq)
 
       /* By the time we resubmit a request, it may be completed */
       if (__i915_request_is_complete(rq)) {
-             __i915_request_submit(rq);
+             virtual_submit_completed(ve, rq);
               goto unlock;
       }
 
       if (ve->request) { /* background completion from preempt-to-busy */
-             GEM_BUG_ON(!__i915_request_is_complete(ve->request));
-             __i915_request_submit(ve->request);
+             virtual_submit_completed(ve, ve->request);
               i915_request_put(ve->request);
       }
 

drivers/gpu/drm/i915/i915_request.c +6 -2

index 970d8f4986bb..aa124adb1051 100644
@@ -61,7 +61,12 @@ static struct i915_global_request {
 
  static const char *i915_fence_get_driver_name(struct dma_fence *fence)
  {
-     return dev_name(to_request(fence)->engine->i915->drm.dev);
+     struct i915_request *rq = to_request(fence);
+
+     if (unlikely(!rq->engine)) /* not yet attached to any device */
+             return DRIVER_NAME;
+
+     return dev_name(rq->engine->i915->drm.dev);
  }
 
  static const char *i915_fence_get_timeline_name(struct dma_fence *fence)
--

Quoted 2 lines

2.26.3

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Quoted 31 lines · Daniel Vetter

Quoted 19 lines · Daniel Vetter

Quoted 9 lines · Matthew Auld

From: Chris Wilson <[email protected]>

The first tracepoint for a request is trace_dma_fence_init called before we have associated the request with a device. The tracepoint uses fence->ops->get_driver_name() as a pretty name, and as we try to report the device name this oopses as it is then NULL. Support the early tracepoint by reporting the DRIVER_NAME instead of the actual device name.

Quoted 7 lines

Note that rq->engine remains during the course of request recycling (SLAB_TYPESAFE_BY_RCU). For the physical engines, the pointer remains valid, however a virtual engine may be destroyed after the request is retired. If we process a preempt-to-busy completed request along the virtual engine, we should make sure we mark the request as no longer belonging to the virtual engine to remove the dangling pointers from the tracepoint.

Quoted 3 lines

Why can't we assign the request beforehand? The idea behind these tracepoints is that they actually match up, if trace_dma_fence_init is different, then we're breaking that.

Ok I looked a bit more and pondered this a bit, and the initial tracepoint is called from dma_fence_init, where we haven't yet set up rq->engine properly. So that part makes sense, but should have a bigger comment that explains this a bit more and why we can't solve this in a neater way. Probably should also drop the unlikely(), this isn't a performance critical path, ever.

Quoted 3 lines

The other changes thgouh feel like they should be split out into a separate path, since they solve a conceptually totally different
issue: SLAB_TYPESAFE_BY_RCU recycling.

Hmm, I thought it all stems from having to tread very carefully around SLAB_TYPESAFE_BY_RCU? If this were "normal" code, we would just allocate the rq, initialise it properly, including the rq->engine, and only then do the dma_fence_init? Or am I missing something?

I'm happy to split it though. And I think that bit at least fixes the user reported issue I think.

Quoted 8 lines

And I'm honestly not sure about
that one whether it's even correct, there's another patch floating around that sprinkles rcu_read_lock around some of these accesssors, and that would be a breakage of dma_fence interaces where outside of i915 rcu isn't required for this stuff. So imo should be split out, and come with a wider analysis of what's going on there and why and how exactly i915 works.

Quoted 4 lines

In generally SLAB_TYPESAFE_BY_RCU is extremely dangerous and I'm frankly not sure we have the perf data (outside of contrived microbenchmarks) showing that it's needed and justifies all the costs it's encurring.

Right, I can try to search the git history.

Quoted 97 lines

-Daniel

Quoted 90 lines

-Daniel

Quoted 81 lines

Fixes 855e39e65cfc ("drm/i915: Initialise basic fence before acquiring seqno")

Signed-off-by Chris Wilson <[email protected]>

Cc Tvrtko Ursulin <[email protected]>

Cc Chintan M Patel <[email protected]>

Cc Andi Shyti <[email protected]>

Cc <[email protected]> # v5.7+

Signed-off-by Matthew Auld <[email protected]>

---
  .../drm/i915/gt/intel_execlists_submission.c  | 20 ++++++++++++++-----
  drivers/gpu/drm/i915/i915_request.c           |  7 ++++++-
  2 files changed, 21 insertions(+), 6 deletions(-)

drivers/gpu/drm/i915/gt/intel_execlists_submission.c +15 -5

index de124870af44..75604e927d34 100644
@@ -3249,6 +3249,18 @@ static struct list_head *virtual_queue(struct virtual_engine *ve)
       return &ve->base.execlists.default_priolist.requests;
  }
 
+static void
+virtual_submit_completed(struct virtual_engine *ve, struct i915_request *rq)
+{
+     GEM_BUG_ON(!__i915_request_is_complete(rq));
+     GEM_BUG_ON(rq->engine != &ve->base);
+
+     __i915_request_submit(rq);
+
+     /* Remove the dangling pointer to the stale virtual engine */
+     WRITE_ONCE(rq->engine, ve->siblings[0]);
+}
+
  static void rcu_virtual_context_destroy(struct work_struct *wrk)
  {
       struct virtual_engine *ve =

@@ -3265,8 +3277,7 @@ static void rcu_virtual_context_destroy(struct work_struct *wrk)
 
               old = fetch_and_zero(&ve->request);
               if (old) {
-                     GEM_BUG_ON(!__i915_request_is_complete(old));
-                     __i915_request_submit(old);
+                     virtual_submit_completed(ve, old);
                       i915_request_put(old);
               }
 

@@ -3538,13 +3549,12 @@ static void virtual_submit_request(struct i915_request *rq)
 
       /* By the time we resubmit a request, it may be completed */
       if (__i915_request_is_complete(rq)) {
-             __i915_request_submit(rq);
+             virtual_submit_completed(ve, rq);
               goto unlock;
       }
 
       if (ve->request) { /* background completion from preempt-to-busy */
-             GEM_BUG_ON(!__i915_request_is_complete(ve->request));
-             __i915_request_submit(ve->request);
+             virtual_submit_completed(ve, ve->request);
               i915_request_put(ve->request);
       }
 

drivers/gpu/drm/i915/i915_request.c +6 -2

index 970d8f4986bb..aa124adb1051 100644
@@ -61,7 +61,12 @@ static struct i915_global_request {
 
  static const char *i915_fence_get_driver_name(struct dma_fence *fence)
  {
-     return dev_name(to_request(fence)->engine->i915->drm.dev);
+     struct i915_request *rq = to_request(fence);
+
+     if (unlikely(!rq->engine)) /* not yet attached to any device */
+             return DRIVER_NAME;
+
+     return dev_name(rq->engine->i915->drm.dev);
  }
 
  static const char *i915_fence_get_timeline_name(struct dma_fence *fence)
--

Quoted 2 lines

2.26.3

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch