intel-gfx.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [Intel-gfx] [PATCH 1/3] drm/i915: Mark concurrent submissions with a weak-dependency
@ 2020-05-07  8:21 Chris Wilson
  2020-05-07  8:21 ` [Intel-gfx] [PATCH 2/3] drm/i915/gem: Treat submit-fence as weak dependency for new clients Chris Wilson
                   ` (4 more replies)
  0 siblings, 5 replies; 17+ messages in thread
From: Chris Wilson @ 2020-05-07  8:21 UTC (permalink / raw)
  To: intel-gfx; +Cc: stable, Chris Wilson

We recorded the dependencies for WAIT_FOR_SUBMIT in order that we could
correctly perform priority inheritance from the parallel branches to the
common trunk. However, for the purpose of timeslicing and reset
handling, the dependency is weak -- as we the pair of requests are
allowed to run in parallel and not in strict succession. So for example
we do need to suspend one if the other hangs.

The real significance though is that this allows us to rearrange
groups of WAIT_FOR_SUBMIT linked requests along the single engine, and
so can resolve user level inter-batch scheduling dependencies from user
semaphores.

Fixes: c81471f5e95c ("drm/i915: Copy across scheduler behaviour flags across submit fences")
Testcase: igt/gem_exec_fence/submit
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: <stable@vger.kernel.org> # v5.6+
---
 drivers/gpu/drm/i915/gt/intel_lrc.c         | 9 +++++++++
 drivers/gpu/drm/i915/i915_request.c         | 8 ++++++--
 drivers/gpu/drm/i915/i915_scheduler.c       | 6 +++---
 drivers/gpu/drm/i915/i915_scheduler.h       | 3 ++-
 drivers/gpu/drm/i915/i915_scheduler_types.h | 1 +
 5 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index dc3f2ee7136d..10109f661bcb 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1880,6 +1880,9 @@ static void defer_request(struct i915_request *rq, struct list_head * const pl)
 			struct i915_request *w =
 				container_of(p->waiter, typeof(*w), sched);
 
+			if (p->flags & I915_DEPENDENCY_WEAK)
+				continue;
+
 			/* Leave semaphores spinning on the other engines */
 			if (w->engine != rq->engine)
 				continue;
@@ -2726,6 +2729,9 @@ static void __execlists_hold(struct i915_request *rq)
 			struct i915_request *w =
 				container_of(p->waiter, typeof(*w), sched);
 
+			if (p->flags & I915_DEPENDENCY_WEAK)
+				continue;
+
 			/* Leave semaphores spinning on the other engines */
 			if (w->engine != rq->engine)
 				continue;
@@ -2850,6 +2856,9 @@ static void __execlists_unhold(struct i915_request *rq)
 			struct i915_request *w =
 				container_of(p->waiter, typeof(*w), sched);
 
+			if (p->flags & I915_DEPENDENCY_WEAK)
+				continue;
+
 			/* Propagate any change in error status */
 			if (rq->fence.error)
 				i915_request_set_error_once(w, rq->fence.error);
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 4d18f808fda2..3c38d61c90f8 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -1040,7 +1040,9 @@ i915_request_await_request(struct i915_request *to, struct i915_request *from)
 	}
 
 	if (to->engine->schedule) {
-		ret = i915_sched_node_add_dependency(&to->sched, &from->sched);
+		ret = i915_sched_node_add_dependency(&to->sched,
+						     &from->sched,
+						     I915_DEPENDENCY_EXTERNAL);
 		if (ret < 0)
 			return ret;
 	}
@@ -1202,7 +1204,9 @@ __i915_request_await_execution(struct i915_request *to,
 
 	/* Couple the dependency tree for PI on this exposed to->fence */
 	if (to->engine->schedule) {
-		err = i915_sched_node_add_dependency(&to->sched, &from->sched);
+		err = i915_sched_node_add_dependency(&to->sched,
+						     &from->sched,
+						     I915_DEPENDENCY_WEAK);
 		if (err < 0)
 			return err;
 	}
diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
index 37cfcf5b321b..6e2d4190099f 100644
--- a/drivers/gpu/drm/i915/i915_scheduler.c
+++ b/drivers/gpu/drm/i915/i915_scheduler.c
@@ -462,7 +462,8 @@ bool __i915_sched_node_add_dependency(struct i915_sched_node *node,
 }
 
 int i915_sched_node_add_dependency(struct i915_sched_node *node,
-				   struct i915_sched_node *signal)
+				   struct i915_sched_node *signal,
+				   unsigned long flags)
 {
 	struct i915_dependency *dep;
 
@@ -473,8 +474,7 @@ int i915_sched_node_add_dependency(struct i915_sched_node *node,
 	local_bh_disable();
 
 	if (!__i915_sched_node_add_dependency(node, signal, dep,
-					      I915_DEPENDENCY_EXTERNAL |
-					      I915_DEPENDENCY_ALLOC))
+					      flags | I915_DEPENDENCY_ALLOC))
 		i915_dependency_free(dep);
 
 	local_bh_enable(); /* kick submission tasklet */
diff --git a/drivers/gpu/drm/i915/i915_scheduler.h b/drivers/gpu/drm/i915/i915_scheduler.h
index d1dc4efef77b..6f0bf00fc569 100644
--- a/drivers/gpu/drm/i915/i915_scheduler.h
+++ b/drivers/gpu/drm/i915/i915_scheduler.h
@@ -34,7 +34,8 @@ bool __i915_sched_node_add_dependency(struct i915_sched_node *node,
 				      unsigned long flags);
 
 int i915_sched_node_add_dependency(struct i915_sched_node *node,
-				   struct i915_sched_node *signal);
+				   struct i915_sched_node *signal,
+				   unsigned long flags);
 
 void i915_sched_node_fini(struct i915_sched_node *node);
 
diff --git a/drivers/gpu/drm/i915/i915_scheduler_types.h b/drivers/gpu/drm/i915/i915_scheduler_types.h
index d18e70550054..7186875088a0 100644
--- a/drivers/gpu/drm/i915/i915_scheduler_types.h
+++ b/drivers/gpu/drm/i915/i915_scheduler_types.h
@@ -78,6 +78,7 @@ struct i915_dependency {
 	unsigned long flags;
 #define I915_DEPENDENCY_ALLOC		BIT(0)
 #define I915_DEPENDENCY_EXTERNAL	BIT(1)
+#define I915_DEPENDENCY_WEAK		BIT(2)
 };
 
 #endif /* _I915_SCHEDULER_TYPES_H_ */
-- 
2.20.1

_______________________________________________
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] [PATCH 2/3] drm/i915/gem: Treat submit-fence as weak dependency for new clients
  2020-05-07  8:21 [Intel-gfx] [PATCH 1/3] drm/i915: Mark concurrent submissions with a weak-dependency Chris Wilson
@ 2020-05-07  8:21 ` Chris Wilson
  2020-05-07 14:59   ` Tvrtko Ursulin
  2020-05-07  8:21 ` [Intel-gfx] [PATCH 3/3] drm/i915: Treat weak-dependencies as bidirectional when applying priorities Chris Wilson
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 17+ messages in thread
From: Chris Wilson @ 2020-05-07  8:21 UTC (permalink / raw)
  To: intel-gfx; +Cc: Chris Wilson

The submit-fence adds a weak dependency to the requests, and for the
purpose of our FQ_CODEL hinting we do not want to treat as a
restriction. This is primarily because clients may treat submit-fences
as a bidirectional bonding between a pair of co-ordinating requests.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 966523a8503f..e8bf0cf02fd7 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -2565,6 +2565,17 @@ static void retire_requests(struct intel_timeline *tl, struct i915_request *end)
 			break;
 }
 
+static bool new_client(struct i915_request *rq)
+{
+	struct i915_dependency *p;
+
+	list_for_each_entry(p, &rq->sched.signalers_list, signal_link)
+		if (!(p->flags & I915_DEPENDENCY_WEAK))
+			return false;
+
+	return true;
+}
+
 static void eb_request_add(struct i915_execbuffer *eb)
 {
 	struct i915_request *rq = eb->request;
@@ -2604,7 +2615,7 @@ static void eb_request_add(struct i915_execbuffer *eb)
 		 * Allow interactive/synchronous clients to jump ahead of
 		 * the bulk clients. (FQ_CODEL)
 		 */
-		if (list_empty(&rq->sched.signalers_list))
+		if (new_client(rq))
 			attr.priority |= I915_PRIORITY_WAIT;
 	} else {
 		/* Serialise with context_close via the add_to_timeline */
-- 
2.20.1

_______________________________________________
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] [PATCH 3/3] drm/i915: Treat weak-dependencies as bidirectional when applying priorities
  2020-05-07  8:21 [Intel-gfx] [PATCH 1/3] drm/i915: Mark concurrent submissions with a weak-dependency Chris Wilson
  2020-05-07  8:21 ` [Intel-gfx] [PATCH 2/3] drm/i915/gem: Treat submit-fence as weak dependency for new clients Chris Wilson
