Re: [PATCH] drm/i915: Flush WC GGTT only on required platforms

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Oct 13, 2023 at 02:28:21PM +0200, Nirmoy Das wrote:
> Hi Ville,
> 
> On 10/13/2023 12:50 PM, Ville Syrjälä wrote:
> > On Fri, Oct 13, 2023 at 12:31:40PM +0200, Nirmoy Das wrote:
> >> gen8_ggtt_invalidate() is only needed for limitted set of platforms
> >> where GGTT is mapped as WC
> > I know there is supposed to be some kind hw snooping of the ggtt
> > pte writes to invalidate the tlb, but are we sure GFX_FLSH_CNTL
> > has no other side effects we depend on?
> 
> I spent some time searching through the gfxspec. This GFX_FLSH_CNTL 
> register only seems to be for
> 
> invalidating TLB for GUnit  and (from git log ) we started to do that to 
> enable WC based GGTT updates.
> 
> 
> So if I am not missing anything obvious then this should be safe.

OK.

The only code related complaint I have is that you are now duplicating
that same platform check in two different places. It's always better to
have a single point of truth instead of two or more, so that there is
no risk of introducing bugs due to mismatches.

> 
> 
> Regards,
> 
> Nirmoy
> 
> >
> >> otherwise this can cause unwanted
> >> side-effects on XE_HP platforms where GFX_FLSH_CNTL_GEN6 is not
> >> valid.
> >>
> >> Fixes: d2eae8e98d59 ("drm/i915/dg2: Drop force_probe requirement")
> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> >> Cc: Jonathan Cavitt <jonathan.cavitt@intel.com>
> >> Cc: John Harrison <john.c.harrison@intel.com>
> >> Cc: Andi Shyti <andi.shyti@linux.intel.com>
> >> Cc: <stable@vger.kernel.org> # v6.2+
> >> Suggested-by: Matt Roper <matthew.d.roper@intel.com>
> >> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/gt/intel_ggtt.c | 6 +++++-
> >>   1 file changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> >> index 4d7d88b92632..c2858d434bce 100644
> >> --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> >> +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> >> @@ -197,13 +197,17 @@ void gen6_ggtt_invalidate(struct i915_ggtt *ggtt)
> >>   
> >>   static void gen8_ggtt_invalidate(struct i915_ggtt *ggtt)
> >>   {
> >> +	struct drm_i915_private *i915 = ggtt->vm.i915;
> >>   	struct intel_uncore *uncore = ggtt->vm.gt->uncore;
> >>   
> >>   	/*
> >>   	 * Note that as an uncached mmio write, this will flush the
> >>   	 * WCB of the writes into the GGTT before it triggers the invalidate.
> >> +	 *
> >> +	 * Only perform this when GGTT is mapped as WC, see ggtt_probe_common().
> >>   	 */
> >> -	intel_uncore_write_fw(uncore, GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
> >> +	if (!IS_GEN9_LP(i915) && GRAPHICS_VER(i915) < 11)
> >> +		intel_uncore_write_fw(uncore, GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
> >>   }
> >>   
> >>   static void guc_ggtt_invalidate(struct i915_ggtt *ggtt)
> >> -- 
> >> 2.41.0

-- 
Ville Syrjälä
Intel