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

Tvrtko Ursulin tvrtko.ursulin at linux.intel.com
Fri Aug 6 10:44:32 UTC 2021


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 at 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 at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Zhen Han <zhen.han at 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
>>


More information about the Intel-gfx mailing list