Re: [Intel-gfx] [PATCH] drm/i915: Be more gentle when exiting non-persistent contexts

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