@ 2020-05-07  8:21 ` Chris Wilson
  2020-05-07  8:29   ` [Intel-gfx] [PATCH] " Chris Wilson
  2020-05-07 14:58   ` [Intel-gfx] [PATCH 3/3] " Tvrtko Ursulin
  2020-05-07  9:55 ` [Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Mark concurrent submissions with a weak-dependency (rev2) Patchwork
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 17+ messages in thread
From: Chris Wilson @ 2020-05-07  8:21 UTC (permalink / raw)
  To: intel-gfx; +Cc: Chris Wilson

Clients may use a submit-fence as bidirectional bond between two or more
co-operating requests, and so if we bump the priority of one, we wish to
bump the priority of the set.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_scheduler.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
index 6e2d4190099f..7194fbfcaa49 100644
--- a/drivers/gpu/drm/i915/i915_scheduler.c
+++ b/drivers/gpu/drm/i915/i915_scheduler.c
@@ -291,6 +291,12 @@ static void __i915_schedule(struct i915_sched_node *node,
 			if (prio > READ_ONCE(p->signaler->attr.priority))
 				list_move_tail(&p->dfs_link, &dfs);
 		}
+
+		list_for_each_entry(p, &node->waiters_list, wait_link) {
+			if (p->flags & I915_DEPENDENCY_WEAK &&
+			    prio > READ_ONCE(p->waiter->attr.priority))
+				list_move_tail(&p->dfs_link, &dfs);
+		}
 	}
 
 	/*
-- 
2.20.1

_______________________________________________
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] [PATCH] drm/i915: Treat weak-dependencies as bidirectional when applying priorities
  2020-05-07  8:21 ` [Intel-gfx] [PATCH 3/3] drm/i915: Treat weak-dependencies as bidirectional when applying priorities Chris Wilson
@ 2020-05-07  8:29   ` Chris Wilson
  2020-05-07 14:58   ` [Intel-gfx] [PATCH 3/3] " Tvrtko Ursulin
  1 sibling, 0 replies; 17+ messages in thread
From: Chris Wilson @ 2020-05-07  8:29 UTC (permalink / raw)
  To: intel-gfx; +Cc: Chris Wilson

Clients may use a submit-fence as bidirectional bond between two or more
co-operating requests, and so if we bump the priority of one, we wish to
bump the priority of the set.

Testcase: igt/gem_exec_fence/submitN
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_scheduler.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
index 6e2d4190099f..1c33973dbd20 100644
--- a/drivers/gpu/drm/i915/i915_scheduler.c
+++ b/drivers/gpu/drm/i915/i915_scheduler.c
@@ -291,6 +291,19 @@ static void __i915_schedule(struct i915_sched_node *node,
 			if (prio > READ_ONCE(p->signaler->attr.priority))
 				list_move_tail(&p->dfs_link, &dfs);
 		}
+
+		/*
+		 * A weak dependency is used for submit-fence, which while
+		 * not strongly coupled, we do need to treat as a coordinated
+		 * set of co-operating requests that need to be run
+		 * concurrently. So if one request of that set receives a
+		 * priority boost, they all receive a priority boost.
+		 */
+		list_for_each_entry(p, &node->waiters_list, wait_link) {
+			if (p->flags & I915_DEPENDENCY_WEAK &&
+			    prio > READ_ONCE(p->waiter->attr.priority))
+				list_move_tail(&p->dfs_link, &dfs);
+		}
 	}
 
 	/*
-- 
2.20.1

_______________________________________________
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 series starting with [1/3] drm/i915: Mark concurrent submissions with a weak-dependency (rev2)
  2020-05-07  8:21 [Intel-gfx] [PATCH 1/3] drm/i915: Mark concurrent submissions with a weak-dependency Chris Wilson
  2020-05-07  8:21 ` [Intel-gfx] [PATCH 2/3] drm/i915/gem: Treat submit-fence as weak dependency for new clients Chris Wilson
  2020-05-07  8:21 ` [Intel-gfx] [PATCH 3/3] drm/i915: Treat weak-dependencies as bidirectional when applying priorities Chris Wilson
@ 2020-05-07  9:55 ` Patchwork
  2020-05-07 12:49 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
  2020-05-07 14:53 ` [Intel-gfx] [PATCH 1/3] drm/i915: Mark concurrent submissions with a weak-dependency Tvrtko Ursulin
  4 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2020-05-07  9:55 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/3] drm/i915: Mark concurrent submissions with a weak-dependency (rev2)
URL   : https://patchwork.freedesktop.org/series/77024/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_8441 -> Patchwork_17598
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@active:
    - fi-kbl-soraka:      [PASS][1] -> [DMESG-FAIL][2] ([i915#666])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/fi-kbl-soraka/igt@i915_selftest@live@active.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/fi-kbl-soraka/igt@i915_selftest@live@active.html

  
  [i915#666]: https://gitlab.freedesktop.org/drm/intel/issues/666


Participating hosts (49 -> 43)
------------------------------

  Additional (1): fi-bdw-gvtdvm 
  Missing    (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper fi-bdw-samus 


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_8441 -> Patchwork_17598

  CI-20190529: 20190529
  CI_DRM_8441: 6c0ee41a7c3201ef2a89800234803a95f65989be @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5637: fdc33f7e1adc5bb6a1ba88b6233aaf224174d75a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_17598: 183fe789671fbe6252b0474dc63a6cca25c998cc @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

183fe789671f drm/i915: Treat weak-dependencies as bidirectional when applying priorities
d62dd47b9d3b drm/i915/gem: Treat submit-fence as weak dependency for new clients
4776c2422f73 drm/i915: Mark concurrent submissions with a weak-dependency

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/index.html
_______________________________________________
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: failure for series starting with [1/3] drm/i915: Mark concurrent submissions with a weak-dependency (rev2)
  2020-05-07  8:21 [Intel-gfx] [PATCH 1/3] drm/i915: Mark concurrent submissions with a weak-dependency Chris Wilson
                   ` (2 preceding siblings ...)
  2020-05-07  9:55 ` [Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Mark concurrent submissions with a weak-dependency (rev2) Patchwork
@ 2020-05-07 12:49 ` Patchwork
  2020-05-07 14:53 ` [Intel-gfx] [PATCH 1/3] drm/i915: Mark concurrent submissions with a weak-dependency Tvrtko Ursulin
  4 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2020-05-07 12:49 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/3] drm/i915: Mark concurrent submissions with a weak-dependency (rev2)
URL   : https://patchwork.freedesktop.org/series/77024/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_8441_full -> Patchwork_17598_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_17598_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_17598_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_17598_full:

### IGT changes ###

#### Possible regressions ####

  * igt@kms_flip_event_leak:
    - shard-kbl:          [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-kbl2/igt@kms_flip_event_leak.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-kbl3/igt@kms_flip_event_leak.html

  
New tests
---------

  New tests have been introduced between CI_DRM_8441_full and Patchwork_17598_full:

### New IGT tests (4) ###

  * igt@gem_exec_fence@submit@bcs0:
    - Statuses : 6 pass(s)
    - Exec time: [0.01, 0.05] s

  * igt@gem_exec_fence@submit@vcs0:
    - Statuses : 6 pass(s)
    - Exec time: [0.01, 0.05] s

  * igt@gem_exec_fence@submit@vcs1:
    - Statuses : 3 pass(s)
    - Exec time: [0.01] s

  * igt@gem_exec_fence@submit@vecs0:
    - Statuses : 6 pass(s)
    - Exec time: [0.01, 0.05] s

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gen9_exec_parse@allowed-all:
    - shard-kbl:          [PASS][3] -> [DMESG-WARN][4] ([i915#1436] / [i915#716])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-kbl1/igt@gen9_exec_parse@allowed-all.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-kbl2/igt@gen9_exec_parse@allowed-all.html

  * igt@i915_suspend@forcewake:
    - shard-kbl:          [PASS][5] -> [DMESG-WARN][6] ([i915#180]) +3 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-kbl3/igt@i915_suspend@forcewake.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-kbl1/igt@i915_suspend@forcewake.html

  * igt@kms_cursor_crc@pipe-a-cursor-suspend:
    - shard-apl:          [PASS][7] -> [DMESG-WARN][8] ([i915#180])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-apl8/igt@kms_cursor_crc@pipe-a-cursor-suspend.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-apl8/igt@kms_cursor_crc@pipe-a-cursor-suspend.html

  * igt@kms_hdr@bpc-switch-suspend:
    - shard-skl:          [PASS][9] -> [FAIL][10] ([i915#1188])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-skl2/igt@kms_hdr@bpc-switch-suspend.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-skl8/igt@kms_hdr@bpc-switch-suspend.html

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - shard-skl:          [PASS][11] -> [INCOMPLETE][12] ([i915#69])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-skl6/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-skl8/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b.html

  * igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
    - shard-skl:          [PASS][13] -> [FAIL][14] ([fdo#108145] / [i915#265])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-skl5/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-skl1/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html

  * igt@kms_psr@psr2_primary_mmap_cpu:
    - shard-iclb:         [PASS][15] -> [SKIP][16] ([fdo#109441])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-iclb2/igt@kms_psr@psr2_primary_mmap_cpu.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-iclb1/igt@kms_psr@psr2_primary_mmap_cpu.html

  * igt@kms_setmode@basic:
    - shard-kbl:          [PASS][17] -> [FAIL][18] ([i915#31])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-kbl6/igt@kms_setmode@basic.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-kbl7/igt@kms_setmode@basic.html

  
#### Possible fixes ####

  * igt@gem_eio@in-flight-suspend:
    - shard-skl:          [INCOMPLETE][19] ([i915#69]) -> [PASS][20] +1 similar issue
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-skl9/igt@gem_eio@in-flight-suspend.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-skl2/igt@gem_eio@in-flight-suspend.html

  * {igt@gem_exec_fence@submit@rcs0}:
    - shard-tglb:         [INCOMPLETE][21] ([i915#1841]) -> [PASS][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-tglb8/igt@gem_exec_fence@submit@rcs0.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-tglb1/igt@gem_exec_fence@submit@rcs0.html
    - shard-kbl:          [INCOMPLETE][23] ([i915#1841]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-kbl6/igt@gem_exec_fence@submit@rcs0.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-kbl7/igt@gem_exec_fence@submit@rcs0.html
    - shard-skl:          [INCOMPLETE][25] ([i915#1841]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-skl5/igt@gem_exec_fence@submit@rcs0.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-skl6/igt@gem_exec_fence@submit@rcs0.html
    - shard-glk:          [INCOMPLETE][27] ([i915#1841] / [i915#58] / [k.org#198133]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-glk6/igt@gem_exec_fence@submit@rcs0.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-glk6/igt@gem_exec_fence@submit@rcs0.html
    - shard-iclb:         [INCOMPLETE][29] ([i915#1841]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-iclb7/igt@gem_exec_fence@submit@rcs0.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-iclb1/igt@gem_exec_fence@submit@rcs0.html
    - shard-apl:          [INCOMPLETE][31] ([i915#1841]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-apl6/igt@gem_exec_fence@submit@rcs0.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-apl1/igt@gem_exec_fence@submit@rcs0.html

  * igt@gen9_exec_parse@allowed-single:
    - shard-skl:          [DMESG-WARN][33] ([i915#716]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-skl5/igt@gen9_exec_parse@allowed-single.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-skl9/igt@gen9_exec_parse@allowed-single.html

  * igt@i915_suspend@fence-restore-tiled2untiled:
    - shard-apl:          [DMESG-WARN][35] ([i915#180]) -> [PASS][36] +4 similar issues
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-apl6/igt@i915_suspend@fence-restore-tiled2untiled.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-apl1/igt@i915_suspend@fence-restore-tiled2untiled.html

  * igt@kms_cursor_crc@pipe-b-cursor-64x64-onscreen:
    - shard-skl:          [FAIL][37] ([i915#54]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-skl9/igt@kms_cursor_crc@pipe-b-cursor-64x64-onscreen.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-skl3/igt@kms_cursor_crc@pipe-b-cursor-64x64-onscreen.html

  * igt@kms_cursor_crc@pipe-b-cursor-suspend:
    - shard-skl:          [INCOMPLETE][39] ([i915#300]) -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-skl10/igt@kms_cursor_crc@pipe-b-cursor-suspend.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-skl10/igt@kms_cursor_crc@pipe-b-cursor-suspend.html

  * igt@kms_cursor_legacy@nonblocking-modeset-vs-cursor-atomic:
    - shard-glk:          [FAIL][41] ([i915#67]) -> [PASS][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-glk5/igt@kms_cursor_legacy@nonblocking-modeset-vs-cursor-atomic.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-glk9/igt@kms_cursor_legacy@nonblocking-modeset-vs-cursor-atomic.html

  * {igt@kms_flip@flip-vs-expired-vblank-interruptible@c-dp1}:
    - shard-apl:          [FAIL][43] ([i915#79]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-apl3/igt@kms_flip@flip-vs-expired-vblank-interruptible@c-dp1.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-apl8/igt@kms_flip@flip-vs-expired-vblank-interruptible@c-dp1.html

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes:
    - shard-kbl:          [DMESG-WARN][45] ([i915#180]) -> [PASS][46]
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-kbl6/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-kbl7/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes.html

  * igt@kms_plane_alpha_blend@pipe-b-constant-alpha-min:
    - shard-skl:          [FAIL][47] ([fdo#108145] / [i915#265]) -> [PASS][48]
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-skl7/igt@kms_plane_alpha_blend@pipe-b-constant-alpha-min.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-skl1/igt@kms_plane_alpha_blend@pipe-b-constant-alpha-min.html

  * igt@kms_plane_lowres@pipe-a-tiling-none:
    - shard-glk:          [FAIL][49] ([i915#899]) -> [PASS][50]
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-glk9/igt@kms_plane_lowres@pipe-a-tiling-none.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-glk7/igt@kms_plane_lowres@pipe-a-tiling-none.html

  * igt@kms_psr@psr2_dpms:
    - shard-iclb:         [SKIP][51] ([fdo#109441]) -> [PASS][52] +1 similar issue
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-iclb4/igt@kms_psr@psr2_dpms.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-iclb2/igt@kms_psr@psr2_dpms.html

  
#### Warnings ####

  * igt@i915_pm_dc@dc6-dpms:
    - shard-tglb:         [SKIP][53] ([i915#468]) -> [FAIL][54] ([i915#454])
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-tglb2/igt@i915_pm_dc@dc6-dpms.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-tglb8/igt@i915_pm_dc@dc6-dpms.html

  * igt@i915_pm_rc6_residency@rc6-idle:
    - shard-iclb:         [FAIL][55] ([i915#1515]) -> [WARN][56] ([i915#1515])
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-iclb5/igt@i915_pm_rc6_residency@rc6-idle.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-iclb6/igt@i915_pm_rc6_residency@rc6-idle.html

  * igt@kms_content_protection@legacy:
    - shard-apl:          [FAIL][57] ([fdo#110321] / [fdo#110336]) -> [TIMEOUT][58] ([i915#1319])
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-apl8/igt@kms_content_protection@legacy.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-apl2/igt@kms_content_protection@legacy.html

  * igt@kms_content_protection@lic:
    - shard-apl:          [TIMEOUT][59] ([i915#1319]) -> [FAIL][60] ([fdo#110321])
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-apl4/igt@kms_content_protection@lic.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-apl6/igt@kms_content_protection@lic.html

  * igt@kms_fbcon_fbt@fbc-suspend:
    - shard-kbl:          [FAIL][61] ([i915#93] / [i915#95]) -> [DMESG-FAIL][62] ([i915#180] / [i915#95])
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8441/shard-kbl4/igt@kms_fbcon_fbt@fbc-suspend.html
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/shard-kbl1/igt@kms_fbcon_fbt@fbc-suspend.html

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

  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#110321]: https://bugs.freedesktop.org/show_bug.cgi?id=110321
  [fdo#110336]: https://bugs.freedesktop.org/show_bug.cgi?id=110336
  [i915#1188]: https://gitlab.freedesktop.org/drm/intel/issues/1188
  [i915#1319]: https://gitlab.freedesktop.org/drm/intel/issues/1319
  [i915#1436]: https://gitlab.freedesktop.org/drm/intel/issues/1436
  [i915#1515]: https://gitlab.freedesktop.org/drm/intel/issues/1515
  [i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
  [i915#1841]: https://gitlab.freedesktop.org/drm/intel/issues/1841
  [i915#265]: https://gitlab.freedesktop.org/drm/intel/issues/265
  [i915#300]: https://gitlab.freedesktop.org/drm/intel/issues/300
  [i915#31]: https://gitlab.freedesktop.org/drm/intel/issues/31
  [i915#454]: https://gitlab.freedesktop.org/drm/intel/issues/454
  [i915#468]: https://gitlab.freedesktop.org/drm/intel/issues/468
  [i915#54]: https://gitlab.freedesktop.org/drm/intel/issues/54
  [i915#58]: https://gitlab.freedesktop.org/drm/intel/issues/58
  [i915#67]: https://gitlab.freedesktop.org/drm/intel/issues/67
  [i915#69]: https://gitlab.freedesktop.org/drm/intel/issues/69
  [i915#716]: https://gitlab.freedesktop.org/drm/intel/issues/716
  [i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79
  [i915#899]: https://gitlab.freedesktop.org/drm/intel/issues/899
  [i915#93]: https://gitlab.freedesktop.org/drm/intel/issues/93
  [i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95
  [k.org#198133]: https://bugzilla.kernel.org/show_bug.cgi?id=198133


Participating hosts (11 -> 11)
------------------------------

  No changes in participating hosts


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_8441 -> Patchwork_17598

  CI-20190529: 20190529
  CI_DRM_8441: 6c0ee41a7c3201ef2a89800234803a95f65989be @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5637: fdc33f7e1adc5bb6a1ba88b6233aaf224174d75a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_17598: 183fe789671fbe6252b0474dc63a6cca25c998cc @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17598/index.html
_______________________________________________
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: [Intel-gfx] [PATCH 1/3] drm/i915: Mark concurrent submissions with a weak-dependency
  2020-05-07  8:21 [Intel-gfx] [PATCH 1/3] drm/i915: Mark concurrent submissions with a weak-dependency Chris Wilson
                   ` (3 preceding siblings ...)
  2020-05-07 12:49 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
@ 2020-05-07 14:53 ` Tvrtko Ursulin
  2020-05-07 15:00   ` Chris Wilson
  4 siblings, 1 reply; 17+ messages in thread
From: Tvrtko Ursulin @ 2020-05-07 14:53 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: stable


On 07/05/2020 09:21, Chris Wilson wrote:
> We recorded the dependencies for WAIT_FOR_SUBMIT in order that we could
> correctly perform priority inheritance from the parallel branches to the
> common trunk. However, for the purpose of timeslicing and reset
> handling, the dependency is weak -- as we the pair of requests are
> allowed to run in parallel and not in strict succession. So for example
> we do need to suspend one if the other hangs.
> 
> The real significance though is that this allows us to rearrange
> groups of WAIT_FOR_SUBMIT linked requests along the single engine, and
> so can resolve user level inter-batch scheduling dependencies from user
> semaphores.
> 
> Fixes: c81471f5e95c ("drm/i915: Copy across scheduler behaviour flags across submit fences")
> Testcase: igt/gem_exec_fence/submit
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: <stable@vger.kernel.org> # v5.6+
> ---
>   drivers/gpu/drm/i915/gt/intel_lrc.c         | 9 +++++++++
>   drivers/gpu/drm/i915/i915_request.c         | 8 ++++++--
>   drivers/gpu/drm/i915/i915_scheduler.c       | 6 +++---
>   drivers/gpu/drm/i915/i915_scheduler.h       | 3 ++-
>   drivers/gpu/drm/i915/i915_scheduler_types.h | 1 +
>   5 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index dc3f2ee7136d..10109f661bcb 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -1880,6 +1880,9 @@ static void defer_request(struct i915_request *rq, struct list_head * const pl)
>   			struct i915_request *w =
>   				container_of(p->waiter, typeof(*w), sched);
>   
> +			if (p->flags & I915_DEPENDENCY_WEAK)
> +				continue;
> +

I did not quite get it - submit fence dependency would mean different 
engines, so the below check (w->engine != rq->engine) would effectively 
have the same effect. What am I missing?

Regards,

Tvrtko

>   			/* Leave semaphores spinning on the other engines */
>   			if (w->engine != rq->engine)
>   				continue;
> @@ -2726,6 +2729,9 @@ static void __execlists_hold(struct i915_request *rq)
>   			struct i915_request *w =
>   				container_of(p->waiter, typeof(*w), sched);
>   
> +			if (p->flags & I915_DEPENDENCY_WEAK)
> +				continue;
> +
>   			/* Leave semaphores spinning on the other engines */
>   			if (w->engine != rq->engine)
>   				continue;
> @@ -2850,6 +2856,9 @@ static void __execlists_unhold(struct i915_request *rq)
>   			struct i915_request *w =
>   				container_of(p->waiter, typeof(*w), sched);
>   
> +			if (p->flags & I915_DEPENDENCY_WEAK)
> +				continue;
> +
>   			/* Propagate any change in error status */
>   			if (rq->fence.error)
>   				i915_request_set_error_once(w, rq->fence.error);
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 4d18f808fda2..3c38d61c90f8 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -1040,7 +1040,9 @@ i915_request_await_request(struct i915_request *to, struct i915_request *from)
>   	}
>   
>   	if (to->engine->schedule) {
> -		ret = i915_sched_node_add_dependency(&to->sched, &from->sched);
> +		ret = i915_sched_node_add_dependency(&to->sched,
> +						     &from->sched,
> +						     I915_DEPENDENCY_EXTERNAL);
>   		if (ret < 0)
>   			return ret;
>   	}
> @@ -1202,7 +1204,9 @@ __i915_request_await_execution(struct i915_request *to,
>   
>   	/* Couple the dependency tree for PI on this exposed to->fence */
>   	if (to->engine->schedule) {
> -		err = i915_sched_node_add_dependency(&to->sched, &from->sched);
> +		err = i915_sched_node_add_dependency(&to->sched,
> +						     &from->sched,
> +						     I915_DEPENDENCY_WEAK);
>   		if (err < 0)
>   			return err;
>   	}
> diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
> index 37cfcf5b321b..6e2d4190099f 100644
> --- a/drivers/gpu/drm/i915/i915_scheduler.c
> +++ b/drivers/gpu/drm/i915/i915_scheduler.c
> @@ -462,7 +462,8 @@ bool __i915_sched_node_add_dependency(struct i915_sched_node *node,
>   }
>   
>   int i915_sched_node_add_dependency(struct i915_sched_node *node,
> -				   struct i915_sched_node *signal)
> +				   struct i915_sched_node *signal,
> +				   unsigned long flags)
>   {
>   	struct i915_dependency *dep;
>   
> @@ -473,8 +474,7 @@ int i915_sched_node_add_dependency(struct i915_sched_node *node,
>   	local_bh_disable();
>   
>   	if (!__i915_sched_node_add_dependency(node, signal, dep,
> -					      I915_DEPENDENCY_EXTERNAL |
> -					      I915_DEPENDENCY_ALLOC))
> +					      flags | I915_DEPENDENCY_ALLOC))
>   		i915_dependency_free(dep);
>   
>   	local_bh_enable(); /* kick submission tasklet */
> diff --git a/drivers/gpu/drm/i915/i915_scheduler.h b/drivers/gpu/drm/i915/i915_scheduler.h
> index d1dc4efef77b..6f0bf00fc569 100644
> --- a/drivers/gpu/drm/i915/i915_scheduler.h
> +++ b/drivers/gpu/drm/i915/i915_scheduler.h
> @@ -34,7 +34,8 @@ bool __i915_sched_node_add_dependency(struct i915_sched_node *node,
>   				      unsigned long flags);
>   
>   int i915_sched_node_add_dependency(struct i915_sched_node *node,
> -				   struct i915_sched_node *signal);
> +				   struct i915_sched_node *signal,
> +				   unsigned long flags);
>   
>   void i915_sched_node_fini(struct i915_sched_node *node);
>   
> diff --git a/drivers/gpu/drm/i915/i915_scheduler_types.h b/drivers/gpu/drm/i915/i915_scheduler_types.h
> index d18e70550054..7186875088a0 100644
> --- a/drivers/gpu/drm/i915/i915_scheduler_types.h
> +++ b/drivers/gpu/drm/i915/i915_scheduler_types.h
> @@ -78,6 +78,7 @@ struct i915_dependency {
>   	unsigned long flags;
>   #define I915_DEPENDENCY_ALLOC		BIT(0)
>   #define I915_DEPENDENCY_EXTERNAL	BIT(1)
> +#define I915_DEPENDENCY_WEAK		BIT(2)
>   };
>   
>   #endif /* _I915_SCHEDULER_TYPES_H_ */
> 
_______________________________________________
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: [Intel-gfx] [PATCH 3/3] drm/i915: Treat weak-dependencies as bidirectional when applying priorities
  2020-05-07  8:21 ` [Intel-gfx] [PATCH 3/3] drm/i915: Treat weak-dependencies as bidirectional when applying priorities Chris Wilson
  2020-05-07  8:29   ` [Intel-gfx] [PATCH] " Chris Wilson
@ 2020-05-07 14:58   ` Tvrtko Ursulin
  1 sibling, 0 replies; 17+ messages in thread
From: Tvrtko Ursulin @ 2020-05-07 14:58 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx


On 07/05/2020 09:21, Chris Wilson wrote:
> Clients may use a submit-fence as bidirectional bond between two or more
> co-operating requests, and so if we bump the priority of one, we wish to
> bump the priority of the set.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_scheduler.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
> index 6e2d4190099f..7194fbfcaa49 100644
> --- a/drivers/gpu/drm/i915/i915_scheduler.c
> +++ b/drivers/gpu/drm/i915/i915_scheduler.c
> @@ -291,6 +291,12 @@ static void __i915_schedule(struct i915_sched_node *node,
>   			if (prio > READ_ONCE(p->signaler->attr.priority))
>   				list_move_tail(&p->dfs_link, &dfs);
>   		}
> +
> +		list_for_each_entry(p, &node->waiters_list, wait_link) {
> +			if (p->flags & I915_DEPENDENCY_WEAK &&
> +			    prio > READ_ONCE(p->waiter->attr.priority))
> +				list_move_tail(&p->dfs_link, &dfs);
> +		}
>   	}
>   
>   	/*
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
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: [Intel-gfx] [PATCH 2/3] drm/i915/gem: Treat submit-fence as weak dependency for new clients
  2020-05-07  8:21 ` [Intel-gfx] [PATCH 2/3] drm/i915/gem: Treat submit-fence as weak dependency for new clients Chris Wilson
