All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: Use DRIVER_NAME for tracing unattached requests
@ 2021-05-20  7:35 ` Matthew Auld
  0 siblings, 0 replies; 17+ messages in thread
From: Matthew Auld @ 2021-05-20  7:35 UTC (permalink / raw)
  To: intel-gfx
  Cc: dri-devel, Chris Wilson, Tvrtko Ursulin, Chintan M Patel,
	Andi Shyti, stable

From: Chris Wilson <chris@chris-wilson.co.uk>

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 <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Chintan M Patel <chintan.m.patel@intel.com>
Cc: Andi Shyti <andi.shyti@intel.com>
Cc: <stable@vger.kernel.org> # v5.7+
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
---
 .../drm/i915/gt/intel_execlists_submission.c  | 20 ++++++++++++++-----
 drivers/gpu/drm/i915/i915_request.c           |  7 ++++++-
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
index de124870af44..75604e927d34 100644
--- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
@@ -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);
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 970d8f4986bb..aa124adb1051 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -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)
-- 
2.26.3


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH] drm/i915: Use DRIVER_NAME for tracing unattached requests
@ 2021-05-20  7:35 ` Matthew Auld
  0 siblings, 0 replies; 17+ messages in thread
From: Matthew Auld @ 2021-05-20  7:35 UTC (permalink / raw)
  To: intel-gfx
  Cc: Tvrtko Ursulin, Andi Shyti, stable, Chris Wilson,
	Chintan M Patel, dri-devel

From: Chris Wilson <chris@chris-wilson.co.uk>

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 <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Chintan M Patel <chintan.m.patel@intel.com>
Cc: Andi Shyti <andi.shyti@intel.com>
Cc: <stable@vger.kernel.org> # v5.7+
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
---
 .../drm/i915/gt/intel_execlists_submission.c  | 20 ++++++++++++++-----
 drivers/gpu/drm/i915/i915_request.c           |  7 ++++++-
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
index de124870af44..75604e927d34 100644
--- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
@@ -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);
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 970d8f4986bb..aa124adb1051 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -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)
-- 
2.26.3


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Intel-gfx] [PATCH] drm/i915: Use DRIVER_NAME for tracing unattached requests
@ 2021-05-20  7:35 ` Matthew Auld
  0 siblings, 0 replies; 17+ messages in thread
From: Matthew Auld @ 2021-05-20  7:35 UTC (permalink / raw)
  To: intel-gfx; +Cc: stable, Chris Wilson, Chintan M Patel, dri-devel

From: Chris Wilson <chris@chris-wilson.co.uk>

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 <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Chintan M Patel <chintan.m.patel@intel.com>
Cc: Andi Shyti <andi.shyti@intel.com>
Cc: <stable@vger.kernel.org> # v5.7+
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
---
 .../drm/i915/gt/intel_execlists_submission.c  | 20 ++++++++++++++-----
 drivers/gpu/drm/i915/i915_request.c           |  7 ++++++-
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
index de124870af44..75604e927d34 100644
--- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
@@ -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);
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 970d8f4986bb..aa124adb1051 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -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)
-- 
2.26.3

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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915: Use DRIVER_NAME for tracing unattached requests
  2021-05-20  7:35 ` Matthew Auld
  (?)
  (?)
@ 2021-05-20  9:45 ` Patchwork
  -1 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2021-05-20  9:45 UTC (permalink / raw)
  To: Matthew Auld; +Cc: intel-gfx


[-- Attachment #1.1: Type: text/plain, Size: 3925 bytes --]

== Series Details ==

Series: drm/i915: Use DRIVER_NAME for tracing unattached requests
URL   : https://patchwork.freedesktop.org/series/90351/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_10110 -> Patchwork_20157
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/index.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_cs_nop@sync-fork-compute0:
    - fi-snb-2600:        NOTRUN -> [SKIP][1] ([fdo#109271]) +17 similar issues
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/fi-snb-2600/igt@amdgpu/amd_cs_nop@sync-fork-compute0.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@hangcheck:
    - fi-snb-2600:        [INCOMPLETE][2] ([i915#2782]) -> [PASS][3]
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/fi-snb-2600/igt@i915_selftest@live@hangcheck.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/fi-snb-2600/igt@i915_selftest@live@hangcheck.html

  * igt@kms_frontbuffer_tracking@basic:
    - fi-icl-u2:          [FAIL][4] ([i915#49]) -> [PASS][5]
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/fi-icl-u2/igt@kms_frontbuffer_tracking@basic.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/fi-icl-u2/igt@kms_frontbuffer_tracking@basic.html

  
#### Warnings ####

  * igt@runner@aborted:
    - fi-cml-u2:          [FAIL][6] ([i915#2082] / [i915#2426] / [i915#3363]) -> [FAIL][7] ([i915#3363])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/fi-cml-u2/igt@runner@aborted.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/fi-cml-u2/igt@runner@aborted.html
    - fi-cfl-guc:         [FAIL][8] ([i915#2426] / [i915#3363]) -> [FAIL][9] ([i915#3363])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/fi-cfl-guc/igt@runner@aborted.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/fi-cfl-guc/igt@runner@aborted.html
    - fi-skl-6700k2:      [FAIL][10] ([i915#1436] / [i915#2426] / [i915#3363]) -> [FAIL][11] ([i915#1436] / [i915#3363])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/fi-skl-6700k2/igt@runner@aborted.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/fi-skl-6700k2/igt@runner@aborted.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#1222]: https://gitlab.freedesktop.org/drm/intel/issues/1222
  [i915#1436]: https://gitlab.freedesktop.org/drm/intel/issues/1436
  [i915#2082]: https://gitlab.freedesktop.org/drm/intel/issues/2082
  [i915#2426]: https://gitlab.freedesktop.org/drm/intel/issues/2426
  [i915#2782]: https://gitlab.freedesktop.org/drm/intel/issues/2782
  [i915#3363]: https://gitlab.freedesktop.org/drm/intel/issues/3363
  [i915#49]: https://gitlab.freedesktop.org/drm/intel/issues/49


Participating hosts (42 -> 38)
------------------------------

  Missing    (4): fi-kbl-soraka fi-bsw-cyan fi-bdw-samus fi-hsw-4200u 


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

  * Linux: CI_DRM_10110 -> Patchwork_20157

  CI-20190529: 20190529
  CI_DRM_10110: 9a93475b2ef77cbd4ea83aebf145fbe89dd01ced @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6089: 698613116728db5000759e69c074ce6ab2131765 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_20157: 212f1a752c55f2a2cdb441f1e36e000448f57245 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

212f1a752c55 drm/i915: Use DRIVER_NAME for tracing unattached requests

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/index.html

[-- Attachment #1.2: Type: text/html, Size: 5108 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] drm/i915: Use DRIVER_NAME for tracing unattached requests
  2021-05-20  7:35 ` Matthew Auld
  (?)
@ 2021-05-20 14:28   ` Daniel Vetter
  -1 siblings, 0 replies; 17+ messages in thread
From: Daniel Vetter @ 2021-05-20 14:28 UTC (permalink / raw)
  To: Matthew Auld
  Cc: intel-gfx, Tvrtko Ursulin, Andi Shyti, stable, Chris Wilson,
	Chintan M Patel, dri-devel

On Thu, May 20, 2021 at 08:35:14AM +0100, Matthew Auld wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> 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.

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

> 
> Fixes: 855e39e65cfc ("drm/i915: Initialise basic fence before acquiring seqno")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Chintan M Patel <chintan.m.patel@intel.com>
> Cc: Andi Shyti <andi.shyti@intel.com>
> Cc: <stable@vger.kernel.org> # v5.7+
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> ---
>  .../drm/i915/gt/intel_execlists_submission.c  | 20 ++++++++++++++-----
>  drivers/gpu/drm/i915/i915_request.c           |  7 ++++++-
>  2 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> index de124870af44..75604e927d34 100644
> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> @@ -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);
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 970d8f4986bb..aa124adb1051 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -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)
> -- 
> 2.26.3
> 

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] drm/i915: Use DRIVER_NAME for tracing unattached requests
@ 2021-05-20 14:28   ` Daniel Vetter
  0 siblings, 0 replies; 17+ messages in thread
