| Message ID | 20231013134439.13579-1-nirmoy.das@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915: Flush WC GGTT only on required platforms | expand |
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@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: Ville Syrjälä <ville.syrjala@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> > Acked-by: Andi Shyti <andi.shyti@linux.intel.com> Reviewed-by: Matt Roper <matthew.d.roper@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 >
Hi Nirmoy, 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@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: Ville Syrjälä <ville.syrjala@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> > Acked-by: Andi Shyti <andi.shyti@linux.intel.com> I took some time to look at this and you can swap the a-b with an r-b: Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> Thanks, Andi
Hi Andi, On 10/14/2023 10:51 AM, Andi Shyti wrote: > Hi Nirmoy, > > 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@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: Ville Syrjälä <ville.syrjala@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> >> Acked-by: Andi Shyti <andi.shyti@linux.intel.com> > I took some time to look at this and you can swap the a-b with > an r-b: > > Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> Thanks! Going to resend one more rev with commit that is started using this register for WC mapping. Regards, Nirmoy > > Thanks, > Andi
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;
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@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: Ville Syrjälä <ville.syrjala@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> Acked-by: Andi Shyti <andi.shyti@linux.intel.com> --- drivers/gpu/drm/i915/gt/intel_ggtt.c | 35 +++++++++++++++++++--------- 1 file changed, 24 insertions(+), 11 deletions(-)