@ 2020-05-07 14:59   ` Tvrtko Ursulin
  2020-05-07 15:05     ` Chris Wilson
  0 siblings, 1 reply; 17+ messages in thread
From: Tvrtko Ursulin @ 2020-05-07 14:59 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx


On 07/05/2020 09:21, Chris Wilson wrote:
> The submit-fence adds a weak dependency to the requests, and for the
> purpose of our FQ_CODEL hinting we do not want to treat as a
> restriction. This is primarily because clients may treat submit-fences
> as a bidirectional bonding between a pair of co-ordinating requests.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index 966523a8503f..e8bf0cf02fd7 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -2565,6 +2565,17 @@ static void retire_requests(struct intel_timeline *tl, struct i915_request *end)
>   			break;
>   }
>   
> +static bool new_client(struct i915_request *rq)
> +{
> +	struct i915_dependency *p;
> +
> +	list_for_each_entry(p, &rq->sched.signalers_list, signal_link)
> +		if (!(p->flags & I915_DEPENDENCY_WEAK))
> +			return false;
> +
> +	return true;
> +}
> +
>   static void eb_request_add(struct i915_execbuffer *eb)
>   {
>   	struct i915_request *rq = eb->request;
> @@ -2604,7 +2615,7 @@ static void eb_request_add(struct i915_execbuffer *eb)
>   		 * Allow interactive/synchronous clients to jump ahead of
>   		 * the bulk clients. (FQ_CODEL)
>   		 */
> -		if (list_empty(&rq->sched.signalers_list))
> +		if (new_client(rq))
>   			attr.priority |= I915_PRIORITY_WAIT;
>   	} else {
>   		/* Serialise with context_close via the add_to_timeline */
> 

Did absence of this have any functional effect? I hope not, but anyway:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
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: [Intel-gfx] [PATCH 1/3] drm/i915: Mark concurrent submissions with a weak-dependency
  2020-05-07 14:53 ` [Intel-gfx] [PATCH 1/3] drm/i915: Mark concurrent submissions with a weak-dependency Tvrtko Ursulin
@ 2020-05-07 15:00   ` Chris Wilson
  2020-05-07 15:23     ` Tvrtko Ursulin
  0 siblings, 1 reply; 17+ messages in thread
From: Chris Wilson @ 2020-05-07 15:00 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx; +Cc: stable

Quoting Tvrtko Ursulin (2020-05-07 15:53:08)
> 
> On 07/05/2020 09:21, Chris Wilson wrote:
> > We recorded the dependencies for WAIT_FOR_SUBMIT in order that we could
> > correctly perform priority inheritance from the parallel branches to the
> > common trunk. However, for the purpose of timeslicing and reset
> > handling, the dependency is weak -- as we the pair of requests are
> > allowed to run in parallel and not in strict succession. So for example
> > we do need to suspend one if the other hangs.
> > 
> > The real significance though is that this allows us to rearrange
> > groups of WAIT_FOR_SUBMIT linked requests along the single engine, and
> > so can resolve user level inter-batch scheduling dependencies from user
> > semaphores.
> > 
> > Fixes: c81471f5e95c ("drm/i915: Copy across scheduler behaviour flags across submit fences")
> > Testcase: igt/gem_exec_fence/submit
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: <stable@vger.kernel.org> # v5.6+
> > ---
> >   drivers/gpu/drm/i915/gt/intel_lrc.c         | 9 +++++++++
> >   drivers/gpu/drm/i915/i915_request.c         | 8 ++++++--
> >   drivers/gpu/drm/i915/i915_scheduler.c       | 6 +++---
> >   drivers/gpu/drm/i915/i915_scheduler.h       | 3 ++-
> >   drivers/gpu/drm/i915/i915_scheduler_types.h | 1 +
> >   5 files changed, 21 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index dc3f2ee7136d..10109f661bcb 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -1880,6 +1880,9 @@ static void defer_request(struct i915_request *rq, struct list_head * const pl)
> >                       struct i915_request *w =
> >                               container_of(p->waiter, typeof(*w), sched);
> >   
> > +                     if (p->flags & I915_DEPENDENCY_WEAK)
> > +                             continue;
> > +
> 
> I did not quite get it - submit fence dependency would mean different 
> engines, so the below check (w->engine != rq->engine) would effectively 
> have the same effect. What am I missing?

That submit fences can be between different contexts on the same engine.
The example (from mesa) is where we have two interdependent clients
which are using their own userlevel scheduling inside each batch, i.e.
waiting on semaphores.
-Chris
_______________________________________________
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: [Intel-gfx] [PATCH 2/3] drm/i915/gem: Treat submit-fence as weak dependency for new clients
  2020-05-07 14:59   ` Tvrtko Ursulin
@ 2020-05-07 15:05     ` Chris Wilson
  2020-05-07 15:10       ` Tvrtko Ursulin
  0 siblings, 1 reply; 17+ messages in thread
From: Chris Wilson @ 2020-05-07 15:05 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx

Quoting Tvrtko Ursulin (2020-05-07 15:59:56)
> 
> On 07/05/2020 09:21, Chris Wilson wrote:
> > The submit-fence adds a weak dependency to the requests, and for the
> > purpose of our FQ_CODEL hinting we do not want to treat as a
> > restriction. This is primarily because clients may treat submit-fences
> > as a bidirectional bonding between a pair of co-ordinating requests.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >   drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 13 ++++++++++++-
> >   1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > index 966523a8503f..e8bf0cf02fd7 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > @@ -2565,6 +2565,17 @@ static void retire_requests(struct intel_timeline *tl, struct i915_request *end)
> >                       break;
> >   }
> >   
> > +static bool new_client(struct i915_request *rq)
> > +{
> > +     struct i915_dependency *p;
> > +
> > +     list_for_each_entry(p, &rq->sched.signalers_list, signal_link)
> > +             if (!(p->flags & I915_DEPENDENCY_WEAK))
> > +                     return false;
> > +
> > +     return true;
> > +}
> > +
> >   static void eb_request_add(struct i915_execbuffer *eb)
> >   {
> >       struct i915_request *rq = eb->request;
> > @@ -2604,7 +2615,7 @@ static void eb_request_add(struct i915_execbuffer *eb)
> >                * Allow interactive/synchronous clients to jump ahead of
> >                * the bulk clients. (FQ_CODEL)
> >                */
> > -             if (list_empty(&rq->sched.signalers_list))
> > +             if (new_client(rq))
> >                       attr.priority |= I915_PRIORITY_WAIT;
> >       } else {
> >               /* Serialise with context_close via the add_to_timeline */
> > 
> 
> Did absence of this have any functional effect? I hope not, but anyway:

Bah, I have a new test case where this WAIT bumping is still upsetting us.

I don't think I have any choice but to rip it out if we have timeslicing
enabled.

Would you prefer a complete remission of I915_PRIORITY_WAIT or keep it
under if (!intel_engine_has_timeslicing(rq->engine)) ?
-Chris
_______________________________________________
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: [Intel-gfx] [PATCH 2/3] drm/i915/gem: Treat submit-fence as weak dependency for new clients
  2020-05-07 15:05     ` Chris Wilson
@ 2020-05-07 15:10       ` Tvrtko Ursulin
  2020-05-07 15:17         ` Chris Wilson
  0 siblings, 1 reply; 17+ messages in thread
From: Tvrtko Ursulin @ 2020-05-07 15:10 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx


On 07/05/2020 16:05, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-05-07 15:59:56)
>>
>> On 07/05/2020 09:21, Chris Wilson wrote:
>>> The submit-fence adds a weak dependency to the requests, and for the
>>> purpose of our FQ_CODEL hinting we do not want to treat as a
>>> restriction. This is primarily because clients may treat submit-fences
>>> as a bidirectional bonding between a pair of co-ordinating requests.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 13 ++++++++++++-
>>>    1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>>> index 966523a8503f..e8bf0cf02fd7 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>>> @@ -2565,6 +2565,17 @@ static void retire_requests(struct intel_timeline *tl, struct i915_request *end)
>>>                        break;
>>>    }
>>>    
>>> +static bool new_client(struct i915_request *rq)
>>> +{
>>> +     struct i915_dependency *p;
>>> +
>>> +     list_for_each_entry(p, &rq->sched.signalers_list, signal_link)
>>> +             if (!(p->flags & I915_DEPENDENCY_WEAK))
>>> +                     return false;
>>> +
>>> +     return true;
>>> +}
>>> +
>>>    static void eb_request_add(struct i915_execbuffer *eb)
>>>    {
>>>        struct i915_request *rq = eb->request;
>>> @@ -2604,7 +2615,7 @@ static void eb_request_add(struct i915_execbuffer *eb)
>>>                 * Allow interactive/synchronous clients to jump ahead of
>>>                 * the bulk clients. (FQ_CODEL)
>>>                 */
>>> -             if (list_empty(&rq->sched.signalers_list))
>>> +             if (new_client(rq))
>>>                        attr.priority |= I915_PRIORITY_WAIT;
>>>        } else {
>>>                /* Serialise with context_close via the add_to_timeline */
>>>
>>
>> Did absence of this have any functional effect? I hope not, but anyway:
> 
> Bah, I have a new test case where this WAIT bumping is still upsetting us.
> 
> I don't think I have any choice but to rip it out if we have timeslicing
> enabled.
> 
> Would you prefer a complete remission of I915_PRIORITY_WAIT or keep it
> under if (!intel_engine_has_timeslicing(rq->engine)) ?

Doesn't feel worthwhile to keep it for just BDW right?

Regards,

Tvrtko
_______________________________________________
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: [Intel-gfx] [PATCH 2/3] drm/i915/gem: Treat submit-fence as weak dependency for new clients
  2020-05-07 15:10       ` Tvrtko Ursulin
@ 2020-05-07 15:17         ` Chris Wilson
  0 siblings, 0 replies; 17+ messages in thread
From: Chris Wilson @ 2020-05-07 15:17 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx

Quoting Tvrtko Ursulin (2020-05-07 16:10:37)
> 
> On 07/05/2020 16:05, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2020-05-07 15:59:56)
> >>
> >> On 07/05/2020 09:21, Chris Wilson wrote:
> >>> The submit-fence adds a weak dependency to the requests, and for the
> >>> purpose of our FQ_CODEL hinting we do not want to treat as a
> >>> restriction. This is primarily because clients may treat submit-fences
> >>> as a bidirectional bonding between a pair of co-ordinating requests.
> >>>
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>> ---
> >>>    drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 13 ++++++++++++-
> >>>    1 file changed, 12 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> >>> index 966523a8503f..e8bf0cf02fd7 100644
> >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> >>> @@ -2565,6 +2565,17 @@ static void retire_requests(struct intel_timeline *tl, struct i915_request *end)
> >>>                        break;
> >>>    }
> >>>    
> >>> +static bool new_client(struct i915_request *rq)
> >>> +{
> >>> +     struct i915_dependency *p;
> >>> +
> >>> +     list_for_each_entry(p, &rq->sched.signalers_list, signal_link)
> >>> +             if (!(p->flags & I915_DEPENDENCY_WEAK))
> >>> +                     return false;
> >>> +
> >>> +     return true;
> >>> +}
> >>> +
> >>>    static void eb_request_add(struct i915_execbuffer *eb)
> >>>    {
> >>>        struct i915_request *rq = eb->request;
> >>> @@ -2604,7 +2615,7 @@ static void eb_request_add(struct i915_execbuffer *eb)
> >>>                 * Allow interactive/synchronous clients to jump ahead of
> >>>                 * the bulk clients. (FQ_CODEL)
> >>>                 */
> >>> -             if (list_empty(&rq->sched.signalers_list))
> >>> +             if (new_client(rq))
> >>>                        attr.priority |= I915_PRIORITY_WAIT;
> >>>        } else {
> >>>                /* Serialise with context_close via the add_to_timeline */
> >>>
> >>
> >> Did absence of this have any functional effect? I hope not, but anyway:
> > 
> > Bah, I have a new test case where this WAIT bumping is still upsetting us.
> > 
> > I don't think I have any choice but to rip it out if we have timeslicing
> > enabled.
> > 
> > Would you prefer a complete remission of I915_PRIORITY_WAIT or keep it
> > under if (!intel_engine_has_timeslicing(rq->engine)) ?
> 
> Doesn't feel worthwhile to keep it for just BDW right?

No. There's ivb waiting in the rings (as I haven't figured out
preemption for it yet), but similarly just doesn't feel worth the
complications. :(
-Chris
_______________________________________________
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: [Intel-gfx] [PATCH 1/3] drm/i915: Mark concurrent submissions with a weak-dependency
  2020-05-07 15:00   ` Chris Wilson
@ 2020-05-07 15:23     ` Tvrtko Ursulin
  2020-05-07 15:34       ` Chris Wilson
  0 siblings, 1 reply; 17+ messages in thread
From: Tvrtko Ursulin @ 2020-05-07 15:23 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: stable



On 07/05/2020 16:00, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-05-07 15:53:08)
>> On 07/05/2020 09:21, Chris Wilson wrote:
>>> We recorded the dependencies for WAIT_FOR_SUBMIT in order that we could
>>> correctly perform priority inheritance from the parallel branches to the
>>> common trunk. However, for the purpose of timeslicing and reset
>>> handling, the dependency is weak -- as we the pair of requests are
>>> allowed to run in parallel and not in strict succession. So for example
>>> we do need to suspend one if the other hangs.
>>>
>>> The real significance though is that this allows us to rearrange
>>> groups of WAIT_FOR_SUBMIT linked requests along the single engine, and
>>> so can resolve user level inter-batch scheduling dependencies from user
>>> semaphores.
>>>
>>> Fixes: c81471f5e95c ("drm/i915: Copy across scheduler behaviour flags across submit fences")
>>> Testcase: igt/gem_exec_fence/submit
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: <stable@vger.kernel.org> # v5.6+
>>> ---
>>>    drivers/gpu/drm/i915/gt/intel_lrc.c         | 9 +++++++++
>>>    drivers/gpu/drm/i915/i915_request.c         | 8 ++++++--
>>>    drivers/gpu/drm/i915/i915_scheduler.c       | 6 +++---
>>>    drivers/gpu/drm/i915/i915_scheduler.h       | 3 ++-
>>>    drivers/gpu/drm/i915/i915_scheduler_types.h | 1 +
>>>    5 files changed, 21 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
>>> index dc3f2ee7136d..10109f661bcb 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
>>> @@ -1880,6 +1880,9 @@ static void defer_request(struct i915_request *rq, struct list_head * const pl)
>>>                        struct i915_request *w =
>>>                                container_of(p->waiter, typeof(*w), sched);
>>>    
>>> +                     if (p->flags & I915_DEPENDENCY_WEAK)
>>> +                             continue;
>>> +
>>
>> I did not quite get it - submit fence dependency would mean different
>> engines, so the below check (w->engine != rq->engine) would effectively
>> have the same effect. What am I missing?
> 
> That submit fences can be between different contexts on the same engine.
> The example (from mesa) is where we have two interdependent clients
> which are using their own userlevel scheduling inside each batch, i.e.
> waiting on semaphores.

