| Message ID | 20231018093815.1349-1-nirmoy.das@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] drm/i915: Flush WC GGTT only on required platforms | expand |
Hi Nirmoy, On Wed, Oct 18, 2023 at 11:38:15AM +0200, Nirmoy Das wrote: > gen8_ggtt_invalidate() is only needed for limited set of platforms > where GGTT is mapped as WC. This was added as way to fix WC based GGTT in > commit 0f9b91c754b7 ("drm/i915: flush system agent TLBs on SNB") and > there are no reference in HW docs that forces us to use this on non-WC > backed GGTT. > > This can also cause unwanted side-effects on XE_HP platforms where > GFX_FLSH_CNTL_GEN6 is not valid anymore. > > v2: Add a func to detect wc ggtt detection (Ville) > v3: Improve commit log and add reference commit (Daniel) > > Fixes: d2eae8e98d59 ("drm/i915/dg2: Drop force_probe requirement") I'm wondering if this is the right Fixes, though. Should this rather be: Fixes: 6266992cf105 ("drm/i915/gt: remove GRAPHICS_VER == 10") ? Andi
Hi Andi, On 10/18/2023 1:49 PM, Andi Shyti wrote: > Hi Nirmoy, > > On Wed, Oct 18, 2023 at 11:38:15AM +0200, Nirmoy Das wrote: >> gen8_ggtt_invalidate() is only needed for limited set of platforms >> where GGTT is mapped as WC. This was added as way to fix WC based GGTT in >> commit 0f9b91c754b7 ("drm/i915: flush system agent TLBs on SNB") and >> there are no reference in HW docs that forces us to use this on non-WC >> backed GGTT. >> >> This can also cause unwanted side-effects on XE_HP platforms where >> GFX_FLSH_CNTL_GEN6 is not valid anymore. >> >> v2: Add a func to detect wc ggtt detection (Ville) >> v3: Improve commit log and add reference commit (Daniel) >> >> Fixes: d2eae8e98d59 ("drm/i915/dg2: Drop force_probe requirement") > I'm wondering if this is the right Fixes, though. Should this > rather be: > > Fixes: 6266992cf105 ("drm/i915/gt: remove GRAPHICS_VER == 10") Hard to find a real Fixes for this. I just want to backport this to dg2 where we can have unwanted side-effects. Regards, Nirmoy > > ? > > Andi
Hi Nirmoy, > > > gen8_ggtt_invalidate() is only needed for limited set of platforms > > > where GGTT is mapped as WC. This was added as way to fix WC based GGTT in > > > commit 0f9b91c754b7 ("drm/i915: flush system agent TLBs on SNB") and > > > there are no reference in HW docs that forces us to use this on non-WC > > > backed GGTT. > > > > > > This can also cause unwanted side-effects on XE_HP platforms where > > > GFX_FLSH_CNTL_GEN6 is not valid anymore. > > > > > > v2: Add a func to detect wc ggtt detection (Ville) > > > v3: Improve commit log and add reference commit (Daniel) > > > > > > Fixes: d2eae8e98d59 ("drm/i915/dg2: Drop force_probe requirement") > > I'm wondering if this is the right Fixes, though. Should this > > rather be: > > > > Fixes: 6266992cf105 ("drm/i915/gt: remove GRAPHICS_VER == 10") > > Hard to find a real Fixes for this. I just want to backport this to dg2 > where we can have unwanted side-effects. yes, this piece of code has moved around enough so to make it diffuclt to track its origin. I think the one I found should be the correct one, but the dg2 force probe removeal can also become a placeholder for DG2 fixes. I won't complain. Andi
On 10/18/2023 3:00 PM, Andi Shyti wrote: > Hi Nirmoy, > >>>> gen8_ggtt_invalidate() is only needed for limited set of platforms >>>> where GGTT is mapped as WC. This was added as way to fix WC based GGTT in >>>> commit 0f9b91c754b7 ("drm/i915: flush system agent TLBs on SNB") and >>>> there are no reference in HW docs that forces us to use this on non-WC >>>> backed GGTT. >>>> >>>> This can also cause unwanted side-effects on XE_HP platforms where >>>> GFX_FLSH_CNTL_GEN6 is not valid anymore. >>>> >>>> v2: Add a func to detect wc ggtt detection (Ville) >>>> v3: Improve commit log and add reference commit (Daniel) >>>> >>>> Fixes: d2eae8e98d59 ("drm/i915/dg2: Drop force_probe requirement") >>> I'm wondering if this is the right Fixes, though. Should this >>> rather be: >>> >>> Fixes: 6266992cf105 ("drm/i915/gt: remove GRAPHICS_VER == 10") >> Hard to find a real Fixes for this. I just want to backport this to dg2 >> where we can have unwanted side-effects. > yes, this piece of code has moved around enough so to make it > diffuclt to track its origin. > > I think the one I found should be the correct one, That just removes a graphics ver, not related to WC GGTT map or XE_HP. > but the dg2 > force probe removeal can also become a placeholder for DG2 fixes. Yes, I have no better ideas too. Regards, Nirmoy > > I won't complain. > > Andi
This is now merged to gt-next Thanks, Nirmoy On 10/18/2023 4:04 PM, Nirmoy Das wrote: > > On 10/18/2023 3:00 PM, Andi Shyti wrote: >> Hi Nirmoy, >> >>>>> gen8_ggtt_invalidate() is only needed for limited set of platforms >>>>> where GGTT is mapped as WC. This was added as way to fix WC based >>>>> GGTT in >>>>> commit 0f9b91c754b7 ("drm/i915: flush system agent TLBs on SNB") and >>>>> there are no reference in HW docs that forces us to use this on >>>>> non-WC >>>>> backed GGTT. >>>>> >>>>> This can also cause unwanted side-effects on XE_HP platforms where >>>>> GFX_FLSH_CNTL_GEN6 is not valid anymore. >>>>> >>>>> v2: Add a func to detect wc ggtt detection (Ville) >>>>> v3: Improve commit log and add reference commit (Daniel) >>>>> >>>>> Fixes: d2eae8e98d59 ("drm/i915/dg2: Drop force_probe requirement") >>>> I'm wondering if this is the right Fixes, though. Should this >>>> rather be: >>>> >>>> Fixes: 6266992cf105 ("drm/i915/gt: remove GRAPHICS_VER == 10") >>> Hard to find a real Fixes for this. I just want to backport this to dg2 >>> where we can have unwanted side-effects. >> yes, this piece of code has moved around enough so to make it >> diffuclt to track its origin. >> >> I think the one I found should be the correct one, > > That just removes a graphics ver, not related to WC GGTT map or XE_HP. > >> but the dg2 >> force probe removeal can also become a placeholder for DG2 fixes. > > Yes, I have no better ideas too. > > > Regards, > > Nirmoy > >> >> I won't complain. >> >> Andi
diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c index 1c93e84278a0..15fc8e4703f4 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_ct_invalidate(struct intel_gt *gt) @@ -1140,17 +1159,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. This was added as way to fix WC based GGTT in commit 0f9b91c754b7 ("drm/i915: flush system agent TLBs on SNB") and there are no reference in HW docs that forces us to use this on non-WC backed GGTT. This can also cause unwanted side-effects on XE_HP platforms where GFX_FLSH_CNTL_GEN6 is not valid anymore. v2: Add a func to detect wc ggtt detection (Ville) v3: Improve commit log and add reference commit (Daniel) 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: Daniel Vetter <daniel@ffwll.ch> 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> Reviewed-by: Matt Roper <matthew.d.roper@intel.com> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> --- drivers/gpu/drm/i915/gt/intel_ggtt.c | 35 +++++++++++++++++++--------- 1 file changed, 24 insertions(+), 11 deletions(-)