Re: [Intel-gfx] [PATCH] drm/i915: Improve user experience and driver robustness under SIGINT or similar

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

Final call to raise objections.

Regards,

Tvrtko

On 07/06/2022 09:36, Tvrtko Ursulin wrote:
> 
> On 27/05/2022 13:07, Andrzej Hajda wrote:
>> On 27.05.2022 09:24, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> We have long standing customer complaints that pressing Ctrl-C (or to 
>>> the
>>> effect of) causes engine resets with otherwise well behaving programs.
>>>
>>> Not only is logging engine resets during normal operation not desirable
>>> since it creates support incidents, but more fundamentally we should 
>>> avoid
>>> going the engine reset path when we can since any engine reset 
>>> introduces
>>> a chance of harming an innocent context.
>>>
>>> Reason for this undesirable behaviour is that the driver currently does
>>> not distinguish between banned contexts and non-persistent contexts 
>>> which
>>> have been closed.
>>>
>>> To fix this we add the distinction between the two reasons for revoking
>>> contexts, which then allows the strict timeout only be applied to 
>>> banned,
>>> while innocent contexts (well behaving) can preempt cleanly and exit
>>> without triggering the engine reset path.
>>>
>>> Note that the added context exiting category applies both to closed non-
>>> persistent context, and any exiting context when hangcheck has been
>>> disabled by the user.
>>>
>>> At the same time we rename the backend operation from 'ban' to 'revoke'
>>> which more accurately describes the actual semantics. (There is no 
>>> ban at
>>> the backend level since banning is a concept driven by the scheduling
>>> frontend. Backends are simply able to revoke a running context so that
>>> is the more appropriate name chosen.)
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/gem/i915_gem_context.c   | 23 +++++++++++------
>>>   drivers/gpu/drm/i915/gt/intel_context.c       | 24 ++++++++++++++++++
>>>   drivers/gpu/drm/i915/gt/intel_context.h       | 25 +++++++++++++------
>>>   drivers/gpu/drm/i915/gt/intel_context_types.h |  4 ++-
>>>   .../drm/i915/gt/intel_execlists_submission.c  |  6 ++---
>>>   .../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, 77 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
>>> b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>>> index ab4c5ab28e4d..6b171c89b1b3 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>>> @@ -1367,7 +1367,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 exit, bool 
>>> persistent)
>>>   {
>>>       struct i915_gem_engines_iter it;
>>>       struct intel_context *ce;
>>> @@ -1381,9 +1382,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))
>>> -            continue;
>>> +        if (exit)
>>> +            skip = intel_context_set_exiting(ce);
>>> +        else if (!persistent)
>>> +            skip = intel_context_exit_nonpersistent(ce, NULL);
>>> +
>>> +        if (skip)
>>> +            continue; /* Already marked. */
>>
>> why not:
>>      if (exit && intel_context_set_exiting(ce))
>>          continue;
>>      else if (!persistent && intel_context_exit_nonpersistent(ce, NULL)
>>          continue;
> 
> Just so I can put the "already marked" comment on single line.
> 
>>
>> Anyway:
>> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
> 
> Thanks!
> 
> John, Daniel - you had reservations against the older version of this 
> patch AFAIR. This time round I believe I conceptually simplified it by 
> doing a clean separation of contexts which should not be scheduled any 
> more becuase they want it so, versus the ones we banned. That is, the 
> patch stops abusing the banned status for contexts which haven't been 
> (banned). This allows to only apply the strict preempt timeout to 
> banned, while there is no reason to add any new timeout values for the 
> rest.
> 
> Any objections to this version?
> 
> Regards,
> 
> Tvrtko
> 
>>
>> Regards
>> Andrzej
>>
>>>           /*
>>>            * Check the current active state of this context; if we
>>> @@ -1395,7 +1402,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) && (exit || 
>>> !persistent))
>>>               /*
>>>                * If we are unable to send a preemptive pulse to bump
>>>                * the context from the GPU, we have to resort to a full
>>> @@ -1407,8 +1414,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);
>>> @@ -1421,7 +1426,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));
>>> @@ -1467,7 +1473,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 4070cb5711d8..654a092ed3d6 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_context.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_context.c
>>> @@ -601,6 +601,30 @@ u64 intel_context_get_avg_runtime_ns(struct 
>>> intel_context *ce)
>>>       return avg;
>>>   }
>>> +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, ce->engine->props.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 b7d3214d2cdd..8e2d70630c49 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_context.h
>>> +++ b/drivers/gpu/drm/i915/gt/intel_context.h
>>> @@ -25,6 +25,8 @@
>>>                ##__VA_ARGS__);                    \
>>>   } while (0)
>>> +#define INTEL_CONTEXT_BANNED_PREEMPT_TIMEOUT_MS (1)
>>> +
>>>   struct i915_gem_ww_ctx;
>>>   void intel_context_init(struct intel_context *ce,
>>> @@ -309,18 +311,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 09f82545789f..d2d75d9c0c8d 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_context_types.h
>>> +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
>>> @@ -40,7 +40,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);
>>> @@ -122,6 +123,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 a4510b5c0c3d..ad72e2c5c4e7 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>>> @@ -480,9 +480,9 @@ __execlists_schedule_in(struct i915_request *rq)
>>>       if (unlikely(intel_context_is_closed(ce) &&
>>>                !intel_engine_has_heartbeat(engine)))
>>> -        intel_context_set_banned(ce);
>>> +        intel_context_set_exiting(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))
>>> @@ -1243,7 +1243,7 @@ static unsigned long 
>>> active_preempt_timeout(struct intel_engine_cs *engine,
>>>       /* Force a fast reset for terminated contexts (ignoring sysfs!) */
>>>       if (unlikely(intel_context_is_banned(rq->context) || 
>>> bad_request(rq)))
>>> -        return 1;
>>> +        return INTEL_CONTEXT_BANNED_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 f8f279a195c0..d5d6f1fadcae 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
>>> @@ -598,8 +598,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;
>>> @@ -634,7 +635,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 5a1dfacf24ea..e62ea35513ea 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>> @@ -2790,7 +2790,9 @@ static void 
>>> __guc_context_set_preemption_timeout(struct intel_guc *guc,
>>>       __guc_context_set_context_policies(guc, &policy, 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 =
>>> @@ -2829,7 +2831,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 {
>>> @@ -2837,7 +2840,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);
>>>       }
>>>   }
>>> @@ -3190,7 +3193,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,
>>> @@ -3439,7 +3442,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,
>>> @@ -3528,7 +3531,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 73d5195146b0..c3937640b119 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)))
>>