But if submit fence was used that means the waiter should never be 
submitted ahead of the signaler. And with this change it could get ahead 
in the priolist, no?

Regards,

Tvrtko
_______________________________________________
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: [Intel-gfx] [PATCH 1/3] drm/i915: Mark concurrent submissions with a weak-dependency
  2020-05-07 15:23     ` Tvrtko Ursulin
@ 2020-05-07 15:34       ` Chris Wilson
  2020-05-07 17:55         ` Tvrtko Ursulin
  0 siblings, 1 reply; 17+ messages in thread
From: Chris Wilson @ 2020-05-07 15:34 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx; +Cc: stable

Quoting Tvrtko Ursulin (2020-05-07 16:23:59)
> 
> 
> On 07/05/2020 16:00, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2020-05-07 15:53:08)
> >> On 07/05/2020 09:21, Chris Wilson wrote:
> >>> We recorded the dependencies for WAIT_FOR_SUBMIT in order that we could
> >>> correctly perform priority inheritance from the parallel branches to the
> >>> common trunk. However, for the purpose of timeslicing and reset
> >>> handling, the dependency is weak -- as we the pair of requests are
> >>> allowed to run in parallel and not in strict succession. So for example
> >>> we do need to suspend one if the other hangs.
> >>>
> >>> The real significance though is that this allows us to rearrange
> >>> groups of WAIT_FOR_SUBMIT linked requests along the single engine, and
> >>> so can resolve user level inter-batch scheduling dependencies from user
> >>> semaphores.
> >>>
> >>> Fixes: c81471f5e95c ("drm/i915: Copy across scheduler behaviour flags across submit fences")
> >>> Testcase: igt/gem_exec_fence/submit
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>> Cc: <stable@vger.kernel.org> # v5.6+
> >>> ---
> >>>    drivers/gpu/drm/i915/gt/intel_lrc.c         | 9 +++++++++
> >>>    drivers/gpu/drm/i915/i915_request.c         | 8 ++++++--
> >>>    drivers/gpu/drm/i915/i915_scheduler.c       | 6 +++---
> >>>    drivers/gpu/drm/i915/i915_scheduler.h       | 3 ++-
> >>>    drivers/gpu/drm/i915/i915_scheduler_types.h | 1 +
> >>>    5 files changed, 21 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> >>> index dc3f2ee7136d..10109f661bcb 100644
> >>> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> >>> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> >>> @@ -1880,6 +1880,9 @@ static void defer_request(struct i915_request *rq, struct list_head * const pl)
> >>>                        struct i915_request *w =
> >>>                                container_of(p->waiter, typeof(*w), sched);
> >>>    
> >>> +                     if (p->flags & I915_DEPENDENCY_WEAK)
> >>> +                             continue;
> >>> +
> >>
> >> I did not quite get it - submit fence dependency would mean different
> >> engines, so the below check (w->engine != rq->engine) would effectively
> >> have the same effect. What am I missing?
> > 
> > That submit fences can be between different contexts on the same engine.
> > The example (from mesa) is where we have two interdependent clients
> > which are using their own userlevel scheduling inside each batch, i.e.
> > waiting on semaphores.
> 
> But if submit fence was used that means the waiter should never be 
> submitted ahead of the signaler. And with this change it could get ahead 
> in the priolist, no?

You do recall the starting point for this series was future fences :)

The test case for this is:

	execbuf.flags = engine | I915_EXEC_FENCE_OUT;
	execbuf.batch_start_offset = 0;
       	gem_execbuf_wr(i915, &execbuf);

       	execbuf.rsvd1 = gem_context_clone_with_engines(i915, 0);
       	execbuf.rsvd2 >>= 32;
       	execbuf.flags = e->flags;
       	execbuf.flags |= I915_EXEC_FENCE_SUBMIT | I915_EXEC_FENCE_OUT;
       	execbuf.batch_start_offset = offset;
       	gem_execbuf_wr(i915, &execbuf);
       	gem_context_destroy(i915, execbuf.rsvd1);

       	gem_sync(i915, obj.handle);

	/* no hangs! */
	out = execbuf.rsvd2;
       	igt_assert_eq(sync_fence_status(out), 1);
       	close(out);

       	out = execbuf.rsvd2 >> 32;
       	igt_assert_eq(sync_fence_status(out), 1);
       	close(out);

Where the batches are a couple of semaphore waits, which is the essence
of a bonded request but being run on a single engine.

Unless we treat the submit fence as a weak dependency here, we can't
timeslice between the two.

The other observation is that we may not have to suspend the request if
the other hangs as the linkage is implicit. If the request does continue
to wait on the hung request, we can only hope it too hangs. I should
make that a second patch, since it is a distinct change.
-Chris
_______________________________________________
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: [Intel-gfx] [PATCH 1/3] drm/i915: Mark concurrent submissions with a weak-dependency
  2020-05-07 15:34       ` Chris Wilson
