[Intel-gfx] [PATCH v2] drm/i915: Flush WC GGTT only on required platforms

[Matt Roper]

On Fri, Oct 13, 2023 at 03:44:39PM +0200, Nirmoy Das wrote:
> gen8_ggtt_invalidate() is only needed for limited set of platforms
> where GGTT is mapped as WC otherwise this can cause unwanted
> side-effects on XE_HP platforms where GFX_FLSH_CNTL_GEN6 is not
> valid.
> 
> v2: Add a func to detect wc ggtt detection (Ville)
> 
> Fixes: d2eae8e98d59 ("drm/i915/dg2: Drop force_probe requirement")
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Jani Nikula <jani.nikula at linux.intel.com>
> Cc: Jonathan Cavitt <jonathan.cavitt at intel.com>
> Cc: John Harrison <john.c.harrison at intel.com>
> Cc: Andi Shyti <andi.shyti at linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Cc: <stable at vger.kernel.org> # v6.2+
> Suggested-by: Matt Roper <matthew.d.roper at intel.com>
> Signed-off-by: Nirmoy Das <nirmoy.das at intel.com>
> Acked-by: Andi Shyti <andi.shyti at linux.intel.com>

Reviewed-by: Matt Roper <matthew.d.roper at intel.com>

Interestingly, bspec 151 indicates that we probably shouldn't have been
using a CPU:WC mapping for the GGTT on gen9bc platforms either (i.e.,
the GTT part of the GTTMMADR has the same "64-bits or less" restriction
listed as later platforms).  But we've been using WC without issue for
the last 8 years, so I guess it's not worth changing it now.


Matt

> ---
>  drivers/gpu/drm/i915/gt/intel_ggtt.c | 35 +++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> index 4d7d88b92632..401667f83f96 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> @@ -195,6 +195,21 @@ void gen6_ggtt_invalidate(struct i915_ggtt *ggtt)
>  	spin_unlock_irq(&uncore->lock);
>  }
>  
> +static bool needs_wc_ggtt_mapping(struct drm_i915_private *i915)
> +{
> +	/*
> +	 * On BXT+/ICL+ writes larger than 64 bit to the GTT pagetable range
> +	 * will be dropped. For WC mappings in general we have 64 byte burst
> +	 * writes when the WC buffer is flushed, so we can't use it, but have to
> +	 * resort to an uncached mapping. The WC issue is easily caught by the
> +	 * readback check when writing GTT PTE entries.
> +	 */
> +	if (!IS_GEN9_LP(i915) && GRAPHICS_VER(i915) < 11)
> +		return true;
> +
> +	return false;
> +}
> +
>  static void gen8_ggtt_invalidate(struct i915_ggtt *ggtt)
>  {
>  	struct intel_uncore *uncore = ggtt->vm.gt->uncore;
> @@ -202,8 +217,12 @@ static void gen8_ggtt_invalidate(struct i915_ggtt *ggtt)
>  	/*
>  	 * 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 (needs_wc_ggtt_mapping(ggtt->vm.i915))
> +		intel_uncore_write_fw(uncore, GFX_FLSH_CNTL_GEN6,
> +				      GFX_FLSH_CNTL_EN);
>  }
>  
>  static void guc_ggtt_invalidate(struct i915_ggtt *ggtt)
> @@ -1126,17 +1145,11 @@ static int ggtt_probe_common(struct i915_ggtt *ggtt, u64 size)
>  	GEM_WARN_ON(pci_resource_len(pdev, GEN4_GTTMMADR_BAR) != gen6_gttmmadr_size(i915));
>  	phys_addr = pci_resource_start(pdev, GEN4_GTTMMADR_BAR) + gen6_gttadr_offset(i915);
>  
> -	/*
> -	 * On BXT+/ICL+ writes larger than 64 bit to the GTT pagetable range
> -	 * will be dropped. For WC mappings in general we have 64 byte burst
> -	 * writes when the WC buffer is flushed, so we can't use it, but have to
> -	 * resort to an uncached mapping. The WC issue is easily caught by the
> -	 * readback check when writing GTT PTE entries.
> -	 */
> -	if (IS_GEN9_LP(i915) || GRAPHICS_VER(i915) >= 11)
> -		ggtt->gsm = ioremap(phys_addr, size);
> -	else
> +	if (needs_wc_ggtt_mapping(i915))
>  		ggtt->gsm = ioremap_wc(phys_addr, size);
> +	else
> +		ggtt->gsm = ioremap(phys_addr, size);
> +
>  	if (!ggtt->gsm) {
>  		drm_err(&i915->drm, "Failed to map the ggtt page table\n");
>  		return -ENOMEM;
> -- 
> 2.41.0
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation