[PATCH 1/3] drm/i915/execlists: Mark up priority boost on preemption

[PATCH 1/3] drm/i915/execlists: Mark up priority boost on preemption

[Chris Wilson]

Record the priority boost we giving to the preempted client or else we
may end up in a situation where the priority queue no longer matches the
request priority order and so we can end up in an infinite loop of
preempting the same pair of requests.

Fixes: e9eaf82d97a2 ("drm/i915: Priority boost for waiting clients")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 436e59724900..8aa8a4862543 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -302,6 +302,7 @@ static void __unwind_incomplete_requests(struct intel_engine_cs *engine)
 	 */
 	if (!(prio & I915_PRIORITY_NEWCLIENT)) {
 		prio |= I915_PRIORITY_NEWCLIENT;
+		active->sched.attr.priority = prio;
 		list_move_tail(&active->sched.link,
 			       i915_sched_lookup_priolist(engine, prio));
 	}
@@ -625,6 +626,9 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 		int i;
 
 		priolist_for_each_request_consume(rq, rn, p, i) {
+			GEM_BUG_ON(last &&
+				   need_preempt(engine, last, rq_prio(rq)));
+
 			/*
 			 * Can we combine this request with the current port?
 			 * It has to be the same context/ringbuffer and not
-- 
2.20.1

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

[PATCH 2/3] drm/i915/execlists: Suppress preempting self

[Chris Wilson]

In order to avoid preempting ourselves, we currently refuse to schedule
the tasklet if we reschedule an inflight context. However, this glosses
over a few issues such as what happens after a CS completion event and
we then preempt the newly executing context with itself, or if something
else causes a tasklet_schedule triggering the same evaluation to
preempt the active context with itself.

To avoid the extra complications, after deciding that we have
potentially queued a request with higher priority than the currently
executing request, inspect the head of the queue to see if it is indeed
higher priority from another context.

References: a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on the current context")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_scheduler.c | 20 ++++++--
 drivers/gpu/drm/i915/intel_lrc.c      | 67 ++++++++++++++++++++++++---
 2 files changed, 76 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
index 340faea6c08a..fb5d953430e5 100644
--- a/drivers/gpu/drm/i915/i915_scheduler.c
+++ b/drivers/gpu/drm/i915/i915_scheduler.c
@@ -239,6 +239,18 @@ sched_lock_engine(struct i915_sched_node *node, struct intel_engine_cs *locked)
 	return engine;
 }
 
+static bool inflight(const struct i915_request *rq,
+		     const struct intel_engine_cs *engine)
+{
+	const struct i915_request *active;
+
+	if (!rq->global_seqno)
+		return false;
+
+	active = port_request(engine->execlists.port);
+	return active->hw_context == rq->hw_context;
+}
+
 static void __i915_schedule(struct i915_request *rq,
 			    const struct i915_sched_attr *attr)
 {
@@ -328,6 +340,7 @@ static void __i915_schedule(struct i915_request *rq,
 		INIT_LIST_HEAD(&dep->dfs_link);
 
 		engine = sched_lock_engine(node, engine);
+		lockdep_assert_held(&engine->timeline.lock);
 
 		/* Recheck after acquiring the engine->timeline.lock */
 		if (prio <= node->attr.priority || node_signaled(node))
@@ -356,17 +369,16 @@ static void __i915_schedule(struct i915_request *rq,
 		if (prio <= engine->execlists.queue_priority)
 			continue;
 
+		engine->execlists.queue_priority = prio;
+
 		/*
 		 * If we are already the currently executing context, don't
 		 * bother evaluating if we should preempt ourselves.
 		 */
-		if (node_to_request(node)->global_seqno &&
-		    i915_seqno_passed(port_request(engine->execlists.port)->global_seqno,
-				      node_to_request(node)->global_seqno))
+		if (inflight(node_to_request(node), engine))
 			continue;
 
 		/* Defer (tasklet) submission until after all of our updates. */
-		engine->execlists.queue_priority = prio;
 		tasklet_hi_schedule(&engine->execlists.tasklet);
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 8aa8a4862543..d9d744f6ab2c 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -182,12 +182,64 @@ static inline int rq_prio(const struct i915_request *rq)
 }
 
 static inline bool need_preempt(const struct intel_engine_cs *engine,
-				const struct i915_request *last,
-				int prio)
+				const struct i915_request *rq,
+				int q_prio)
 {
-	return (intel_engine_has_preemption(engine) &&
-		__execlists_need_preempt(prio, rq_prio(last)) &&
-		!i915_request_completed(last));
+	const struct intel_context *ctx = rq->hw_context;
+	const int last_prio = rq_prio(rq);
+	struct rb_node *rb;
+	int idx;
+
+	if (!intel_engine_has_preemption(engine))
+		return false;
+
+	if (i915_request_completed(rq))
+		return false;
+
+	if (!__execlists_need_preempt(q_prio, last_prio))
+		return false;
+
+	/*
+	 * The queue_priority is a mere hint that we may need to preempt.
+	 * If that hint is stale or we may be trying to preempt ourselves,
+	 * ignore the request.
+	 */
+
+	list_for_each_entry_continue(rq, &engine->timeline.requests, link) {
+		GEM_BUG_ON(rq->hw_context == ctx);
+		if (rq_prio(rq) > last_prio)
+			return true;
+	}
+
+	rb = rb_first_cached(&engine->execlists.queue);
+	if (!rb)
+		return false;
+
+	priolist_for_each_request(rq, to_priolist(rb), idx)
+		return rq->hw_context != ctx && rq_prio(rq) > last_prio;
+
+	return false;
+}
+
+__maybe_unused static inline bool
+assert_priority_queue(const struct intel_engine_execlists *execlists,
+		      const struct i915_request *prev,
+		      const struct i915_request *next)
+{
+	if (!prev)
+		return true;
+
+	/*
+	 * Without preemption, the prev may refer to the still active element
+	 * which we refuse to let go.
+	 *
+	 * Even with premption, there are times when we think it is better not
+	 * to preempt and leave an ostensibly lower priority request in flight.
+	 */
+	if (port_request(execlists->port) == prev)
+		return true;
+
+	return rq_prio(prev) >= rq_prio(next);
 }
 
 /*
@@ -626,8 +678,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 		int i;
 
 		priolist_for_each_request_consume(rq, rn, p, i) {
-			GEM_BUG_ON(last &&
-				   need_preempt(engine, last, rq_prio(rq)));
+			GEM_BUG_ON(!assert_priority_queue(execlists, last, rq));
 
 			/*
 			 * Can we combine this request with the current port?
@@ -872,6 +923,8 @@ static void process_csb(struct intel_engine_cs *engine)
 	const u32 * const buf = execlists->csb_status;
 	u8 head, tail;
 
+	lockdep_assert_held(&engine->timeline.lock);
+
 	/*
 	 * Note that csb_write, csb_status may be either in HWSP or mmio.
 	 * When reading from the csb_write mmio register, we have to be
-- 
2.20.1

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

[PATCH 3/3] drm/i915/execlists: Suppress redundant preemption

[Chris Wilson]

On unwinding the active request we give it a small (limited to internal
priority levels) boost to prevent it from being gazumped a second time.
However, this means that it can be promoted to above the request that
triggered the preemption request, causing a preempt-to-idle cycle for no
change. We can avoid this if we take the boost into account when
checking if the preemption request is valid.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_lrc.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index d9d744f6ab2c..74726f647e47 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -163,6 +163,8 @@
 #define WA_TAIL_DWORDS 2
 #define WA_TAIL_BYTES (sizeof(u32) * WA_TAIL_DWORDS)
 
+#define ACTIVE_PRIORITY (I915_PRIORITY_NEWCLIENT)
+
 static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
 					    struct intel_engine_cs *engine,
 					    struct intel_context *ce);
@@ -181,13 +183,31 @@ static inline int rq_prio(const struct i915_request *rq)
 	return rq->sched.attr.priority;
 }
 
+static inline int active_prio(const struct i915_request *rq)
+{
+	int prio = rq_prio(rq);
+
+	/*
+	 * On unwinding the active request, we give it a priority bump
+	 * equivalent to a freshly submitted request. This protects it from
+	 * being gazumped again, but it would be preferrable if we didn't
+	 * let it be gazumped in the first place!
+	 *
+	 * See __unwind_incomplete_requests()
+	 */
+	if (i915_request_started(rq))
+		prio |= ACTIVE_PRIORITY;
+
+	return prio;
+}
+
 static inline bool need_preempt(const struct intel_engine_cs *engine,
 				const struct i915_request *rq,
 				int q_prio)
 {
 	const struct intel_context *ctx = rq->hw_context;
-	const int last_prio = rq_prio(rq);
 	struct rb_node *rb;
+	int last_prio;
 	int idx;
 
 	if (!intel_engine_has_preemption(engine))
@@ -196,6 +216,7 @@ static inline bool need_preempt(const struct intel_engine_cs *engine,
 	if (i915_request_completed(rq))
 		return false;
 
+	last_prio = active_prio(rq);
 	if (!__execlists_need_preempt(q_prio, last_prio))
 		return false;
 
@@ -320,7 +341,7 @@ static void __unwind_incomplete_requests(struct intel_engine_cs *engine)
 {
 	struct i915_request *rq, *rn, *active = NULL;
 	struct list_head *uninitialized_var(pl);
-	int prio = I915_PRIORITY_INVALID | I915_PRIORITY_NEWCLIENT;
+	int prio = I915_PRIORITY_INVALID | ACTIVE_PRIORITY;
 
 	lockdep_assert_held(&engine->timeline.lock);
 
@@ -352,8 +373,8 @@ static void __unwind_incomplete_requests(struct intel_engine_cs *engine)
 	 * stream, so give it the equivalent small priority bump to prevent
 	 * it being gazumped a second time by another peer.
 	 */
-	if (!(prio & I915_PRIORITY_NEWCLIENT)) {
-		prio |= I915_PRIORITY_NEWCLIENT;
+	if ((prio & ACTIVE_PRIORITY) != ACTIVE_PRIORITY) {
+		prio |= ACTIVE_PRIORITY;
 		active->sched.attr.priority = prio;
 		list_move_tail(&active->sched.link,
 			       i915_sched_lookup_priolist(engine, prio));
-- 
2.20.1

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

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption
URL   : https://patchwork.freedesktop.org/series/55638/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
6817512a013e drm/i915/execlists: Mark up priority boost on preemption
7085cc180979 drm/i915/execlists: Suppress preempting self
-:18: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#18: 
References: a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on the current context")

-:18: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on the current context")'
#18: 
References: a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on the current context")

-:138: WARNING:TYPO_SPELLING: 'premption' may be misspelled - perhaps 'preemption'?
#138: FILE: drivers/gpu/drm/i915/intel_lrc.c:236:
+	 * Even with premption, there are times when we think it is better not

total: 1 errors, 2 warnings, 0 checks, 131 lines checked
97e0a1959b72 drm/i915/execlists: Suppress redundant preemption

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

✗ Fi.CI.SPARSE: warning for series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption
URL   : https://patchwork.freedesktop.org/series/55638/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915/execlists: Mark up priority boost on preemption
+drivers/gpu/drm/i915/intel_ringbuffer.h:602:23: warning: expression using sizeof(void)

Commit: drm/i915/execlists: Suppress preempting self
-drivers/gpu/drm/i915/intel_ringbuffer.h:602:23: warning: expression using sizeof(void)

Commit: drm/i915/execlists: Suppress redundant preemption
Okay!

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

Re: [PATCH 1/3] drm/i915/execlists: Mark up priority boost on preemption

[Tvrtko Ursulin]

On 23/01/2019 12:36, Chris Wilson wrote:
> Record the priority boost we giving to the preempted client or else we
> may end up in a situation where the priority queue no longer matches the
> request priority order and so we can end up in an infinite loop of
> preempting the same pair of requests.
> 
> Fixes: e9eaf82d97a2 ("drm/i915: Priority boost for waiting clients")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_lrc.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 436e59724900..8aa8a4862543 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -302,6 +302,7 @@ static void __unwind_incomplete_requests(struct intel_engine_cs *engine)
>   	 */
>   	if (!(prio & I915_PRIORITY_NEWCLIENT)) {
>   		prio |= I915_PRIORITY_NEWCLIENT;
> +		active->sched.attr.priority = prio;
>   		list_move_tail(&active->sched.link,
>   			       i915_sched_lookup_priolist(engine, prio));
>   	}
> @@ -625,6 +626,9 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>   		int i;
>   
>   		priolist_for_each_request_consume(rq, rn, p, i) {
> +			GEM_BUG_ON(last &&
> +				   need_preempt(engine, last, rq_prio(rq)));
> +
>   			/*
>   			 * Can we combine this request with the current port?
>   			 * It has to be the same context/ringbuffer and not
> 

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

✗ Fi.CI.BAT: failure for series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption
URL   : https://patchwork.freedesktop.org/series/55638/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5470 -> Patchwork_12015
====================================================

Summary
-------

  **FAILURE**

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

  External URL: https://patchwork.freedesktop.org/api/1.0/series/55638/revisions/1/mbox/

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@pm_rpm@module-reload:
    - fi-skl-6770hq:      PASS -> FAIL

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

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live_execlists:
    - fi-apl-guc:         PASS -> DMESG-WARN [fdo#108622]

  * igt@kms_chamelium@dp-crc-fast:
    - fi-kbl-7500u:       PASS -> DMESG-FAIL [fdo#103841]

  * igt@kms_flip@basic-flip-vs-dpms:
    - fi-skl-6700hq:      PASS -> DMESG-WARN [fdo#105998]

  * igt@kms_frontbuffer_tracking@basic:
    - fi-byt-clapper:     PASS -> FAIL [fdo#103167]

  * igt@kms_pipe_crc_basic@hang-read-crc-pipe-a:
    - fi-byt-clapper:     PASS -> FAIL [fdo#103191] / [fdo#107362]

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b:
    - fi-byt-clapper:     PASS -> FAIL [fdo#107362]

  
#### Possible fixes ####

  * igt@i915_selftest@live_hangcheck:
    - fi-bwr-2160:        DMESG-FAIL [fdo#108735] -> PASS

  * igt@kms_frontbuffer_tracking@basic:
    - {fi-icl-u3}:        FAIL [fdo#103167] -> PASS

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - fi-blb-e6850:       INCOMPLETE [fdo#107718] -> PASS

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
    - fi-hsw-4770:        {SKIP} [fdo#109271] -> PASS +3

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

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103841]: https://bugs.freedesktop.org/show_bug.cgi?id=103841
  [fdo#105998]: https://bugs.freedesktop.org/show_bug.cgi?id=105998
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108622]: https://bugs.freedesktop.org/show_bug.cgi?id=108622
  [fdo#108735]: https://bugs.freedesktop.org/show_bug.cgi?id=108735
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271


Participating hosts (45 -> 40)
------------------------------

  Missing    (5): fi-kbl-soraka fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-pnv-d510 


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

    * Linux: CI_DRM_5470 -> Patchwork_12015

  CI_DRM_5470: bfac8844379563353df6f77b0cde827ed74653eb @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4785: 70749c70926f12043d3408b160606e1e6238ed3a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12015: 97e0a1959b722739a349f620c1e83e12359923e6 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

97e0a1959b72 drm/i915/execlists: Suppress redundant preemption
7085cc180979 drm/i915/execlists: Suppress preempting self
6817512a013e drm/i915/execlists: Mark up priority boost on preemption

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12015/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915/execlists: Suppress redundant preemption

[Chris Wilson]

Quoting Chris Wilson (2019-01-23 12:36:02)
> On unwinding the active request we give it a small (limited to internal
> priority levels) boost to prevent it from being gazumped a second time.
> However, this means that it can be promoted to above the request that
> triggered the preemption request, causing a preempt-to-idle cycle for no
> change. We can avoid this if we take the boost into account when
> checking if the preemption request is valid.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index d9d744f6ab2c..74726f647e47 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -163,6 +163,8 @@
>  #define WA_TAIL_DWORDS 2
>  #define WA_TAIL_BYTES (sizeof(u32) * WA_TAIL_DWORDS)
>  
> +#define ACTIVE_PRIORITY (I915_PRIORITY_NEWCLIENT)
> +
>  static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
>                                             struct intel_engine_cs *engine,
>                                             struct intel_context *ce);
> @@ -181,13 +183,31 @@ static inline int rq_prio(const struct i915_request *rq)
>         return rq->sched.attr.priority;
>  }
>  
> +static inline int active_prio(const struct i915_request *rq)
> +{
> +       int prio = rq_prio(rq);
> +
> +       /*
> +        * On unwinding the active request, we give it a priority bump
> +        * equivalent to a freshly submitted request. This protects it from
> +        * being gazumped again, but it would be preferrable if we didn't
> +        * let it be gazumped in the first place!
> +        *
> +        * See __unwind_incomplete_requests()
> +        */
> +       if (i915_request_started(rq))
> +               prio |= ACTIVE_PRIORITY;

Hmm, actually we are put to the tail of that priolist so we don't get
rerun ahead of the preemptee if we end up at the same priority.
ACTIVE_PRIORITY - 1 would seem to be the right compromise.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915/execlists: Suppress preempting self

[Tvrtko Ursulin]

On 23/01/2019 12:36, Chris Wilson wrote:
> In order to avoid preempting ourselves, we currently refuse to schedule
> the tasklet if we reschedule an inflight context. However, this glosses
> over a few issues such as what happens after a CS completion event and
> we then preempt the newly executing context with itself, or if something
> else causes a tasklet_schedule triggering the same evaluation to
> preempt the active context with itself.
> 
> To avoid the extra complications, after deciding that we have
> potentially queued a request with higher priority than the currently
> executing request, inspect the head of the queue to see if it is indeed
> higher priority from another context.
> 
> References: a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on the current context")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_scheduler.c | 20 ++++++--
>   drivers/gpu/drm/i915/intel_lrc.c      | 67 ++++++++++++++++++++++++---
>   2 files changed, 76 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
> index 340faea6c08a..fb5d953430e5 100644
> --- a/drivers/gpu/drm/i915/i915_scheduler.c
> +++ b/drivers/gpu/drm/i915/i915_scheduler.c
> @@ -239,6 +239,18 @@ sched_lock_engine(struct i915_sched_node *node, struct intel_engine_cs *locked)
>   	return engine;
>   }
>   
> +static bool inflight(const struct i915_request *rq,
> +		     const struct intel_engine_cs *engine)
> +{
> +	const struct i915_request *active;
> +
> +	if (!rq->global_seqno)
> +		return false;
> +
> +	active = port_request(engine->execlists.port);
> +	return active->hw_context == rq->hw_context;
> +}
> +
>   static void __i915_schedule(struct i915_request *rq,
>   			    const struct i915_sched_attr *attr)
>   {
> @@ -328,6 +340,7 @@ static void __i915_schedule(struct i915_request *rq,
>   		INIT_LIST_HEAD(&dep->dfs_link);
>   
>   		engine = sched_lock_engine(node, engine);
> +		lockdep_assert_held(&engine->timeline.lock);
>   
>   		/* Recheck after acquiring the engine->timeline.lock */
>   		if (prio <= node->attr.priority || node_signaled(node))
> @@ -356,17 +369,16 @@ static void __i915_schedule(struct i915_request *rq,
>   		if (prio <= engine->execlists.queue_priority)
>   			continue;
>   
> +		engine->execlists.queue_priority = prio;
> +
>   		/*
>   		 * If we are already the currently executing context, don't
>   		 * bother evaluating if we should preempt ourselves.
>   		 */
> -		if (node_to_request(node)->global_seqno &&
> -		    i915_seqno_passed(port_request(engine->execlists.port)->global_seqno,
> -				      node_to_request(node)->global_seqno))
> +		if (inflight(node_to_request(node), engine))
>   			continue;
>   
>   		/* Defer (tasklet) submission until after all of our updates. */
> -		engine->execlists.queue_priority = prio;
>   		tasklet_hi_schedule(&engine->execlists.tasklet);
>   	}
>   
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 8aa8a4862543..d9d744f6ab2c 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -182,12 +182,64 @@ static inline int rq_prio(const struct i915_request *rq)
>   }
>   
>   static inline bool need_preempt(const struct intel_engine_cs *engine,
> -				const struct i915_request *last,
> -				int prio)
> +				const struct i915_request *rq,
> +				int q_prio)
>   {
> -	return (intel_engine_has_preemption(engine) &&
> -		__execlists_need_preempt(prio, rq_prio(last)) &&
> -		!i915_request_completed(last));
> +	const struct intel_context *ctx = rq->hw_context;
> +	const int last_prio = rq_prio(rq);
> +	struct rb_node *rb;
> +	int idx;
> +
> +	if (!intel_engine_has_preemption(engine))
> +		return false;
> +
> +	if (i915_request_completed(rq))
> +		return false;
> +
> +	if (!__execlists_need_preempt(q_prio, last_prio))
> +		return false;
> +
> +	/*
> +	 * The queue_priority is a mere hint that we may need to preempt.
> +	 * If that hint is stale or we may be trying to preempt ourselves,
> +	 * ignore the request.
> +	 */
> +
> +	list_for_each_entry_continue(rq, &engine->timeline.requests, link) {
> +		GEM_BUG_ON(rq->hw_context == ctx);

Why would there be no more requests belonging to the same context on the 
engine timeline after the first one? Did you mean "if (rq->hw_context == 
ctx) continue;" ?

> +		if (rq_prio(rq) > last_prio)
> +			return true;
> +	}
> +
> +	rb = rb_first_cached(&engine->execlists.queue);
> +	if (!rb)
> +		return false;
> +
> +	priolist_for_each_request(rq, to_priolist(rb), idx)
> +		return rq->hw_context != ctx && rq_prio(rq) > last_prio;

This isn't equivalent to top of the queue priority 
(engine->execlists.queue_priority)? Apart from the different ctx check. 
So I guess it is easier than storing new engine->execlists.queue_top_ctx 
and wondering about the validity of that pointer.

Regards,

Tvrtko

> +
> +	return false;
> +}
> +
> +__maybe_unused static inline bool
> +assert_priority_queue(const struct intel_engine_execlists *execlists,
> +		      const struct i915_request *prev,
> +		      const struct i915_request *next)
> +{
> +	if (!prev)
> +		return true;
> +
> +	/*
> +	 * Without preemption, the prev may refer to the still active element
> +	 * which we refuse to let go.
> +	 *
> +	 * Even with premption, there are times when we think it is better not
> +	 * to preempt and leave an ostensibly lower priority request in flight.
> +	 */
> +	if (port_request(execlists->port) == prev)
> +		return true;
> +
> +	return rq_prio(prev) >= rq_prio(next);
>   }
>   
>   /*
> @@ -626,8 +678,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>   		int i;
>   
>   		priolist_for_each_request_consume(rq, rn, p, i) {
> -			GEM_BUG_ON(last &&
> -				   need_preempt(engine, last, rq_prio(rq)));
> +			GEM_BUG_ON(!assert_priority_queue(execlists, last, rq));
>   
>   			/*
>   			 * Can we combine this request with the current port?
> @@ -872,6 +923,8 @@ static void process_csb(struct intel_engine_cs *engine)
>   	const u32 * const buf = execlists->csb_status;
>   	u8 head, tail;
>   
> +	lockdep_assert_held(&engine->timeline.lock);
> +
>   	/*
>   	 * Note that csb_write, csb_status may be either in HWSP or mmio.
>   	 * When reading from the csb_write mmio register, we have to be
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915/execlists: Suppress redundant preemption

[Chris Wilson]

Quoting Chris Wilson (2019-01-23 13:47:29)
> Quoting Chris Wilson (2019-01-23 12:36:02)
> > On unwinding the active request we give it a small (limited to internal
> > priority levels) boost to prevent it from being gazumped a second time.
> > However, this means that it can be promoted to above the request that
> > triggered the preemption request, causing a preempt-to-idle cycle for no
> > change. We can avoid this if we take the boost into account when
> > checking if the preemption request is valid.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_lrc.c | 29 +++++++++++++++++++++++++----
> >  1 file changed, 25 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > index d9d744f6ab2c..74726f647e47 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -163,6 +163,8 @@
> >  #define WA_TAIL_DWORDS 2
> >  #define WA_TAIL_BYTES (sizeof(u32) * WA_TAIL_DWORDS)
> >  
> > +#define ACTIVE_PRIORITY (I915_PRIORITY_NEWCLIENT)
> > +
> >  static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
> >                                             struct intel_engine_cs *engine,
> >                                             struct intel_context *ce);
> > @@ -181,13 +183,31 @@ static inline int rq_prio(const struct i915_request *rq)
> >         return rq->sched.attr.priority;
> >  }
> >  
> > +static inline int active_prio(const struct i915_request *rq)
> > +{
> > +       int prio = rq_prio(rq);
> > +
> > +       /*
> > +        * On unwinding the active request, we give it a priority bump
> > +        * equivalent to a freshly submitted request. This protects it from
> > +        * being gazumped again, but it would be preferrable if we didn't
> > +        * let it be gazumped in the first place!
> > +        *
> > +        * See __unwind_incomplete_requests()
> > +        */
> > +       if (i915_request_started(rq))
> > +               prio |= ACTIVE_PRIORITY;
> 
> Hmm, actually we are put to the tail of that priolist so we don't get
> rerun ahead of the preemptee if we end up at the same priority.
> ACTIVE_PRIORITY - 1 would seem to be the right compromise.

gem_sync/switch-default says ACTIVE_PRIORITY though. Hmm.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915/execlists: Suppress preempting self

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-01-23 14:08:56)
> 
> On 23/01/2019 12:36, Chris Wilson wrote:
> > In order to avoid preempting ourselves, we currently refuse to schedule
> > the tasklet if we reschedule an inflight context. However, this glosses
> > over a few issues such as what happens after a CS completion event and
> > we then preempt the newly executing context with itself, or if something
> > else causes a tasklet_schedule triggering the same evaluation to
> > preempt the active context with itself.
> > 
> > To avoid the extra complications, after deciding that we have
> > potentially queued a request with higher priority than the currently
> > executing request, inspect the head of the queue to see if it is indeed
> > higher priority from another context.
> > 
> > References: a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on the current context")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_scheduler.c | 20 ++++++--
> >   drivers/gpu/drm/i915/intel_lrc.c      | 67 ++++++++++++++++++++++++---
> >   2 files changed, 76 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
> > index 340faea6c08a..fb5d953430e5 100644
> > --- a/drivers/gpu/drm/i915/i915_scheduler.c
> > +++ b/drivers/gpu/drm/i915/i915_scheduler.c
> > @@ -239,6 +239,18 @@ sched_lock_engine(struct i915_sched_node *node, struct intel_engine_cs *locked)
> >       return engine;
> >   }
> >   
> > +static bool inflight(const struct i915_request *rq,
> > +                  const struct intel_engine_cs *engine)
> > +{
> > +     const struct i915_request *active;
> > +
> > +     if (!rq->global_seqno)
> > +             return false;
> > +
> > +     active = port_request(engine->execlists.port);
> > +     return active->hw_context == rq->hw_context;
> > +}
> > +
> >   static void __i915_schedule(struct i915_request *rq,
> >                           const struct i915_sched_attr *attr)
> >   {
> > @@ -328,6 +340,7 @@ static void __i915_schedule(struct i915_request *rq,
> >               INIT_LIST_HEAD(&dep->dfs_link);
> >   
> >               engine = sched_lock_engine(node, engine);
> > +             lockdep_assert_held(&engine->timeline.lock);
> >   
> >               /* Recheck after acquiring the engine->timeline.lock */
> >               if (prio <= node->attr.priority || node_signaled(node))
> > @@ -356,17 +369,16 @@ static void __i915_schedule(struct i915_request *rq,
> >               if (prio <= engine->execlists.queue_priority)
> >                       continue;
> >   
> > +             engine->execlists.queue_priority = prio;
> > +
> >               /*
> >                * If we are already the currently executing context, don't
> >                * bother evaluating if we should preempt ourselves.
> >                */
> > -             if (node_to_request(node)->global_seqno &&
> > -                 i915_seqno_passed(port_request(engine->execlists.port)->global_seqno,
> > -                                   node_to_request(node)->global_seqno))
> > +             if (inflight(node_to_request(node), engine))
> >                       continue;
> >   
> >               /* Defer (tasklet) submission until after all of our updates. */
> > -             engine->execlists.queue_priority = prio;
> >               tasklet_hi_schedule(&engine->execlists.tasklet);
> >       }
> >   
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > index 8aa8a4862543..d9d744f6ab2c 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -182,12 +182,64 @@ static inline int rq_prio(const struct i915_request *rq)
> >   }
> >   
> >   static inline bool need_preempt(const struct intel_engine_cs *engine,
> > -                             const struct i915_request *last,
> > -                             int prio)
> > +                             const struct i915_request *rq,
> > +                             int q_prio)
> >   {
> > -     return (intel_engine_has_preemption(engine) &&
> > -             __execlists_need_preempt(prio, rq_prio(last)) &&
> > -             !i915_request_completed(last));
> > +     const struct intel_context *ctx = rq->hw_context;
> > +     const int last_prio = rq_prio(rq);
> > +     struct rb_node *rb;
> > +     int idx;
> > +
> > +     if (!intel_engine_has_preemption(engine))
> > +             return false;
> > +
> > +     if (i915_request_completed(rq))
> > +             return false;
> > +
> > +     if (!__execlists_need_preempt(q_prio, last_prio))
> > +             return false;
> > +
> > +     /*
> > +      * The queue_priority is a mere hint that we may need to preempt.
> > +      * If that hint is stale or we may be trying to preempt ourselves,
> > +      * ignore the request.
> > +      */
> > +
> > +     list_for_each_entry_continue(rq, &engine->timeline.requests, link) {
> > +             GEM_BUG_ON(rq->hw_context == ctx);
> 
> Why would there be no more requests belonging to the same context on the 
> engine timeline after the first one? Did you mean "if (rq->hw_context == 
> ctx) continue;" ?

We enter the function with rq == execlist->port, i.e. the last request
submitted to ELSP[0]. In this loop, we iterate from the start of ELSP[1]
and all the request here belong to that context. It is illegal for
ELSP[0].lrca == ELSP[1].lrca, i.e. the context must be different.

> 
> > +             if (rq_prio(rq) > last_prio)
> > +                     return true;
> > +     }
> > +
> > +     rb = rb_first_cached(&engine->execlists.queue);
> > +     if (!rb)
> > +             return false;
> > +
> > +     priolist_for_each_request(rq, to_priolist(rb), idx)
> > +             return rq->hw_context != ctx && rq_prio(rq) > last_prio;
> 
> This isn't equivalent to top of the queue priority 
> (engine->execlists.queue_priority)? Apart from the different ctx check. 
> So I guess it is easier than storing new engine->execlists.queue_top_ctx 
> and wondering about the validity of that pointer.

The problem being that queue_priority may not always match the top of
the execlists->queue. Right, the first attempt I tried was to store the
queue_context matching the queue_priority, but due to the suppression of
inflight preemptions, it doesn't match for long, and is not as accurate
as one would like across CS events.

priolist_for_each_request() isn't too horrible for finding the first
pointer. I noted that we teach it to do: for(idx = __ffs(p->used); ...)
though. If we didn't care about checking hw_context, we can compute the
prio from (p->priority+1)<<SHIFT - ffs(p->used).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915/execlists: Suppress redundant preemption

[Chris Wilson]

Quoting Chris Wilson (2019-01-23 14:14:05)
> Quoting Chris Wilson (2019-01-23 13:47:29)
> > Quoting Chris Wilson (2019-01-23 12:36:02)
> > > On unwinding the active request we give it a small (limited to internal
> > > priority levels) boost to prevent it from being gazumped a second time.
> > > However, this means that it can be promoted to above the request that
> > > triggered the preemption request, causing a preempt-to-idle cycle for no
> > > change. We can avoid this if we take the boost into account when
> > > checking if the preemption request is valid.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >  drivers/gpu/drm/i915/intel_lrc.c | 29 +++++++++++++++++++++++++----
> > >  1 file changed, 25 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > > index d9d744f6ab2c..74726f647e47 100644
> > > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > > @@ -163,6 +163,8 @@
> > >  #define WA_TAIL_DWORDS 2
> > >  #define WA_TAIL_BYTES (sizeof(u32) * WA_TAIL_DWORDS)
> > >  
> > > +#define ACTIVE_PRIORITY (I915_PRIORITY_NEWCLIENT)
> > > +
> > >  static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
> > >                                             struct intel_engine_cs *engine,
> > >                                             struct intel_context *ce);
> > > @@ -181,13 +183,31 @@ static inline int rq_prio(const struct i915_request *rq)
> > >         return rq->sched.attr.priority;
> > >  }
> > >  
> > > +static inline int active_prio(const struct i915_request *rq)
> > > +{
> > > +       int prio = rq_prio(rq);
> > > +
> > > +       /*
> > > +        * On unwinding the active request, we give it a priority bump
> > > +        * equivalent to a freshly submitted request. This protects it from
> > > +        * being gazumped again, but it would be preferrable if we didn't
> > > +        * let it be gazumped in the first place!
> > > +        *
> > > +        * See __unwind_incomplete_requests()
> > > +        */
> > > +       if (i915_request_started(rq))
> > > +               prio |= ACTIVE_PRIORITY;
> > 
> > Hmm, actually we are put to the tail of that priolist so we don't get
> > rerun ahead of the preemptee if we end up at the same priority.
> > ACTIVE_PRIORITY - 1 would seem to be the right compromise.
> 
> gem_sync/switch-default says ACTIVE_PRIORITY though. Hmm.

The answer is don't be lazy.

-       if (i915_request_started(rq))
+       if ((prio & ACTIVE_PRIORITY) != ACTIVE_PRIORITY &&
+           i915_request_started(rq)) {
                prio |= ACTIVE_PRIORITY;
+               prio--;
+       }

That doesn't break switch-default while providing a more accurate
estimate of prio after preemption.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915/execlists: Suppress redundant preemption

[Chris Wilson]

On unwinding the active request we give it a small (limited to internal
priority levels) boost to prevent it from being gazumped a second time.
However, this means that it can be promoted to above the request that
triggered the preemption request, causing a preempt-to-idle cycle for no
change. We can avoid this if we take the boost into account when
checking if the preemption request is valid.

v2: After preemption the active request will be after the preemptee if
they end up with equal priority.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_lrc.c | 39 ++++++++++++++++++++++++++++----
 1 file changed, 35 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index d9d744f6ab2c..65997ed055b2 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -163,6 +163,8 @@
 #define WA_TAIL_DWORDS 2
 #define WA_TAIL_BYTES (sizeof(u32) * WA_TAIL_DWORDS)
 
+#define ACTIVE_PRIORITY (I915_PRIORITY_NEWCLIENT)
+
 static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
 					    struct intel_engine_cs *engine,
 					    struct intel_context *ce);