@ 2020-05-07 17:55         ` Tvrtko Ursulin
  2020-05-07 18:05           ` Chris Wilson
  0 siblings, 1 reply; 17+ messages in thread
From: Tvrtko Ursulin @ 2020-05-07 17:55 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: stable


On 07/05/2020 16:34, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-05-07 16:23:59)
>> On 07/05/2020 16:00, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2020-05-07 15:53:08)
>>>> On 07/05/2020 09:21, Chris Wilson wrote:
>>>>> We recorded the dependencies for WAIT_FOR_SUBMIT in order that we could
>>>>> correctly perform priority inheritance from the parallel branches to the
>>>>> common trunk. However, for the purpose of timeslicing and reset
>>>>> handling, the dependency is weak -- as we the pair of requests are
>>>>> allowed to run in parallel and not in strict succession. So for example
>>>>> we do need to suspend one if the other hangs.
>>>>>
>>>>> The real significance though is that this allows us to rearrange
>>>>> groups of WAIT_FOR_SUBMIT linked requests along the single engine, and
>>>>> so can resolve user level inter-batch scheduling dependencies from user
>>>>> semaphores.
>>>>>
>>>>> Fixes: c81471f5e95c ("drm/i915: Copy across scheduler behaviour flags across submit fences")
>>>>> Testcase: igt/gem_exec_fence/submit
>>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>> Cc: <stable@vger.kernel.org> # v5.6+
>>>>> ---
>>>>>     drivers/gpu/drm/i915/gt/intel_lrc.c         | 9 +++++++++
>>>>>     drivers/gpu/drm/i915/i915_request.c         | 8 ++++++--
>>>>>     drivers/gpu/drm/i915/i915_scheduler.c       | 6 +++---
>>>>>     drivers/gpu/drm/i915/i915_scheduler.h       | 3 ++-
>>>>>     drivers/gpu/drm/i915/i915_scheduler_types.h | 1 +
>>>>>     5 files changed, 21 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
>>>>> index dc3f2ee7136d..10109f661bcb 100644
>>>>> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
>>>>> @@ -1880,6 +1880,9 @@ static void defer_request(struct i915_request *rq, struct list_head * const pl)
>>>>>                         struct i915_request *w =
>>>>>                                 container_of(p->waiter, typeof(*w), sched);
>>>>>     
>>>>> +                     if (p->flags & I915_DEPENDENCY_WEAK)
>>>>> +                             continue;
>>>>> +
>>>>
>>>> I did not quite get it - submit fence dependency would mean different
>>>> engines, so the below check (w->engine != rq->engine) would effectively
>>>> have the same effect. What am I missing?
>>>
>>> That submit fences can be between different contexts on the same engine.
>>> The example (from mesa) is where we have two interdependent clients
>>> which are using their own userlevel scheduling inside each batch, i.e.
>>> waiting on semaphores.
>>
>> But if submit fence was used that means the waiter should never be
>> submitted ahead of the signaler. And with this change it could get ahead
>> in the priolist, no?
> 
> You do recall the starting point for this series was future fences :)
> 
> The test case for this is:
> 
> 	execbuf.flags = engine | I915_EXEC_FENCE_OUT;
> 	execbuf.batch_start_offset = 0;
>         	gem_execbuf_wr(i915, &execbuf);
> 
>         	execbuf.rsvd1 = gem_context_clone_with_engines(i915, 0);
>         	execbuf.rsvd2 >>= 32;
>         	execbuf.flags = e->flags;
>         	execbuf.flags |= I915_EXEC_FENCE_SUBMIT | I915_EXEC_FENCE_OUT;
>         	execbuf.batch_start_offset = offset;
>         	gem_execbuf_wr(i915, &execbuf);
>         	gem_context_destroy(i915, execbuf.rsvd1);
> 
>         	gem_sync(i915, obj.handle);
> 
> 	/* no hangs! */
> 	out = execbuf.rsvd2;
>         	igt_assert_eq(sync_fence_status(out), 1);
>         	close(out);
> 
>         	out = execbuf.rsvd2 >> 32;
>         	igt_assert_eq(sync_fence_status(out), 1);
>         	close(out);
> 
> Where the batches are a couple of semaphore waits, which is the essence
> of a bonded request but being run on a single engine.
> 
> Unless we treat the submit fence as a weak dependency here, we can't
> timeslice between the two.