From: Daniel Vetter @ 2021-05-20 14:28 UTC (permalink / raw)
  To: Matthew Auld
  Cc: Tvrtko Ursulin, Andi Shyti, intel-gfx, stable, Chris Wilson,
	Chintan M Patel, dri-devel

On Thu, May 20, 2021 at 08:35:14AM +0100, Matthew Auld wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> 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.

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

> 
> Fixes: 855e39e65cfc ("drm/i915: Initialise basic fence before acquiring seqno")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Chintan M Patel <chintan.m.patel@intel.com>
> Cc: Andi Shyti <andi.shyti@intel.com>
> Cc: <stable@vger.kernel.org> # v5.7+
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> ---
>  .../drm/i915/gt/intel_execlists_submission.c  | 20 ++++++++++++++-----
>  drivers/gpu/drm/i915/i915_request.c           |  7 ++++++-
>  2 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> index de124870af44..75604e927d34 100644
> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> @@ -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);
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 970d8f4986bb..aa124adb1051 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -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)
> -- 
> 2.26.3
> 

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915: Use DRIVER_NAME for tracing unattached requests
@ 2021-05-20 14:28   ` Daniel Vetter
  0 siblings, 0 replies; 17+ messages in thread
From: Daniel Vetter @ 2021-05-20 14:28 UTC (permalink / raw)
  To: Matthew Auld; +Cc: intel-gfx, stable, Chris Wilson, Chintan M Patel, dri-devel

On Thu, May 20, 2021 at 08:35:14AM +0100, Matthew Auld wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> 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.

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

> 
> Fixes: 855e39e65cfc ("drm/i915: Initialise basic fence before acquiring seqno")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Chintan M Patel <chintan.m.patel@intel.com>
> Cc: Andi Shyti <andi.shyti@intel.com>
> Cc: <stable@vger.kernel.org> # v5.7+
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> ---
>  .../drm/i915/gt/intel_execlists_submission.c  | 20 ++++++++++++++-----
>  drivers/gpu/drm/i915/i915_request.c           |  7 ++++++-
>  2 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> index de124870af44..75604e927d34 100644
> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> @@ -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);
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 970d8f4986bb..aa124adb1051 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -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)
> -- 
> 2.26.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [Intel-gfx] ✓ Fi.CI.IGT: success for drm/i915: Use DRIVER_NAME for tracing unattached requests
  2021-05-20  7:35 ` Matthew Auld
                   ` (3 preceding siblings ...)
  (?)
@ 2021-05-21 14:52 ` Patchwork
  -1 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2021-05-21 14:52 UTC (permalink / raw)
  To: Matthew Auld; +Cc: intel-gfx


