[Intel-gfx] [PATCH v13 3/7] drm/i915: Define and use GuC and CTB TLB invalidation routines

[John Harrison]

On 10/13/2023 07:52, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Harrison, John C <john.c.harrison at intel.com>
> Sent: Thursday, October 12, 2023 6:11 PM
> To: Cavitt, Jonathan <jonathan.cavitt at intel.com>; intel-gfx at lists.freedesktop.org
> Cc: Gupta, saurabhg <saurabhg.gupta at intel.com>; chris.p.wilson at linux.intel.com; Iddamsetty, Aravind <aravind.iddamsetty at intel.com>; Yang, Fei <fei.yang at intel.com>; Shyti, Andi <andi.shyti at intel.com>; Das, Nirmoy <nirmoy.das at intel.com>; Krzysztofik, Janusz <janusz.krzysztofik at intel.com>; Roper, Matthew D <matthew.d.roper at intel.com>; tvrtko.ursulin at linux.intel.com; jani.nikula at linux.intel.com
> 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 at intel.com>
>>>
>>> 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 at intel.com>
>>> Signed-off-by: Bruce Chang <yu.bruce.chang at intel.com>
>>> Signed-off-by: Chris Wilson <chris.p.wilson at intel.com>
>>> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa at intel.com>
>>> Signed-off-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
>>> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty at intel.com>
>>> Signed-off-by: Fei Yang <fei.yang at intel.com>
>>> CC: Andi Shyti <andi.shyti at linux.intel.com>
>>> Reviewed-by: Andi Shyti <andi.shyti at linux.intel.com>
>>> Acked-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>> Acked-by: Nirmoy Das <nirmoy.das at intel.com>
>>> Reviewed-by: John Harrison <John.C.Harrison at Intel.com>
>>> ---
>>>    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 = &gt->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.