dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drm/i915: Be more gentle with exiting non-persistent context
@ 2021-04-29  9:46 Tvrtko Ursulin
  2021-04-30 11:48 ` [Intel-gfx] " Daniel Vetter
  2021-04-30 14:10 ` [PATCH v2] " Tvrtko Ursulin
  0 siblings, 2 replies; 5+ messages in thread
From: Tvrtko Ursulin @ 2021-04-29  9:46 UTC (permalink / raw)
  To: Intel-gfx; +Cc: Zhen Han, Chris Wilson, dri-devel, Tvrtko Ursulin

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

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     | 15 +++++++++------
 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    | 12 ++++++++++--
 drivers/gpu/drm/i915/i915_request.c             |  2 +-
 5 files changed, 38 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index fd8ee52e17a4..5a6eba1232cd 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -426,7 +426,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;
@@ -443,6 +444,8 @@ static void kill_engines(struct i915_gem_engines *engines, bool ban)
 
 		if (ban && intel_context_set_banned(ce))
 			continue;
+		else if (!persistent && intel_context_set_non_persistent(ce))
+			continue;
 
 		/*
 		 * Check the current active state of this context; if we
@@ -454,7 +457,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
@@ -466,8 +469,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);
@@ -480,7 +481,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));
@@ -526,7 +528,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.h b/drivers/gpu/drm/i915/gt/intel_context.h
index f83a73a2b39f..b0e812b8ce39 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.h
+++ b/drivers/gpu/drm/i915/gt/intel_context.h
@@ -220,6 +220,23 @@ 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_is_non_persistent(const struct intel_context *ce)
+{
+	return test_bit(CONTEXT_NON_PERSISTENT, &ce->flags);
+}
+
+static inline bool intel_context_set_non_persistent(struct intel_context *ce)
+{
+	return test_and_set_bit(CONTEXT_NON_PERSISTENT, &ce->flags);
+}
+
+static inline bool intel_context_is_schedulable(const struct intel_context *ce)
+{
+	return !intel_context_is_banned(ce) &&
+	       !(intel_context_is_closed(ce) &&
+	         intel_context_is_non_persistent(ce));
+}
+
 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 ed8c447a7346..aa949c539e93 100644
--- a/drivers/gpu/drm/i915/gt/intel_context_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
@@ -95,6 +95,7 @@ struct intel_context {
 #define CONTEXT_BANNED			6
 #define CONTEXT_FORCE_SINGLE_SUBMISSION	7
 #define CONTEXT_NOPREEMPT		8
+#define CONTEXT_NON_PERSISTENT		9 /* Only if also 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 de124870af44..5ad7272fbbc4 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))
@@ -1204,12 +1204,20 @@ 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_closed(ce) &&
+			  intel_context_is_non_persistent(ce)))
+		return 15;
 
 	return READ_ONCE(engine->props.preempt_timeout_ms);
 }
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index bec9c3652188..bbac87535923 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -660,7 +660,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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* Re: [Intel-gfx] [PATCH] drm/i915: Be more gentle with exiting non-persistent context
  2021-04-29  9:46 [PATCH] drm/i915: Be more gentle with exiting non-persistent context Tvrtko Ursulin
@ 2021-04-30 11:48 ` Daniel Vetter
  2021-04-30 13:27   ` Tvrtko Ursulin
  2021-04-30 14:10 ` [PATCH v2] " Tvrtko Ursulin
  1 sibling, 1 reply; 5+ messages in thread
From: Daniel Vetter @ 2021-04-30 11:48 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: Intel-gfx, Chris Wilson, dri-devel, Zhen Han

On Thu, Apr 29, 2021 at 10:46:40AM +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.

Can you pls explain here why this is preferrable?

> To achieve this the patch splits handling of banned contexts from simply
> exited 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.

Same here, a bit more detail on what exactly was the problem to be fixed
is needed.
-Daniel

> 
> 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     | 15 +++++++++------
>  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    | 12 ++++++++++--
>  drivers/gpu/drm/i915/i915_request.c             |  2 +-
>  5 files changed, 38 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index fd8ee52e17a4..5a6eba1232cd 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -426,7 +426,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;
> @@ -443,6 +444,8 @@ static void kill_engines(struct i915_gem_engines *engines, bool ban)
>  
>  		if (ban && intel_context_set_banned(ce))
>  			continue;
> +		else if (!persistent && intel_context_set_non_persistent(ce))
> +			continue;
>  
>  		/*
>  		 * Check the current active state of this context; if we
> @@ -454,7 +457,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
> @@ -466,8 +469,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);
> @@ -480,7 +481,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));
> @@ -526,7 +528,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.h b/drivers/gpu/drm/i915/gt/intel_context.h
> index f83a73a2b39f..b0e812b8ce39 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context.h
> @@ -220,6 +220,23 @@ 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_is_non_persistent(const struct intel_context *ce)
> +{
> +	return test_bit(CONTEXT_NON_PERSISTENT, &ce->flags);
> +}
> +
> +static inline bool intel_context_set_non_persistent(struct intel_context *ce)
> +{
> +	return test_and_set_bit(CONTEXT_NON_PERSISTENT, &ce->flags);
> +}
> +
> +static inline bool intel_context_is_schedulable(const struct intel_context *ce)
> +{
> +	return !intel_context_is_banned(ce) &&
> +	       !(intel_context_is_closed(ce) &&
> +	         intel_context_is_non_persistent(ce));
> +}
> +
>  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 ed8c447a7346..aa949c539e93 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
> @@ -95,6 +95,7 @@ struct intel_context {
>  #define CONTEXT_BANNED			6
>  #define CONTEXT_FORCE_SINGLE_SUBMISSION	7
>  #define CONTEXT_NOPREEMPT		8
> +#define CONTEXT_NON_PERSISTENT		9 /* Only if also 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 de124870af44..5ad7272fbbc4 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))
> @@ -1204,12 +1204,20 @@ 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_closed(ce) &&
> +			  intel_context_is_non_persistent(ce)))
> +		return 15;
>  
>  	return READ_ONCE(engine->props.preempt_timeout_ms);
>  }
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index bec9c3652188..bbac87535923 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -660,7 +660,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
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* Re: [Intel-gfx] [PATCH] drm/i915: Be more gentle with exiting non-persistent context
  2021-04-30 11:48 ` [Intel-gfx] " Daniel Vetter
@ 2021-04-30 13:27   ` Tvrtko Ursulin
  2021-04-30 14:34     ` Daniel Vetter
  0 siblings, 1 reply; 5+ messages in thread
From: Tvrtko Ursulin @ 2021-04-30 13:27 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Intel-gfx, Chris Wilson, dri-devel, Zhen Han


On 30/04/2021 12:48, Daniel Vetter wrote:
> On Thu, Apr 29, 2021 at 10:46:40AM +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.
> 
> Can you pls explain here why this is preferrable?

I think there's always the risk of an innocent request getting axed with 
preempt to busy being very asynchronous and also engine reset can 
sometimes fail as well.

>> To achieve this the patch splits handling of banned contexts from simply
>> exited 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.
> 
> Same here, a bit more detail on what exactly was the problem to be fixed
> is needed.

It is a bit multi-faceted. Start with how in some cultures errors 
messages are much bigger error flags than in others and much more 
difficult to hand-wave "oh that's not a problem really". The the 
previous considerations about why not avoid engine reset if we can. Add 
on top how non-persistent context exiting is not really an error worthy 
of a reset, *if* it exits cleanly reasonably quickly.

You could say make it clean up for itself before it exits, not a kernel 
problem. But on the balance of everything, to me it sounds saleable to 
just give it some longer time compared to banned contexts, which are the 
unquestionably naughty/dangerous ones. Also, how fast non-persistent 
contexts will be cleaned up hasn't been defined really. As long as 15ms 
is an order of magnitude, plus some, shorter than the normal preempt 
timeout I think it is fine.

Regards,

Tvrtko

P.S. Otherwise I plan to respin v2 with consolidated CONTEXT_SCHEDULABLE 
flag so fast paths do not have to do too many individual checks.

> -Daniel
> 
>>
>> 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     | 15 +++++++++------
>>   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    | 12 ++++++++++--
>>   drivers/gpu/drm/i915/i915_request.c             |  2 +-
>>   5 files changed, 38 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>> index fd8ee52e17a4..5a6eba1232cd 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>> @@ -426,7 +426,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;
>> @@ -443,6 +444,8 @@ static void kill_engines(struct i915_gem_engines *engines, bool ban)
>>   
>>   		if (ban && intel_context_set_banned(ce))
>>   			continue;
>> +		else if (!persistent && intel_context_set_non_persistent(ce))
>> +			continue;
>>   
>>   		/*
>>   		 * Check the current active state of this context; if we
>> @@ -454,7 +457,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
>> @@ -466,8 +469,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);
>> @@ -480,7 +481,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));
>> @@ -526,7 +528,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.h b/drivers/gpu/drm/i915/gt/intel_context.h
>> index f83a73a2b39f..b0e812b8ce39 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_context.h
>> +++ b/drivers/gpu/drm/i915/gt/intel_context.h
>> @@ -220,6 +220,23 @@ 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_is_non_persistent(const struct intel_context *ce)
>> +{
>> +	return test_bit(CONTEXT_NON_PERSISTENT, &ce->flags);
>> +}
>> +
>> +static inline bool intel_context_set_non_persistent(struct intel_context *ce)
>> +{
>> +	return test_and_set_bit(CONTEXT_NON_PERSISTENT, &ce->flags);
>> +}
>> +
>> +static inline bool intel_context_is_schedulable(const struct intel_context *ce)
>> +{
>> +	return !intel_context_is_banned(ce) &&
>> +	       !(intel_context_is_closed(ce) &&
>> +	         intel_context_is_non_persistent(ce));
>> +}
>> +
>>   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 ed8c447a7346..aa949c539e93 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_context_types.h
>> +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
>> @@ -95,6 +95,7 @@ struct intel_context {
>>   #define CONTEXT_BANNED			6
>>   #define CONTEXT_FORCE_SINGLE_SUBMISSION	7
>>   #define CONTEXT_NOPREEMPT		8
>> +#define CONTEXT_NON_PERSISTENT		9 /* Only if also 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 de124870af44..5ad7272fbbc4 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))
>> @@ -1204,12 +1204,20 @@ 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_closed(ce) &&
>> +			  intel_context_is_non_persistent(ce)))
>> +		return 15;
>>   
>>   	return READ_ONCE(engine->props.preempt_timeout_ms);
>>   }
>> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
>> index bec9c3652188..bbac87535923 100644
>> --- a/drivers/gpu/drm/i915/i915_request.c
>> +++ b/drivers/gpu/drm/i915/i915_request.c
>> @@ -660,7 +660,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
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* [PATCH v2] drm/i915: Be more gentle with exiting non-persistent context
  2021-04-29  9:46 [PATCH] drm/i915: Be more gentle with exiting non-persistent context Tvrtko Ursulin
  2021-04-30 11:48 ` [Intel-gfx] " Daniel Vetter
@ 2021-04-30 14:10 ` Tvrtko Ursulin
  1 sibling, 0 replies; 5+ messages in thread
From: Tvrtko Ursulin @ 2021-04-30 14:10 UTC (permalink / raw)
  To: Intel-gfx; +Cc: Zhen Han, Chris Wilson, dri-devel, Tvrtko Ursulin

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

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   | 16 +++++++++------
 drivers/gpu/drm/i915/gt/intel_context.c       |  2 ++
 drivers/gpu/drm/i915/gt/intel_context.h       | 20 +++++++++++++++++++
 drivers/gpu/drm/i915/gt/intel_context_types.h |  2 ++
 .../drm/i915/gt/intel_execlists_submission.c  | 11 ++++++++--
 drivers/gpu/drm/i915/i915_request.c           |  2 +-
 6 files changed, 44 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index fd8ee52e17a4..090c891f029b 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -426,7 +426,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;
@@ -443,6 +444,9 @@ static void kill_engines(struct i915_gem_engines *engines, bool ban)
 
 		if (ban && intel_context_set_banned(ce))
 			continue;
+		else if (!persistent &&
+			 intel_context_set_closed_non_persistent(ce))
+			continue;
 
 		/*
 		 * Check the current active state of this context; if we
@@ -454,7 +458,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
@@ -466,8 +470,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);
@@ -480,7 +482,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));
@@ -526,7 +529,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 17cf2640b082..efbda677a0a8 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -374,6 +374,8 @@ intel_context_init(struct intel_context *ce, struct intel_engine_cs *engine)
 	ce->sseu = engine->sseu;
 	ce->ring = __intel_context_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 f83a73a2b39f..a6d4c2d7f2f5 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.h
+++ b/drivers/gpu/drm/i915/gt/intel_context.h
@@ -217,9 +217,29 @@ static inline bool intel_context_is_banned(const struct intel_context *ce)
 
 static inline bool intel_context_set_banned(struct intel_context *ce)
 {
+	clear_bit(CONTEXT_SCHEDULABLE, &ce->flags);
 	return test_and_set_bit(CONTEXT_BANNED, &ce->flags);
 }
 
+static inline bool
+intel_context_is_closed_non_persistent(const struct intel_context *ce)
+{
+	return test_bit(CONTEXT_NON_PERSISTENT, &ce->flags);
+}
+
+static inline bool
+intel_context_set_closed_non_persistent(struct intel_context *ce)
+{
+	GEM_BUG_ON(!intel_context_is_closed(ce));
+	clear_bit(CONTEXT_SCHEDULABLE, &ce->flags);
+	return test_and_set_bit(CONTEXT_NON_PERSISTENT, &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_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 ed8c447a7346..2d07b821a20e 100644
--- a/drivers/gpu/drm/i915/gt/intel_context_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
@@ -95,6 +95,8 @@ struct intel_context {
 #define CONTEXT_BANNED			6
 #define CONTEXT_FORCE_SINGLE_SUBMISSION	7
 #define CONTEXT_NOPREEMPT		8
+#define CONTEXT_SCHEDULABLE		9  /* Unless banned or non-persistent closed. */
+#define CONTEXT_NON_PERSISTENT		10 /* Only if also 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 de124870af44..3f3e2ee6a24d 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))
@@ -1204,12 +1204,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_closed_non_persistent(ce)))
+		return 15;
 
 	return READ_ONCE(engine->props.preempt_timeout_ms);
 }
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index bec9c3652188..bbac87535923 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -660,7 +660,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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