[-- Attachment #1.1: Type: text/plain, Size: 30280 bytes --]

== Series Details ==

Series: drm/i915: Use DRIVER_NAME for tracing unattached requests
URL   : https://patchwork.freedesktop.org/series/90351/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_10110_full -> Patchwork_20157_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_isolation@preservation-s3@vcs0:
    - shard-skl:          [PASS][1] -> [INCOMPLETE][2] ([i915#198])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-skl2/igt@gem_ctx_isolation@preservation-s3@vcs0.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-skl1/igt@gem_ctx_isolation@preservation-s3@vcs0.html

  * igt@gem_ctx_param@set-priority-not-supported:
    - shard-tglb:         NOTRUN -> [SKIP][3] ([fdo#109314])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-tglb7/igt@gem_ctx_param@set-priority-not-supported.html

  * igt@gem_ctx_persistence@legacy-engines-queued:
    - shard-snb:          NOTRUN -> [SKIP][4] ([fdo#109271] / [i915#1099]) +1 similar issue
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-snb7/igt@gem_ctx_persistence@legacy-engines-queued.html

  * igt@gem_eio@unwedge-stress:
    - shard-snb:          NOTRUN -> [FAIL][5] ([i915#3354])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-snb7/igt@gem_eio@unwedge-stress.html

  * igt@gem_exec_fair@basic-deadline:
    - shard-kbl:          NOTRUN -> [FAIL][6] ([i915#2846])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-kbl1/igt@gem_exec_fair@basic-deadline.html

  * igt@gem_exec_fair@basic-none-rrul@rcs0:
    - shard-glk:          [PASS][7] -> [FAIL][8] ([i915#2842])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-glk7/igt@gem_exec_fair@basic-none-rrul@rcs0.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-glk3/igt@gem_exec_fair@basic-none-rrul@rcs0.html

  * igt@gem_exec_fair@basic-none-vip@rcs0:
    - shard-kbl:          [PASS][9] -> [FAIL][10] ([i915#2842])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-kbl4/igt@gem_exec_fair@basic-none-vip@rcs0.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-kbl3/igt@gem_exec_fair@basic-none-vip@rcs0.html

  * igt@gem_exec_fair@basic-pace-share@rcs0:
    - shard-tglb:         [PASS][11] -> [FAIL][12] ([i915#2842]) +1 similar issue
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-tglb7/igt@gem_exec_fair@basic-pace-share@rcs0.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-tglb5/igt@gem_exec_fair@basic-pace-share@rcs0.html

  * igt@gem_exec_fair@basic-pace@bcs0:
    - shard-tglb:         NOTRUN -> [FAIL][13] ([i915#2842]) +4 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-tglb7/igt@gem_exec_fair@basic-pace@bcs0.html

  * igt@gem_exec_flush@basic-batch-kernel-default-cmd:
    - shard-tglb:         NOTRUN -> [SKIP][14] ([fdo#109313])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-tglb7/igt@gem_exec_flush@basic-batch-kernel-default-cmd.html

  * igt@gem_exec_reloc@basic-wide-active@bcs0:
    - shard-apl:          NOTRUN -> [FAIL][15] ([i915#2389]) +3 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-apl7/igt@gem_exec_reloc@basic-wide-active@bcs0.html

  * igt@gem_exec_reloc@basic-wide-active@vcs1:
    - shard-iclb:         NOTRUN -> [FAIL][16] ([i915#2389])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-iclb2/igt@gem_exec_reloc@basic-wide-active@vcs1.html

  * igt@gem_mmap_gtt@big-copy-xy:
    - shard-skl:          [PASS][17] -> [FAIL][18] ([i915#307])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-skl7/igt@gem_mmap_gtt@big-copy-xy.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-skl7/igt@gem_mmap_gtt@big-copy-xy.html

  * igt@gem_mmap_gtt@cpuset-basic-small-copy:
    - shard-apl:          NOTRUN -> [INCOMPLETE][19] ([i915#3468]) +1 similar issue
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-apl7/igt@gem_mmap_gtt@cpuset-basic-small-copy.html

  * igt@gem_mmap_gtt@cpuset-basic-small-copy-odd:
    - shard-glk:          NOTRUN -> [INCOMPLETE][20] ([i915#2055] / [i915#3468])
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-glk6/igt@gem_mmap_gtt@cpuset-basic-small-copy-odd.html

  * igt@gem_mmap_gtt@cpuset-basic-small-copy-xy:
    - shard-snb:          NOTRUN -> [INCOMPLETE][21] ([i915#2055] / [i915#3468])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-snb2/igt@gem_mmap_gtt@cpuset-basic-small-copy-xy.html
    - shard-kbl:          NOTRUN -> [INCOMPLETE][22] ([i915#3468]) +1 similar issue
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-kbl4/igt@gem_mmap_gtt@cpuset-basic-small-copy-xy.html

  * igt@gem_mmap_gtt@cpuset-big-copy:
    - shard-iclb:         [PASS][23] -> [FAIL][24] ([i915#307])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-iclb5/igt@gem_mmap_gtt@cpuset-big-copy.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-iclb2/igt@gem_mmap_gtt@cpuset-big-copy.html

  * igt@gem_mmap_gtt@cpuset-medium-copy-xy:
    - shard-tglb:         [PASS][25] -> [INCOMPLETE][26] ([i915#3468] / [i915#750])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-tglb8/igt@gem_mmap_gtt@cpuset-medium-copy-xy.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-tglb2/igt@gem_mmap_gtt@cpuset-medium-copy-xy.html
    - shard-iclb:         [PASS][27] -> [FAIL][28] ([i915#2428]) +1 similar issue
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-iclb2/igt@gem_mmap_gtt@cpuset-medium-copy-xy.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-iclb1/igt@gem_mmap_gtt@cpuset-medium-copy-xy.html

  * igt@gem_mmap_gtt@fault-concurrent:
    - shard-glk:          NOTRUN -> [INCOMPLETE][29] ([i915#3468])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-glk3/igt@gem_mmap_gtt@fault-concurrent.html

  * igt@gem_mmap_gtt@fault-concurrent-x:
    - shard-skl:          NOTRUN -> [INCOMPLETE][30] ([i915#198] / [i915#3468])
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-skl3/igt@gem_mmap_gtt@fault-concurrent-x.html

  * igt@gem_pread@exhaustion:
    - shard-snb:          NOTRUN -> [WARN][31] ([i915#2658])
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-snb7/igt@gem_pread@exhaustion.html

  * igt@gem_pwrite@basic-exhaustion:
    - shard-apl:          NOTRUN -> [WARN][32] ([i915#2658])
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-apl7/igt@gem_pwrite@basic-exhaustion.html

  * igt@gem_render_copy@x-tiled-to-vebox-yf-tiled:
    - shard-kbl:          NOTRUN -> [SKIP][33] ([fdo#109271]) +149 similar issues
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-kbl7/igt@gem_render_copy@x-tiled-to-vebox-yf-tiled.html

  * igt@gem_userptr_blits@dmabuf-sync:
    - shard-kbl:          NOTRUN -> [SKIP][34] ([fdo#109271] / [i915#3323])
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-kbl4/igt@gem_userptr_blits@dmabuf-sync.html

  * igt@gem_userptr_blits@input-checking:
    - shard-snb:          NOTRUN -> [DMESG-WARN][35] ([i915#3002])
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-snb7/igt@gem_userptr_blits@input-checking.html

  * igt@gem_userptr_blits@set-cache-level:
    - shard-apl:          NOTRUN -> [FAIL][36] ([i915#3324])
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-apl7/igt@gem_userptr_blits@set-cache-level.html

  * igt@gem_userptr_blits@unsync-unmap:
    - shard-tglb:         NOTRUN -> [SKIP][37] ([i915#3297])
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-tglb7/igt@gem_userptr_blits@unsync-unmap.html

  * igt@gen7_exec_parse@basic-offset:
    - shard-apl:          NOTRUN -> [SKIP][38] ([fdo#109271]) +145 similar issues
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-apl7/igt@gen7_exec_parse@basic-offset.html

  * igt@gen9_exec_parse@allowed-single:
    - shard-skl:          [PASS][39] -> [DMESG-WARN][40] ([i915#1436] / [i915#716])
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-skl1/igt@gen9_exec_parse@allowed-single.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-skl7/igt@gen9_exec_parse@allowed-single.html

  * igt@kms_async_flips@alternate-sync-async-flip:
    - shard-skl:          NOTRUN -> [FAIL][41] ([i915#2521])
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-skl3/igt@kms_async_flips@alternate-sync-async-flip.html

  * igt@kms_big_fb@yf-tiled-32bpp-rotate-0:
    - shard-tglb:         NOTRUN -> [SKIP][42] ([fdo#111615]) +1 similar issue
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-tglb7/igt@kms_big_fb@yf-tiled-32bpp-rotate-0.html

  * igt@kms_big_joiner@basic:
    - shard-apl:          NOTRUN -> [SKIP][43] ([fdo#109271] / [i915#2705])
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-apl7/igt@kms_big_joiner@basic.html

  * igt@kms_ccs@pipe-a-ccs-on-another-bo:
    - shard-snb:          NOTRUN -> [SKIP][44] ([fdo#109271]) +359 similar issues
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-snb2/igt@kms_ccs@pipe-a-ccs-on-another-bo.html

  * igt@kms_ccs@pipe-a-crc-sprite-planes-basic:
    - shard-skl:          [PASS][45] -> [DMESG-WARN][46] ([i915#1982])
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-skl6/igt@kms_ccs@pipe-a-crc-sprite-planes-basic.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-skl6/igt@kms_ccs@pipe-a-crc-sprite-planes-basic.html

  * igt@kms_chamelium@dp-hpd-for-each-pipe:
    - shard-apl:          NOTRUN -> [SKIP][47] ([fdo#109271] / [fdo#111827]) +9 similar issues
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-apl7/igt@kms_chamelium@dp-hpd-for-each-pipe.html

  * igt@kms_chamelium@hdmi-mode-timings:
    - shard-snb:          NOTRUN -> [SKIP][48] ([fdo#109271] / [fdo#111827]) +21 similar issues
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-snb2/igt@kms_chamelium@hdmi-mode-timings.html

  * igt@kms_color_chamelium@pipe-c-ctm-0-25:
    - shard-kbl:          NOTRUN -> [SKIP][49] ([fdo#109271] / [fdo#111827]) +10 similar issues
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-kbl1/igt@kms_color_chamelium@pipe-c-ctm-0-25.html

  * igt@kms_color_chamelium@pipe-c-ctm-red-to-blue:
    - shard-glk:          NOTRUN -> [SKIP][50] ([fdo#109271] / [fdo#111827]) +7 similar issues
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-glk3/igt@kms_color_chamelium@pipe-c-ctm-red-to-blue.html

  * igt@kms_color_chamelium@pipe-d-ctm-red-to-blue:
    - shard-tglb:         NOTRUN -> [SKIP][51] ([fdo#109284] / [fdo#111827]) +3 similar issues
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-tglb7/igt@kms_color_chamelium@pipe-d-ctm-red-to-blue.html

  * igt@kms_content_protection@atomic:
    - shard-kbl:          NOTRUN -> [TIMEOUT][52] ([i915#1319]) +2 similar issues
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-kbl1/igt@kms_content_protection@atomic.html

  * igt@kms_content_protection@type1:
    - shard-tglb:         NOTRUN -> [SKIP][53] ([fdo#111828])
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-tglb7/igt@kms_content_protection@type1.html

  * igt@kms_cursor_crc@pipe-b-cursor-32x32-offscreen:
    - shard-tglb:         NOTRUN -> [SKIP][54] ([i915#3319])
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-tglb7/igt@kms_cursor_crc@pipe-b-cursor-32x32-offscreen.html

  * igt@kms_cursor_crc@pipe-c-cursor-suspend:
    - shard-kbl:          [PASS][55] -> [DMESG-WARN][56] ([i915#180]) +5 similar issues
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-kbl1/igt@kms_cursor_crc@pipe-c-cursor-suspend.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-kbl7/igt@kms_cursor_crc@pipe-c-cursor-suspend.html

  * igt@kms_cursor_crc@pipe-d-cursor-512x512-offscreen:
    - shard-tglb:         NOTRUN -> [SKIP][57] ([fdo#109279] / [i915#3359])
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-tglb7/igt@kms_cursor_crc@pipe-d-cursor-512x512-offscreen.html

  * igt@kms_cursor_crc@pipe-d-cursor-max-size-rapid-movement:
    - shard-tglb:         NOTRUN -> [SKIP][58] ([i915#3359]) +2 similar issues
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-tglb7/igt@kms_cursor_crc@pipe-d-cursor-max-size-rapid-movement.html

  * igt@kms_flip@2x-flip-vs-panning-vs-hang:
    - shard-skl:          NOTRUN -> [SKIP][59] ([fdo#109271]) +19 similar issues
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-skl8/igt@kms_flip@2x-flip-vs-panning-vs-hang.html

  * igt@kms_frontbuffer_tracking@fbc-2p-rte:
    - shard-tglb:         NOTRUN -> [SKIP][60] ([fdo#111825]) +24 similar issues
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-tglb7/igt@kms_frontbuffer_tracking@fbc-2p-rte.html

  * igt@kms_frontbuffer_tracking@fbc-modesetfrombusy:
    - shard-glk:          [PASS][61] -> [FAIL][62] ([i915#49])
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-glk8/igt@kms_frontbuffer_tracking@fbc-modesetfrombusy.html
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-glk4/igt@kms_frontbuffer_tracking@fbc-modesetfrombusy.html

  * igt@kms_frontbuffer_tracking@fbcpsr-rgb101010-draw-mmap-cpu:
    - shard-glk:          NOTRUN -> [SKIP][63] ([fdo#109271]) +40 similar issues
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-glk5/igt@kms_frontbuffer_tracking@fbcpsr-rgb101010-draw-mmap-cpu.html

  * igt@kms_pipe_b_c_ivb@enable-pipe-c-while-b-has-3-lanes:
    - shard-tglb:         NOTRUN -> [SKIP][64] ([fdo#109289]) +2 similar issues
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-tglb7/igt@kms_pipe_b_c_ivb@enable-pipe-c-while-b-has-3-lanes.html

  * igt@kms_pipe_crc_basic@hang-read-crc-pipe-d:
    - shard-kbl:          NOTRUN -> [SKIP][65] ([fdo#109271] / [i915#533])
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-kbl4/igt@kms_pipe_crc_basic@hang-read-crc-pipe-d.html
    - shard-apl:          NOTRUN -> [SKIP][66] ([fdo#109271] / [i915#533])
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-apl7/igt@kms_pipe_crc_basic@hang-read-crc-pipe-d.html

  * igt@kms_plane_alpha_blend@pipe-a-alpha-basic:
    - shard-apl:          NOTRUN -> [FAIL][67] ([fdo#108145] / [i915#265]) +2 similar issues
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-apl6/igt@kms_plane_alpha_blend@pipe-a-alpha-basic.html

  * igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min:
    - shard-skl:          [PASS][68] -> [FAIL][69] ([fdo#108145] / [i915#265])
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-skl9/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-skl5/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html

  * igt@kms_plane_alpha_blend@pipe-b-alpha-transparent-fb:
    - shard-kbl:          NOTRUN -> [FAIL][70] ([i915#265])
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-kbl1/igt@kms_plane_alpha_blend@pipe-b-alpha-transparent-fb.html

  * igt@kms_plane_alpha_blend@pipe-b-constant-alpha-max:
    - shard-kbl:          NOTRUN -> [FAIL][71] ([fdo#108145] / [i915#265])
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-kbl4/igt@kms_plane_alpha_blend@pipe-b-constant-alpha-max.html

  * igt@kms_prime@basic-crc@first-to-second:
    - shard-tglb:         NOTRUN -> [SKIP][72] ([i915#1836])
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-tglb7/igt@kms_prime@basic-crc@first-to-second.html

  * igt@kms_psr2_sf@overlay-plane-update-sf-dmg-area-1:
    - shard-tglb:         NOTRUN -> [SKIP][73] ([i915#2920])
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-tglb7/igt@kms_psr2_sf@overlay-plane-update-sf-dmg-area-1.html

  * igt@kms_psr2_sf@plane-move-sf-dmg-area-2:
    - shard-apl:          NOTRUN -> [SKIP][74] ([fdo#109271] / [i915#658]) +2 similar issues
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-apl7/igt@kms_psr2_sf@plane-move-sf-dmg-area-2.html

  * igt@kms_psr2_sf@primary-plane-update-sf-dmg-area-2:
    - shard-kbl:          NOTRUN -> [SKIP][75] ([fdo#109271] / [i915#658]) +5 similar issues
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-kbl1/igt@kms_psr2_sf@primary-plane-update-sf-dmg-area-2.html

  * igt@kms_psr2_su@page_flip:
    - shard-glk:          NOTRUN -> [SKIP][76] ([fdo#109271] / [i915#658])
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-glk6/igt@kms_psr2_su@page_flip.html

  * igt@kms_psr@psr2_cursor_plane_onoff:
    - shard-iclb:         [PASS][77] -> [SKIP][78] ([fdo#109441]) +1 similar issue
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-iclb2/igt@kms_psr@psr2_cursor_plane_onoff.html
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-iclb1/igt@kms_psr@psr2_cursor_plane_onoff.html

  * igt@kms_vrr@flipline:
    - shard-tglb:         NOTRUN -> [SKIP][79] ([fdo#109502]) +1 similar issue
   [79]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-tglb7/igt@kms_vrr@flipline.html

  * igt@kms_writeback@writeback-pixel-formats:
    - shard-apl:          NOTRUN -> [SKIP][80] ([fdo#109271] / [i915#2437])
   [80]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-apl7/igt@kms_writeback@writeback-pixel-formats.html

  * igt@nouveau_crc@pipe-c-source-outp-inactive:
    - shard-tglb:         NOTRUN -> [SKIP][81] ([i915#2530])
   [81]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-tglb7/igt@nouveau_crc@pipe-c-source-outp-inactive.html

  * igt@perf@polling-parameterized:
    - shard-tglb:         [PASS][82] -> [FAIL][83] ([i915#1542])
   [82]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-tglb1/igt@perf@polling-parameterized.html
   [83]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-tglb5/igt@perf@polling-parameterized.html

  * igt@perf_pmu@rc6-suspend:
    - shard-skl:          [PASS][84] -> [INCOMPLETE][85] ([i915#198] / [i915#2910])
   [84]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-skl2/igt@perf_pmu@rc6-suspend.html
   [85]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-skl5/igt@perf_pmu@rc6-suspend.html

  * igt@prime_nv_api@i915_self_import:
    - shard-tglb:         NOTRUN -> [SKIP][86] ([fdo#109291]) +1 similar issue
   [86]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-tglb7/igt@prime_nv_api@i915_self_import.html

  * igt@prime_vgem@fence-write-hang:
    - shard-tglb:         NOTRUN -> [SKIP][87] ([fdo#109295])
   [87]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-tglb7/igt@prime_vgem@fence-write-hang.html

  * igt@sysfs_clients@fair-0:
    - shard-apl:          NOTRUN -> [SKIP][88] ([fdo#109271] / [i915#2994])
   [88]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-apl7/igt@sysfs_clients@fair-0.html

  * igt@sysfs_clients@fair-1:
    - shard-tglb:         NOTRUN -> [SKIP][89] ([i915#2994])
   [89]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-tglb7/igt@sysfs_clients@fair-1.html

  * igt@sysfs_clients@fair-7:
    - shard-glk:          NOTRUN -> [SKIP][90] ([fdo#109271] / [i915#2994])
   [90]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-glk6/igt@sysfs_clients@fair-7.html

  * igt@sysfs_clients@recycle:
    - shard-kbl:          NOTRUN -> [SKIP][91] ([fdo#109271] / [i915#2994]) +1 similar issue
   [91]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-kbl4/igt@sysfs_clients@recycle.html

  
#### Possible fixes ####

  * igt@gem_ctx_persistence@many-contexts:
    - shard-tglb:         [FAIL][92] ([i915#2410]) -> [PASS][93]
   [92]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-tglb5/igt@gem_ctx_persistence@many-contexts.html
   [93]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-tglb3/igt@gem_ctx_persistence@many-contexts.html

  * igt@gem_eio@in-flight-suspend:
    - shard-kbl:          [DMESG-WARN][94] ([i915#180]) -> [PASS][95] +2 similar issues
   [94]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-kbl2/igt@gem_eio@in-flight-suspend.html
   [95]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-kbl4/igt@gem_eio@in-flight-suspend.html

  * igt@gem_exec_fair@basic-pace-solo@rcs0:
    - shard-glk:          [FAIL][96] ([i915#2842]) -> [PASS][97]
   [96]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-glk2/igt@gem_exec_fair@basic-pace-solo@rcs0.html
   [97]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-glk5/igt@gem_exec_fair@basic-pace-solo@rcs0.html

  * igt@gem_mmap_gtt@cpuset-basic-small-copy:
    - shard-kbl:          [INCOMPLETE][98] ([i915#3468]) -> [PASS][99]
   [98]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-kbl2/igt@gem_mmap_gtt@cpuset-basic-small-copy.html
   [99]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-kbl1/igt@gem_mmap_gtt@cpuset-basic-small-copy.html

  * igt@gem_mmap_gtt@cpuset-basic-small-copy-xy:
    - shard-tglb:         [INCOMPLETE][100] ([i915#3468]) -> [PASS][101]
   [100]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-tglb1/igt@gem_mmap_gtt@cpuset-basic-small-copy-xy.html
   [101]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-tglb7/igt@gem_mmap_gtt@cpuset-basic-small-copy-xy.html

  * igt@gem_mmap_gtt@cpuset-medium-copy-odd:
    - shard-skl:          [DMESG-WARN][102] ([i915#1982]) -> [PASS][103]
   [102]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-skl10/igt@gem_mmap_gtt@cpuset-medium-copy-odd.html
   [103]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-skl8/igt@gem_mmap_gtt@cpuset-medium-copy-odd.html

  * igt@gem_mmap_gtt@cpuset-medium-copy-xy:
    - shard-glk:          [INCOMPLETE][104] ([i915#3468]) -> [PASS][105]
   [104]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-glk2/igt@gem_mmap_gtt@cpuset-medium-copy-xy.html
   [105]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-glk5/igt@gem_mmap_gtt@cpuset-medium-copy-xy.html

  * igt@gem_vm_create@destroy-race:
    - shard-tglb:         [DMESG-FAIL][106] -> [PASS][107]
   [106]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-tglb7/igt@gem_vm_create@destroy-race.html
   [107]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-tglb7/igt@gem_vm_create@destroy-race.html

  * igt@kms_fbcon_fbt@fbc-suspend:
    - shard-kbl:          [INCOMPLETE][108] ([i915#155] / [i915#180] / [i915#636]) -> [PASS][109]
   [108]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-kbl1/igt@kms_fbcon_fbt@fbc-suspend.html
   [109]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-kbl7/igt@kms_fbcon_fbt@fbc-suspend.html

  * igt@kms_flip@flip-vs-dpms-off-vs-modeset-interruptible@d-edp1:
    - shard-tglb:         [INCOMPLETE][110] -> [PASS][111]
   [110]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-tglb3/igt@kms_flip@flip-vs-dpms-off-vs-modeset-interruptible@d-edp1.html
   [111]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-tglb1/igt@kms_flip@flip-vs-dpms-off-vs-modeset-interruptible@d-edp1.html

  * igt@kms_flip@flip-vs-expired-vblank@c-edp1:
    - shard-skl:          [FAIL][112] ([i915#79]) -> [PASS][113]
   [112]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-skl5/igt@kms_flip@flip-vs-expired-vblank@c-edp1.html
   [113]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-skl10/igt@kms_flip@flip-vs-expired-vblank@c-edp1.html

  * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
    - shard-skl:          [FAIL][114] ([fdo#108145] / [i915#265]) -> [PASS][115]
   [114]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-skl9/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
   [115]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-skl6/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html

  * igt@kms_psr@psr2_primary_page_flip:
    - shard-iclb:         [SKIP][116] ([fdo#109441]) -> [PASS][117] +2 similar issues
   [116]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-iclb4/igt@kms_psr@psr2_primary_page_flip.html
   [117]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-iclb2/igt@kms_psr@psr2_primary_page_flip.html

  * igt@kms_vblank@pipe-a-ts-continuation-suspend:
    - shard-kbl:          [DMESG-WARN][118] ([i915#180] / [i915#295]) -> [PASS][119]
   [118]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-kbl3/igt@kms_vblank@pipe-a-ts-continuation-suspend.html
   [119]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-kbl1/igt@kms_vblank@pipe-a-ts-continuation-suspend.html

  
#### Warnings ####

  * igt@gem_exec_fair@basic-none-rrul@rcs0:
    - shard-iclb:         [FAIL][120] ([i915#2842]) -> [FAIL][121] ([i915#2852])
   [120]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-iclb1/igt@gem_exec_fair@basic-none-rrul@rcs0.html
   [121]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-iclb8/igt@gem_exec_fair@basic-none-rrul@rcs0.html

  * igt@i915_pm_rc6_residency@rc6-fence:
    - shard-iclb:         [WARN][122] ([i915#2684]) -> [WARN][123] ([i915#1804] / [i915#2684])
   [122]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-iclb5/igt@i915_pm_rc6_residency@rc6-fence.html
   [123]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-iclb3/igt@i915_pm_rc6_residency@rc6-fence.html

  * igt@kms_psr2_sf@plane-move-sf-dmg-area-0:
    - shard-iclb:         [SKIP][124] ([i915#658]) -> [SKIP][125] ([i915#2920]) +2 similar issues
   [124]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-iclb4/igt@kms_psr2_sf@plane-move-sf-dmg-area-0.html
   [125]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-iclb2/igt@kms_psr2_sf@plane-move-sf-dmg-area-0.html

  * igt@runner@aborted:
    - shard-kbl:          ([FAIL][126], [FAIL][127], [FAIL][128], [FAIL][129], [FAIL][130], [FAIL][131], [FAIL][132], [FAIL][133], [FAIL][134], [FAIL][135], [FAIL][136], [FAIL][137], [FAIL][138], [FAIL][139], [FAIL][140], [FAIL][141], [FAIL][142]) ([i915#1436] / [i915#180] / [i915#1814] / [i915#2722] / [i915#3002] / [i915#3363] / [i915#602] / [i915#92]) -> ([FAIL][143], [FAIL][144], [FAIL][145], [FAIL][146], [FAIL][147], [FAIL][148], [FAIL][149], [FAIL][150], [FAIL][151], [FAIL][152], [FAIL][153], [FAIL][154], [FAIL][155], [FAIL][156], [FAIL][157]) ([i915#1436] / [i915#180] / [i915#1814] / [i915#2292] / [i915#2722] / [i915#3002] / [i915#3363] / [i915#602])
   [126]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-kbl2/igt@runner@aborted.html
   [127]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-kbl2/igt@runner@aborted.html
   [128]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-kbl1/igt@runner@aborted.html
   [129]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-kbl2/igt@runner@aborted.html
   [130]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-kbl1/igt@runner@aborted.html
   [131]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-kbl2/igt@runner@aborted.html
   [132]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-kbl1/igt@runner@aborted.html
   [133]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-kbl1/igt@runner@aborted.html
   [134]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-kbl4/igt@runner@aborted.html
   [135]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-kbl3/igt@runner@aborted.html
   [136]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-kbl7/igt@runner@aborted.html
   [137]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-kbl3/igt@runner@aborted.html
   [138]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-kbl3/igt@runner@aborted.html
   [139]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-kbl4/igt@runner@aborted.html
   [140]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-kbl3/igt@runner@aborted.html
   [141]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-kbl3/igt@runner@aborted.html
   [142]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10110/shard-kbl7/igt@runner@aborted.html
   [143]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-kbl4/igt@runner@aborted.html
   [144]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-kbl7/igt@runner@aborted.html
   [145]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-kbl4/igt@runner@aborted.html
   [146]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-kbl7/igt@runner@aborted.html
   [147]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-kbl7/igt@runner@aborted.html
   [148]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-kbl7/igt@runner@aborted.html
   [149]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-kbl7/igt@runner@aborted.html
   [150]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-kbl4/igt@runner@aborted.html
   [151]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-kbl3/igt@runner@aborted.html
   [152]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/shard-kbl3/igt@runner@aborted.html
   [153]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/sh

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20157/index.html

[-- Attachment #1.2: Type: text/html, Size: 33960 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] drm/i915: Use DRIVER_NAME for tracing unattached requests
  2021-05-20 14:28   ` Daniel Vetter
  (?)
@ 2021-05-31  7:53     ` Daniel Vetter
  -1 siblings, 0 replies; 17+ messages in thread
From: Daniel Vetter @ 2021-05-31  7:53 UTC (permalink / raw)
  To: Matthew Auld
  Cc: intel-gfx, Tvrtko Ursulin, Andi Shyti, stable, Chris Wilson,
	Chintan M Patel, dri-devel

On Thu, May 20, 2021 at 4:28 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Thu, May 20, 2021 at 08:35:14AM +0100, Matthew Auld wrote:
> > From: Chris Wilson <chris@chris-wilson.co.uk>
> >
> > 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.
>
> 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

> -Daniel
>
> >
> > Fixes: 855e39e65cfc ("drm/i915: Initialise basic fence before acquiring seqno")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Cc: Chintan M Patel <chintan.m.patel@intel.com>
> > Cc: Andi Shyti <andi.shyti@intel.com>
> > Cc: <stable@vger.kernel.org> # v5.7+
> > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > ---
> >  .../drm/i915/gt/intel_execlists_submission.c  | 20 ++++++++++++++-----
> >  drivers/gpu/drm/i915/i915_request.c           |  7 ++++++-
> >  2 files changed, 21 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > index de124870af44..75604e927d34 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > @@ -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);
> >       }
> >
> > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> > index 970d8f4986bb..aa124adb1051 100644
> > --- a/drivers/gpu/drm/i915/i915_request.c
> > +++ b/drivers/gpu/drm/i915/i915_request.c
> > @@ -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)
> > --
> > 2.26.3
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch



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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] drm/i915: Use DRIVER_NAME for tracing unattached requests
@ 2021-05-31  7:53     ` Daniel Vetter
  0 siblings, 0 replies; 17+ messages in thread
From: Daniel Vetter @ 2021-05-31  7:53 UTC (permalink / raw)
  To: Matthew Auld
  Cc: Tvrtko Ursulin, Andi Shyti, intel-gfx, stable, Chris Wilson,
	Chintan M Patel, dri-devel

On Thu, May 20, 2021 at 4:28 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Thu, May 20, 2021 at 08:35:14AM +0100, Matthew Auld wrote:
> > From: Chris Wilson <chris@chris-wilson.co.uk>
> >
> > 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.
>
> 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

> -Daniel
>
> >
> > Fixes: 855e39e65cfc ("drm/i915: Initialise basic fence before acquiring seqno")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Cc: Chintan M Patel <chintan.m.patel@intel.com>
> > Cc: Andi Shyti <andi.shyti@intel.com>
> > Cc: <stable@vger.kernel.org> # v5.7+
> > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > ---
> >  .../drm/i915/gt/intel_execlists_submission.c  | 20 ++++++++++++++-----
> >  drivers/gpu/drm/i915/i915_request.c           |  7 ++++++-
> >  2 files changed, 21 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > index de124870af44..75604e927d34 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > @@ -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);
> >       }
> >
> > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> > index 970d8f4986bb..aa124adb1051 100644
> > --- a/drivers/gpu/drm/i915/i915_request.c
> > +++ b/drivers/gpu/drm/i915/i915_request.c
> > @@ -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)
> > --
> > 2.26.3
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch



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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915: Use DRIVER_NAME for tracing unattached requests
@ 2021-05-31  7:53     ` Daniel Vetter
  0 siblings, 0 replies; 17+ messages in thread
From: Daniel Vetter @ 2021-05-31  7:53 UTC (permalink / raw)
  To: Matthew Auld; +Cc: intel-gfx, stable, Chris Wilson, Chintan M Patel, dri-devel

On Thu, May 20, 2021 at 4:28 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Thu, May 20, 2021 at 08:35:14AM +0100, Matthew Auld wrote:
> > From: Chris Wilson <chris@chris-wilson.co.uk>
> >
> > 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.
>
> 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

> -Daniel
>
> >
> > Fixes: 855e39e65cfc ("drm/i915: Initialise basic fence before acquiring seqno")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Cc: Chintan M Patel <chintan.m.patel@intel.com>
> > Cc: Andi Shyti <andi.shyti@intel.com>
> > Cc: <stable@vger.kernel.org> # v5.7+
> > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > ---
> >  .../drm/i915/gt/intel_execlists_submission.c  | 20 ++++++++++++++-----
> >  drivers/gpu/drm/i915/i915_request.c           |  7 ++++++-
> >  2 files changed, 21 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > index de124870af44..75604e927d34 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > @@ -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);
> >       }
> >
> > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> > index 970d8f4986bb..aa124adb1051 100644
> > --- a/drivers/gpu/drm/i915/i915_request.c
> > +++ b/drivers/gpu/drm/i915/i915_request.c
> > @@ -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)
> > --
> > 2.26.3
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] drm/i915: Use DRIVER_NAME for tracing unattached requests
  2021-05-31  7:53     ` Daniel Vetter
  (?)
@ 2021-06-01 11:13       ` Matthew Auld
  -1 siblings, 0 replies; 17+ messages in thread
From: Matthew Auld @ 2021-06-01 11:13 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: intel-gfx, Tvrtko Ursulin, Andi Shyti, stable, Chris Wilson,
	Chintan M Patel, dri-devel

On 31/05/2021 08:53, Daniel Vetter wrote:
> On Thu, May 20, 2021 at 4:28 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>>
>> On Thu, May 20, 2021 at 08:35:14AM +0100, Matthew Auld wrote:
>>> From: Chris Wilson <chris@chris-wilson.co.uk>
>>>
>>> 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.
>>
>> 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.

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.


> 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.

Right, I can try to search the git history.


> -Daniel
> 
>> -Daniel
>>
>>>
>>> Fixes: 855e39e65cfc ("drm/i915: Initialise basic fence before acquiring seqno")
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>>> Cc: Chintan M Patel <chintan.m.patel@intel.com>
>>> Cc: Andi Shyti <andi.shyti@intel.com>
>>> Cc: <stable@vger.kernel.org> # v5.7+
>>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>>> ---
>>>   .../drm/i915/gt/intel_execlists_submission.c  | 20 ++++++++++++++-----
>>>   drivers/gpu/drm/i915/i915_request.c           |  7 ++++++-
>>>   2 files changed, 21 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>>> index de124870af44..75604e927d34 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>>> @@ -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);
>>>        }
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
>>> index 970d8f4986bb..aa124adb1051 100644
>>> --- a/drivers/gpu/drm/i915/i915_request.c
>>> +++ b/drivers/gpu/drm/i915/i915_request.c
>>> @@ -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)
>>> --
>>> 2.26.3
>>>
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
> 
> 
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] drm/i915: Use DRIVER_NAME for tracing unattached requests
@ 2021-06-01 11:13       ` Matthew Auld
  0 siblings, 0 replies; 17+ messages in thread
From: Matthew Auld @ 2021-06-01 11:13 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Tvrtko Ursulin, Andi Shyti, intel-gfx, stable, Chris Wilson,
	Chintan M Patel, dri-devel

On 31/05/2021 08:53, Daniel Vetter wrote:
> On Thu, May 20, 2021 at 4:28 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>>
>> On Thu, May 20, 2021 at 08:35:14AM +0100, Matthew Auld wrote:
>>> From: Chris Wilson <chris@chris-wilson.co.uk>
>>>
>>> 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.
>>
>> 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.

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.


> 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.

Right, I can try to search the git history.


> -Daniel
> 
>> -Daniel
>>
>>>
>>> Fixes: 855e39e65cfc ("drm/i915: Initialise basic fence before acquiring seqno")
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>>> Cc: Chintan M Patel <chintan.m.patel@intel.com>
>>> Cc: Andi Shyti <andi.shyti@intel.com>
>>> Cc: <stable@vger.kernel.org> # v5.7+
>>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>>> ---
>>>   .../drm/i915/gt/intel_execlists_submission.c  | 20 ++++++++++++++-----
>>>   drivers/gpu/drm/i915/i915_request.c           |  7 ++++++-
>>>   2 files changed, 21 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>>> index de124870af44..75604e927d34 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>>> @@ -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);
>>>        }
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
>>> index 970d8f4986bb..aa124adb1051 100644
>>> --- a/drivers/gpu/drm/i915/i915_request.c
>>> +++ b/drivers/gpu/drm/i915/i915_request.c
>>> @@ -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)
>>> --
>>> 2.26.3
>>>
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
> 
> 
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915: Use DRIVER_NAME for tracing unattached requests
@ 2021-06-01 11:13       ` Matthew Auld
  0 siblings, 0 replies; 17+ messages in thread
From: Matthew Auld @ 2021-06-01 11:13 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx, stable, Chris Wilson, Chintan M Patel, dri-devel

On 31/05/2021 08:53, Daniel Vetter wrote:
> On Thu, May 20, 2021 at 4:28 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>>
>> On Thu, May 20, 2021 at 08:35:14AM +0100, Matthew Auld wrote:
>>> From: Chris Wilson <chris@chris-wilson.co.uk>
>>>
>>> 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.
>>
>> 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.

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.


> 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.

Right, I can try to search the git history.


> -Daniel
> 
>> -Daniel
>>
>>>
>>> Fixes: 855e39e65cfc ("drm/i915: Initialise basic fence before acquiring seqno")
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>>> Cc: Chintan M Patel <chintan.m.patel@intel.com>
>>> Cc: Andi Shyti <andi.shyti@intel.com>
>>> Cc: <stable@vger.kernel.org> # v5.7+
>>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>>> ---
>>>   .../drm/i915/gt/intel_execlists_submission.c  | 20 ++++++++++++++-----
>>>   drivers/gpu/drm/i915/i915_request.c           |  7 ++++++-
>>>   2 files changed, 21 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>>> index de124870af44..75604e927d34 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>>> @@ -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);
>>>        }
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
>>> index 970d8f4986bb..aa124adb1051 100644
>>> --- a/drivers/gpu/drm/i915/i915_request.c
>>> +++ b/drivers/gpu/drm/i915/i915_request.c
>>> @@ -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)
>>> --
>>> 2.26.3
>>>
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
> 
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] drm/i915: Use DRIVER_NAME for tracing unattached requests
  2021-06-01 11:13       ` Matthew Auld
  (?)
@ 2021-06-01 12:20         ` Daniel Vetter
  -1 siblings, 0 replies; 17+ messages in thread
From: Daniel Vetter @ 2021-06-01 12:20 UTC (permalink / raw)
  To: Matthew Auld
  Cc: intel-gfx, Tvrtko Ursulin, Andi Shyti, stable, Chris Wilson,
	Chintan M Patel, dri-devel

On Tue, Jun 1, 2021 at 1:13 PM Matthew Auld <matthew.auld@intel.com> wrote:
> On 31/05/2021 08:53, Daniel Vetter wrote:
> > On Thu, May 20, 2021 at 4:28 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> >>
> >> On Thu, May 20, 2021 at 08:35:14AM +0100, Matthew Auld wrote:
> >>> From: Chris Wilson <chris@chris-wilson.co.uk>
> >>>
> >>> 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.
> >>
> >> 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.
>
> 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?

Uh, if this is the bug it's a lot more scary. SLAB_TYPESAFE_BY_RCU
should only rear it's ugly head if we do clever tricks where we access
pointers to dma_fence under rcu alone, _without_ holding a full
dma_fence reference. As soon as we have a full reference (and checked
that the reference is to the right fence, since we could race) then
all this recycle issues are gonne since the kref_t provides the right
barrier here.

If we hit any of the dma_fence tracepoints without a full reference
held then I think that's a bug an needs to be fixed. Maybe we should
have a debug WARN_ON(!kref(dma_fence)>0)); in these tracepoints
somewhere to prevent this. Doing real dma_fence ops without a refcount
held is really too much clever imo, and even if we'd find some
microbenchmark showing that e.g. the dma_fence_get/put around some
dma_fence function we're calling is measurable, it's not worth the
cost in bugfixes like this one here.

And when we do hold a full reference, then the only problem I've found
is that we call dma_fence_init before the request is fully set up,
which is at least semi-reasonable and can easily be checked for and
explained with a comment. I thought I looked at the code, and
reordering the request init to not have this problem looked tricky.

Another issue which would also be very questionable design that we
need to re-analyze would be if the engine can disappear before the
last reference for the dma_fence has been dropped. I'd also just call
this a bug in our refcounting, this should be impossible, but I
haven't checked.

In all these cases SLAB_TYPESAFE_BY_RCU shouldn't make the situation
worse, and if it does, it's a separate issue really.

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

So thinking about this some more, if you think this can be easily
fixed by pushing the dma_fence_init past the initialization of
rq->engine, then that would probably be the cleanest fix of all of
them. Assuming none of the above consideration point at further
trouble (but then further trouble probably needs separate patches to
address them).

> > 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.
>
> Right, I can try to search the git history.

Yeah might be good to dig that out too while we're at it. I think i915
is the only driver that recycles it's dma_fence without an rcu
barrier. We're also the only driver that does lots of very clever
tricks which are protected by rcu only, and not grabbing a full
dma_fence reference. Or at least I've seen a bunch of those.
-Daniel

>
>
> > -Daniel
> >
> >> -Daniel
> >>
> >>>
> >>> Fixes: 855e39e65cfc ("drm/i915: Initialise basic fence before acquiring seqno")
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> >>> Cc: Chintan M Patel <chintan.m.patel@intel.com>
> >>> Cc: Andi Shyti <andi.shyti@intel.com>
> >>> Cc: <stable@vger.kernel.org> # v5.7+
> >>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> >>> ---
> >>>   .../drm/i915/gt/intel_execlists_submission.c  | 20 ++++++++++++++-----
> >>>   drivers/gpu/drm/i915/i915_request.c           |  7 ++++++-
> >>>   2 files changed, 21 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> >>> index de124870af44..75604e927d34 100644
> >>> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> >>> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> >>> @@ -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);
> >>>        }
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> >>> index 970d8f4986bb..aa124adb1051 100644
> >>> --- a/drivers/gpu/drm/i915/i915_request.c
> >>> +++ b/drivers/gpu/drm/i915/i915_request.c
> >>> @@ -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)
> >>> --
> >>> 2.26.3
> >>>
> >>
> >> --
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> http://blog.ffwll.ch
> >
> >
> >



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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] drm/i915: Use DRIVER_NAME for tracing unattached requests
@ 2021-06-01 12:20         ` Daniel Vetter
  0 siblings, 0 replies; 17+ messages in thread
From: Daniel Vetter @ 2021-06-01 12:20 UTC (permalink / raw)
  To: Matthew Auld
  Cc: Tvrtko Ursulin, Andi Shyti, intel-gfx, stable, Chris Wilson,
	Chintan M Patel, dri-devel

On Tue, Jun 1, 2021 at 1:13 PM Matthew Auld <matthew.auld@intel.com> wrote:
> On 31/05/2021 08:53, Daniel Vetter wrote:
> > On Thu, May 20, 2021 at 4:28 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> >>
> >> On Thu, May 20, 2021 at 08:35:14AM +0100, Matthew Auld wrote:
> >>> From: Chris Wilson <chris@chris-wilson.co.uk>
> >>>
> >>> 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.
> >>
> >> 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.
>
> 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?

Uh, if this is the bug it's a lot more scary. SLAB_TYPESAFE_BY_RCU
should only rear it's ugly head if we do clever tricks where we access
pointers to dma_fence under rcu alone, _without_ holding a full
dma_fence reference. As soon as we have a full reference (and checked
that the reference is to the right fence, since we could race) then
all this recycle issues are gonne since the kref_t provides the right
barrier here.

If we hit any of the dma_fence tracepoints without a full reference
held then I think that's a bug an needs to be fixed. Maybe we should
have a debug WARN_ON(!kref(dma_fence)>0)); in these tracepoints
somewhere to prevent this. Doing real dma_fence ops without a refcount
held is really too much clever imo, and even if we'd find some
microbenchmark showing that e.g. the dma_fence_get/put around some
dma_fence function we're calling is measurable, it's not worth the
cost in bugfixes like this one here.

And when we do hold a full reference, then the only problem I've found
is that we call dma_fence_init before the request is fully set up,
which is at least semi-reasonable and can easily be checked for and
explained with a comment. I thought I looked at the code, and
reordering the request init to not have this problem looked tricky.

Another issue which would also be very questionable design that we
need to re-analyze would be if the engine can disappear before the
last reference for the dma_fence has been dropped. I'd also just call
this a bug in our refcounting, this should be impossible, but I
haven't checked.

In all these cases SLAB_TYPESAFE_BY_RCU shouldn't make the situation
worse, and if it does, it's a separate issue really.

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

So thinking about this some more, if you think this can be easily
fixed by pushing the dma_fence_init past the initialization of
rq->engine, then that would probably be the cleanest fix of all of
them. Assuming none of the above consideration point at further
trouble (but then further trouble probably needs separate patches to
address them).

> > 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.
>
> Right, I can try to search the git history.

Yeah might be good to dig that out too while we're at it. I think i915
is the only driver that recycles it's dma_fence without an rcu
barrier. We're also the only driver that does lots of very clever
tricks which are protected by rcu only, and not grabbing a full
dma_fence reference. Or at least I've seen a bunch of those.
-Daniel

>
>
> > -Daniel
> >
> >> -Daniel
> >>
> >>>
> >>> Fixes: 855e39e65cfc ("drm/i915: Initialise basic fence before acquiring seqno")
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> >>> Cc: Chintan M Patel <chintan.m.patel@intel.com>
> >>> Cc: Andi Shyti <andi.shyti@intel.com>
> >>> Cc: <stable@vger.kernel.org> # v5.7+
> >>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> >>> ---
> >>>   .../drm/i915/gt/intel_execlists_submission.c  | 20 ++++++++++++++-----
> >>>   drivers/gpu/drm/i915/i915_request.c           |  7 ++++++-
> >>>   2 files changed, 21 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> >>> index de124870af44..75604e927d34 100644
> >>> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> >>> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> >>> @@ -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);
> >>>        }
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> >>> index 970d8f4986bb..aa124adb1051 100644
> >>> --- a/drivers/gpu/drm/i915/i915_request.c
> >>> +++ b/drivers/gpu/drm/i915/i915_request.c
> >>> @@ -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)
> >>> --
> >>> 2.26.3
> >>>
> >>
> >> --
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> http://blog.ffwll.ch
> >
> >
> >



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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915: Use DRIVER_NAME for tracing unattached requests
@ 2021-06-01 12:20         ` Daniel Vetter
  0 siblings, 0 replies; 17+ messages in thread
From: Daniel Vetter @ 2021-06-01 12:20 UTC (permalink / raw)
  To: Matthew Auld; +Cc: intel-gfx, stable, Chris Wilson, Chintan M Patel, dri-devel

On Tue, Jun 1, 2021 at 1:13 PM Matthew Auld <matthew.auld@intel.com> wrote:
> On 31/05/2021 08:53, Daniel Vetter wrote:
> > On Thu, May 20, 2021 at 4:28 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> >>
> >> On Thu, May 20, 2021 at 08:35:14AM +0100, Matthew Auld wrote:
> >>> From: Chris Wilson <chris@chris-wilson.co.uk>
> >>>
> >>> 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.
> >>
> >> 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.
>
> 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?

Uh, if this is the bug it's a lot more scary. SLAB_TYPESAFE_BY_RCU
should only rear it's ugly head if we do clever tricks where we access
pointers to dma_fence under rcu alone, _without_ holding a full
dma_fence reference. As soon as we have a full reference (and checked
that the reference is to the right fence, since we could race) then
all this recycle issues are gonne since the kref_t provides the right
barrier here.

If we hit any of the dma_fence tracepoints without a full reference
held then I think that's a bug an needs to be fixed. Maybe we should
have a debug WARN_ON(!kref(dma_fence)>0)); in these tracepoints
somewhere to prevent this. Doing real dma_fence ops without a refcount
held is really too much clever imo, and even if we'd find some
microbenchmark showing that e.g. the dma_fence_get/put around some
dma_fence function we're calling is measurable, it's not worth the
cost in bugfixes like this one here.

And when we do hold a full reference, then the only problem I've found
is that we call dma_fence_init before the request is fully set up,
which is at least semi-reasonable and can easily be checked for and
explained with a comment. I thought I looked at the code, and
reordering the request init to not have this problem looked tricky.

Another issue which would also be very questionable design that we
need to re-analyze would be if the engine can disappear before the
last reference for the dma_fence has been dropped. I'd also just call
this a bug in our refcounting, this should be impossible, but I
haven't checked.

In all these cases SLAB_TYPESAFE_BY_RCU shouldn't make the situation
worse, and if it does, it's a separate issue really.

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

So thinking about this some more, if you think this can be easily
fixed by pushing the dma_fence_init past the initialization of
rq->engine, then that would probably be the cleanest fix of all of
them. Assuming none of the above consideration point at further
trouble (but then further trouble probably needs separate patches to
address them).

> > 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.
>
> Right, I can try to search the git history.

Yeah might be good to dig that out too while we're at it. I think i915
is the only driver that recycles it's dma_fence without an rcu
barrier. We're also the only driver that does lots of very clever
tricks which are protected by rcu only, and not grabbing a full
dma_fence reference. Or at least I've seen a bunch of those.
-Daniel

>
>
> > -Daniel
> >
> >> -Daniel
> >>
> >>>
> >>> Fixes: 855e39e65cfc ("drm/i915: Initialise basic fence before acquiring seqno")
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> >>> Cc: Chintan M Patel <chintan.m.patel@intel.com>
> >>> Cc: Andi Shyti <andi.shyti@intel.com>
> >>> Cc: <stable@vger.kernel.org> # v5.7+
> >>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> >>> ---
> >>>   .../drm/i915/gt/intel_execlists_submission.c  | 20 ++++++++++++++-----
> >>>   drivers/gpu/drm/i915/i915_request.c           |  7 ++++++-
> >>>   2 files changed, 21 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> >>> index de124870af44..75604e927d34 100644
> >>> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> >>> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> >>> @@ -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);
> >>>        }
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> >>> index 970d8f4986bb..aa124adb1051 100644
> >>> --- a/drivers/gpu/drm/i915/i915_request.c
> >>> +++ b/drivers/gpu/drm/i915/i915_request.c
> >>> @@ -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)
> >>> --
> >>> 2.26.3
> >>>
> >>
> >> --
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> http://blog.ffwll.ch
> >
> >
> >



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2021-06-01 12:20 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-20  7:35 [PATCH] drm/i915: Use DRIVER_NAME for tracing unattached requests Matthew Auld
2021-05-20  7:35 ` [Intel-gfx] " Matthew Auld
2021-05-20  7:35 ` Matthew Auld
2021-05-20  9:45 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
2021-05-20 14:28 ` [PATCH] " Daniel Vetter
2021-05-20 14:28   ` [Intel-gfx] " Daniel Vetter
2021-05-20 14:28   ` Daniel Vetter
2021-05-31  7:53   ` Daniel Vetter
2021-05-31  7:53     ` [Intel-gfx] " Daniel Vetter
2021-05-31  7:53     ` Daniel Vetter
2021-06-01 11:13     ` Matthew Auld
2021-06-01 11:13       ` [Intel-gfx] " Matthew Auld
2021-06-01 11:13       ` Matthew Auld
2021-06-01 12:20       ` Daniel Vetter
2021-06-01 12:20         ` [Intel-gfx] " Daniel Vetter
2021-06-01 12:20         ` Daniel Vetter
2021-05-21 14:52 ` [Intel-gfx] ✓ Fi.CI.IGT: success for " Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.