Yes it is fine, once the initial submit was controlled by a fence it is 
okay to re-order.

Regards,

Tvrtko

> The other observation is that we may not have to suspend the request if
> the other hangs as the linkage is implicit. If the request does continue
> to wait on the hung request, we can only hope it too hangs. I should
> make that a second patch, since it is a distinct change.


_______________________________________________
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: [Intel-gfx] [PATCH 1/3] drm/i915: Mark concurrent submissions with a weak-dependency
  2020-05-07 17:55         ` Tvrtko Ursulin
@ 2020-05-07 18:05           ` Chris Wilson
  0 siblings, 0 replies; 17+ messages in thread
From: Chris Wilson @ 2020-05-07 18:05 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx; +Cc: stable

Quoting Tvrtko Ursulin (2020-05-07 18:55:17)
> 
> On 07/05/2020 16:34, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2020-05-07 16:23:59)
> >> On 07/05/2020 16:00, Chris Wilson wrote:
> >>> Quoting Tvrtko Ursulin (2020-05-07 15:53:08)
> >>>> On 07/05/2020 09:21, Chris Wilson wrote:
> >>>>> We recorded the dependencies for WAIT_FOR_SUBMIT in order that we could
> >>>>> correctly perform priority inheritance from the parallel branches to the
> >>>>> common trunk. However, for the purpose of timeslicing and reset
> >>>>> handling, the dependency is weak -- as we the pair of requests are
> >>>>> allowed to run in parallel and not in strict succession. So for example
> >>>>> we do need to suspend one if the other hangs.
> >>>>>
> >>>>> The real significance though is that this allows us to rearrange
> >>>>> groups of WAIT_FOR_SUBMIT linked requests along the single engine, and
> >>>>> so can resolve user level inter-batch scheduling dependencies from user
> >>>>> semaphores.
> >>>>>
> >>>>> Fixes: c81471f5e95c ("drm/i915: Copy across scheduler behaviour flags across submit fences")
> >>>>> Testcase: igt/gem_exec_fence/submit
> >>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>> Cc: <stable@vger.kernel.org> # v5.6+
> >>>>> ---
> >>>>>     drivers/gpu/drm/i915/gt/intel_lrc.c         | 9 +++++++++
> >>>>>     drivers/gpu/drm/i915/i915_request.c         | 8 ++++++--
> >>>>>     drivers/gpu/drm/i915/i915_scheduler.c       | 6 +++---
> >>>>>     drivers/gpu/drm/i915/i915_scheduler.h       | 3 ++-
> >>>>>     drivers/gpu/drm/i915/i915_scheduler_types.h | 1 +
> >>>>>     5 files changed, 21 insertions(+), 6 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> >>>>> index dc3f2ee7136d..10109f661bcb 100644
> >>>>> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> >>>>> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> >>>>> @@ -1880,6 +1880,9 @@ static void defer_request(struct i915_request *rq, struct list_head * const pl)
> >>>>>                         struct i915_request *w =
> >>>>>                                 container_of(p->waiter, typeof(*w), sched);
> >>>>>     
> >>>>> +                     if (p->flags & I915_DEPENDENCY_WEAK)
> >>>>> +                             continue;
> >>>>> +
> >>>>
> >>>> I did not quite get it - submit fence dependency would mean different
> >>>> engines, so the below check (w->engine != rq->engine) would effectively
> >>>> have the same effect. What am I missing?
> >>>
> >>> That submit fences can be between different contexts on the same engine.
> >>> The example (from mesa) is where we have two interdependent clients
> >>> which are using their own userlevel scheduling inside each batch, i.e.
> >>> waiting on semaphores.
> >>
> >> But if submit fence was used that means the waiter should never be
> >> submitted ahead of the signaler. And with this change it could get ahead
> >> in the priolist, no?
> > 
> > You do recall the starting point for this series was future fences :)
> > 
> > The test case for this is:
> > 
> >       execbuf.flags = engine | I915_EXEC_FENCE_OUT;
> >       execbuf.batch_start_offset = 0;
> >               gem_execbuf_wr(i915, &execbuf);
> > 
> >               execbuf.rsvd1 = gem_context_clone_with_engines(i915, 0);
> >               execbuf.rsvd2 >>= 32;
> >               execbuf.flags = e->flags;
> >               execbuf.flags |= I915_EXEC_FENCE_SUBMIT | I915_EXEC_FENCE_OUT;
> >               execbuf.batch_start_offset = offset;
> >               gem_execbuf_wr(i915, &execbuf);
> >               gem_context_destroy(i915, execbuf.rsvd1);
> > 
> >               gem_sync(i915, obj.handle);
> > 
> >       /* no hangs! */
> >       out = execbuf.rsvd2;
> >               igt_assert_eq(sync_fence_status(out), 1);
> >               close(out);
> > 
> >               out = execbuf.rsvd2 >> 32;
> >               igt_assert_eq(sync_fence_status(out), 1);
> >               close(out);
> > 
> > Where the batches are a couple of semaphore waits, which is the essence
> > of a bonded request but being run on a single engine.
> > 
> > Unless we treat the submit fence as a weak dependency here, we can't
> > timeslice between the two.
> 
> Yes it is fine, once the initial submit was controlled by a fence it is 
> okay to re-order.

