On 10/13/2023 07:52, Cavitt, Jonathan wrote:
-----Original Message-----
From: Harrison, John C <john.c.harrison@xxxxxxxxx>
Sent: Thursday, October 12, 2023 6:11 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 3/7] drm/i915: Define and use GuC and CTB TLB invalidation routines
On 10/12/2023 15:38, Jonathan Cavitt wrote:
From: Prathap Kumar Valsan <prathap.kumar.valsan@xxxxxxxxx>
The GuC firmware had defined the interface for Translation Look-Aside
Buffer (TLB) invalidation. We should use this interface when
invalidating the engine and GuC TLBs.
Add additional functionality to intel_gt_invalidate_tlb, invalidating
the GuC TLBs and falling back to GT invalidation when the GuC is
disabled.
The invalidation is done by sending a request directly to the GuC
tlb_lookup that invalidates the table. The invalidation is submitted as
a wait request and is performed in the CT event handler. This means we
cannot perform this TLB invalidation path if the CT is not enabled.
If the request isn't fulfilled in two seconds, this would constitute
an error in the invalidation as that would constitute either a lost
request or a severe GuC overload.
With this new invalidation routine, we can perform GuC-based GGTT
invalidations. GuC-based GGTT invalidation is incompatible with
MMIO invalidation so we should not perform MMIO invalidation when
GuC-based GGTT invalidation is expected.
The additional complexity incurred in this patch will be necessary for
range-based tlb invalidations, which will be platformed in the future.
Signed-off-by: Prathap Kumar Valsan <prathap.kumar.valsan@xxxxxxxxx>
Signed-off-by: Bruce Chang <yu.bruce.chang@xxxxxxxxx>
Signed-off-by: Chris Wilson <chris.p.wilson@xxxxxxxxx>
Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@xxxxxxxxx>
Signed-off-by: Jonathan Cavitt <jonathan.cavitt@xxxxxxxxx>
Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@xxxxxxxxx>
Signed-off-by: Fei Yang <fei.yang@xxxxxxxxx>
CC: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
Reviewed-by: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
Acked-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Acked-by: Nirmoy Das <nirmoy.das@xxxxxxxxx>
Reviewed-by: John Harrison <John.C.Harrison@xxxxxxxxx>
---
drivers/gpu/drm/i915/gt/intel_ggtt.c | 33 ++-
drivers/gpu/drm/i915/gt/intel_tlb.c | 16 +-
.../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h | 33 +++
drivers/gpu/drm/i915/gt/uc/intel_guc.h | 22 ++
drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 11 +
drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h | 1 +
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 195 +++++++++++++++++-
7 files changed, 299 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c
index 4d7d88b92632b..7d145b2d3cb17 100644
--- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
@@ -206,22 +206,37 @@ static void gen8_ggtt_invalidate(struct i915_ggtt *ggtt)
intel_uncore_write_fw(uncore, GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
}
+static void guc_ggtt_ct_invalidate(struct intel_gt *gt)
+{
+ struct intel_uncore *uncore = gt->uncore;
+ intel_wakeref_t wakeref;
+
+ with_intel_runtime_pm_if_active(uncore->rpm, wakeref) {
+ struct intel_guc *guc = >->uc.guc;
+
+ intel_guc_invalidate_tlb_guc(guc);
+ }
+}
+
static void guc_ggtt_invalidate(struct i915_ggtt *ggtt)
{
struct drm_i915_private *i915 = ggtt->vm.i915;
+ struct intel_gt *gt;
- gen8_ggtt_invalidate(ggtt);
-
- if (GRAPHICS_VER(i915) >= 12) {
- struct intel_gt *gt;
+ if (!HAS_GUC_TLB_INVALIDATION(i915))
+ gen8_ggtt_invalidate(ggtt);
This has not changed? As per comments from Matthew Roper and Nirmoy Das,
there needs to be a fixup patch first to stop gen8_ggtt_invalidate()
from being called on invalid platforms.
Given the sounds of things, it seems like this change here is irrelevant to this patch series, as the reason we're
guarding against gen8_ggtt_invalidate isn't related to GuC-based TLB invalidations at all. Ergo, it would actually
make more sense for me to not skip it here and leave the respective guard change to a different patch series.
-Jonathan Cavitt
The point was that if this code needs to change then that patch needs to
happen first. Otherwise there would be merge conflicts when pushing that
patch to the stable trees.
However, it looks like the change is all happening inside the gen8_
function and the intention is to keep calling it even on Gen12+
platforms that don't need it. Seems odd but people appear to be happy
with it. And therefore no conflicts should happen with this patch no
matter what order they land in.
John.