* Re: [Intel-gfx] [PATCH] drm/i915: Be more gentle with exiting non-persistent context
  2021-04-30 13:27   ` Tvrtko Ursulin
@ 2021-04-30 14:34     ` Daniel Vetter
  0 siblings, 0 replies; 5+ messages in thread
From: Daniel Vetter @ 2021-04-30 14:34 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: intel-gfx, Chris Wilson, dri-devel, Zhen Han

On Fri, Apr 30, 2021 at 3:27 PM Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
> On 30/04/2021 12:48, Daniel Vetter wrote:
> > On Thu, Apr 29, 2021 at 10:46:40AM +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.
> >
> > Can you pls explain here why this is preferrable?
>
> I think there's always the risk of an innocent request getting axed with
> preempt to busy being very asynchronous and also engine reset can
> sometimes fail as well.
>
> >> To achieve this the patch splits handling of banned contexts from simply
> >> exited 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.
> >
> > Same here, a bit more detail on what exactly was the problem to be fixed
> > is needed.
>
> It is a bit multi-faceted. Start with how in some cultures errors
> messages are much bigger error flags than in others and much more
> difficult to hand-wave "oh that's not a problem really". The the
> previous considerations about why not avoid engine reset if we can. Add
> on top how non-persistent context exiting is not really an error worthy
> of a reset, *if* it exits cleanly reasonably quickly.
>
> You could say make it clean up for itself before it exits, not a kernel
> problem. But on the balance of everything, to me it sounds saleable to
> just give it some longer time compared to banned contexts, which are the
> unquestionably naughty/dangerous ones. Also, how fast non-persistent
> contexts will be cleaned up hasn't been defined really. As long as 15ms
> is an order of magnitude, plus some, shorter than the normal preempt
> timeout I think it is fine.

