On 10/13/2023 12:12, John Harrison wrote:
On 10/13/2023 07:42, Cavitt, Jonathan wrote:
-----Original Message-----
From: Harrison, John C <john.c.harrison@xxxxxxxxx>
Sent: Thursday, October 12, 2023 6:08 PM
To: Cavitt, Jonathan <jonathan.cavitt@xxxxxxxxx>;
intel-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: Gupta, saurabhg <saurabhg.gupta@xxxxxxxxx>;
chris.p.wilson@xxxxxxxxxxxxxxx; Iddamsetty, Aravind
<aravind.iddamsetty@xxxxxxxxx>; Yang, Fei <fei.yang@xxxxxxxxx>;
Shyti, Andi <andi.shyti@xxxxxxxxx>; Das, Nirmoy
<nirmoy.das@xxxxxxxxx>; Krzysztofik, Janusz
<janusz.krzysztofik@xxxxxxxxx>; Roper, Matthew D
<matthew.d.roper@xxxxxxxxx>; tvrtko.ursulin@xxxxxxxxxxxxxxx;
jani.nikula@xxxxxxxxxxxxxxx
Subject: Re: [PATCH v13 4/7] drm/i915: No TLB invalidation on
suspended GT
On 10/12/2023 15:38, Jonathan Cavitt wrote:
In case of GT is suspended, don't allow submission of new TLB
invalidation
request and cancel all pending requests. The TLB entries will be
invalidated either during GuC reload or on system resume.
Signed-off-by: Fei Yang <fei.yang@xxxxxxxxx>
Signed-off-by: Jonathan Cavitt <jonathan.cavitt@xxxxxxxxx>
CC: John Harrison <john.c.harrison@xxxxxxxxx>
Reviewed-by: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
Acked-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Acked-by: Nirmoy Das <nirmoy.das@xxxxxxxxx>
---
drivers/gpu/drm/i915/gt/uc/intel_guc.h | 1 +
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 22
++++++++++++-------
drivers/gpu/drm/i915/gt/uc/intel_uc.c | 7 ++++++
3 files changed, 22 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index 0949628d69f8b..2b6dfe62c8f2a 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -537,4 +537,5 @@ int intel_guc_invalidate_tlb_engines(struct
intel_guc *guc);
int intel_guc_invalidate_tlb_guc(struct intel_guc *guc);
int intel_guc_tlb_invalidation_done(struct intel_guc *guc,
const u32 *payload, u32 len);
+void wake_up_all_tlb_invalidate(struct intel_guc *guc);
#endif
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 1377398afcdfa..3a0d20064878a 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -1796,13 +1796,24 @@ static void __guc_reset_context(struct
intel_context *ce, intel_engine_mask_t st
intel_context_put(parent);
}
-void intel_guc_submission_reset(struct intel_guc *guc,
intel_engine_mask_t stalled)
+void wake_up_all_tlb_invalidate(struct intel_guc *guc)
{
struct intel_guc_tlb_wait *wait;
+ unsigned long i;
+
+ if (HAS_GUC_TLB_INVALIDATION(guc_to_gt(guc)->i915)) {
Why the change from 'if(!is_available) return' to 'if(HAS_) {doStuff}'?
I feel like this question has two parts, so I'll answer them separately:
1. Why HAS_GUC_TLB_INVALIDATION and not
intel_guc_tlb_invalidation_is_available?
Wake_up_all_tlb_invalidate is called during the suspend/resume path,
specifically in the
middle of suspend. It's required for it to be called here to clean
up any invalidations left
in the queue during the suspend/resume phase because they are no
longer valid requests.
However, the suspend/resume phase also resets GuC, so
intel_guc_is_ready returns false.
In short, using intel_guc_invalidation_is_available was causing us to
skip this code section
incorrectly, resulting in spurious GuC TLB invalidation timeout
errors during gt reset.
I'm not following this argument. If a reset is occurring then there is
no need to issue the invalidate. And the previous version was skipping
if GuC is in reset but this version does not. Which means it is now
sending invalidate requests to GuC when GuC is not able to respond and
therefore more likely to cause timeout errors not less likely.
Hang on. I'm getting confused between sending the request and waking up
blocked threads. Apologies.
Okay, that makes sense now.
John.