Right. That we can't submit the second batch before the master, even if
the master is blocked is covered in gem_exec_fence/parallel. Hmm, but
that only covers submit fences on other engines (simply because it wants
to check the submit fences complete before the master, and didn't assume
timeslicing). So there's room for another test here, where we have both
requests on the same engine, and block the master.
-Chris
_______________________________________________
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:[~2020-05-07 18:05 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-07  8:21 [Intel-gfx] [PATCH 1/3] drm/i915: Mark concurrent submissions with a weak-dependency Chris Wilson
2020-05-07  8:21 ` [Intel-gfx] [PATCH 2/3] drm/i915/gem: Treat submit-fence as weak dependency for new clients Chris Wilson
2020-05-07 14:59   ` Tvrtko Ursulin
2020-05-07 15:05     ` Chris Wilson
2020-05-07 15:10       ` Tvrtko Ursulin
2020-05-07 15:17         ` Chris Wilson
2020-05-07  8:21 ` [Intel-gfx] [PATCH 3/3] drm/i915: Treat weak-dependencies as bidirectional when applying priorities Chris Wilson
2020-05-07  8:29   ` [Intel-gfx] [PATCH] " Chris Wilson
2020-05-07 14:58   ` [Intel-gfx] [PATCH 3/3] " Tvrtko Ursulin
2020-05-07  9:55 ` [Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Mark concurrent submissions with a weak-dependency (rev2) Patchwork
2020-05-07 12:49 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
2020-05-07 14:53 ` [Intel-gfx] [PATCH 1/3] drm/i915: Mark concurrent submissions with a weak-dependency Tvrtko Ursulin
2020-05-07 15:00   ` Chris Wilson
2020-05-07 15:23     ` Tvrtko Ursulin
2020-05-07 15:34       ` Chris Wilson
2020-05-07 17:55         ` Tvrtko Ursulin
2020-05-07 18:05           ` Chris Wilson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).