@@ -181,13 +183,41 @@ static inline int rq_prio(const struct i915_request *rq)
 	return rq->sched.attr.priority;
 }
 
+static inline int active_prio(const struct i915_request *rq)
+{
+	int prio = rq_prio(rq);
+
+	/*
+	 * On unwinding the active request, we give it a priority bump
+	 * equivalent to a freshly submitted request. This protects it from
+	 * being gazumped again, but it would be preferable if we didn't
+	 * let it be gazumped in the first place!
+	 *
+	 * See __unwind_incomplete_requests()
+	 */
+	if ((prio & ACTIVE_PRIORITY) != ACTIVE_PRIORITY &&
+	    i915_request_started(rq)) {
+		/*
+		 * After preemption, we insert the active request at the
+		 * end of the new priority level. This means that we will be
+		 * _lower_ priority than the preemptee all things equal (and
+		 * so the preemption is valid), so adjust our comparison
+		 * accordingly.
+		 */
+		prio |= ACTIVE_PRIORITY;
+		prio--;
+	}
+
+	return prio;
+}
+
 static inline bool need_preempt(const struct intel_engine_cs *engine,
 				const struct i915_request *rq,
 				int q_prio)
 {
 	const struct intel_context *ctx = rq->hw_context;
-	const int last_prio = rq_prio(rq);
 	struct rb_node *rb;
+	int last_prio;
 	int idx;
 
 	if (!intel_engine_has_preemption(engine))
@@ -196,6 +226,7 @@ static inline bool need_preempt(const struct intel_engine_cs *engine,
 	if (i915_request_completed(rq))
 		return false;
 
+	last_prio = active_prio(rq);
 	if (!__execlists_need_preempt(q_prio, last_prio))
 		return false;
 
@@ -320,7 +351,7 @@ static void __unwind_incomplete_requests(struct intel_engine_cs *engine)
 {
 	struct i915_request *rq, *rn, *active = NULL;
 	struct list_head *uninitialized_var(pl);
-	int prio = I915_PRIORITY_INVALID | I915_PRIORITY_NEWCLIENT;
+	int prio = I915_PRIORITY_INVALID | ACTIVE_PRIORITY;
 
 	lockdep_assert_held(&engine->timeline.lock);
 
@@ -352,8 +383,8 @@ static void __unwind_incomplete_requests(struct intel_engine_cs *engine)
 	 * stream, so give it the equivalent small priority bump to prevent
 	 * it being gazumped a second time by another peer.
 	 */
-	if (!(prio & I915_PRIORITY_NEWCLIENT)) {
-		prio |= I915_PRIORITY_NEWCLIENT;
+	if ((prio & ACTIVE_PRIORITY) != ACTIVE_PRIORITY) {
+		prio |= ACTIVE_PRIORITY;
 		active->sched.attr.priority = prio;
 		list_move_tail(&active->sched.link,
 			       i915_sched_lookup_priolist(engine, prio));
-- 
2.20.1

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

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption (rev2)
URL   : https://patchwork.freedesktop.org/series/55638/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
8de30aea3cc3 drm/i915/execlists: Mark up priority boost on preemption
49059512f804 drm/i915/execlists: Suppress preempting self
-:18: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#18: 
References: a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on the current context")

-:18: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on the current context")'
#18: 
References: a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on the current context")

-:138: WARNING:TYPO_SPELLING: 'premption' may be misspelled - perhaps 'preemption'?
#138: FILE: drivers/gpu/drm/i915/intel_lrc.c:236:
+	 * Even with premption, there are times when we think it is better not

total: 1 errors, 2 warnings, 0 checks, 131 lines checked
81300ac71039 drm/i915/execlists: Suppress redundant preemption

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

✗ Fi.CI.SPARSE: warning for series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption (rev2)
URL   : https://patchwork.freedesktop.org/series/55638/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915/execlists: Mark up priority boost on preemption
+drivers/gpu/drm/i915/intel_ringbuffer.h:602:23: warning: expression using sizeof(void)

Commit: drm/i915/execlists: Suppress preempting self
-drivers/gpu/drm/i915/intel_ringbuffer.h:602:23: warning: expression using sizeof(void)

Commit: drm/i915/execlists: Suppress redundant preemption
Okay!

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

✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption (rev2)
URL   : https://patchwork.freedesktop.org/series/55638/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5470 -> Patchwork_12017
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/55638/revisions/2/mbox/

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

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

### IGT changes ###

#### Issues hit ####

  * igt@kms_frontbuffer_tracking@basic:
    - fi-byt-clapper:     PASS -> FAIL [fdo#103167]

  * igt@kms_pipe_crc_basic@hang-read-crc-pipe-a:
    - fi-byt-clapper:     PASS -> FAIL [fdo#103191] / [fdo#107362]

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b:
    - fi-byt-clapper:     PASS -> FAIL [fdo#107362]

  
#### Possible fixes ####

  * igt@i915_selftest@live_hangcheck:
    - fi-bwr-2160:        DMESG-FAIL [fdo#108735] -> PASS

  * igt@kms_frontbuffer_tracking@basic:
    - {fi-icl-u3}:        FAIL [fdo#103167] -> PASS

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - fi-blb-e6850:       INCOMPLETE [fdo#107718] -> PASS

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
    - fi-hsw-4770:        {SKIP} [fdo#109271] -> PASS +3

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

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108735]: https://bugs.freedesktop.org/show_bug.cgi?id=108735
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271


Participating hosts (45 -> 41)
------------------------------

  Missing    (4): fi-kbl-soraka fi-ilk-m540 fi-byt-squawks fi-bsw-cyan 


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

    * Linux: CI_DRM_5470 -> Patchwork_12017

  CI_DRM_5470: bfac8844379563353df6f77b0cde827ed74653eb @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4785: 70749c70926f12043d3408b160606e1e6238ed3a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12017: 81300ac710397ca473f7cebe94a2100e454340c8 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

81300ac71039 drm/i915/execlists: Suppress redundant preemption
49059512f804 drm/i915/execlists: Suppress preempting self
8de30aea3cc3 drm/i915/execlists: Mark up priority boost on preemption

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12017/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915/execlists: Suppress preempting self

[Tvrtko Ursulin]

On 23/01/2019 14:22, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-01-23 14:08:56)
>>
>> On 23/01/2019 12:36, Chris Wilson wrote:
>>> In order to avoid preempting ourselves, we currently refuse to schedule
>>> the tasklet if we reschedule an inflight context. However, this glosses
>>> over a few issues such as what happens after a CS completion event and
>>> we then preempt the newly executing context with itself, or if something
>>> else causes a tasklet_schedule triggering the same evaluation to
>>> preempt the active context with itself.
>>>
>>> To avoid the extra complications, after deciding that we have
>>> potentially queued a request with higher priority than the currently
>>> executing request, inspect the head of the queue to see if it is indeed
>>> higher priority from another context.
>>>
>>> References: a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on the current context")
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/i915_scheduler.c | 20 ++++++--
>>>    drivers/gpu/drm/i915/intel_lrc.c      | 67 ++++++++++++++++++++++++---
>>>    2 files changed, 76 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
>>> index 340faea6c08a..fb5d953430e5 100644
>>> --- a/drivers/gpu/drm/i915/i915_scheduler.c
>>> +++ b/drivers/gpu/drm/i915/i915_scheduler.c
>>> @@ -239,6 +239,18 @@ sched_lock_engine(struct i915_sched_node *node, struct intel_engine_cs *locked)
>>>        return engine;
>>>    }
>>>    
>>> +static bool inflight(const struct i915_request *rq,
>>> +                  const struct intel_engine_cs *engine)
>>> +{
>>> +     const struct i915_request *active;
>>> +
>>> +     if (!rq->global_seqno)
>>> +             return false;
>>> +
>>> +     active = port_request(engine->execlists.port);
>>> +     return active->hw_context == rq->hw_context;
>>> +}
>>> +
>>>    static void __i915_schedule(struct i915_request *rq,
>>>                            const struct i915_sched_attr *attr)
>>>    {
>>> @@ -328,6 +340,7 @@ static void __i915_schedule(struct i915_request *rq,
>>>                INIT_LIST_HEAD(&dep->dfs_link);
>>>    
>>>                engine = sched_lock_engine(node, engine);
>>> +             lockdep_assert_held(&engine->timeline.lock);
>>>    
>>>                /* Recheck after acquiring the engine->timeline.lock */
>>>                if (prio <= node->attr.priority || node_signaled(node))
>>> @@ -356,17 +369,16 @@ static void __i915_schedule(struct i915_request *rq,
>>>                if (prio <= engine->execlists.queue_priority)
>>>                        continue;
>>>    
>>> +             engine->execlists.queue_priority = prio;
>>> +
>>>                /*
>>>                 * If we are already the currently executing context, don't
>>>                 * bother evaluating if we should preempt ourselves.
>>>                 */
>>> -             if (node_to_request(node)->global_seqno &&
>>> -                 i915_seqno_passed(port_request(engine->execlists.port)->global_seqno,
>>> -                                   node_to_request(node)->global_seqno))
>>> +             if (inflight(node_to_request(node), engine))
>>>                        continue;
>>>    
>>>                /* Defer (tasklet) submission until after all of our updates. */
>>> -             engine->execlists.queue_priority = prio;
>>>                tasklet_hi_schedule(&engine->execlists.tasklet);
>>>        }
>>>    
>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>>> index 8aa8a4862543..d9d744f6ab2c 100644
>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>> @@ -182,12 +182,64 @@ static inline int rq_prio(const struct i915_request *rq)
>>>    }
>>>    
>>>    static inline bool need_preempt(const struct intel_engine_cs *engine,
>>> -                             const struct i915_request *last,
>>> -                             int prio)
>>> +                             const struct i915_request *rq,
>>> +                             int q_prio)
>>>    {
>>> -     return (intel_engine_has_preemption(engine) &&
>>> -             __execlists_need_preempt(prio, rq_prio(last)) &&
>>> -             !i915_request_completed(last));
>>> +     const struct intel_context *ctx = rq->hw_context;
>>> +     const int last_prio = rq_prio(rq);
>>> +     struct rb_node *rb;
>>> +     int idx;
>>> +
>>> +     if (!intel_engine_has_preemption(engine))
>>> +             return false;
>>> +
>>> +     if (i915_request_completed(rq))
>>> +             return false;
>>> +
>>> +     if (!__execlists_need_preempt(q_prio, last_prio))
>>> +             return false;
>>> +
>>> +     /*
>>> +      * The queue_priority is a mere hint that we may need to preempt.
>>> +      * If that hint is stale or we may be trying to preempt ourselves,
>>> +      * ignore the request.
>>> +      */
>>> +
>>> +     list_for_each_entry_continue(rq, &engine->timeline.requests, link) {
>>> +             GEM_BUG_ON(rq->hw_context == ctx);
>>
>> Why would there be no more requests belonging to the same context on the
>> engine timeline after the first one? Did you mean "if (rq->hw_context ==
>> ctx) continue;" ?
> 
> We enter the function with rq == execlist->port, i.e. the last request
> submitted to ELSP[0]. In this loop, we iterate from the start of ELSP[1]
> and all the request here belong to that context. It is illegal for
> ELSP[0].lrca == ELSP[1].lrca, i.e. the context must be different.

Yes, you are right yet again. I missed the fact it is guaranteed this is 
called with port0. I wonder if function signature should change to make 
this obvious so someone doesn't get the idea to call it with any old 
request.

> 
>>
>>> +             if (rq_prio(rq) > last_prio)
>>> +                     return true;
>>> +     }
>>> +
>>> +     rb = rb_first_cached(&engine->execlists.queue);
>>> +     if (!rb)
>>> +             return false;
>>> +
>>> +     priolist_for_each_request(rq, to_priolist(rb), idx)
>>> +             return rq->hw_context != ctx && rq_prio(rq) > last_prio;
>>
>> This isn't equivalent to top of the queue priority
>> (engine->execlists.queue_priority)? Apart from the different ctx check.
>> So I guess it is easier than storing new engine->execlists.queue_top_ctx
>> and wondering about the validity of that pointer.
> 
> The problem being that queue_priority may not always match the top of
> the execlists->queue. Right, the first attempt I tried was to store the
> queue_context matching the queue_priority, but due to the suppression of
> inflight preemptions, it doesn't match for long, and is not as accurate
> as one would like across CS events.
> 
> priolist_for_each_request() isn't too horrible for finding the first
> pointer. I noted that we teach it to do: for(idx = __ffs(p->used); ...)
> though. If we didn't care about checking hw_context, we can compute the
> prio from (p->priority+1)<<SHIFT - ffs(p->used).

And this check is definitely needed to avoid some issue? I'll need to 
have another try of understanding the commit and code paths fully 
tomorrow. I mean, only because it would be good to have something more 
elegant that full blown tree lookup.

Regards,

Tvrtko


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

Re: [PATCH 2/3] drm/i915/execlists: Suppress preempting self

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-01-23 16:40:44)
> 
> On 23/01/2019 14:22, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-01-23 14:08:56)
> >>
> >> On 23/01/2019 12:36, Chris Wilson wrote:
> >>> In order to avoid preempting ourselves, we currently refuse to schedule
> >>> the tasklet if we reschedule an inflight context. However, this glosses
> >>> over a few issues such as what happens after a CS completion event and
> >>> we then preempt the newly executing context with itself, or if something
> >>> else causes a tasklet_schedule triggering the same evaluation to
> >>> preempt the active context with itself.
> >>>
> >>> To avoid the extra complications, after deciding that we have
> >>> potentially queued a request with higher priority than the currently
> >>> executing request, inspect the head of the queue to see if it is indeed
> >>> higher priority from another context.
> >>>
> >>> References: a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on the current context")
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>> ---
> >>>    drivers/gpu/drm/i915/i915_scheduler.c | 20 ++++++--
> >>>    drivers/gpu/drm/i915/intel_lrc.c      | 67 ++++++++++++++++++++++++---
> >>>    2 files changed, 76 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
> >>> index 340faea6c08a..fb5d953430e5 100644
> >>> --- a/drivers/gpu/drm/i915/i915_scheduler.c
> >>> +++ b/drivers/gpu/drm/i915/i915_scheduler.c
> >>> @@ -239,6 +239,18 @@ sched_lock_engine(struct i915_sched_node *node, struct intel_engine_cs *locked)
> >>>        return engine;
> >>>    }
> >>>    
> >>> +static bool inflight(const struct i915_request *rq,
> >>> +                  const struct intel_engine_cs *engine)
> >>> +{
> >>> +     const struct i915_request *active;
> >>> +
> >>> +     if (!rq->global_seqno)
> >>> +             return false;
> >>> +
> >>> +     active = port_request(engine->execlists.port);
> >>> +     return active->hw_context == rq->hw_context;
> >>> +}
> >>> +
> >>>    static void __i915_schedule(struct i915_request *rq,
> >>>                            const struct i915_sched_attr *attr)
> >>>    {
> >>> @@ -328,6 +340,7 @@ static void __i915_schedule(struct i915_request *rq,
> >>>                INIT_LIST_HEAD(&dep->dfs_link);
> >>>    
> >>>                engine = sched_lock_engine(node, engine);
> >>> +             lockdep_assert_held(&engine->timeline.lock);
> >>>    
> >>>                /* Recheck after acquiring the engine->timeline.lock */
> >>>                if (prio <= node->attr.priority || node_signaled(node))
> >>> @@ -356,17 +369,16 @@ static void __i915_schedule(struct i915_request *rq,
> >>>                if (prio <= engine->execlists.queue_priority)
> >>>                        continue;
> >>>    
> >>> +             engine->execlists.queue_priority = prio;
> >>> +
> >>>                /*
> >>>                 * If we are already the currently executing context, don't
> >>>                 * bother evaluating if we should preempt ourselves.
> >>>                 */
> >>> -             if (node_to_request(node)->global_seqno &&
> >>> -                 i915_seqno_passed(port_request(engine->execlists.port)->global_seqno,
> >>> -                                   node_to_request(node)->global_seqno))
> >>> +             if (inflight(node_to_request(node), engine))
> >>>                        continue;
> >>>    
> >>>                /* Defer (tasklet) submission until after all of our updates. */
> >>> -             engine->execlists.queue_priority = prio;
> >>>                tasklet_hi_schedule(&engine->execlists.tasklet);
> >>>        }
> >>>    
> >>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >>> index 8aa8a4862543..d9d744f6ab2c 100644
> >>> --- a/drivers/gpu/drm/i915/intel_lrc.c
> >>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> >>> @@ -182,12 +182,64 @@ static inline int rq_prio(const struct i915_request *rq)
> >>>    }
> >>>    
> >>>    static inline bool need_preempt(const struct intel_engine_cs *engine,
> >>> -                             const struct i915_request *last,
> >>> -                             int prio)
> >>> +                             const struct i915_request *rq,
> >>> +                             int q_prio)
> >>>    {
> >>> -     return (intel_engine_has_preemption(engine) &&
> >>> -             __execlists_need_preempt(prio, rq_prio(last)) &&
> >>> -             !i915_request_completed(last));
> >>> +     const struct intel_context *ctx = rq->hw_context;
> >>> +     const int last_prio = rq_prio(rq);
> >>> +     struct rb_node *rb;
> >>> +     int idx;
> >>> +
> >>> +     if (!intel_engine_has_preemption(engine))
> >>> +             return false;
> >>> +
> >>> +     if (i915_request_completed(rq))
> >>> +             return false;
> >>> +
> >>> +     if (!__execlists_need_preempt(q_prio, last_prio))
> >>> +             return false;
> >>> +
> >>> +     /*
> >>> +      * The queue_priority is a mere hint that we may need to preempt.
> >>> +      * If that hint is stale or we may be trying to preempt ourselves,
> >>> +      * ignore the request.
> >>> +      */
> >>> +
> >>> +     list_for_each_entry_continue(rq, &engine->timeline.requests, link) {
> >>> +             GEM_BUG_ON(rq->hw_context == ctx);
> >>
> >> Why would there be no more requests belonging to the same context on the
> >> engine timeline after the first one? Did you mean "if (rq->hw_context ==
> >> ctx) continue;" ?
> > 
> > We enter the function with rq == execlist->port, i.e. the last request
> > submitted to ELSP[0]. In this loop, we iterate from the start of ELSP[1]
> > and all the request here belong to that context. It is illegal for
> > ELSP[0].lrca == ELSP[1].lrca, i.e. the context must be different.
> 
> Yes, you are right yet again. I missed the fact it is guaranteed this is 
> called with port0. I wonder if function signature should change to make 
> this obvious so someone doesn't get the idea to call it with any old 
> request.

I did miss something obvious though. Due to PI, the first request on
ELSP[1] is also the highest priority (we make sure to promote all
inflight requests along the same context).

> >>> +             if (rq_prio(rq) > last_prio)
> >>> +                     return true;
> >>> +     }
> >>> +
> >>> +     rb = rb_first_cached(&engine->execlists.queue);
> >>> +     if (!rb)
> >>> +             return false;
> >>> +
> >>> +     priolist_for_each_request(rq, to_priolist(rb), idx)
> >>> +             return rq->hw_context != ctx && rq_prio(rq) > last_prio;
> >>
> >> This isn't equivalent to top of the queue priority
> >> (engine->execlists.queue_priority)? Apart from the different ctx check.
> >> So I guess it is easier than storing new engine->execlists.queue_top_ctx
> >> and wondering about the validity of that pointer.
> > 
> > The problem being that queue_priority may not always match the top of
> > the execlists->queue. Right, the first attempt I tried was to store the
> > queue_context matching the queue_priority, but due to the suppression of
> > inflight preemptions, it doesn't match for long, and is not as accurate
> > as one would like across CS events.
> > 
> > priolist_for_each_request() isn't too horrible for finding the first
> > pointer. I noted that we teach it to do: for(idx = __ffs(p->used); ...)
> > though. If we didn't care about checking hw_context, we can compute the
> > prio from (p->priority+1)<<SHIFT - ffs(p->used).
> 
> And this check is definitely needed to avoid some issue? I'll need to 
> have another try of understanding the commit and code paths fully 
> tomorrow. I mean, only because it would be good to have something more 
> elegant that full blown tree lookup.

Hmm. No, the hw_context check should be redundant. If it were the same
context as ELSP[0], we would have applied PI to last_prio already, so
rq_prio(rq) can never be greater. All we need to realise is that we
cannot trust queue_priority alone.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915/execlists: Suppress preempting self

[Chris Wilson]

In order to avoid preempting ourselves, we currently refuse to schedule
the tasklet if we reschedule an inflight context. However, this glosses
over a few issues such as what happens after a CS completion event and
we then preempt the newly executing context with itself, or if something
else causes a tasklet_schedule triggering the same evaluation to
preempt the active context with itself.

To avoid the extra complications, after deciding that we have
potentially queued a request with higher priority than the currently
executing request, inspect the head of the queue to see if it is indeed
higher priority from another context.

v2: We can simplify a bunch of tests based on the knowledge that PI will
ensure that earlier requests along the same context will have the highest
priority.

References: a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on the current context")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_scheduler.c | 20 ++++--
 drivers/gpu/drm/i915/intel_lrc.c      | 91 ++++++++++++++++++++++++---
 2 files changed, 100 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
index 340faea6c08a..fb5d953430e5 100644
--- a/drivers/gpu/drm/i915/i915_scheduler.c
+++ b/drivers/gpu/drm/i915/i915_scheduler.c
@@ -239,6 +239,18 @@ sched_lock_engine(struct i915_sched_node *node, struct intel_engine_cs *locked)
 	return engine;
 }
 
+static bool inflight(const struct i915_request *rq,
+		     const struct intel_engine_cs *engine)
+{
+	const struct i915_request *active;
+
+	if (!rq->global_seqno)
+		return false;
+
+	active = port_request(engine->execlists.port);
+	return active->hw_context == rq->hw_context;
+}
+
 static void __i915_schedule(struct i915_request *rq,
 			    const struct i915_sched_attr *attr)
 {
@@ -328,6 +340,7 @@ static void __i915_schedule(struct i915_request *rq,
 		INIT_LIST_HEAD(&dep->dfs_link);
 
 		engine = sched_lock_engine(node, engine);
+		lockdep_assert_held(&engine->timeline.lock);
 
 		/* Recheck after acquiring the engine->timeline.lock */
 		if (prio <= node->attr.priority || node_signaled(node))
@@ -356,17 +369,16 @@ static void __i915_schedule(struct i915_request *rq,
 		if (prio <= engine->execlists.queue_priority)
 			continue;
 
+		engine->execlists.queue_priority = prio;
+
 		/*
 		 * If we are already the currently executing context, don't
 		 * bother evaluating if we should preempt ourselves.
 		 */
-		if (node_to_request(node)->global_seqno &&
-		    i915_seqno_passed(port_request(engine->execlists.port)->global_seqno,
-				      node_to_request(node)->global_seqno))
+		if (inflight(node_to_request(node), engine))
 			continue;
 
 		/* Defer (tasklet) submission until after all of our updates. */
-		engine->execlists.queue_priority = prio;
 		tasklet_hi_schedule(&engine->execlists.tasklet);
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 8aa8a4862543..11d7fa810d9a 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -181,13 +181,89 @@ static inline int rq_prio(const struct i915_request *rq)
 	return rq->sched.attr.priority;
 }
 
+static int queue_priority(const struct intel_engine_execlists *execlists)
+{
+	struct i915_priolist *p;
+	struct rb_node *rb;
+
+	rb = rb_first_cached(&execlists->queue);
+	if (!rb)
+		return INT_MIN;
+
+	/*
+	 * As the priolist[] are inverted, with the highest priority in [0],
+	 * we have to flip the index value to become priority.
+	 */
+	p = to_priolist(rb);
+	return ((p->priority + 1) << I915_USER_PRIORITY_SHIFT) - ffs(p->used);
+}
+
 static inline bool need_preempt(const struct intel_engine_cs *engine,
-				const struct i915_request *last,
-				int prio)
+				const struct i915_request *rq,
+				int q_prio)
 {
-	return (intel_engine_has_preemption(engine) &&
-		__execlists_need_preempt(prio, rq_prio(last)) &&
-		!i915_request_completed(last));
+	const struct intel_context *ctx = rq->hw_context;
+	const int last_prio = rq_prio(rq);
+
+	if (!intel_engine_has_preemption(engine))
+		return false;
+
+	if (i915_request_completed(rq))
+		return false;
+
+	/*
+	 * Check if the current queue_priority merits a preemption attempt.
+	 *
+	 * However, the queue_priority is a mere hint that we may need to
+	 * preempt. If that hint is stale or we may be trying to preempt
+	 * ourselves, ignore the request.
+	 */
+	if (!__execlists_need_preempt(q_prio, last_prio))
+		return false;
+
+	/*
+	 * Check against the first request in ELSP[1], it will, thanks to the
+	 * power of PI, be the highest priority of that context.
+	 */
+	if (!list_is_last(&rq->link, &engine->timeline.requests)) {
+		rq = list_next_entry(rq, link);
+		GEM_BUG_ON(rq->hw_context == ctx);
+		if (rq_prio(rq) > last_prio)
+			return true;
+	}
+
+	/*
+	 * If the inflight context did not trigger the preemption, then maybe
+	 * it was the set of queued requests? Pick the highest priority in
+	 * the queue (the first active priolist) and see if it deserves to be
+	 * running instead of ELSP[0].
+	 *
+	 * The highest priority request in the queue can not be either
+	 * ELSP[0] or ELSP[1] as, thanks again to PI, if it was the same
+	 * context, it's priority would not exceed ELSP[0] aka last_prio.
+	 */
+	return queue_priority(&engine->execlists) > last_prio;
+}
+
+__maybe_unused static inline bool
+assert_priority_queue(const struct intel_engine_execlists *execlists,
+		      const struct i915_request *prev,
+		      const struct i915_request *next)
+{
+	if (!prev)
+		return true;
+
+	/*
+	 * Without preemption, the prev may refer to the still active element
+	 * which we refuse to let go.
+	 *
+	 * Even with premption, there are times when we think it is better not
+	 * to preempt and leave an ostensibly lower priority request in flight.
+	 */
+	if (port_request(execlists->port) == prev)
+		return true;
+
+	return rq_prio(prev) >= rq_prio(next);
 }
 
 /*
@@ -626,8 +702,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 		int i;
 
 		priolist_for_each_request_consume(rq, rn, p, i) {
-			GEM_BUG_ON(last &&
-				   need_preempt(engine, last, rq_prio(rq)));
+			GEM_BUG_ON(!assert_priority_queue(execlists, last, rq));
 
 			/*
 			 * Can we combine this request with the current port?
@@ -872,6 +947,8 @@ static void process_csb(struct intel_engine_cs *engine)
 	const u32 * const buf = execlists->csb_status;
 	u8 head, tail;
 
+	lockdep_assert_held(&engine->timeline.lock);
+
 	/*
 	 * Note that csb_write, csb_status may be either in HWSP or mmio.
 	 * When reading from the csb_write mmio register, we have to be
-- 
2.20.1

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

✗ Fi.CI.BAT: failure for series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption (rev3)

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption (rev3)
URL   : https://patchwork.freedesktop.org/series/55638/
State : failure

== Summary ==

Applying: drm/i915/execlists: Mark up priority boost on preemption
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/intel_lrc.c
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.
Applying: drm/i915/execlists: Suppress preempting self
Applying: drm/i915/execlists: Suppress redundant preemption
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/intel_lrc.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/intel_lrc.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/intel_lrc.c
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0003 drm/i915/execlists: Suppress redundant preemption
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

✓ Fi.CI.IGT: success for series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption (rev2)
URL   : https://patchwork.freedesktop.org/series/55638/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5470_full -> Patchwork_12017_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-a:
    - shard-kbl:          NOTRUN -> DMESG-WARN [fdo#107956]

  * igt@kms_ccs@pipe-a-crc-sprite-planes-basic:
    - shard-apl:          PASS -> FAIL [fdo#106510] / [fdo#108145]

  * igt@kms_cursor_crc@cursor-128x128-random:
    - shard-apl:          PASS -> FAIL [fdo#103232] +5

  * igt@kms_cursor_crc@cursor-128x42-random:
    - shard-glk:          PASS -> FAIL [fdo#103232] +4

  * igt@kms_cursor_crc@cursor-256x256-suspend:
    - shard-apl:          PASS -> FAIL [fdo#103191] / [fdo#103232]

  * igt@kms_flip@flip-vs-panning:
    - shard-snb:          PASS -> INCOMPLETE [fdo#105411]

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-indfb-pgflip-blt:
    - shard-snb:          NOTRUN -> INCOMPLETE [fdo#105411] / [fdo#107469]

  * igt@kms_plane@pixel-format-pipe-b-planes-source-clamping:
    - shard-glk:          PASS -> FAIL [fdo#108948]

  * igt@kms_plane_alpha_blend@pipe-a-constant-alpha-max:
    - shard-kbl:          NOTRUN -> FAIL [fdo#108145]

  * igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
    - shard-apl:          PASS -> FAIL [fdo#103166] +2

  * igt@kms_plane_multiple@atomic-pipe-b-tiling-yf:
    - shard-glk:          PASS -> FAIL [fdo#103166]

  * igt@kms_setmode@basic:
    - shard-kbl:          PASS -> FAIL [fdo#99912]

  
#### Possible fixes ####

  * igt@gem_softpin@noreloc-s3:
    - shard-apl:          INCOMPLETE [fdo#103927] -> PASS

  * igt@kms_cursor_crc@cursor-128x42-onscreen:
    - shard-glk:          FAIL [fdo#103232] -> PASS +2

  * igt@kms_cursor_crc@cursor-256x85-sliding:
    - shard-apl:          FAIL [fdo#103232] -> PASS +2

  * igt@kms_cursor_legacy@2x-long-flip-vs-cursor-legacy:
    - shard-glk:          FAIL [fdo#104873] -> PASS

  * igt@kms_plane@pixel-format-pipe-b-planes:
    - shard-apl:          FAIL [fdo#103166] -> PASS

  * igt@kms_plane_alpha_blend@pipe-c-constant-alpha-max:
    - shard-glk:          FAIL [fdo#108145] -> PASS +1

  * igt@kms_plane_multiple@atomic-pipe-b-tiling-y:
    - shard-glk:          FAIL [fdo#103166] -> PASS +2

  * igt@kms_rotation_crc@multiplane-rotation-cropping-top:
    - shard-apl:          DMESG-FAIL [fdo#108950] -> PASS

  * igt@kms_setmode@basic:
    - shard-hsw:          FAIL [fdo#99912] -> PASS

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

  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#104873]: https://bugs.freedesktop.org/show_bug.cgi?id=104873
  [fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
  [fdo#106510]: https://bugs.freedesktop.org/show_bug.cgi?id=106510
  [fdo#107469]: https://bugs.freedesktop.org/show_bug.cgi?id=107469
  [fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108948]: https://bugs.freedesktop.org/show_bug.cgi?id=108948
  [fdo#108950]: https://bugs.freedesktop.org/show_bug.cgi?id=108950
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (7 -> 5)
------------------------------

  Missing    (2): shard-skl shard-iclb 


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

    * Linux: CI_DRM_5470 -> Patchwork_12017

  CI_DRM_5470: bfac8844379563353df6f77b0cde827ed74653eb @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4785: 70749c70926f12043d3408b160606e1e6238ed3a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12017: 81300ac710397ca473f7cebe94a2100e454340c8 @ 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_12017/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] drm/i915/execlists: Suppress preempting self

[Chris Wilson]

In order to avoid preempting ourselves, we currently refuse to schedule
the tasklet if we reschedule an inflight context. However, this glosses
over a few issues such as what happens after a CS completion event and
we then preempt the newly executing context with itself, or if something
else causes a tasklet_schedule triggering the same evaluation to
preempt the active context with itself.

To avoid the extra complications, after deciding that we have
potentially queued a request with higher priority than the currently
executing request, inspect the head of the queue to see if it is indeed
higher priority from another context.

v2: We can simplify a bunch of tests based on the knowledge that PI will
ensure that earlier requests along the same context will have the highest
priority.

References: a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on the current context")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_scheduler.c | 20 ++++--
 drivers/gpu/drm/i915/intel_lrc.c      | 91 ++++++++++++++++++++++++---
 2 files changed, 100 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
index 0da718ceab43..7db1255665a8 100644
--- a/drivers/gpu/drm/i915/i915_scheduler.c
+++ b/drivers/gpu/drm/i915/i915_scheduler.c
@@ -238,6 +238,18 @@ sched_lock_engine(struct i915_sched_node *node, struct intel_engine_cs *locked)
 	return engine;
 }
 
+static bool inflight(const struct i915_request *rq,
+		     const struct intel_engine_cs *engine)
+{
+	const struct i915_request *active;
+
+	if (!rq->global_seqno)
+		return false;
+
+	active = port_request(engine->execlists.port);
+	return active->hw_context == rq->hw_context;
+}
+
 static void __i915_schedule(struct i915_request *rq,
 			    const struct i915_sched_attr *attr)
 {
@@ -327,6 +339,7 @@ static void __i915_schedule(struct i915_request *rq,
 		INIT_LIST_HEAD(&dep->dfs_link);
 
 		engine = sched_lock_engine(node, engine);
+		lockdep_assert_held(&engine->timeline.lock);
 
 		/* Recheck after acquiring the engine->timeline.lock */
 		if (prio <= node->attr.priority || node_signaled(node))
@@ -355,17 +368,16 @@ static void __i915_schedule(struct i915_request *rq,
 		if (prio <= engine->execlists.preempt_priority_hint)
 			continue;
 
+		engine->execlists.preempt_priority_hint = prio;
+
 		/*
 		 * If we are already the currently executing context, don't
 		 * bother evaluating if we should preempt ourselves.
 		 */
-		if (node_to_request(node)->global_seqno &&
-		    i915_seqno_passed(port_request(engine->execlists.port)->global_seqno,
-				      node_to_request(node)->global_seqno))
+		if (inflight(node_to_request(node), engine))
 			continue;
 
 		/* Defer (tasklet) submission until after all of our updates. */
-		engine->execlists.preempt_priority_hint = prio;
 		tasklet_hi_schedule(&engine->execlists.tasklet);
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 71006b031f54..b44db7d49584 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -182,13 +182,89 @@ static inline int rq_prio(const struct i915_request *rq)
 	return rq->sched.attr.priority;
 }
 
+static int queue_prio(const struct intel_engine_execlists *execlists)
+{
+	struct i915_priolist *p;
+	struct rb_node *rb;
+
+	rb = rb_first_cached(&execlists->queue);
+	if (!rb)
+		return INT_MIN;
+
+	/*
+	 * As the priolist[] are inverted, with the highest priority in [0],
+	 * we have to flip the index value to become priority.
+	 */
+	p = to_priolist(rb);
+	return ((p->priority + 1) << I915_USER_PRIORITY_SHIFT) - ffs(p->used);
+}
+
 static inline bool need_preempt(const struct intel_engine_cs *engine,
-				const struct i915_request *last,
-				int prio)
+				const struct i915_request *rq,
+				int hint)
 {
-	return (intel_engine_has_preemption(engine) &&
-		__execlists_need_preempt(prio, rq_prio(last)) &&
-		!i915_request_completed(last));
+	const struct intel_context *ctx = rq->hw_context;
+	const int last_prio = rq_prio(rq);
+
+	if (!intel_engine_has_preemption(engine))
+		return false;
+
+	if (i915_request_completed(rq))
+		return false;
+
+	/*
+	 * Check if the current priority hint merits a preemption attempt.
+	 *
+	 * However, the priority hint is a mere hint that we may need to
+	 * preempt. If that hint is stale or we may be trying to preempt
+	 * ourselves, ignore the request.
+	 */
+	if (!__execlists_need_preempt(hint, last_prio))
+		return false;
+
+	/*
+	 * Check against the first request in ELSP[1], it will, thanks to the
+	 * power of PI, be the highest priority of that context.
+	 */
+	if (!list_is_last(&rq->link, &engine->timeline.requests)) {
+		rq = list_next_entry(rq, link);
+		GEM_BUG_ON(rq->hw_context == ctx);
+		if (rq_prio(rq) > last_prio)
+			return true;
+	}
+
+	/*
+	 * If the inflight context did not trigger the preemption, then maybe
+	 * it was the set of queued requests? Pick the highest priority in
+	 * the queue (the first active priolist) and see if it deserves to be
+	 * running instead of ELSP[0].
+	 *
+	 * The highest priority request in the queue can not be either
+	 * ELSP[0] or ELSP[1] as, thanks again to PI, if it was the same
+	 * context, it's priority would not exceed ELSP[0] aka last_prio.
+	 */
+	return queue_prio(&engine->execlists) > last_prio;
+}
+
+__maybe_unused static inline bool
+assert_priority_queue(const struct intel_engine_execlists *execlists,
+		      const struct i915_request *prev,
+		      const struct i915_request *next)
+{
+	if (!prev)
+		return true;
+
+	/*
+	 * Without preemption, the prev may refer to the still active element
+	 * which we refuse to let go.
+	 *
+	 * Even with preemption, there are times when we think it is better not
+	 * to preempt and leave an ostensibly lower priority request in flight.
+	 */
+	if (port_request(execlists->port) == prev)
+		return true;
+
+	return rq_prio(prev) >= rq_prio(next);
 }
 
 /*
@@ -630,8 +706,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 		int i;
 
 		priolist_for_each_request_consume(rq, rn, p, i) {
-			GEM_BUG_ON(last &&
-				   need_preempt(engine, last, rq_prio(rq)));
+			GEM_BUG_ON(!assert_priority_queue(execlists, last, rq));
 
 			/*
 			 * Can we combine this request with the current port?
@@ -876,6 +951,8 @@ static void process_csb(struct intel_engine_cs *engine)
 	const u32 * const buf = execlists->csb_status;
 	u8 head, tail;
 
+	lockdep_assert_held(&engine->timeline.lock);
+
 	/*
 	 * Note that csb_write, csb_status may be either in HWSP or mmio.
 	 * When reading from the csb_write mmio register, we have to be
-- 
2.20.1

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