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

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

[Nirmoy Das]

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(-)

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;
-- 
2.41.0

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

[Andi Shyti]

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

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

[Nirmoy Das]

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

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

[Andi Shyti]

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

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

[Nirmoy Das]

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

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

[Nirmoy Das]

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