intel-gfx.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [Intel-gfx] [PATCH] drm/i915: Be more gentle when exiting non-persistent contexts
@ 2021-08-05 12:05 Tvrtko Ursulin
  2021-08-05 15:04 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for " Patchwork
  2021-08-05 16:32 ` [Intel-gfx] [PATCH] " Matthew Brost
  0 siblings, 2 replies; 9+ messages in thread
From: Tvrtko Ursulin @ 2021-08-05 12:05 UTC (permalink / raw)
  To: Intel-gfx; +Cc: dri-devel, Tvrtko Ursulin, Chris Wilson, Zhen Han

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

When a non-persistent context exits we currently mark it as banned in
order to trigger fast termination of any outstanding GPU jobs it may have
left running.

In doing so we apply a very strict 1ms limit in which the left over job
has to preempt before we issues an engine resets.

Some workloads are not able to cleanly preempt in that time window and it
can be argued that it would instead be better to give them a bit more
grace since avoiding engine resets is generally preferrable.

To achieve this the patch splits handling of banned contexts from simply
closed non-persistent ones and then applies different timeouts for both
and also extends the criteria which determines if a request should be
scheduled back in after preemption or not.

15ms preempt timeout grace is given to exited non-persistent contexts
which have been empirically tested to satisfy customers requirements
and still provides reasonably quick cleanup post exit.

v2:
 * Streamline fast path checks.

v3:
 * Simplify by using only schedulable status.
 * Increase timeout to 20ms.

v4:
 * Fix live_execlists selftest.

v5:
 * Fix logic in kill_engines.

v6:
 * Rebase.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Zhen Han <zhen.han@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_context.c   | 22 +++++++++++++------
 drivers/gpu/drm/i915/gt/intel_context.c       |  2 ++
 drivers/gpu/drm/i915/gt/intel_context.h       | 17 +++++++++++++-
 drivers/gpu/drm/i915/gt/intel_context_types.h |  1 +
 .../drm/i915/gt/intel_execlists_submission.c  | 11 ++++++++--
 drivers/gpu/drm/i915/gt/selftest_execlists.c  | 20 +++++++++++------
 drivers/gpu/drm/i915/i915_request.c           |  2 +-
 7 files changed, 57 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index cff72679ad7c..21fe5d4057ab 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -1065,7 +1065,8 @@ static struct intel_engine_cs *active_engine(struct intel_context *ce)
 	return engine;
 }
 
-static void kill_engines(struct i915_gem_engines *engines, bool ban)
+static void
+kill_engines(struct i915_gem_engines *engines, bool ban, bool persistent)
 {
 	struct i915_gem_engines_iter it;
 	struct intel_context *ce;
@@ -1079,8 +1080,15 @@ static void kill_engines(struct i915_gem_engines *engines, bool ban)
 	 */
 	for_each_gem_engine(ce, engines, it) {
 		struct intel_engine_cs *engine;
+		bool skip = false;
+
+		if (ban)
+			skip = intel_context_ban(ce, NULL);
+		else if (!persistent)
+			skip = !intel_context_clear_schedulable(ce);
 
-		if (ban && intel_context_ban(ce, NULL))
+		/* Already previously banned or made non-schedulable? */
+		if (skip)
 			continue;
 
 		/*
@@ -1093,7 +1101,7 @@ static void kill_engines(struct i915_gem_engines *engines, bool ban)
 		engine = active_engine(ce);
 
 		/* First attempt to gracefully cancel the context */
-		if (engine && !__cancel_engine(engine) && ban)
+		if (engine && !__cancel_engine(engine) && (ban || !persistent))
 			/*
 			 * If we are unable to send a preemptive pulse to bump
 			 * the context from the GPU, we have to resort to a full
@@ -1105,8 +1113,6 @@ static void kill_engines(struct i915_gem_engines *engines, bool ban)
 
 static void kill_context(struct i915_gem_context *ctx)
 {
-	bool ban = (!i915_gem_context_is_persistent(ctx) ||
-		    !ctx->i915->params.enable_hangcheck);
 	struct i915_gem_engines *pos, *next;
 
 	spin_lock_irq(&ctx->stale.lock);
@@ -1119,7 +1125,8 @@ static void kill_context(struct i915_gem_context *ctx)
 
 		spin_unlock_irq(&ctx->stale.lock);
 
-		kill_engines(pos, ban);
+		kill_engines(pos, !ctx->i915->params.enable_hangcheck,
+			     i915_gem_context_is_persistent(ctx));
 
 		spin_lock_irq(&ctx->stale.lock);
 		GEM_BUG_ON(i915_sw_fence_signaled(&pos->fence));
@@ -1165,7 +1172,8 @@ static void engines_idle_release(struct i915_gem_context *ctx,
 
 kill:
 	if (list_empty(&engines->link)) /* raced, already closed */
-		kill_engines(engines, true);
+		kill_engines(engines, true,
+			     i915_gem_context_is_persistent(ctx));
 
 	i915_sw_fence_commit(&engines->fence);
 }
diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
index 745e84c72c90..bc1701ef1578 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -382,6 +382,8 @@ intel_context_init(struct intel_context *ce, struct intel_engine_cs *engine)
 	ce->ring = NULL;
 	ce->ring_size = SZ_4K;
 
+	__set_bit(CONTEXT_SCHEDULABLE, &ce->flags);
+
 	ewma_runtime_init(&ce->runtime.avg);
 
 	ce->vm = i915_vm_get(engine->gt->vm);
diff --git a/drivers/gpu/drm/i915/gt/intel_context.h b/drivers/gpu/drm/i915/gt/intel_context.h
index c41098950746..5b50716654dd 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.h
+++ b/drivers/gpu/drm/i915/gt/intel_context.h
@@ -251,7 +251,22 @@ static inline bool intel_context_is_banned(const struct intel_context *ce)
 
 static inline bool intel_context_set_banned(struct intel_context *ce)
 {
-	return test_and_set_bit(CONTEXT_BANNED, &ce->flags);
+	bool banned = test_and_set_bit(CONTEXT_BANNED, &ce->flags);
+
+	if (!banned)
+		clear_bit(CONTEXT_SCHEDULABLE, &ce->flags);
+
+	return banned;
+}
+
+static inline bool intel_context_clear_schedulable(struct intel_context *ce)
+{
+	return test_and_clear_bit(CONTEXT_SCHEDULABLE, &ce->flags);
+}
+
+static inline bool intel_context_is_schedulable(const struct intel_context *ce)
+{
+	return test_bit(CONTEXT_SCHEDULABLE, &ce->flags);
 }
 
 static inline bool intel_context_ban(struct intel_context *ce,
diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
index e54351a170e2..3306c70c9c54 100644
--- a/drivers/gpu/drm/i915/gt/intel_context_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
@@ -112,6 +112,7 @@ struct intel_context {
 #define CONTEXT_FORCE_SINGLE_SUBMISSION	7
 #define CONTEXT_NOPREEMPT		8
 #define CONTEXT_LRCA_DIRTY		9
+#define CONTEXT_SCHEDULABLE		10  /* Unless banned or non-persistent closed. */
 
 	struct {
 		u64 timeout_us;
diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
index de5f9c86b9a4..778f3cda3c71 100644
--- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
@@ -478,7 +478,7 @@ __execlists_schedule_in(struct i915_request *rq)
 		     !intel_engine_has_heartbeat(engine)))
 		intel_context_set_banned(ce);
 
-	if (unlikely(intel_context_is_banned(ce) || bad_request(rq)))
+	if (unlikely(!intel_context_is_schedulable(ce) || bad_request(rq)))
 		reset_active(rq, engine);
 
 	if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
@@ -1222,12 +1222,19 @@ static void record_preemption(struct intel_engine_execlists *execlists)
 static unsigned long active_preempt_timeout(struct intel_engine_cs *engine,
 					    const struct i915_request *rq)
 {
+	struct intel_context *ce;
+
 	if (!rq)
 		return 0;
 
+	ce = rq->context;
+
 	/* Force a fast reset for terminated contexts (ignoring sysfs!) */
-	if (unlikely(intel_context_is_banned(rq->context) || bad_request(rq)))
+	if (unlikely(intel_context_is_banned(ce) || bad_request(rq)))
 		return 1;
+	/* Longer grace for closed non-persistent contexts to avoid resets. */
+	else if (unlikely(!intel_context_is_schedulable(ce)))
+		return 20;
 
 	return READ_ONCE(engine->props.preempt_timeout_ms);
 }
diff --git a/drivers/gpu/drm/i915/gt/selftest_execlists.c b/drivers/gpu/drm/i915/gt/selftest_execlists.c
index f12ffe797639..da36c015caf4 100644
--- a/drivers/gpu/drm/i915/gt/selftest_execlists.c
+++ b/drivers/gpu/drm/i915/gt/selftest_execlists.c
@@ -2050,6 +2050,12 @@ struct live_preempt_cancel {
 	struct preempt_client a, b;
 };
 
+static void context_clear_banned(struct intel_context *ce)
+{
+	clear_bit(CONTEXT_BANNED, &ce->flags);
+	set_bit(CONTEXT_SCHEDULABLE, &ce->flags);
+}
+
 static int __cancel_active0(struct live_preempt_cancel *arg)
 {
 	struct i915_request *rq;
@@ -2068,7 +2074,7 @@ static int __cancel_active0(struct live_preempt_cancel *arg)
 	if (IS_ERR(rq))
 		return PTR_ERR(rq);
 
-	clear_bit(CONTEXT_BANNED, &rq->context->flags);
+	context_clear_banned(rq->context);
 	i915_request_get(rq);
 	i915_request_add(rq);
 	if (!igt_wait_for_spinner(&arg->a.spin, rq)) {
@@ -2112,7 +2118,7 @@ static int __cancel_active1(struct live_preempt_cancel *arg)
 	if (IS_ERR(rq[0]))
 		return PTR_ERR(rq[0]);
 
-	clear_bit(CONTEXT_BANNED, &rq[0]->context->flags);
+	context_clear_banned(rq[0]->context);
 	i915_request_get(rq[0]);
 	i915_request_add(rq[0]);
 	if (!igt_wait_for_spinner(&arg->a.spin, rq[0])) {
@@ -2128,7 +2134,7 @@ static int __cancel_active1(struct live_preempt_cancel *arg)
 		goto out;
 	}
 
-	clear_bit(CONTEXT_BANNED, &rq[1]->context->flags);
+	context_clear_banned(rq[1]->context);
 	i915_request_get(rq[1]);
 	err = i915_request_await_dma_fence(rq[1], &rq[0]->fence);
 	i915_request_add(rq[1]);
@@ -2183,7 +2189,7 @@ static int __cancel_queued(struct live_preempt_cancel *arg)
 	if (IS_ERR(rq[0]))
 		return PTR_ERR(rq[0]);
 
-	clear_bit(CONTEXT_BANNED, &rq[0]->context->flags);
+	context_clear_banned(rq[0]->context);
 	i915_request_get(rq[0]);
 	i915_request_add(rq[0]);
 	if (!igt_wait_for_spinner(&arg->a.spin, rq[0])) {
@@ -2197,7 +2203,7 @@ static int __cancel_queued(struct live_preempt_cancel *arg)
 		goto out;
 	}
 
-	clear_bit(CONTEXT_BANNED, &rq[1]->context->flags);
+	context_clear_banned(rq[1]->context);
 	i915_request_get(rq[1]);
 	err = i915_request_await_dma_fence(rq[1], &rq[0]->fence);
 	i915_request_add(rq[1]);
@@ -2273,7 +2279,7 @@ static int __cancel_hostile(struct live_preempt_cancel *arg)
 	if (IS_ERR(rq))
 		return PTR_ERR(rq);
 
-	clear_bit(CONTEXT_BANNED, &rq->context->flags);
+	context_clear_banned(rq->context);
 	i915_request_get(rq);
 	i915_request_add(rq);
 	if (!igt_wait_for_spinner(&arg->a.spin, rq)) {
@@ -2329,7 +2335,7 @@ static int __cancel_fail(struct live_preempt_cancel *arg)
 	if (IS_ERR(rq))
 		return PTR_ERR(rq);
 
-	clear_bit(CONTEXT_BANNED, &rq->context->flags);
+	context_clear_banned(rq->context);
 	i915_request_get(rq);
 	i915_request_add(rq);
 	if (!igt_wait_for_spinner(&arg->a.spin, rq)) {
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index ce446716d092..b1a9bec83339 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -583,7 +583,7 @@ bool __i915_request_submit(struct i915_request *request)
 		goto active;
 	}
 
-	if (unlikely(intel_context_is_banned(request->context)))
+	if (unlikely(!intel_context_is_schedulable(request->context)))
 		i915_request_set_error_once(request, -EIO);
 
 	if (unlikely(fatal_error(request->fence.error)))
-- 
2.30.2


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

* [Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915: Be more gentle when exiting non-persistent contexts
  2021-08-05 12:05 [Intel-gfx] [PATCH] drm/i915: Be more gentle when exiting non-persistent contexts Tvrtko Ursulin
@ 2021-08-05 15:04 ` Patchwork
  2021-08-05 16:10   ` Tvrtko Ursulin
  2021-08-05 16:32 ` [Intel-gfx] [PATCH] " Matthew Brost
  1 sibling, 1 reply; 9+ messages in thread