Yeah I think makes all sense, I just asked since the commit message
hinted at reasons but didn't explain any of them. I also assume that
generally the full reset flow is also on the order of 1ms (haven't
checked, but it's not cheap), so not being aggressive with engaging
that hammer is imo itself a very good reason to be a notch more lax
here.
-Danie

>
> Regards,
>
> Tvrtko
>
> P.S. Otherwise I plan to respin v2 with consolidated CONTEXT_SCHEDULABLE
> flag so fast paths do not have to do too many individual checks.
>
> > -Daniel
> >
> >>
> >> 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     | 15 +++++++++------
> >>   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    | 12 ++++++++++--
> >>   drivers/gpu/drm/i915/i915_request.c             |  2 +-
> >>   5 files changed, 38 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> >> index fd8ee52e17a4..5a6eba1232cd 100644
> >> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> >> @@ -426,7 +426,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;
> >> @@ -443,6 +444,8 @@ static void kill_engines(struct i915_gem_engines *engines, bool ban)
> >>
> >>              if (ban && intel_context_set_banned(ce))
> >>                      continue;
> >> +            else if (!persistent && intel_context_set_non_persistent(ce))
> >> +                    continue;
> >>
> >>              /*
> >>               * Check the current active state of this context; if we
> >> @@ -454,7 +457,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
> >> @@ -466,8 +469,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);
> >> @@ -480,7 +481,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));
> >> @@ -526,7 +528,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.h b/drivers/gpu/drm/i915/gt/intel_context.h
> >> index f83a73a2b39f..b0e812b8ce39 100644
> >> --- a/drivers/gpu/drm/i915/gt/intel_context.h
> >> +++ b/drivers/gpu/drm/i915/gt/intel_context.h
> >> @@ -220,6 +220,23 @@ 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_is_non_persistent(const struct intel_context *ce)
> >> +{
> >> +    return test_bit(CONTEXT_NON_PERSISTENT, &ce->flags);
> >> +}
> >> +
> >> +static inline bool intel_context_set_non_persistent(struct intel_context *ce)
> >> +{
> >> +    return test_and_set_bit(CONTEXT_NON_PERSISTENT, &ce->flags);
> >> +}
> >> +
> >> +static inline bool intel_context_is_schedulable(const struct intel_context *ce)
> >> +{
> >> +    return !intel_context_is_banned(ce) &&
> >> +           !(intel_context_is_closed(ce) &&
> >> +             intel_context_is_non_persistent(ce));
> >> +}
> >> +
> >>   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 ed8c447a7346..aa949c539e93 100644
> >> --- a/drivers/gpu/drm/i915/gt/intel_context_types.h
> >> +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
> >> @@ -95,6 +95,7 @@ struct intel_context {
> >>   #define CONTEXT_BANNED                     6
> >>   #define CONTEXT_FORCE_SINGLE_SUBMISSION    7
> >>   #define CONTEXT_NOPREEMPT          8
> >> +#define CONTEXT_NON_PERSISTENT              9 /* Only if also 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 de124870af44..5ad7272fbbc4 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))
> >> @@ -1204,12 +1204,20 @@ 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_closed(ce) &&
> >> +                      intel_context_is_non_persistent(ce)))
> >> +            return 15;
> >>
> >>      return READ_ONCE(engine->props.preempt_timeout_ms);
> >>   }
> >> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> >> index bec9c3652188..bbac87535923 100644
> >> --- a/drivers/gpu/drm/i915/i915_request.c
> >> +++ b/drivers/gpu/drm/i915/i915_request.c
> >> @@ -660,7 +660,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
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

end of thread, other threads:[~2021-04-30 14:34 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-29  9:46 [PATCH] drm/i915: Be more gentle with exiting non-persistent context Tvrtko Ursulin
2021-04-30 11:48 ` [Intel-gfx] " Daniel Vetter
2021-04-30 13:27   ` Tvrtko Ursulin
2021-04-30 14:34     ` Daniel Vetter
2021-04-30 14:10 ` [PATCH v2] " 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).