From: Matthew Brost <matthew.brost@intel.com> To: <intel-gfx@lists.freedesktop.org>, <dri-devel@lists.freedesktop.org> Cc: <daniele.ceraolospurio@intel.com>, <john.c.harrison@intel.com> Subject: [PATCH 4/4] drm/i915/guc: Refcount context during error capture Date: Mon, 13 Sep 2021 22:09:56 -0700 [thread overview] Message-ID: <20210914050956.30685-5-matthew.brost@intel.com> (raw) In-Reply-To: <20210914050956.30685-1-matthew.brost@intel.com> From: John Harrison <John.C.Harrison@Intel.com> When i915 receives a context reset notification from GuC, it triggers an error capture before resetting any outstanding requsts of that context. Unfortunately, the error capture is not a time bound operation. In certain situations it can take a long time, particularly when multiple large LMEM buffers must be read back and eoncoded. If this delay is longer than other timeouts (heartbeat, test recovery, etc.) then a full GT reset can be triggered in the middle. That can result in the context being reset by GuC actually being destroyed before the error capture completes and the GuC submission code resumes. Thus, the GuC side can start dereferencing stale pointers and Bad Things ensue. So add a refcount get of the context during the entire reset operation. That way, the context can't be destroyed part way through no matter what other resets or user interactions occur. v2: (Matthew Brost) - Update patch to work with async error capture Signed-off-by: John Harrison <John.C.Harrison@Intel.com> Signed-off-by: Matthew Brost <matthew.brost@intel.com> --- .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 24 +++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) 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 1986a57b52cc..02917fc4d4a8 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -2888,6 +2888,8 @@ static void capture_worker_func(struct work_struct *w) intel_engine_set_hung_context(engine, ce); with_intel_runtime_pm(&i915->runtime_pm, wakeref) i915_capture_error_state(gt, ce->engine->mask); + + intel_context_put(ce); } static void capture_error_state(struct intel_guc *guc, @@ -2924,7 +2926,7 @@ static void guc_context_replay(struct intel_context *ce) tasklet_hi_schedule(&sched_engine->tasklet); } -static void guc_handle_context_reset(struct intel_guc *guc, +static bool guc_handle_context_reset(struct intel_guc *guc, struct intel_context *ce) { trace_intel_context_reset(ce); @@ -2937,7 +2939,11 @@ static void guc_handle_context_reset(struct intel_guc *guc, !context_blocked(ce))) { capture_error_state(guc, ce); guc_context_replay(ce); + + return false; } + + return true; } int intel_guc_context_reset_process_msg(struct intel_guc *guc, @@ -2945,6 +2951,7 @@ int intel_guc_context_reset_process_msg(struct intel_guc *guc, { struct intel_context *ce; int desc_idx; + unsigned long flags; if (unlikely(len != 1)) { drm_err(&guc_to_gt(guc)->i915->drm, "Invalid length %u", len); @@ -2952,11 +2959,24 @@ int intel_guc_context_reset_process_msg(struct intel_guc *guc, } desc_idx = msg[0]; + + /* + * The context lookup uses the xarray but lookups only require an RCU lock + * not the full spinlock. So take the lock explicitly and keep it until the + * context has been reference count locked to ensure it can't be destroyed + * asynchronously until the reset is done. + */ + xa_lock_irqsave(&guc->context_lookup, flags); ce = g2h_context_lookup(guc, desc_idx); + if (ce) + intel_context_get(ce); + xa_unlock_irqrestore(&guc->context_lookup, flags); + if (unlikely(!ce)) return -EPROTO; - guc_handle_context_reset(guc, ce); + if (guc_handle_context_reset(guc, ce)) + intel_context_put(ce); return 0; } -- 2.32.0
WARNING: multiple messages have this Message-ID (diff)
From: Matthew Brost <matthew.brost@intel.com> To: <intel-gfx@lists.freedesktop.org>, <dri-devel@lists.freedesktop.org> Cc: <daniele.ceraolospurio@intel.com>, <john.c.harrison@intel.com> Subject: [Intel-gfx] [PATCH 4/4] drm/i915/guc: Refcount context during error capture Date: Mon, 13 Sep 2021 22:09:56 -0700 [thread overview] Message-ID: <20210914050956.30685-5-matthew.brost@intel.com> (raw) In-Reply-To: <20210914050956.30685-1-matthew.brost@intel.com> From: John Harrison <John.C.Harrison@Intel.com> When i915 receives a context reset notification from GuC, it triggers an error capture before resetting any outstanding requsts of that context. Unfortunately, the error capture is not a time bound operation. In certain situations it can take a long time, particularly when multiple large LMEM buffers must be read back and eoncoded. If this delay is longer than other timeouts (heartbeat, test recovery, etc.) then a full GT reset can be triggered in the middle. That can result in the context being reset by GuC actually being destroyed before the error capture completes and the GuC submission code resumes. Thus, the GuC side can start dereferencing stale pointers and Bad Things ensue. So add a refcount get of the context during the entire reset operation. That way, the context can't be destroyed part way through no matter what other resets or user interactions occur. v2: (Matthew Brost) - Update patch to work with async error capture Signed-off-by: John Harrison <John.C.Harrison@Intel.com> Signed-off-by: Matthew Brost <matthew.brost@intel.com> --- .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 24 +++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) 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 1986a57b52cc..02917fc4d4a8 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -2888,6 +2888,8 @@ static void capture_worker_func(struct work_struct *w) intel_engine_set_hung_context(engine, ce); with_intel_runtime_pm(&i915->runtime_pm, wakeref) i915_capture_error_state(gt, ce->engine->mask); + + intel_context_put(ce); } static void capture_error_state(struct intel_guc *guc, @@ -2924,7 +2926,7 @@ static void guc_context_replay(struct intel_context *ce) tasklet_hi_schedule(&sched_engine->tasklet); } -static void guc_handle_context_reset(struct intel_guc *guc, +static bool guc_handle_context_reset(struct intel_guc *guc, struct intel_context *ce) { trace_intel_context_reset(ce); @@ -2937,7 +2939,11 @@ static void guc_handle_context_reset(struct intel_guc *guc, !context_blocked(ce))) { capture_error_state(guc, ce); guc_context_replay(ce); + + return false; } + + return true; } int intel_guc_context_reset_process_msg(struct intel_guc *guc, @@ -2945,6 +2951,7 @@ int intel_guc_context_reset_process_msg(struct intel_guc *guc, { struct intel_context *ce; int desc_idx; + unsigned long flags; if (unlikely(len != 1)) { drm_err(&guc_to_gt(guc)->i915->drm, "Invalid length %u", len); @@ -2952,11 +2959,24 @@ int intel_guc_context_reset_process_msg(struct intel_guc *guc, } desc_idx = msg[0]; + + /* + * The context lookup uses the xarray but lookups only require an RCU lock + * not the full spinlock. So take the lock explicitly and keep it until the + * context has been reference count locked to ensure it can't be destroyed + * asynchronously until the reset is done. + */ + xa_lock_irqsave(&guc->context_lookup, flags); ce = g2h_context_lookup(guc, desc_idx); + if (ce) + intel_context_get(ce); + xa_unlock_irqrestore(&guc->context_lookup, flags); + if (unlikely(!ce)) return -EPROTO; - guc_handle_context_reset(guc, ce); + if (guc_handle_context_reset(guc, ce)) + intel_context_put(ce); return 0; } -- 2.32.0
next prev parent reply other threads:[~2021-09-14 5:15 UTC|newest] Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-09-14 5:09 [PATCH 0/4] Do error capture async, flush G2H processing on reset Matthew Brost 2021-09-14 5:09 ` [Intel-gfx] " Matthew Brost 2021-09-14 5:09 ` [PATCH 1/4] drm/i915/guc: Move guc_ids under submission_state sub-structure Matthew Brost 2021-09-14 5:09 ` [Intel-gfx] " Matthew Brost 2021-09-14 5:09 ` [PATCH 2/4] drm/i915/guc: Do error capture asynchronously Matthew Brost 2021-09-14 5:09 ` [Intel-gfx] " Matthew Brost 2021-09-14 14:25 ` Daniel Vetter 2021-09-15 0:04 ` Matthew Brost 2021-09-14 5:09 ` [PATCH 3/4] drm/i915/guc: Flush G2H work queue during reset Matthew Brost 2021-09-14 5:09 ` [Intel-gfx] " Matthew Brost 2021-09-14 5:09 ` Matthew Brost [this message] 2021-09-14 5:09 ` [Intel-gfx] [PATCH 4/4] drm/i915/guc: Refcount context during error capture Matthew Brost 2021-09-14 14:29 ` Daniel Vetter 2021-09-14 23:23 ` John Harrison 2021-09-17 12:37 ` Daniel Vetter 2021-09-14 23:36 ` Matthew Brost 2021-09-17 12:40 ` Daniel Vetter 2021-09-14 6:06 ` [Intel-gfx] ✗ Fi.CI.SPARSE: warning for Do error capture async, flush G2H processing on reset (rev2) Patchwork 2021-09-14 6:36 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork 2021-09-14 16:28 ` [Intel-gfx] ✗ Fi.CI.SPARSE: warning for Do error capture async, flush G2H processing on reset (rev3) Patchwork 2021-09-14 16:53 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork -- strict thread matches above, loose matches on Subject: below -- 2021-09-14 4:24 [PATCH 0/4] Do error capture async, flush G2H processing on reset Matthew Brost 2021-09-14 4:24 ` [PATCH 4/4] drm/i915/guc: Refcount context during error capture Matthew Brost
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210914050956.30685-5-matthew.brost@intel.com \ --to=matthew.brost@intel.com \ --cc=daniele.ceraolospurio@intel.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=intel-gfx@lists.freedesktop.org \ --cc=john.c.harrison@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.