From: Patchwork @ 2021-08-05 15:04 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: intel-gfx

[-- Attachment #1: Type: text/plain, Size: 5650 bytes --]

== Series Details ==

Series: drm/i915: Be more gentle when exiting non-persistent contexts
URL   : https://patchwork.freedesktop.org/series/93420/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_10450 -> Patchwork_20775
====================================================

Summary
-------

  **FAILURE**

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

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

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@gt_lrc:
    - fi-rkl-guc:         [PASS][1] -> [DMESG-WARN][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10450/fi-rkl-guc/igt@i915_selftest@live@gt_lrc.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-rkl-guc/igt@i915_selftest@live@gt_lrc.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_basic@query-info:
    - fi-bsw-kefka:       NOTRUN -> [SKIP][3] ([fdo#109271]) +17 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-bsw-kefka/igt@amdgpu/amd_basic@query-info.html

  * igt@gem_exec_fence@basic-busy@bcs0:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][4] ([fdo#109271]) +26 similar issues
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-kbl-soraka/igt@gem_exec_fence@basic-busy@bcs0.html

  * igt@gem_huc_copy@huc-copy:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][5] ([fdo#109271] / [i915#2190])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-kbl-soraka/igt@gem_huc_copy@huc-copy.html

  * igt@i915_pm_rpm@basic-rte:
    - fi-kbl-soraka:      NOTRUN -> [FAIL][6] ([i915#579])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-kbl-soraka/igt@i915_pm_rpm@basic-rte.html

  * igt@i915_selftest@live@gt_pm:
    - fi-kbl-soraka:      NOTRUN -> [DMESG-FAIL][7] ([i915#1886] / [i915#2291])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-kbl-soraka/igt@i915_selftest@live@gt_pm.html

  * igt@i915_selftest@live@late_gt_pm:
    - fi-bsw-nick:        [PASS][8] -> [DMESG-FAIL][9] ([i915#2927])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10450/fi-bsw-nick/igt@i915_selftest@live@late_gt_pm.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-bsw-nick/igt@i915_selftest@live@late_gt_pm.html

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][10] ([fdo#109271] / [fdo#111827]) +8 similar issues
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-kbl-soraka/igt@kms_chamelium@common-hpd-after-suspend.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][11] ([fdo#109271] / [i915#533])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-kbl-soraka/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d.html

  * igt@runner@aborted:
    - fi-bsw-nick:        NOTRUN -> [FAIL][12] ([fdo#109271] / [i915#1436])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-bsw-nick/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@execlists:
    - fi-bsw-kefka:       [INCOMPLETE][13] ([i915#2940]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10450/fi-bsw-kefka/igt@i915_selftest@live@execlists.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-bsw-kefka/igt@i915_selftest@live@execlists.html

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

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1436]: https://gitlab.freedesktop.org/drm/intel/issues/1436
  [i915#1886]: https://gitlab.freedesktop.org/drm/intel/issues/1886
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#2291]: https://gitlab.freedesktop.org/drm/intel/issues/2291
  [i915#2927]: https://gitlab.freedesktop.org/drm/intel/issues/2927
  [i915#2940]: https://gitlab.freedesktop.org/drm/intel/issues/2940
  [i915#3303]: https://gitlab.freedesktop.org/drm/intel/issues/3303
  [i915#533]: https://gitlab.freedesktop.org/drm/intel/issues/533
  [i915#579]: https://gitlab.freedesktop.org/drm/intel/issues/579


Participating hosts (40 -> 35)
------------------------------

  Additional (1): fi-kbl-soraka 
  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-bsw-cyan fi-ctg-p8600 fi-bdw-samus bat-jsl-1 


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

  * Linux: CI_DRM_10450 -> Patchwork_20775

  CI-20190529: 20190529
  CI_DRM_10450: 51d9c8293e8446e921b74d996982ade862fcfa5c @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6160: 4287344dd6a39d9036c5fb9a047a7d8f10bee981 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_20775: 62efd2820c620b384c9688fa71ad6e8a5f6d27fc @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

62efd2820c62 drm/i915: Be more gentle when exiting non-persistent contexts

== Logs ==

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

[-- Attachment #2: Type: text/html, Size: 6800 bytes --]

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

* Re: [Intel-gfx]  ✗ Fi.CI.BAT: failure for drm/i915: Be more gentle when exiting non-persistent contexts
  2021-08-05 15:04 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for " Patchwork
@ 2021-08-05 16:10   ` Tvrtko Ursulin
  2021-08-05 18:24     ` Matthew Brost
  0 siblings, 1 reply; 9+ messages in thread
From: Tvrtko Ursulin @ 2021-08-05 16:10 UTC (permalink / raw)
  To: intel-gfx


On 05/08/2021 16:04, Patchwork wrote:
> *Patch Details*
> *Series:*	drm/i915: Be more gentle when exiting non-persistent contexts
> *URL:*	https://patchwork.freedesktop.org/series/93420/ 
> <https://patchwork.freedesktop.org/series/93420/>
> *State:*	failure
> *Details:* 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/index.html 
> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/index.html>
> 
> 
>   CI Bug Log - changes from CI_DRM_10450 -> Patchwork_20775
> 
> 
>     Summary
> 
> *FAILURE*
> 
> Serious unknown changes coming with Patchwork_20775 absolutely need to be
> verified manually.
> 
> If you think the reported changes have nothing to do with the changes
> introduced in Patchwork_20775, please notify your bug team to allow them
> to document this new failure mode, which will reduce false positives in CI.
> 
> External URL: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/index.html
> 
> 
>     Possible new issues
> 
> Here are the unknown changes that may have been introduced in 
> Patchwork_20775:
> 
> 
>       IGT changes
> 
> 
>         Possible regressions
> 
>   * igt@i915_selftest@live@gt_lrc:
>       o fi-rkl-guc: PASS
>         <https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10450/fi-rkl-guc/igt@i915_selftest@live@gt_lrc.html>
>         -> DMESG-WARN
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-rkl-guc/igt@i915_selftest@live@gt_lrc.html>

<6> [233.928677] i915: Running intel_lrc_live_selftests/live_lrc_isolation
<3> [233.988780] i915 0000:00:02.0: [drm] *ERROR* rcs0 context redzone overwritten!

Something GuC specific by the look of it, or at least I haven't found the same signature elsewhere. But in any case it is not related to this patch.

Regards,

Tvrtko

> 
> 
>     Known issues
> 
> Here are the changes found in Patchwork_20775 that come from known issues:
> 
> 
>       IGT changes
> 
> 
>         Issues hit
> 
>   *
> 
>     igt@amdgpu/amd_basic@query-info:
> 
>       o fi-bsw-kefka: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-bsw-kefka/igt@amdgpu/amd_basic@query-info.html>
>         (fdo#109271
>         <https://bugs.freedesktop.org/show_bug.cgi?id=109271>) +17
>         similar issues
>   *
> 
>     igt@gem_exec_fence@basic-busy@bcs0:
> 
>       o fi-kbl-soraka: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-kbl-soraka/igt@gem_exec_fence@basic-busy@bcs0.html>
>         (fdo#109271
>         <https://bugs.freedesktop.org/show_bug.cgi?id=109271>) +26
>         similar issues
>   *
> 
>     igt@gem_huc_copy@huc-copy:
> 
>       o fi-kbl-soraka: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-kbl-soraka/igt@gem_huc_copy@huc-copy.html>
>         (fdo#109271
>         <https://bugs.freedesktop.org/show_bug.cgi?id=109271> /
>         i915#2190 <https://gitlab.freedesktop.org/drm/intel/issues/2190>)
>   *
> 
>     igt@i915_pm_rpm@basic-rte:
> 
>       o fi-kbl-soraka: NOTRUN -> FAIL
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-kbl-soraka/igt@i915_pm_rpm@basic-rte.html>
>         (i915#579 <https://gitlab.freedesktop.org/drm/intel/issues/579>)
>   *
> 
>     igt@i915_selftest@live@gt_pm:
> 
>       o fi-kbl-soraka: NOTRUN -> DMESG-FAIL
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-kbl-soraka/igt@i915_selftest@live@gt_pm.html>
>         (i915#1886
>         <https://gitlab.freedesktop.org/drm/intel/issues/1886> /
>         i915#2291 <https://gitlab.freedesktop.org/drm/intel/issues/2291>)
>   *
> 
>     igt@i915_selftest@live@late_gt_pm:
> 
>       o fi-bsw-nick: PASS
>         <https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10450/fi-bsw-nick/igt@i915_selftest@live@late_gt_pm.html>
>         -> DMESG-FAIL
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-bsw-nick/igt@i915_selftest@live@late_gt_pm.html>
>         (i915#2927 <https://gitlab.freedesktop.org/drm/intel/issues/2927>)
>   *
> 
>     igt@kms_chamelium@common-hpd-after-suspend:
> 
>       o fi-kbl-soraka: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-kbl-soraka/igt@kms_chamelium@common-hpd-after-suspend.html>
>         (fdo#109271
>         <https://bugs.freedesktop.org/show_bug.cgi?id=109271> /
>         fdo#111827
>         <https://bugs.freedesktop.org/show_bug.cgi?id=111827>) +8
>         similar issues
>   *
> 
>     igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d:
> 
>       o fi-kbl-soraka: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-kbl-soraka/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d.html>
>         (fdo#109271
>         <https://bugs.freedesktop.org/show_bug.cgi?id=109271> / i915#533
>         <https://gitlab.freedesktop.org/drm/intel/issues/533>)
>   *
> 
>     igt@runner@aborted:
> 
>       o fi-bsw-nick: NOTRUN -> FAIL
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-bsw-nick/igt@runner@aborted.html>
>         (fdo#109271
>         <https://bugs.freedesktop.org/show_bug.cgi?id=109271> /
>         i915#1436 <https://gitlab.freedesktop.org/drm/intel/issues/1436>)
> 
> 
>         Possible fixes
> 
>   * igt@i915_selftest@live@execlists:
>       o fi-bsw-kefka: INCOMPLETE
>         <https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10450/fi-bsw-kefka/igt@i915_selftest@live@execlists.html>
>         (i915#2940
>         <https://gitlab.freedesktop.org/drm/intel/issues/2940>) -> PASS
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-bsw-kefka/igt@i915_selftest@live@execlists.html>
> 
> {name}: This element is suppressed. This means it is ignored when computing
> the status of the difference (SUCCESS, WARNING, or FAILURE).
> 
> 
>     Participating hosts (40 -> 35)
> 
> Additional (1): fi-kbl-soraka
> Missing (6): fi-ilk-m540 fi-hsw-4200u fi-bsw-cyan fi-ctg-p8600 
> fi-bdw-samus bat-jsl-1
> 
> 
>     Build changes
> 
>   * Linux: CI_DRM_10450 -> Patchwork_20775
> 
> CI-20190529: 20190529
> CI_DRM_10450: 51d9c8293e8446e921b74d996982ade862fcfa5c @ 
> git://anongit.freedesktop.org/gfx-ci/linux
> IGT_6160: 4287344dd6a39d9036c5fb9a047a7d8f10bee981 @ 
> https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
> Patchwork_20775: 62efd2820c620b384c9688fa71ad6e8a5f6d27fc @ 
> git://anongit.freedesktop.org/gfx-ci/linux
> 
> == Linux commits ==
> 
> 62efd2820c62 drm/i915: Be more gentle when exiting non-persistent contexts
> 

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

* Re: [Intel-gfx] [PATCH] drm/i915: Be more gentle when exiting non-persistent contexts
  2021-08-05 12:05 [Intel-gfx] [PATCH] drm/i915: Be more gentle when exiting non-persistent contexts Tvrtko Ursulin
  2021-08-05 15:04 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for " Patchwork
@ 2021-08-05 16:32 ` Matthew Brost
  2021-08-06 10:44   ` Tvrtko Ursulin
  1 sibling, 1 reply; 9+ messages in thread
From: Matthew Brost @ 2021-08-05 16:32 UTC (permalink / raw)
  To: Tvrtko Ursulin
  Cc: Intel-gfx, dri-devel, Tvrtko Ursulin, Chris Wilson, Zhen Han

On Thu, Aug 05, 2021 at 01:05:09PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> When a non-persistent context exits we currently mark it as banned in
> order to trigger fast termination of any outstanding GPU jobs it may have
> left running.
> 
> In doing so we apply a very strict 1ms limit in which the left over job
> has to preempt before we issues an engine resets.
> 
> Some workloads are not able to cleanly preempt in that time window and it
> can be argued that it would instead be better to give them a bit more
> grace since avoiding engine resets is generally preferrable.
> 
> To achieve this the patch splits handling of banned contexts from simply
> closed non-persistent ones and then applies different timeouts for both
> and also extends the criteria which determines if a request should be
> scheduled back in after preemption or not.
> 
> 15ms preempt timeout grace is given to exited non-persistent contexts
> which have been empirically tested to satisfy customers requirements
> and still provides reasonably quick cleanup post exit.
> 

I think you need to rework your thinking here a bit as this a very
execlists specific solution and the GuC needs to be considered.

> v2:
>  * Streamline fast path checks.
> 
> v3:
>  * Simplify by using only schedulable status.
>  * Increase timeout to 20ms.
> 
> v4:
>  * Fix live_execlists selftest.
> 
> v5:
>  * Fix logic in kill_engines.
> 
> v6:
>  * Rebase.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Zhen Han <zhen.han@intel.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_context.c   | 22 +++++++++++++------
>  drivers/gpu/drm/i915/gt/intel_context.c       |  2 ++
>  drivers/gpu/drm/i915/gt/intel_context.h       | 17 +++++++++++++-
>  drivers/gpu/drm/i915/gt/intel_context_types.h |  1 +
>  .../drm/i915/gt/intel_execlists_submission.c  | 11 ++++++++--
>  drivers/gpu/drm/i915/gt/selftest_execlists.c  | 20 +++++++++++------
>  drivers/gpu/drm/i915/i915_request.c           |  2 +-
>  7 files changed, 57 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index cff72679ad7c..21fe5d4057ab 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -1065,7 +1065,8 @@ static struct intel_engine_cs *active_engine(struct intel_context *ce)
>  	return engine;
>  }
>  
> -static void kill_engines(struct i915_gem_engines *engines, bool ban)
> +static void
> +kill_engines(struct i915_gem_engines *engines, bool ban, bool persistent)
>  {
>  	struct i915_gem_engines_iter it;
>  	struct intel_context *ce;
> @@ -1079,8 +1080,15 @@ static void kill_engines(struct i915_gem_engines *engines, bool ban)
>  	 */
>  	for_each_gem_engine(ce, engines, it) {
>  		struct intel_engine_cs *engine;
> +		bool skip = false;
> +
> +		if (ban)
> +			skip = intel_context_ban(ce, NULL);
> +		else if (!persistent)
> +			skip = !intel_context_clear_schedulable(ce);

schedulable doesn't hook into the backend at all, while
intel_context_ban does. In the case of GuC submission intel_context_ban
changes to preemption timeout to 1 us and disables scheduling resulting
in the context getting kicked off the hardware immediately. You likely
need to update intel_context_clear_schedulable to use the same vfunc as
intel_context_ban() but accept an argument for the value of the
preemption timeout. For a ban user a lower value, for clearing
schedulable use a higher value.

>  
> -		if (ban && intel_context_ban(ce, NULL))
> +		/* Already previously banned or made non-schedulable? */
> +		if (skip)
>  			continue;
>  
>  		/*
> @@ -1093,7 +1101,7 @@ static void kill_engines(struct i915_gem_engines *engines, bool ban)
>  		engine = active_engine(ce);
>  
>  		/* First attempt to gracefully cancel the context */
> -		if (engine && !__cancel_engine(engine) && ban)
> +		if (engine && !__cancel_engine(engine) && (ban || !persistent))
>  			/*
>  			 * If we are unable to send a preemptive pulse to bump
>  			 * the context from the GPU, we have to resort to a full
> @@ -1105,8 +1113,6 @@ static void kill_engines(struct i915_gem_engines *engines, bool ban)
>  
>  static void kill_context(struct i915_gem_context *ctx)
>  {
> -	bool ban = (!i915_gem_context_is_persistent(ctx) ||
> -		    !ctx->i915->params.enable_hangcheck);
>  	struct i915_gem_engines *pos, *next;
>  
>  	spin_lock_irq(&ctx->stale.lock);
> @@ -1119,7 +1125,8 @@ static void kill_context(struct i915_gem_context *ctx)
>  
>  		spin_unlock_irq(&ctx->stale.lock);
>  
> -		kill_engines(pos, ban);
> +		kill_engines(pos, !ctx->i915->params.enable_hangcheck,
> +			     i915_gem_context_is_persistent(ctx));
>  
>  		spin_lock_irq(&ctx->stale.lock);
>  		GEM_BUG_ON(i915_sw_fence_signaled(&pos->fence));
> @@ -1165,7 +1172,8 @@ static void engines_idle_release(struct i915_gem_context *ctx,
>  
>  kill:
>  	if (list_empty(&engines->link)) /* raced, already closed */
> -		kill_engines(engines, true);
> +		kill_engines(engines, true,
> +			     i915_gem_context_is_persistent(ctx));
>  
>  	i915_sw_fence_commit(&engines->fence);
>  }
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
> index 745e84c72c90..bc1701ef1578 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.c
> +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> @@ -382,6 +382,8 @@ intel_context_init(struct intel_context *ce, struct intel_engine_cs *engine)
>  	ce->ring = NULL;
>  	ce->ring_size = SZ_4K;
>  
> +	__set_bit(CONTEXT_SCHEDULABLE, &ce->flags);
> +
>  	ewma_runtime_init(&ce->runtime.avg);
>  
>  	ce->vm = i915_vm_get(engine->gt->vm);
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.h b/drivers/gpu/drm/i915/gt/intel_context.h
> index c41098950746..5b50716654dd 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context.h
> @@ -251,7 +251,22 @@ static inline bool intel_context_is_banned(const struct intel_context *ce)
>  
>  static inline bool intel_context_set_banned(struct intel_context *ce)
>  {
> -	return test_and_set_bit(CONTEXT_BANNED, &ce->flags);
> +	bool banned = test_and_set_bit(CONTEXT_BANNED, &ce->flags);
> +
> +	if (!banned)
> +		clear_bit(CONTEXT_SCHEDULABLE, &ce->flags);
> +
> +	return banned;
> +}
> +
> +static inline bool intel_context_clear_schedulable(struct intel_context *ce)
> +{
> +	return test_and_clear_bit(CONTEXT_SCHEDULABLE, &ce->flags);
> +}
> +
> +static inline bool intel_context_is_schedulable(const struct intel_context *ce)
> +{
> +	return test_bit(CONTEXT_SCHEDULABLE, &ce->flags);
>  }
>  
>  static inline bool intel_context_ban(struct intel_context *ce,
> diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
> index e54351a170e2..3306c70c9c54 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
> @@ -112,6 +112,7 @@ struct intel_context {
>  #define CONTEXT_FORCE_SINGLE_SUBMISSION	7
>  #define CONTEXT_NOPREEMPT		8
>  #define CONTEXT_LRCA_DIRTY		9
> +#define CONTEXT_SCHEDULABLE		10  /* Unless banned or non-persistent closed. */
>  
>  	struct {
>  		u64 timeout_us;
> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> index de5f9c86b9a4..778f3cda3c71 100644
> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> @@ -478,7 +478,7 @@ __execlists_schedule_in(struct i915_request *rq)
>  		     !intel_engine_has_heartbeat(engine)))
>  		intel_context_set_banned(ce);
>  
> -	if (unlikely(intel_context_is_banned(ce) || bad_request(rq)))
> +	if (unlikely(!intel_context_is_schedulable(ce) || bad_request(rq)))
>  		reset_active(rq, engine);
>  
>  	if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
> @@ -1222,12 +1222,19 @@ static void record_preemption(struct intel_engine_execlists *execlists)
>  static unsigned long active_preempt_timeout(struct intel_engine_cs *engine,
>  					    const struct i915_request *rq)
>  {
> +	struct intel_context *ce;
> +
>  	if (!rq)
>  		return 0;
>  
> +	ce = rq->context;
> +
>  	/* Force a fast reset for terminated contexts (ignoring sysfs!) */
> -	if (unlikely(intel_context_is_banned(rq->context) || bad_request(rq)))
> +	if (unlikely(intel_context_is_banned(ce) || bad_request(rq)))
>  		return 1;
> +	/* Longer grace for closed non-persistent contexts to avoid resets. */
> +	else if (unlikely(!intel_context_is_schedulable(ce)))
> +		return 20;

Likely want a define for '1' and '20' too.

Matt

>  
>  	return READ_ONCE(engine->props.preempt_timeout_ms);
>  }
> diff --git a/drivers/gpu/drm/i915/gt/selftest_execlists.c b/drivers/gpu/drm/i915/gt/selftest_execlists.c
> index f12ffe797639..da36c015caf4 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_execlists.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_execlists.c
> @@ -2050,6 +2050,12 @@ struct live_preempt_cancel {
>  	struct preempt_client a, b;
>  };
>  
> +static void context_clear_banned(struct intel_context *ce)
> +{
> +	clear_bit(CONTEXT_BANNED, &ce->flags);
> +	set_bit(CONTEXT_SCHEDULABLE, &ce->flags);
> +}
> +
>  static int __cancel_active0(struct live_preempt_cancel *arg)
>  {
>  	struct i915_request *rq;
> @@ -2068,7 +2074,7 @@ static int __cancel_active0(struct live_preempt_cancel *arg)
>  	if (IS_ERR(rq))
>  		return PTR_ERR(rq);
>  
> -	clear_bit(CONTEXT_BANNED, &rq->context->flags);
> +	context_clear_banned(rq->context);
>  	i915_request_get(rq);
>  	i915_request_add(rq);
>  	if (!igt_wait_for_spinner(&arg->a.spin, rq)) {
> @@ -2112,7 +2118,7 @@ static int __cancel_active1(struct live_preempt_cancel *arg)
>  	if (IS_ERR(rq[0]))
>  		return PTR_ERR(rq[0]);
>  
> -	clear_bit(CONTEXT_BANNED, &rq[0]->context->flags);
> +	context_clear_banned(rq[0]->context);
>  	i915_request_get(rq[0]);
>  	i915_request_add(rq[0]);
>  	if (!igt_wait_for_spinner(&arg->a.spin, rq[0])) {
> @@ -2128,7 +2134,7 @@ static int __cancel_active1(struct live_preempt_cancel *arg)
>  		goto out;
>  	}
>  
> -	clear_bit(CONTEXT_BANNED, &rq[1]->context->flags);
> +	context_clear_banned(rq[1]->context);
>  	i915_request_get(rq[1]);
>  	err = i915_request_await_dma_fence(rq[1], &rq[0]->fence);
>  	i915_request_add(rq[1]);
> @@ -2183,7 +2189,7 @@ static int __cancel_queued(struct live_preempt_cancel *arg)
>  	if (IS_ERR(rq[0]))
>  		return PTR_ERR(rq[0]);
>  
> -	clear_bit(CONTEXT_BANNED, &rq[0]->context->flags);
> +	context_clear_banned(rq[0]->context);
>  	i915_request_get(rq[0]);
>  	i915_request_add(rq[0]);
>  	if (!igt_wait_for_spinner(&arg->a.spin, rq[0])) {
> @@ -2197,7 +2203,7 @@ static int __cancel_queued(struct live_preempt_cancel *arg)
>  		goto out;
>  	}
>  
> -	clear_bit(CONTEXT_BANNED, &rq[1]->context->flags);
> +	context_clear_banned(rq[1]->context);
>  	i915_request_get(rq[1]);
>  	err = i915_request_await_dma_fence(rq[1], &rq[0]->fence);
>  	i915_request_add(rq[1]);
> @@ -2273,7 +2279,7 @@ static int __cancel_hostile(struct live_preempt_cancel *arg)
>  	if (IS_ERR(rq))
>  		return PTR_ERR(rq);
>  
> -	clear_bit(CONTEXT_BANNED, &rq->context->flags);
> +	context_clear_banned(rq->context);
>  	i915_request_get(rq);
>  	i915_request_add(rq);
>  	if (!igt_wait_for_spinner(&arg->a.spin, rq)) {
> @@ -2329,7 +2335,7 @@ static int __cancel_fail(struct live_preempt_cancel *arg)
>  	if (IS_ERR(rq))
>  		return PTR_ERR(rq);
>  
> -	clear_bit(CONTEXT_BANNED, &rq->context->flags);
> +	context_clear_banned(rq->context);
>  	i915_request_get(rq);
>  	i915_request_add(rq);
>  	if (!igt_wait_for_spinner(&arg->a.spin, rq)) {
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index ce446716d092..b1a9bec83339 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -583,7 +583,7 @@ bool __i915_request_submit(struct i915_request *request)
>  		goto active;
>  	}
>  
> -	if (unlikely(intel_context_is_banned(request->context)))
> +	if (unlikely(!intel_context_is_schedulable(request->context)))
>  		i915_request_set_error_once(request, -EIO);
>  
>  	if (unlikely(fatal_error(request->fence.error)))
> -- 
> 2.30.2
> 

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

* Re: [Intel-gfx]  ✗ Fi.CI.BAT: failure for drm/i915: Be more gentle when exiting non-persistent contexts
  2021-08-05 16:10   ` Tvrtko Ursulin
@ 2021-08-05 18:24     ` Matthew Brost
  0 siblings, 0 replies; 9+ messages in thread
From: Matthew Brost @ 2021-08-05 18:24 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: intel-gfx

On Thu, Aug 05, 2021 at 05:10:29PM +0100, Tvrtko Ursulin wrote:
> 
> On 05/08/2021 16:04, Patchwork wrote:
> > *Patch Details*
> > *Series:*	drm/i915: Be more gentle when exiting non-persistent contexts
> > *URL:*	https://patchwork.freedesktop.org/series/93420/
> > <https://patchwork.freedesktop.org/series/93420/>
> > *State:*	failure
> > *Details:*
> > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/index.html
> > <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/index.html>
> > 
> > 
> >   CI Bug Log - changes from CI_DRM_10450 -> Patchwork_20775
> > 
> > 
> >     Summary
> > 
> > *FAILURE*
> > 
> > Serious unknown changes coming with Patchwork_20775 absolutely need to be
> > verified manually.
> > 
> > If you think the reported changes have nothing to do with the changes
> > introduced in Patchwork_20775, please notify your bug team to allow them
> > to document this new failure mode, which will reduce false positives in CI.
> > 
> > External URL:
> > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/index.html
> > 
> > 
> >     Possible new issues
> > 
> > Here are the unknown changes that may have been introduced in
> > Patchwork_20775:
> > 
> > 
> >       IGT changes
> > 
> > 
> >         Possible regressions
> > 
> >   * igt@i915_selftest@live@gt_lrc:
> >       o fi-rkl-guc: PASS
> >         <https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10450/fi-rkl-guc/igt@i915_selftest@live@gt_lrc.html>
> >         -> DMESG-WARN
> >         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-rkl-guc/igt@i915_selftest@live@gt_lrc.html>
> 
> <6> [233.928677] i915: Running intel_lrc_live_selftests/live_lrc_isolation
> <3> [233.988780] i915 0000:00:02.0: [drm] *ERROR* rcs0 context redzone overwritten!
> 
> Something GuC specific by the look of it, or at least I haven't found the same signature elsewhere. But in any case it is not related to this patch.
> 

No sure what this is about. Ran this locally on a RKL machine and it
passed just fine for me. Something to keep an eye on as CI gets fully
enabled with GuC submission.

Also BTW, speaking of CI & GuC submission it isn't all that great yet.
Maybe ping me when you have the next rev of this patch and I can run
series of tests with GuC submission related to banning / persistence.

Matt

> Regards,
> 
> Tvrtko
> 
> > 
> > 
> >     Known issues
> > 
> > Here are the changes found in Patchwork_20775 that come from known issues:
> > 
> > 
> >       IGT changes
> > 
> > 
> >         Issues hit
> > 
> >   *
> > 
> >     igt@amdgpu/amd_basic@query-info:
> > 
> >       o fi-bsw-kefka: NOTRUN -> SKIP
> >         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-bsw-kefka/igt@amdgpu/amd_basic@query-info.html>
> >         (fdo#109271
> >         <https://bugs.freedesktop.org/show_bug.cgi?id=109271>) +17
> >         similar issues
> >   *
> > 
> >     igt@gem_exec_fence@basic-busy@bcs0:
> > 
> >       o fi-kbl-soraka: NOTRUN -> SKIP
> >         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-kbl-soraka/igt@gem_exec_fence@basic-busy@bcs0.html>
> >         (fdo#109271
> >         <https://bugs.freedesktop.org/show_bug.cgi?id=109271>) +26
> >         similar issues
> >   *
> > 
> >     igt@gem_huc_copy@huc-copy:
> > 
> >       o fi-kbl-soraka: NOTRUN -> SKIP
> >         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-kbl-soraka/igt@gem_huc_copy@huc-copy.html>
> >         (fdo#109271
> >         <https://bugs.freedesktop.org/show_bug.cgi?id=109271> /
> >         i915#2190 <https://gitlab.freedesktop.org/drm/intel/issues/2190>)
> >   *
> > 
> >     igt@i915_pm_rpm@basic-rte:
> > 
> >       o fi-kbl-soraka: NOTRUN -> FAIL
> >         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-kbl-soraka/igt@i915_pm_rpm@basic-rte.html>
> >         (i915#579 <https://gitlab.freedesktop.org/drm/intel/issues/579>)
> >   *
> > 
> >     igt@i915_selftest@live@gt_pm:
> > 
> >       o fi-kbl-soraka: NOTRUN -> DMESG-FAIL
> >         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-kbl-soraka/igt@i915_selftest@live@gt_pm.html>
> >         (i915#1886
> >         <https://gitlab.freedesktop.org/drm/intel/issues/1886> /
> >         i915#2291 <https://gitlab.freedesktop.org/drm/intel/issues/2291>)
> >   *
> > 
> >     igt@i915_selftest@live@late_gt_pm:
> > 
> >       o fi-bsw-nick: PASS
> >         <https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10450/fi-bsw-nick/igt@i915_selftest@live@late_gt_pm.html>
> >         -> DMESG-FAIL
> >         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-bsw-nick/igt@i915_selftest@live@late_gt_pm.html>
> >         (i915#2927 <https://gitlab.freedesktop.org/drm/intel/issues/2927>)
> >   *
> > 
> >     igt@kms_chamelium@common-hpd-after-suspend:
> > 
> >       o fi-kbl-soraka: NOTRUN -> SKIP
> >         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-kbl-soraka/igt@kms_chamelium@common-hpd-after-suspend.html>
> >         (fdo#109271
> >         <https://bugs.freedesktop.org/show_bug.cgi?id=109271> /
> >         fdo#111827
> >         <https://bugs.freedesktop.org/show_bug.cgi?id=111827>) +8
> >         similar issues
> >   *
> > 
> >     igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d:
> > 
> >       o fi-kbl-soraka: NOTRUN -> SKIP
> >         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-kbl-soraka/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d.html>
> >         (fdo#109271
> >         <https://bugs.freedesktop.org/show_bug.cgi?id=109271> / i915#533
> >         <https://gitlab.freedesktop.org/drm/intel/issues/533>)
> >   *
> > 
> >     igt@runner@aborted:
> > 
> >       o fi-bsw-nick: NOTRUN -> FAIL
> >         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-bsw-nick/igt@runner@aborted.html>
> >         (fdo#109271
> >         <https://bugs.freedesktop.org/show_bug.cgi?id=109271> /
> >         i915#1436 <https://gitlab.freedesktop.org/drm/intel/issues/1436>)
> > 
> > 
> >         Possible fixes
> > 
> >   * igt@i915_selftest@live@execlists:
> >       o fi-bsw-kefka: INCOMPLETE
> >         <https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10450/fi-bsw-kefka/igt@i915_selftest@live@execlists.html>
> >         (i915#2940
> >         <https://gitlab.freedesktop.org/drm/intel/issues/2940>) -> PASS
> >         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20775/fi-bsw-kefka/igt@i915_selftest@live@execlists.html>
> > 
> > {name}: This element is suppressed. This means it is ignored when computing
> > the status of the difference (SUCCESS, WARNING, or FAILURE).
> > 
> > 
> >     Participating hosts (40 -> 35)
> > 
> > Additional (1): fi-kbl-soraka
> > Missing (6): fi-ilk-m540 fi-hsw-4200u fi-bsw-cyan fi-ctg-p8600
> > fi-bdw-samus bat-jsl-1
> > 
> > 
> >     Build changes
> > 
> >   * Linux: CI_DRM_10450 -> Patchwork_20775
> > 
> > CI-20190529: 20190529
> > CI_DRM_10450: 51d9c8293e8446e921b74d996982ade862fcfa5c @
> > git://anongit.freedesktop.org/gfx-ci/linux
> > IGT_6160: 4287344dd6a39d9036c5fb9a047a7d8f10bee981 @
> > https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
> > Patchwork_20775: 62efd2820c620b384c9688fa71ad6e8a5f6d27fc @
> > git://anongit.freedesktop.org/gfx-ci/linux
> > 
> > == Linux commits ==
> > 
> > 62efd2820c62 drm/i915: Be more gentle when exiting non-persistent contexts
> > 

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

* Re: [Intel-gfx] [PATCH] drm/i915: Be more gentle when exiting non-persistent contexts
  2021-08-05 16:32 ` [Intel-gfx] [PATCH] " Matthew Brost
@ 2021-08-06 10:44   ` Tvrtko Ursulin
  0 siblings, 0 replies; 9+ messages in thread
From: Tvrtko Ursulin @ 2021-08-06 10:44 UTC (permalink / raw)
  To: Matthew Brost
  Cc: Intel-gfx, dri-devel, Tvrtko Ursulin, Chris Wilson, Zhen Han


On 05/08/2021 17:32, Matthew Brost wrote:
> On Thu, Aug 05, 2021 at 01:05:09PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> When a non-persistent context exits we currently mark it as banned in
>> order to trigger fast termination of any outstanding GPU jobs it may have
>> left running.
>>
>> In doing so we apply a very strict 1ms limit in which the left over job
>> has to preempt before we issues an engine resets.
>>
>> Some workloads are not able to cleanly preempt in that time window and it
>> can be argued that it would instead be better to give them a bit more
>> grace since avoiding engine resets is generally preferrable.
>>
>> To achieve this the patch splits handling of banned contexts from simply
>> closed non-persistent ones and then applies different timeouts for both
>> and also extends the criteria which determines if a request should be
>> scheduled back in after preemption or not.
>>
>> 15ms preempt timeout grace is given to exited non-persistent contexts
>> which have been empirically tested to satisfy customers requirements
>> and still provides reasonably quick cleanup post exit.
>>
> 
> I think you need to rework your thinking here a bit as this a very
> execlists specific solution and the GuC needs to be considered.

Slipped my mind GuC patches were merged in the meantime. (This patch 
predates that.) But I think wording in the commit message is fine. It is 
just the implementation that now has to handle the GuC as well.

>> v2:
>>   * Streamline fast path checks.
>>
>> v3:
>>   * Simplify by using only schedulable status.
>>   * Increase timeout to 20ms.
>>
>> v4:
>>   * Fix live_execlists selftest.
>>
>> v5:
>>   * Fix logic in kill_engines.
>>
>> v6:
>>   * Rebase.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Zhen Han <zhen.han@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gem/i915_gem_context.c   | 22 +++++++++++++------
>>   drivers/gpu/drm/i915/gt/intel_context.c       |  2 ++
>>   drivers/gpu/drm/i915/gt/intel_context.h       | 17 +++++++++++++-
>>   drivers/gpu/drm/i915/gt/intel_context_types.h |  1 +
>>   .../drm/i915/gt/intel_execlists_submission.c  | 11 ++++++++--
>>   drivers/gpu/drm/i915/gt/selftest_execlists.c  | 20 +++++++++++------
>>   drivers/gpu/drm/i915/i915_request.c           |  2 +-
>>   7 files changed, 57 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>> index cff72679ad7c..21fe5d4057ab 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>> @@ -1065,7 +1065,8 @@ static struct intel_engine_cs *active_engine(struct intel_context *ce)
>>   	return engine;
>>   }
>>   
>> -static void kill_engines(struct i915_gem_engines *engines, bool ban)
>> +static void
>> +kill_engines(struct i915_gem_engines *engines, bool ban, bool persistent)
>>   {
>>   	struct i915_gem_engines_iter it;
>>   	struct intel_context *ce;
>> @@ -1079,8 +1080,15 @@ static void kill_engines(struct i915_gem_engines *engines, bool ban)
>>   	 */
>>   	for_each_gem_engine(ce, engines, it) {
>>   		struct intel_engine_cs *engine;
>> +		bool skip = false;
>> +
>> +		if (ban)
>> +			skip = intel_context_ban(ce, NULL);
>> +		else if (!persistent)
>> +			skip = !intel_context_clear_schedulable(ce);
> 
> schedulable doesn't hook into the backend at all, while
> intel_context_ban does. In the case of GuC submission intel_context_ban
> changes to preemption timeout to 1 us and disables scheduling resulting
> in the context getting kicked off the hardware immediately. You likely
> need to update intel_context_clear_schedulable to use the same vfunc as
> intel_context_ban() but accept an argument for the value of the
> preemption timeout. For a ban user a lower value, for clearing
> schedulable use a higher value.

Okay I'll have a look. Might go back to closed flag as opposed to 
schedulable as well since I don't quite like schedulable being the odd 
one out.

> 
>>   
>> -		if (ban && intel_context_ban(ce, NULL))
>> +		/* Already previously banned or made non-schedulable? */
>> +		if (skip)
>>   			continue;
>>   
>>   		/*
>> @@ -1093,7 +1101,7 @@ static void kill_engines(struct i915_gem_engines *engines, bool ban)
>>   		engine = active_engine(ce);
>>   
>>   		/* First attempt to gracefully cancel the context */
>> -		if (engine && !__cancel_engine(engine) && ban)
>> +		if (engine && !__cancel_engine(engine) && (ban || !persistent))
>>   			/*
>>   			 * If we are unable to send a preemptive pulse to bump
>>   			 * the context from the GPU, we have to resort to a full
>> @@ -1105,8 +1113,6 @@ static void kill_engines(struct i915_gem_engines *engines, bool ban)
>>   
>>   static void kill_context(struct i915_gem_context *ctx)
>>   {
>> -	bool ban = (!i915_gem_context_is_persistent(ctx) ||
>> -		    !ctx->i915->params.enable_hangcheck);
>>   	struct i915_gem_engines *pos, *next;
>>   
>>   	spin_lock_irq(&ctx->stale.lock);
>> @@ -1119,7 +1125,8 @@ static void kill_context(struct i915_gem_context *ctx)
>>   
>>   		spin_unlock_irq(&ctx->stale.lock);
>>   
>> -		kill_engines(pos, ban);
>> +		kill_engines(pos, !ctx->i915->params.enable_hangcheck,
>> +			     i915_gem_context_is_persistent(ctx));
>>   
>>   		spin_lock_irq(&ctx->stale.lock);
>>   		GEM_BUG_ON(i915_sw_fence_signaled(&pos->fence));
>> @@ -1165,7 +1172,8 @@ static void engines_idle_release(struct i915_gem_context *ctx,
>>   
>>   kill:
>>   	if (list_empty(&engines->link)) /* raced, already closed */
>> -		kill_engines(engines, true);
>> +		kill_engines(engines, true,
>> +			     i915_gem_context_is_persistent(ctx));
>>   
>>   	i915_sw_fence_commit(&engines->fence);
>>   }
>> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
>> index 745e84c72c90..bc1701ef1578 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_context.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_context.c
>> @@ -382,6 +382,8 @@ intel_context_init(struct intel_context *ce, struct intel_engine_cs *engine)
>>   	ce->ring = NULL;
>>   	ce->ring_size = SZ_4K;
>>   
>> +	__set_bit(CONTEXT_SCHEDULABLE, &ce->flags);
>> +
>>   	ewma_runtime_init(&ce->runtime.avg);
>>   
>>   	ce->vm = i915_vm_get(engine->gt->vm);
>> diff --git a/drivers/gpu/drm/i915/gt/intel_context.h b/drivers/gpu/drm/i915/gt/intel_context.h
>> index c41098950746..5b50716654dd 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_context.h
>> +++ b/drivers/gpu/drm/i915/gt/intel_context.h
>> @@ -251,7 +251,22 @@ static inline bool intel_context_is_banned(const struct intel_context *ce)
>>   
>>   static inline bool intel_context_set_banned(struct intel_context *ce)
>>   {
>> -	return test_and_set_bit(CONTEXT_BANNED, &ce->flags);
>> +	bool banned = test_and_set_bit(CONTEXT_BANNED, &ce->flags);
>> +
>> +	if (!banned)
>> +		clear_bit(CONTEXT_SCHEDULABLE, &ce->flags);
>> +
>> +	return banned;
>> +}
>> +
>> +static inline bool intel_context_clear_schedulable(struct intel_context *ce)
>> +{
>> +	return test_and_clear_bit(CONTEXT_SCHEDULABLE, &ce->flags);
>> +}
>> +
>> +static inline bool intel_context_is_schedulable(const struct intel_context *ce)
>> +{
>> +	return test_bit(CONTEXT_SCHEDULABLE, &ce->flags);
>>   }
>>   
>>   static inline bool intel_context_ban(struct intel_context *ce,
>> diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
>> index e54351a170e2..3306c70c9c54 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_context_types.h
>> +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
>> @@ -112,6 +112,7 @@ struct intel_context {
>>   #define CONTEXT_FORCE_SINGLE_SUBMISSION	7
>>   #define CONTEXT_NOPREEMPT		8
>>   #define CONTEXT_LRCA_DIRTY		9
>> +#define CONTEXT_SCHEDULABLE		10  /* Unless banned or non-persistent closed. */
>>   
>>   	struct {
>>   		u64 timeout_us;
>> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>> index de5f9c86b9a4..778f3cda3c71 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>> @@ -478,7 +478,7 @@ __execlists_schedule_in(struct i915_request *rq)
>>   		     !intel_engine_has_heartbeat(engine)))
>>   		intel_context_set_banned(ce);
>>   
>> -	if (unlikely(intel_context_is_banned(ce) || bad_request(rq)))
>> +	if (unlikely(!intel_context_is_schedulable(ce) || bad_request(rq)))
>>   		reset_active(rq, engine);
>>   
>>   	if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
>> @@ -1222,12 +1222,19 @@ static void record_preemption(struct intel_engine_execlists *execlists)
>>   static unsigned long active_preempt_timeout(struct intel_engine_cs *engine,
>>   					    const struct i915_request *rq)
>>   {
>> +	struct intel_context *ce;
>> +
>>   	if (!rq)
>>   		return 0;
>>   
>> +	ce = rq->context;
>> +
>>   	/* Force a fast reset for terminated contexts (ignoring sysfs!) */
>> -	if (unlikely(intel_context_is_banned(rq->context) || bad_request(rq)))
>> +	if (unlikely(intel_context_is_banned(ce) || bad_request(rq)))
>>   		return 1;
>> +	/* Longer grace for closed non-persistent contexts to avoid resets. */
>> +	else if (unlikely(!intel_context_is_schedulable(ce)))
>> +		return 20;
> 
> Likely want a define for '1' and '20' too.

Since the addition of GuC yeah, true.

Regards,

Tvrtko

> 
> Matt
> 
>>   
>>   	return READ_ONCE(engine->props.preempt_timeout_ms);
>>   }
>> diff --git a/drivers/gpu/drm/i915/gt/selftest_execlists.c b/drivers/gpu/drm/i915/gt/selftest_execlists.c
>> index f12ffe797639..da36c015caf4 100644
>> --- a/drivers/gpu/drm/i915/gt/selftest_execlists.c
>> +++ b/drivers/gpu/drm/i915/gt/selftest_execlists.c
>> @@ -2050,6 +2050,12 @@ struct live_preempt_cancel {
>>   	struct preempt_client a, b;
>>   };
>>   
>> +static void context_clear_banned(struct intel_context *ce)
>> +{
>> +	clear_bit(CONTEXT_BANNED, &ce->flags);
>> +	set_bit(CONTEXT_SCHEDULABLE, &ce->flags);
>> +}
>> +
>>   static int __cancel_active0(struct live_preempt_cancel *arg)
>>   {
>>   	struct i915_request *rq;
>> @@ -2068,7 +2074,7 @@ static int __cancel_active0(struct live_preempt_cancel *arg)
>>   	if (IS_ERR(rq))
>>   		return PTR_ERR(rq);
>>   
>> -	clear_bit(CONTEXT_BANNED, &rq->context->flags);
>> +	context_clear_banned(rq->context);
>>   	i915_request_get(rq);
>>   	i915_request_add(rq);
>>   	if (!igt_wait_for_spinner(&arg->a.spin, rq)) {
>> @@ -2112,7 +2118,7 @@ static int __cancel_active1(struct live_preempt_cancel *arg)
>>   	if (IS_ERR(rq[0]))
>>   		return PTR_ERR(rq[0]);
>>   
>> -	clear_bit(CONTEXT_BANNED, &rq[0]->context->flags);
>> +	context_clear_banned(rq[0]->context);
>>   	i915_request_get(rq[0]);
>>   	i915_request_add(rq[0]);
>>   	if (!igt_wait_for_spinner(&arg->a.spin, rq[0])) {
>> @@ -2128,7 +2134,7 @@ static int __cancel_active1(struct live_preempt_cancel *arg)
>>   		goto out;
>>   	}
>>   
>> -	clear_bit(CONTEXT_BANNED, &rq[1]->context->flags);
>> +	context_clear_banned(rq[1]->context);
>>   	i915_request_get(rq[1]);
>>   	err = i915_request_await_dma_fence(rq[1], &rq[0]->fence);
>>   	i915_request_add(rq[1]);
>> @@ -2183,7 +2189,7 @@ static int __cancel_queued(struct live_preempt_cancel *arg)
>>   	if (IS_ERR(rq[0]))
>>   		return PTR_ERR(rq[0]);
>>   
>> -	clear_bit(CONTEXT_BANNED, &rq[0]->context->flags);
>> +	context_clear_banned(rq[0]->context);
>>   	i915_request_get(rq[0]);
>>   	i915_request_add(rq[0]);
>>   	if (!igt_wait_for_spinner(&arg->a.spin, rq[0])) {
>> @@ -2197,7 +2203,7 @@ static int __cancel_queued(struct live_preempt_cancel *arg)
>>   		goto out;
>>   	}
>>   
>> -	clear_bit(CONTEXT_BANNED, &rq[1]->context->flags);
>> +	context_clear_banned(rq[1]->context);
>>   	i915_request_get(rq[1]);
>>   	err = i915_request_await_dma_fence(rq[1], &rq[0]->fence);
>>   	i915_request_add(rq[1]);
>> @@ -2273,7 +2279,7 @@ static int __cancel_hostile(struct live_preempt_cancel *arg)
>>   	if (IS_ERR(rq))
>>   		return PTR_ERR(rq);
>>   
>> -	clear_bit(CONTEXT_BANNED, &rq->context->flags);
>> +	context_clear_banned(rq->context);
>>   	i915_request_get(rq);
>>   	i915_request_add(rq);
>>   	if (!igt_wait_for_spinner(&arg->a.spin, rq)) {
>> @@ -2329,7 +2335,7 @@ static int __cancel_fail(struct live_preempt_cancel *arg)
>>   	if (IS_ERR(rq))
>>   		return PTR_ERR(rq);
>>   
>> -	clear_bit(CONTEXT_BANNED, &rq->context->flags);
>> +	context_clear_banned(rq->context);
>>   	i915_request_get(rq);
>>   	i915_request_add(rq);
>>   	if (!igt_wait_for_spinner(&arg->a.spin, rq)) {
>> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
>> index ce446716d092..b1a9bec83339 100644
>> --- a/drivers/gpu/drm/i915/i915_request.c
>> +++ b/drivers/gpu/drm/i915/i915_request.c
>> @@ -583,7 +583,7 @@ bool __i915_request_submit(struct i915_request *request)
>>   		goto active;
>>   	}
>>   
>> -	if (unlikely(intel_context_is_banned(request->context)))
>> +	if (unlikely(!intel_context_is_schedulable(request->context)))
>>   		i915_request_set_error_once(request, -EIO);
>>   
>>   	if (unlikely(fatal_error(request->fence.error)))
>> -- 
>> 2.30.2
>>

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

* [Intel-gfx] [PATCH] drm/i915: Be more gentle when exiting non-persistent contexts
@ 2022-02-24 13:03 Tvrtko Ursulin
  0 siblings, 0 replies; 9+ messages in thread
From: Tvrtko Ursulin @ 2022-02-24 13:03 UTC (permalink / raw)
  To: Intel-gfx; +Cc: dri-devel, Zhen Han, Chris Wilson

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

When a non-persistent context exits we currently mark it as banned in
order to trigger fast termination of any outstanding GPU jobs it may have
left running.

In doing so we apply a very strict 1ms limit in which the left over job
has to preempt before we issues an engine resets.

Some workloads are not able to cleanly preempt in that time window and it
can be argued that it would instead be better to give them a bit more
grace since avoiding engine resets is generally preferrable.

To achieve this the patch splits handling of banned contexts from simply
closed non-persistent ones and then applies different timeouts for both
and also extends the criteria which determines if a request should be
scheduled back in after preemption or not.

20ms preempt timeout grace is given to exited non-persistent contexts
which have been empirically tested to satisfy customers requirements
and still provides reasonably quick cleanup post exit.

v2:
 * Streamline fast path checks.

v3:
 * Simplify by using only schedulable status.
 * Increase timeout to 20ms.

v4:
 * Fix live_execlists selftest.

v5:
 * Fix logic in kill_engines.

v6:
 * Rebase.

v7:
 * Add GuC support.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Zhen Han <zhen.han@intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_context.c   | 22 +++++++++++-----
 drivers/gpu/drm/i915/gt/intel_context.c       | 25 ++++++++++++++++++
 drivers/gpu/drm/i915/gt/intel_context.h       | 26 ++++++++++++++-----
 drivers/gpu/drm/i915/gt/intel_context_types.h |  4 ++-
 .../drm/i915/gt/intel_execlists_submission.c  | 13 +++++++---
 .../gpu/drm/i915/gt/intel_ring_submission.c   |  7 ++---
 .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 15 ++++++-----
 drivers/gpu/drm/i915/i915_request.c           |  2 +-
 8 files changed, 86 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index bc6d59df064d..3a61ec753894 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -1332,7 +1332,8 @@ static struct intel_engine_cs *active_engine(struct intel_context *ce)
 	return engine;
 }
 
-static void kill_engines(struct i915_gem_engines *engines, bool ban)
+static void
+kill_engines(struct i915_gem_engines *engines, bool ban, bool persistent)
 {
 	struct i915_gem_engines_iter it;
 	struct intel_context *ce;
@@ -1346,8 +1347,15 @@ static void kill_engines(struct i915_gem_engines *engines, bool ban)
 	 */
 	for_each_gem_engine(ce, engines, it) {
 		struct intel_engine_cs *engine;
+		bool skip = false;
 
-		if (ban && intel_context_ban(ce, NULL))
+		if (ban)
+			skip = intel_context_ban(ce, NULL);
+		else if (!persistent)
+			skip = intel_context_exit_nonpersistent(ce, NULL);
+
+		/* Already banned or non-persistent closed. */
+		if (skip)
 			continue;
 
 		/*
@@ -1360,7 +1368,7 @@ static void kill_engines(struct i915_gem_engines *engines, bool ban)
 		engine = active_engine(ce);
 
 		/* First attempt to gracefully cancel the context */
-		if (engine && !__cancel_engine(engine) && ban)
+		if (engine && !__cancel_engine(engine) && (ban || !persistent))
 			/*
 			 * If we are unable to send a preemptive pulse to bump
 			 * the context from the GPU, we have to resort to a full
@@ -1372,8 +1380,6 @@ static void kill_engines(struct i915_gem_engines *engines, bool ban)
 
 static void kill_context(struct i915_gem_context *ctx)
 {
-	bool ban = (!i915_gem_context_is_persistent(ctx) ||
-		    !ctx->i915->params.enable_hangcheck);
 	struct i915_gem_engines *pos, *next;
 
 	spin_lock_irq(&ctx->stale.lock);
@@ -1386,7 +1392,8 @@ static void kill_context(struct i915_gem_context *ctx)
 
 		spin_unlock_irq(&ctx->stale.lock);
 
-		kill_engines(pos, ban);
+		kill_engines(pos, !ctx->i915->params.enable_hangcheck,
+			     i915_gem_context_is_persistent(ctx));
 
 		spin_lock_irq(&ctx->stale.lock);
 		GEM_BUG_ON(i915_sw_fence_signaled(&pos->fence));
@@ -1432,7 +1439,8 @@ static void engines_idle_release(struct i915_gem_context *ctx,
 
 kill:
 	if (list_empty(&engines->link)) /* raced, already closed */
-		kill_engines(engines, true);
+		kill_engines(engines, true,
+			     i915_gem_context_is_persistent(ctx));
 
 	i915_sw_fence_commit(&engines->fence);
 }
diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
index 5d0ec7c49b6a..27cd71c13097 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -576,6 +576,31 @@ void intel_context_bind_parent_child(struct intel_context *parent,
 	child->parallel.parent = parent;
 }
 
+bool intel_context_ban(struct intel_context *ce, struct i915_request *rq)
+{
+	bool ret = intel_context_set_banned(ce);
+
+	trace_intel_context_ban(ce);
+
+	if (ce->ops->revoke)
+		ce->ops->revoke(ce, rq,
+				INTEL_CONTEXT_BANNED_PREEMPT_TIMEOUT_MS);
+
+	return ret;
+}
+
+bool intel_context_exit_nonpersistent(struct intel_context *ce,
+				      struct i915_request *rq)
+{
+	bool ret = intel_context_set_exiting(ce);
+
+	if (ce->ops->revoke)
+		ce->ops->revoke(ce, rq,
+				INTEL_CONTEXT_EXITING_PREEMPT_TIMEOUT_MS);
+
+	return ret;
+}
+
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
 #include "selftest_context.c"
 #endif
diff --git a/drivers/gpu/drm/i915/gt/intel_context.h b/drivers/gpu/drm/i915/gt/intel_context.h
index d8c74bbf9aae..11c95e9d76ab 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.h
+++ b/drivers/gpu/drm/i915/gt/intel_context.h
@@ -25,6 +25,9 @@
 		     ##__VA_ARGS__);					\
 } while (0)
 
+#define INTEL_CONTEXT_EXITING_PREEMPT_TIMEOUT_MS (20)
+#define INTEL_CONTEXT_BANNED_PREEMPT_TIMEOUT_MS  (1)
+
 struct i915_gem_ww_ctx;
 
 void intel_context_init(struct intel_context *ce,
@@ -309,18 +312,27 @@ static inline bool intel_context_set_banned(struct intel_context *ce)
 	return test_and_set_bit(CONTEXT_BANNED, &ce->flags);
 }
 
-static inline bool intel_context_ban(struct intel_context *ce,
-				     struct i915_request *rq)
+bool intel_context_ban(struct intel_context *ce, struct i915_request *rq);
+
+static inline bool intel_context_is_schedulable(const struct intel_context *ce)
 {
-	bool ret = intel_context_set_banned(ce);
+	return !test_bit(CONTEXT_EXITING, &ce->flags) &&
+	       !test_bit(CONTEXT_BANNED, &ce->flags);
+}
 
-	trace_intel_context_ban(ce);
-	if (ce->ops->ban)
-		ce->ops->ban(ce, rq);
+static inline bool intel_context_is_exiting(const struct intel_context *ce)
+{
+	return test_bit(CONTEXT_EXITING, &ce->flags);
+}
 
-	return ret;
+static inline bool intel_context_set_exiting(struct intel_context *ce)
+{
+	return test_and_set_bit(CONTEXT_EXITING, &ce->flags);
 }
 
+bool intel_context_exit_nonpersistent(struct intel_context *ce,
+				      struct i915_request *rq);
+
 static inline bool
 intel_context_force_single_submission(const struct intel_context *ce)
 {
diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
index 30cd81ad8911..34073430cf8a 100644
--- a/drivers/gpu/drm/i915/gt/intel_context_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
@@ -37,7 +37,8 @@ struct intel_context_ops {
 
 	int (*alloc)(struct intel_context *ce);
 
-	void (*ban)(struct intel_context *ce, struct i915_request *rq);
+	void (*revoke)(struct intel_context *ce, struct i915_request *rq,
+		       unsigned int preempt_timeout_ms);
 
 	int (*pre_pin)(struct intel_context *ce, struct i915_gem_ww_ctx *ww, void **vaddr);
 	int (*pin)(struct intel_context *ce, void *vaddr);
@@ -119,6 +120,7 @@ struct intel_context {
 #define CONTEXT_GUC_INIT		10
 #define CONTEXT_PERMA_PIN		11
 #define CONTEXT_IS_PARKING		12
+#define CONTEXT_EXITING			13
 
 	struct {
 		u64 timeout_us;
diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
index 961d795220a3..953d1e7453c6 100644
--- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
@@ -481,7 +481,7 @@ __execlists_schedule_in(struct i915_request *rq)
 		     !intel_engine_has_heartbeat(engine)))
 		intel_context_set_banned(ce);
 
-	if (unlikely(intel_context_is_banned(ce) || bad_request(rq)))
+	if (unlikely(!intel_context_is_schedulable(ce) || bad_request(rq)))
 		reset_active(rq, engine);
 
 	if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
@@ -1225,12 +1225,19 @@ static void record_preemption(struct intel_engine_execlists *execlists)
 static unsigned long active_preempt_timeout(struct intel_engine_cs *engine,
 					    const struct i915_request *rq)
 {
+	struct intel_context *ce;
+
 	if (!rq)
 		return 0;
 
+	ce = rq->context;
+
 	/* Force a fast reset for terminated contexts (ignoring sysfs!) */
-	if (unlikely(intel_context_is_banned(rq->context) || bad_request(rq)))
-		return 1;
+	if (unlikely(intel_context_is_banned(ce) || bad_request(rq)))
+		return INTEL_CONTEXT_BANNED_PREEMPT_TIMEOUT_MS;
+	/* Longer grace for closed non-persistent contexts to avoid resets. */
+	else if (unlikely(intel_context_is_exiting(ce)))
+		return INTEL_CONTEXT_EXITING_PREEMPT_TIMEOUT_MS;
 
 	return READ_ONCE(engine->props.preempt_timeout_ms);
 }
diff --git a/drivers/gpu/drm/i915/gt/intel_ring_submission.c b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
index 6d7ec3bf1f32..a3dcc227a702 100644
--- a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
@@ -596,8 +596,9 @@ static void ring_context_reset(struct intel_context *ce)
 	clear_bit(CONTEXT_VALID_BIT, &ce->flags);
 }
 
-static void ring_context_ban(struct intel_context *ce,
-			     struct i915_request *rq)
+static void ring_context_revoke(struct intel_context *ce,
+				struct i915_request *rq,
+				unsigned int preempt_timeout_ms)
 {
 	struct intel_engine_cs *engine;
 
@@ -632,7 +633,7 @@ static const struct intel_context_ops ring_context_ops = {
 
 	.cancel_request = ring_context_cancel_request,
 
-	.ban = ring_context_ban,
+	.revoke = ring_context_revoke,
 
 	.pre_pin = ring_context_pre_pin,
 	.pin = ring_context_pin,
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index b3a429a92c0d..23fdbc7b34ab 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -2602,7 +2602,9 @@ static void __guc_context_set_preemption_timeout(struct intel_guc *guc,
 	intel_guc_send_busy_loop(guc, action, ARRAY_SIZE(action), 0, true);
 }
 
-static void guc_context_ban(struct intel_context *ce, struct i915_request *rq)
+static void
+guc_context_revoke(struct intel_context *ce, struct i915_request *rq,
+		   unsigned int preempt_timeout_ms)
 {
 	struct intel_guc *guc = ce_to_guc(ce);
 	struct intel_runtime_pm *runtime_pm =
@@ -2641,7 +2643,8 @@ static void guc_context_ban(struct intel_context *ce, struct i915_request *rq)
 		 * gets kicked off the HW ASAP.
 		 */
 		with_intel_runtime_pm(runtime_pm, wakeref) {
-			__guc_context_set_preemption_timeout(guc, guc_id, 1);
+			__guc_context_set_preemption_timeout(guc, guc_id,
+							     preempt_timeout_ms);
 			__guc_context_sched_disable(guc, ce, guc_id);
 		}
 	} else {
@@ -2649,7 +2652,7 @@ static void guc_context_ban(struct intel_context *ce, struct i915_request *rq)
 			with_intel_runtime_pm(runtime_pm, wakeref)
 				__guc_context_set_preemption_timeout(guc,
 								     ce->guc_id.id,
-								     1);
+								     preempt_timeout_ms);
 		spin_unlock_irqrestore(&ce->guc_state.lock, flags);
 	}
 }
@@ -2998,7 +3001,7 @@ static const struct intel_context_ops guc_context_ops = {
 	.unpin = guc_context_unpin,
 	.post_unpin = guc_context_post_unpin,
 
-	.ban = guc_context_ban,
+	.revoke = guc_context_revoke,
 
 	.cancel_request = guc_context_cancel_request,
 
@@ -3247,7 +3250,7 @@ static const struct intel_context_ops virtual_guc_context_ops = {
 	.unpin = guc_virtual_context_unpin,
 	.post_unpin = guc_context_post_unpin,
 
-	.ban = guc_context_ban,
+	.revoke = guc_context_revoke,
 
 	.cancel_request = guc_context_cancel_request,
 
@@ -3336,7 +3339,7 @@ static const struct intel_context_ops virtual_parent_context_ops = {
 	.unpin = guc_parent_context_unpin,
 	.post_unpin = guc_context_post_unpin,
 
-	.ban = guc_context_ban,
+	.revoke = guc_context_revoke,
 
 	.cancel_request = guc_context_cancel_request,
 
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 582770360ad1..13c555d190a0 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -611,7 +611,7 @@ bool __i915_request_submit(struct i915_request *request)
 		goto active;
 	}
 
-	if (unlikely(intel_context_is_banned(request->context)))
+	if (unlikely(!intel_context_is_schedulable(request->context)))
 		i915_request_set_error_once(request, -EIO);
 
 	if (unlikely(fatal_error(request->fence.error)))
-- 
2.32.0


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

* Re: [Intel-gfx] [PATCH] drm/i915: Be more gentle when exiting non-persistent contexts
  2021-08-26 10:52 Tvrtko Ursulin
@ 2021-08-26 13:32 ` Daniel Vetter
  0 siblings, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2021-08-26 13:32 UTC (permalink / raw)
  To: Tvrtko Ursulin
  Cc: Intel-gfx, dri-devel, Tvrtko Ursulin, Chris Wilson, Zhen Han,
	Matthew Brost

On Thu, Aug 26, 2021 at 11:52:14AM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> When a non-persistent context exits we currently mark it as banned in
> order to trigger fast termination of any outstanding GPU jobs it may have
> left running.
> 
> In doing so we apply a very strict 1ms limit in which the left over job
> has to preempt before we issues an engine resets.
> 
> Some workloads are not able to cleanly preempt in that time window and it
> can be argued that it would instead be better to give them a bit more
> grace since avoiding engine resets is generally preferrable.
> 
> To achieve this the patch splits handling of banned contexts from simply
> closed non-persistent ones and then applies different timeouts for both
> and also extends the criteria which determines if a request should be
> scheduled back in after preemption or not.
> 
> 20ms preempt timeout grace is given to exited non-persistent contexts
> which have been empirically tested to satisfy customers requirements
> and still provides reasonably quick cleanup post exit.
> 
> v2:
>  * Streamline fast path checks.
> 
> v3:
>  * Simplify by using only schedulable status.
>  * Increase timeout to 20ms.
> 
> v4:
>  * Fix live_execlists selftest.
> 
> v5:
>  * Fix logic in kill_engines.
> 
> v6:
>  * Rebase.
> 
> v7:
>  * Add GuC support.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Zhen Han <zhen.han@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_context.c   | 22 +++++++++++-----
>  drivers/gpu/drm/i915/gt/intel_context.c       | 25 ++++++++++++++++++
>  drivers/gpu/drm/i915/gt/intel_context.h       | 26 ++++++++++++++-----
>  drivers/gpu/drm/i915/gt/intel_context_types.h |  3 ++-
>  .../drm/i915/gt/intel_execlists_submission.c  | 13 +++++++---
>  .../gpu/drm/i915/gt/intel_ring_submission.c   |  7 ++---
>  .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 13 ++++++----
>  drivers/gpu/drm/i915/i915_request.c           |  2 +-
>  8 files changed, 84 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index fd169cf2f75a..6ae803cb4de3 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -1072,7 +1072,8 @@ static struct intel_engine_cs *active_engine(struct intel_context *ce)
>  	return engine;
>  }
>  
> -static void kill_engines(struct i915_gem_engines *engines, bool ban)
> +static void
> +kill_engines(struct i915_gem_engines *engines, bool ban, bool persistent)
>  {
>  	struct i915_gem_engines_iter it;
>  	struct intel_context *ce;
> @@ -1086,8 +1087,15 @@ static void kill_engines(struct i915_gem_engines *engines, bool ban)
>  	 */
>  	for_each_gem_engine(ce, engines, it) {
>  		struct intel_engine_cs *engine;
> +		bool skip = false;
>  
> -		if (ban && intel_context_ban(ce, NULL))
> +		if (ban)
> +			skip = intel_context_ban(ce, NULL);
> +		else if (!persistent)
> +			skip = intel_context_exit_nonpersistent(ce, NULL);
> +
> +		/* Already banned or non-persistent closed. */
> +		if (skip)
>  			continue;
>  
>  		/*
> @@ -1100,7 +1108,7 @@ static void kill_engines(struct i915_gem_engines *engines, bool ban)
>  		engine = active_engine(ce);
>  
>  		/* First attempt to gracefully cancel the context */
> -		if (engine && !__cancel_engine(engine) && ban)
> +		if (engine && !__cancel_engine(engine) && (ban || !persistent))
>  			/*
>  			 * If we are unable to send a preemptive pulse to bump
>  			 * the context from the GPU, we have to resort to a full
> @@ -1112,8 +1120,6 @@ static void kill_engines(struct i915_gem_engines *engines, bool ban)
>  
>  static void kill_context(struct i915_gem_context *ctx)
>  {
> -	bool ban = (!i915_gem_context_is_persistent(ctx) ||
> -		    !ctx->i915->params.enable_hangcheck);
>  	struct i915_gem_engines *pos, *next;
>  
>  	spin_lock_irq(&ctx->stale.lock);
> @@ -1126,7 +1132,8 @@ static void kill_context(struct i915_gem_context *ctx)
>  
>  		spin_unlock_irq(&ctx->stale.lock);
>  
> -		kill_engines(pos, ban);
> +		kill_engines(pos, !ctx->i915->params.enable_hangcheck,
> +			     i915_gem_context_is_persistent(ctx));
>  
>  		spin_lock_irq(&ctx->stale.lock);
>  		GEM_BUG_ON(i915_sw_fence_signaled(&pos->fence));
> @@ -1172,7 +1179,8 @@ static void engines_idle_release(struct i915_gem_context *ctx,
>  
>  kill:
>  	if (list_empty(&engines->link)) /* raced, already closed */
> -		kill_engines(engines, true);
> +		kill_engines(engines, true,
> +			     i915_gem_context_is_persistent(ctx));
>  
>  	i915_sw_fence_commit(&engines->fence);
>  }
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
> index 745e84c72c90..b9880ffe5da7 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.c
> +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> @@ -533,6 +533,31 @@ struct i915_request *intel_context_find_active_request(struct intel_context *ce)
>  	return active;
>  }
>  
> +bool intel_context_ban(struct intel_context *ce, struct i915_request *rq)
> +{
> +	bool ret = intel_context_set_banned(ce);
> +
> +	trace_intel_context_ban(ce);
> +
> +	if (ce->ops->revoke)
> +		ce->ops->revoke(ce, rq,
> +				INTEL_CONTEXT_BANNED_PREEMPT_TIMEOUT_MS);
> +
> +	return ret;
> +}
> +
> +bool intel_context_exit_nonpersistent(struct intel_context *ce,
> +				      struct i915_request *rq)
> +{
> +	bool ret = intel_context_set_exiting(ce);
> +
> +	if (ce->ops->revoke)
> +		ce->ops->revoke(ce, rq,
> +				INTEL_CONTEXT_EXITING_PREEMPT_TIMEOUT_MS);
> +
> +	return ret;
> +}
> +
>  #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
>  #include "selftest_context.c"
>  #endif
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.h b/drivers/gpu/drm/i915/gt/intel_context.h
> index c41098950746..8b77f3e7449a 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context.h
> @@ -25,6 +25,9 @@
>  		     ##__VA_ARGS__);					\
>  } while (0)
>  
> +#define INTEL_CONTEXT_EXITING_PREEMPT_TIMEOUT_MS (20)
> +#define INTEL_CONTEXT_BANNED_PREEMPT_TIMEOUT_MS  (1)
> +
>  struct i915_gem_ww_ctx;
>  
>  void intel_context_init(struct intel_context *ce,
> @@ -254,18 +257,27 @@ static inline bool intel_context_set_banned(struct intel_context *ce)
>  	return test_and_set_bit(CONTEXT_BANNED, &ce->flags);
>  }
>  
> -static inline bool intel_context_ban(struct intel_context *ce,
> -				     struct i915_request *rq)
> +bool intel_context_ban(struct intel_context *ce, struct i915_request *rq);
> +
> +static inline bool intel_context_is_schedulable(const struct intel_context *ce)
>  {
> -	bool ret = intel_context_set_banned(ce);
> +	return !test_bit(CONTEXT_EXITING, &ce->flags) &&
> +	       !test_bit(CONTEXT_BANNED, &ce->flags);
> +}
>  
> -	trace_intel_context_ban(ce);
> -	if (ce->ops->ban)
> -		ce->ops->ban(ce, rq);
> +static inline bool intel_context_is_exiting(const struct intel_context *ce)
> +{
> +	return test_bit(CONTEXT_EXITING, &ce->flags);
> +}
>  
> -	return ret;
> +static inline bool intel_context_set_exiting(struct intel_context *ce)
> +{
> +	return test_and_set_bit(CONTEXT_EXITING, &ce->flags);

Uh more unordered atomics without barriers and without comments explaining
the barriers and pointing at the counter-party barrier. This needs to be
fixed.

In general it would be really good if all the context status bits are at
least properly documented. As-is this is an extremely challenging maze,
and I'm not sure it's really correct in all cases.
-Daniel

>  }
>  
> +bool intel_context_exit_nonpersistent(struct intel_context *ce,
> +				      struct i915_request *rq);
> +
>  static inline bool
>  intel_context_force_single_submission(const struct intel_context *ce)
>  {
> diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
> index e54351a170e2..4a0a1e736972 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
> @@ -37,7 +37,7 @@ struct intel_context_ops {
>  
>  	int (*alloc)(struct intel_context *ce);
>  
> -	void (*ban)(struct intel_context *ce, struct i915_request *rq);
> +	void (*revoke)(struct intel_context *ce, struct i915_request *rq, unsigned int preempt_timeout_ms);
>  
>  	int (*pre_pin)(struct intel_context *ce, struct i915_gem_ww_ctx *ww, void **vaddr);
>  	int (*pin)(struct intel_context *ce, void *vaddr);
> @@ -112,6 +112,7 @@ struct intel_context {
>  #define CONTEXT_FORCE_SINGLE_SUBMISSION	7
>  #define CONTEXT_NOPREEMPT		8
>  #define CONTEXT_LRCA_DIRTY		9
> +#define CONTEXT_EXITING			10
>  
>  	struct {
>  		u64 timeout_us;
> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> index de5f9c86b9a4..43823b9d4013 100644
> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> @@ -478,7 +478,7 @@ __execlists_schedule_in(struct i915_request *rq)
>  		     !intel_engine_has_heartbeat(engine)))
>  		intel_context_set_banned(ce);
>  
> -	if (unlikely(intel_context_is_banned(ce) || bad_request(rq)))
> +	if (unlikely(!intel_context_is_schedulable(ce) || bad_request(rq)))
>  		reset_active(rq, engine);
>  
>  	if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
> @@ -1222,12 +1222,19 @@ static void record_preemption(struct intel_engine_execlists *execlists)
>  static unsigned long active_preempt_timeout(struct intel_engine_cs *engine,
>  					    const struct i915_request *rq)
>  {
> +	struct intel_context *ce;
> +
>  	if (!rq)
>  		return 0;
>  
> +	ce = rq->context;
> +
>  	/* Force a fast reset for terminated contexts (ignoring sysfs!) */
> -	if (unlikely(intel_context_is_banned(rq->context) || bad_request(rq)))
> -		return 1;
> +	if (unlikely(intel_context_is_banned(ce) || bad_request(rq)))
> +		return INTEL_CONTEXT_BANNED_PREEMPT_TIMEOUT_MS;
> +	/* Longer grace for closed non-persistent contexts to avoid resets. */
> +	else if (unlikely(intel_context_is_exiting(ce)))
> +		return INTEL_CONTEXT_EXITING_PREEMPT_TIMEOUT_MS;
>  
>  	return READ_ONCE(engine->props.preempt_timeout_ms);
>  }
> diff --git a/drivers/gpu/drm/i915/gt/intel_ring_submission.c b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> index 3c65efcb7bed..a28862bbe51e 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> @@ -587,8 +587,9 @@ static void ring_context_reset(struct intel_context *ce)
>  	clear_bit(CONTEXT_VALID_BIT, &ce->flags);
>  }
>  
> -static void ring_context_ban(struct intel_context *ce,
> -			     struct i915_request *rq)
> +static void ring_context_revoke(struct intel_context *ce,
> +				struct i915_request *rq,
> +				unsigned int preempt_timeout_ms)
>  {
>  	struct intel_engine_cs *engine;
>  
> @@ -623,7 +624,7 @@ static const struct intel_context_ops ring_context_ops = {
>  
>  	.cancel_request = ring_context_cancel_request,
>  
> -	.ban = ring_context_ban,
> +	.revoke = ring_context_revoke,
>  
>  	.pre_pin = ring_context_pre_pin,
>  	.pin = ring_context_pin,
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> index 87d8dc8f51b9..b0a8629b26e3 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -1618,7 +1618,9 @@ static void __guc_context_set_preemption_timeout(struct intel_guc *guc,
>  	intel_guc_send_busy_loop(guc, action, ARRAY_SIZE(action), 0, true);
>  }
>  
> -static void guc_context_ban(struct intel_context *ce, struct i915_request *rq)
> +static void
> +guc_context_revoke(struct intel_context *ce, struct i915_request *rq,
> +		   unsigned int preempt_timeout_ms)
>  {
>  	struct intel_guc *guc = ce_to_guc(ce);
>  	struct intel_runtime_pm *runtime_pm =
> @@ -1655,7 +1657,8 @@ static void guc_context_ban(struct intel_context *ce, struct i915_request *rq)
>  		 * gets kicked off the HW ASAP.
>  		 */
>  		with_intel_runtime_pm(runtime_pm, wakeref) {
> -			__guc_context_set_preemption_timeout(guc, guc_id, 1);
> +			__guc_context_set_preemption_timeout(guc, guc_id,
> +							     preempt_timeout_ms);
>  			__guc_context_sched_disable(guc, ce, guc_id);
>  		}
>  	} else {
> @@ -1663,7 +1666,7 @@ static void guc_context_ban(struct intel_context *ce, struct i915_request *rq)
>  			with_intel_runtime_pm(runtime_pm, wakeref)
>  				__guc_context_set_preemption_timeout(guc,
>  								     ce->guc_id,
> -								     1);
> +								     preempt_timeout_ms);
>  		spin_unlock_irqrestore(&ce->guc_state.lock, flags);
>  	}
>  }
> @@ -1979,7 +1982,7 @@ static const struct intel_context_ops guc_context_ops = {
>  	.unpin = guc_context_unpin,
>  	.post_unpin = guc_context_post_unpin,
>  
> -	.ban = guc_context_ban,
> +	.revoke = guc_context_revoke,
>  
>  	.cancel_request = guc_context_cancel_request,
>  
> @@ -2176,7 +2179,7 @@ static const struct intel_context_ops virtual_guc_context_ops = {
>  	.unpin = guc_context_unpin,
>  	.post_unpin = guc_context_post_unpin,
>  
> -	.ban = guc_context_ban,
> +	.revoke = guc_context_revoke,
>  
>  	.cancel_request = guc_context_cancel_request,
>  
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index ce446716d092..b1a9bec83339 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -583,7 +583,7 @@ bool __i915_request_submit(struct i915_request *request)
>  		goto active;
>  	}
>  
> -	if (unlikely(intel_context_is_banned(request->context)))
> +	if (unlikely(!intel_context_is_schedulable(request->context)))
>  		i915_request_set_error_once(request, -EIO);
>  
>  	if (unlikely(fatal_error(request->fence.error)))
> -- 
> 2.30.2
> 

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

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

* [Intel-gfx] [PATCH] drm/i915: Be more gentle when exiting non-persistent contexts
@ 2021-08-26 10:52 Tvrtko Ursulin
  2021-08-26 13:32 ` Daniel Vetter
  0 siblings, 1 reply; 9+ messages in thread
From: Tvrtko Ursulin @ 2021-08-26 10:52 UTC (permalink / raw)
  To: Intel-gfx
  Cc: dri-devel, Tvrtko Ursulin, Chris Wilson, Zhen Han, Matthew Brost

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

When a non-persistent context exits we currently mark it as banned in
order to trigger fast termination of any outstanding GPU jobs it may have
left running.

In doing so we apply a very strict 1ms limit in which the left over job
has to preempt before we issues an engine resets.

Some workloads are not able to cleanly preempt in that time window and it
can be argued that it would instead be better to give them a bit more
grace since avoiding engine resets is generally preferrable.

To achieve this the patch splits handling of banned contexts from simply
closed non-persistent ones and then applies different timeouts for both
and also extends the criteria which determines if a request should be
scheduled back in after preemption or not.

20ms preempt timeout grace is given to exited non-persistent contexts
which have been empirically tested to satisfy customers requirements
and still provides reasonably quick cleanup post exit.

v2:
 * Streamline fast path checks.

v3:
 * Simplify by using only schedulable status.
 * Increase timeout to 20ms.

v4:
 * Fix live_execlists selftest.

v5:
 * Fix logic in kill_engines.

v6:
 * Rebase.

v7:
 * Add GuC support.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Zhen Han <zhen.han@intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_context.c   | 22 +++++++++++-----
 drivers/gpu/drm/i915/gt/intel_context.c       | 25 ++++++++++++++++++
 drivers/gpu/drm/i915/gt/intel_context.h       | 26 ++++++++++++++-----
 drivers/gpu/drm/i915/gt/intel_context_types.h |  3 ++-
 .../drm/i915/gt/intel_execlists_submission.c  | 13 +++++++---
 .../gpu/drm/i915/gt/intel_ring_submission.c   |  7 ++---
 .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 13 ++++++----
 drivers/gpu/drm/i915/i915_request.c           |  2 +-
 8 files changed, 84 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index fd169cf2f75a..6ae803cb4de3 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -1072,7 +1072,8 @@ static struct intel_engine_cs *active_engine(struct intel_context *ce)
 	return engine;
 }
 
-static void kill_engines(struct i915_gem_engines *engines, bool ban)
+static void
+kill_engines(struct i915_gem_engines *engines, bool ban, bool persistent)
 {
 	struct i915_gem_engines_iter it;
 	struct intel_context *ce;
@@ -1086,8 +1087,15 @@ static void kill_engines(struct i915_gem_engines *engines, bool ban)
 	 */
 	for_each_gem_engine(ce, engines, it) {
 		struct intel_engine_cs *engine;
+		bool skip = false;
 
-		if (ban && intel_context_ban(ce, NULL))
+		if (ban)
+			skip = intel_context_ban(ce, NULL);
+		else if (!persistent)
+			skip = intel_context_exit_nonpersistent(ce, NULL);
+
+		/* Already banned or non-persistent closed. */
+		if (skip)
 			continue;
 
 		/*
@@ -1100,7 +1108,7 @@ static void kill_engines(struct i915_gem_engines *engines, bool ban)
 		engine = active_engine(ce);
 
 		/* First attempt to gracefully cancel the context */
-		if (engine && !__cancel_engine(engine) && ban)
+		if (engine && !__cancel_engine(engine) && (ban || !persistent))
 			/*
 			 * If we are unable to send a preemptive pulse to bump
 			 * the context from the GPU, we have to resort to a full
@@ -1112,8 +1120,6 @@ static void kill_engines(struct i915_gem_engines *engines, bool ban)
 
 static void kill_context(struct i915_gem_context *ctx)
 {
-	bool ban = (!i915_gem_context_is_persistent(ctx) ||
-		    !ctx->i915->params.enable_hangcheck);
 	struct i915_gem_engines *pos, *next;
 
 	spin_lock_irq(&ctx->stale.lock);
@@ -1126,7 +1132,8 @@ static void kill_context(struct i915_gem_context *ctx)
 
 		spin_unlock_irq(&ctx->stale.lock);
 
-		kill_engines(pos, ban);
+		kill_engines(pos, !ctx->i915->params.enable_hangcheck,
+			     i915_gem_context_is_persistent(ctx));
 
 		spin_lock_irq(&ctx->stale.lock);
 		GEM_BUG_ON(i915_sw_fence_signaled(&pos->fence));
@@ -1172,7 +1179,8 @@ static void engines_idle_release(struct i915_gem_context *ctx,
 
 kill:
 	if (list_empty(&engines->link)) /* raced, already closed */
-		kill_engines(engines, true);
+		kill_engines(engines, true,
+			     i915_gem_context_is_persistent(ctx));
 
 	i915_sw_fence_commit(&engines->fence);
 }
diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
index 745e84c72c90..b9880ffe5da7 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -533,6 +533,31 @@ struct i915_request *intel_context_find_active_request(struct intel_context *ce)
 	return active;
 }
 
+bool intel_context_ban(struct intel_context *ce, struct i915_request *rq)
+{
+	bool ret = intel_context_set_banned(ce);
+
+	trace_intel_context_ban(ce);
+
+	if (ce->ops->revoke)
+		ce->ops->revoke(ce, rq,
+				INTEL_CONTEXT_BANNED_PREEMPT_TIMEOUT_MS);
+
+	return ret;
+}
+
+bool intel_context_exit_nonpersistent(struct intel_context *ce,
+				      struct i915_request *rq)
+{
+	bool ret = intel_context_set_exiting(ce);
+
+	if (ce->ops->revoke)
+		ce->ops->revoke(ce, rq,
+				INTEL_CONTEXT_EXITING_PREEMPT_TIMEOUT_MS);
+
+	return ret;
+}
+
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
 #include "selftest_context.c"
 #endif
diff --git a/drivers/gpu/drm/i915/gt/intel_context.h b/drivers/gpu/drm/i915/gt/intel_context.h
index c41098950746..8b77f3e7449a 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.h
+++ b/drivers/gpu/drm/i915/gt/intel_context.h
@@ -25,6 +25,9 @@
 		     ##__VA_ARGS__);					\
 } while (0)
 
+#define INTEL_CONTEXT_EXITING_PREEMPT_TIMEOUT_MS (20)
+#define INTEL_CONTEXT_BANNED_PREEMPT_TIMEOUT_MS  (1)
+
 struct i915_gem_ww_ctx;
 
 void intel_context_init(struct intel_context *ce,
@@ -254,18 +257,27 @@ static inline bool intel_context_set_banned(struct intel_context *ce)
 	return test_and_set_bit(CONTEXT_BANNED, &ce->flags);
 }
 
-static inline bool intel_context_ban(struct intel_context *ce,
-				     struct i915_request *rq)
+bool intel_context_ban(struct intel_context *ce, struct i915_request *rq);
+
+static inline bool intel_context_is_schedulable(const struct intel_context *ce)
 {
-	bool ret = intel_context_set_banned(ce);
+	return !test_bit(CONTEXT_EXITING, &ce->flags) &&
+	       !test_bit(CONTEXT_BANNED, &ce->flags);
+}
 
-	trace_intel_context_ban(ce);
-	if (ce->ops->ban)
-		ce->ops->ban(ce, rq);
+static inline bool intel_context_is_exiting(const struct intel_context *ce)
+{
+	return test_bit(CONTEXT_EXITING, &ce->flags);
+}
 
-	return ret;
+static inline bool intel_context_set_exiting(struct intel_context *ce)
+{
+	return test_and_set_bit(CONTEXT_EXITING, &ce->flags);
 }
 
+bool intel_context_exit_nonpersistent(struct intel_context *ce,
+				      struct i915_request *rq);
+
 static inline bool
 intel_context_force_single_submission(const struct intel_context *ce)
 {
diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
index e54351a170e2..4a0a1e736972 100644
--- a/drivers/gpu/drm/i915/gt/intel_context_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
@@ -37,7 +37,7 @@ struct intel_context_ops {
 
 	int (*alloc)(struct intel_context *ce);
 
-	void (*ban)(struct intel_context *ce, struct i915_request *rq);
+	void (*revoke)(struct intel_context *ce, struct i915_request *rq, unsigned int preempt_timeout_ms);
 
 	int (*pre_pin)(struct intel_context *ce, struct i915_gem_ww_ctx *ww, void **vaddr);
 	int (*pin)(struct intel_context *ce, void *vaddr);
@@ -112,6 +112,7 @@ struct intel_context {
 #define CONTEXT_FORCE_SINGLE_SUBMISSION	7
 #define CONTEXT_NOPREEMPT		8
 #define CONTEXT_LRCA_DIRTY		9
+#define CONTEXT_EXITING			10
 
 	struct {
 		u64 timeout_us;
diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
index de5f9c86b9a4..43823b9d4013 100644
--- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
@@ -478,7 +478,7 @@ __execlists_schedule_in(struct i915_request *rq)
 		     !intel_engine_has_heartbeat(engine)))
 		intel_context_set_banned(ce);
 
-	if (unlikely(intel_context_is_banned(ce) || bad_request(rq)))
+	if (unlikely(!intel_context_is_schedulable(ce) || bad_request(rq)))
 		reset_active(rq, engine);
 
 	if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
@@ -1222,12 +1222,19 @@ static void record_preemption(struct intel_engine_execlists *execlists)
 static unsigned long active_preempt_timeout(struct intel_engine_cs *engine,
 					    const struct i915_request *rq)
 {
+	struct intel_context *ce;
+
 	if (!rq)
 		return 0;
 
+	ce = rq->context;
+
 	/* Force a fast reset for terminated contexts (ignoring sysfs!) */
-	if (unlikely(intel_context_is_banned(rq->context) || bad_request(rq)))
-		return 1;
+	if (unlikely(intel_context_is_banned(ce) || bad_request(rq)))
+		return INTEL_CONTEXT_BANNED_PREEMPT_TIMEOUT_MS;
+	/* Longer grace for closed non-persistent contexts to avoid resets. */
+	else if (unlikely(intel_context_is_exiting(ce)))
+		return INTEL_CONTEXT_EXITING_PREEMPT_TIMEOUT_MS;
 
 	return READ_ONCE(engine->props.preempt_timeout_ms);
 }
diff --git a/drivers/gpu/drm/i915/gt/intel_ring_submission.c b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
index 3c65efcb7bed..a28862bbe51e 100644
--- a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
@@ -587,8 +587,9 @@ static void ring_context_reset(struct intel_context *ce)
 	clear_bit(CONTEXT_VALID_BIT, &ce->flags);
 }
 
-static void ring_context_ban(struct intel_context *ce,
-			     struct i915_request *rq)
+static void ring_context_revoke(struct intel_context *ce,
+				struct i915_request *rq,
+				unsigned int preempt_timeout_ms)
 {
 	struct intel_engine_cs *engine;
 
@@ -623,7 +624,7 @@ static const struct intel_context_ops ring_context_ops = {
 
 	.cancel_request = ring_context_cancel_request,
 
-	.ban = ring_context_ban,
+	.revoke = ring_context_revoke,
 
 	.pre_pin = ring_context_pre_pin,
 	.pin = ring_context_pin,
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index 87d8dc8f51b9..b0a8629b26e3 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -1618,7 +1618,9 @@ static void __guc_context_set_preemption_timeout(struct intel_guc *guc,
 	intel_guc_send_busy_loop(guc, action, ARRAY_SIZE(action), 0, true);
 }
 
-static void guc_context_ban(struct intel_context *ce, struct i915_request *rq)
+static void
+guc_context_revoke(struct intel_context *ce, struct i915_request *rq,
+		   unsigned int preempt_timeout_ms)
 {
 	struct intel_guc *guc = ce_to_guc(ce);
 	struct intel_runtime_pm *runtime_pm =
@@ -1655,7 +1657,8 @@ static void guc_context_ban(struct intel_context *ce, struct i915_request *rq)
 		 * gets kicked off the HW ASAP.
 		 */
 		with_intel_runtime_pm(runtime_pm, wakeref) {
-			__guc_context_set_preemption_timeout(guc, guc_id, 1);
+			__guc_context_set_preemption_timeout(guc, guc_id,
+							     preempt_timeout_ms);
 			__guc_context_sched_disable(guc, ce, guc_id);
 		}
 	} else {
@@ -1663,7 +1666,7 @@ static void guc_context_ban(struct intel_context *ce, struct i915_request *rq)
 			with_intel_runtime_pm(runtime_pm, wakeref)
 				__guc_context_set_preemption_timeout(guc,
 								     ce->guc_id,
-								     1);
+								     preempt_timeout_ms);
 		spin_unlock_irqrestore(&ce->guc_state.lock, flags);
 	}
 }
@@ -1979,7 +1982,7 @@ static const struct intel_context_ops guc_context_ops = {
 	.unpin = guc_context_unpin,
 	.post_unpin = guc_context_post_unpin,
 
-	.ban = guc_context_ban,
+	.revoke = guc_context_revoke,
 
 	.cancel_request = guc_context_cancel_request,
 
@@ -2176,7 +2179,7 @@ static const struct intel_context_ops virtual_guc_context_ops = {
 	.unpin = guc_context_unpin,
 	.post_unpin = guc_context_post_unpin,
 
-	.ban = guc_context_ban,
+	.revoke = guc_context_revoke,
 
 	.cancel_request = guc_context_cancel_request,
 
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index ce446716d092..b1a9bec83339 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -583,7 +583,7 @@ bool __i915_request_submit(struct i915_request *request)
 		goto active;
 	}
 
-	if (unlikely(intel_context_is_banned(request->context)))
+	if (unlikely(!intel_context_is_schedulable(request->context)))
 		i915_request_set_error_once(request, -EIO);
 
 	if (unlikely(fatal_error(request->fence.error)))
-- 
2.30.2


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

end of thread, other threads:[~2022-02-24 13:03 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-05 12:05 [Intel-gfx] [PATCH] drm/i915: Be more gentle when exiting non-persistent contexts Tvrtko Ursulin
2021-08-05 15:04 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for " Patchwork
2021-08-05 16:10   ` Tvrtko Ursulin
2021-08-05 18:24     ` Matthew Brost
2021-08-05 16:32 ` [Intel-gfx] [PATCH] " Matthew Brost
2021-08-06 10:44   ` Tvrtko Ursulin
2021-08-26 10:52 Tvrtko Ursulin
2021-08-26 13:32 ` Daniel Vetter
2022-02-24 13:03 Tvrtko Ursulin

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