* Re: [PATCH] drm/i915: Flush WC GGTT only on required platforms
@ 2023-10-13 12:28 ` Nirmoy Das
0 siblings, 0 replies; 27+ messages in thread
From: Nirmoy Das @ 2023-10-13 12:28 UTC (permalink / raw)
To: Ville Syrjälä
Cc: Tvrtko Ursulin, intel-gfx, Jonathan Cavitt, dri-devel, stable,
Andi Shyti, Rodrigo Vivi, Matt Roper, John Harrison
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.
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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH] drm/i915: Flush WC GGTT only on required platforms
@ 2023-10-13 12:28 ` Nirmoy Das
0 siblings, 0 replies; 27+ messages in thread
From: Nirmoy Das @ 2023-10-13 12:28 UTC (permalink / raw)
To: Ville Syrjälä
Cc: intel-gfx, Tvrtko Ursulin, Jonathan Cavitt, dri-devel, Andi Shyti,
Rodrigo Vivi, stable, Matt Roper, John Harrison
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.
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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915: Flush WC GGTT only on required platforms
2023-10-13 12:28 ` Nirmoy Das
(?)
@ 2023-10-13 12:55 ` Ville Syrjälä
-1 siblings, 0 replies; 27+ messages in thread
From: Ville Syrjälä @ 2023-10-13 12:55 UTC (permalink / raw)
To: Nirmoy Das
Cc: intel-gfx, Jonathan Cavitt, dri-devel, stable, Rodrigo Vivi,
Matt Roper
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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH] drm/i915: Flush WC GGTT only on required platforms
@ 2023-10-13 12:55 ` Ville Syrjälä
0 siblings, 0 replies; 27+ messages in thread
From: Ville Syrjälä @ 2023-10-13 12:55 UTC (permalink / raw)
To: Nirmoy Das
Cc: Tvrtko Ursulin, intel-gfx, Jonathan Cavitt, dri-devel, stable,
Andi Shyti, Rodrigo Vivi, Matt Roper, John Harrison
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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH] drm/i915: Flush WC GGTT only on required platforms
@ 2023-10-13 12:55 ` Ville Syrjälä
0 siblings, 0 replies; 27+ messages in thread
From: Ville Syrjälä @ 2023-10-13 12:55 UTC (permalink / raw)
To: Nirmoy Das
Cc: intel-gfx, Tvrtko Ursulin, Jonathan Cavitt, dri-devel, Andi Shyti,
Rodrigo Vivi, stable, Matt Roper, John Harrison
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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915: Flush WC GGTT only on required platforms
2023-10-13 12:55 ` Ville Syrjälä
(?)
@ 2023-10-13 13:13 ` Nirmoy Das
-1 siblings, 0 replies; 27+ messages in thread
From: Nirmoy Das @ 2023-10-13 13:13 UTC (permalink / raw)
To: Ville Syrjälä
Cc: intel-gfx, Jonathan Cavitt, dri-devel, stable, Rodrigo Vivi,
Matt Roper
On 10/13/2023 2:55 PM, Ville Syrjälä wrote:
> 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.
I agree. I will resend with a static helper function to detect that.
Thanks,
Nirmoy
>
>>
>> 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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH] drm/i915: Flush WC GGTT only on required platforms
@ 2023-10-13 13:13 ` Nirmoy Das
0 siblings, 0 replies; 27+ messages in thread
From: Nirmoy Das @ 2023-10-13 13:13 UTC (permalink / raw)
To: Ville Syrjälä
Cc: Tvrtko Ursulin, intel-gfx, Jonathan Cavitt, dri-devel, stable,
Andi Shyti, Rodrigo Vivi, Matt Roper, John Harrison
On 10/13/2023 2:55 PM, Ville Syrjälä wrote:
> 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.
I agree. I will resend with a static helper function to detect that.
Thanks,
Nirmoy
>
>>
>> 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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH] drm/i915: Flush WC GGTT only on required platforms
@ 2023-10-13 13:13 ` Nirmoy Das
0 siblings, 0 replies; 27+ messages in thread
From: Nirmoy Das @ 2023-10-13 13:13 UTC (permalink / raw)
To: Ville Syrjälä
Cc: intel-gfx, Tvrtko Ursulin, Jonathan Cavitt, dri-devel, Andi Shyti,
Rodrigo Vivi, stable, Matt Roper, John Harrison
On 10/13/2023 2:55 PM, Ville Syrjälä wrote:
> 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.
I agree. I will resend with a static helper function to detect that.
Thanks,
Nirmoy
>
>>
>> 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Flush WC GGTT only on required platforms
2023-10-13 12:28 ` Nirmoy Das
(?)
@ 2023-10-13 16:15 ` Daniel Vetter
-1 siblings, 0 replies; 27+ messages in thread
From: Daniel Vetter @ 2023-10-13 16:15 UTC (permalink / raw)
To: Nirmoy Das
Cc: intel-gfx, Jonathan Cavitt, stable, dri-devel, Rodrigo Vivi,
Matt Roper
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.
Might be good to cite the relevant git commits in the commit message to
make this clear.
-Sima
>
>
> So if I am not missing anything obvious then this should be safe.
>
>
> 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH] drm/i915: Flush WC GGTT only on required platforms
@ 2023-10-13 16:15 ` Daniel Vetter
0 siblings, 0 replies; 27+ messages in thread
From: Daniel Vetter @ 2023-10-13 16:15 UTC (permalink / raw)
To: Nirmoy Das
Cc: Tvrtko Ursulin, intel-gfx, Jonathan Cavitt, stable, dri-devel,
Andi Shyti, Rodrigo Vivi, Matt Roper, John Harrison
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.
Might be good to cite the relevant git commits in the commit message to
make this clear.
-Sima
>
>
> So if I am not missing anything obvious then this should be safe.
>
>
> 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH] drm/i915: Flush WC GGTT only on required platforms
@ 2023-10-13 16:15 ` Daniel Vetter
0 siblings, 0 replies; 27+ messages in thread
From: Daniel Vetter @ 2023-10-13 16:15 UTC (permalink / raw)
To: Nirmoy Das
Cc: Ville Syrjälä, Tvrtko Ursulin, intel-gfx,
Jonathan Cavitt, dri-devel, stable, Andi Shyti, Rodrigo Vivi,
Matt Roper, John Harrison
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.
Might be good to cite the relevant git commits in the commit message to
make this clear.
-Sima
>
>
> So if I am not missing anything obvious then this should be safe.
>
>
> 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915: Flush WC GGTT only on required platforms
2023-10-13 16:15 ` Daniel Vetter
(?)
@ 2023-10-16 7:12 ` Nirmoy Das
-1 siblings, 0 replies; 27+ messages in thread
From: Nirmoy Das @ 2023-10-16 7:12 UTC (permalink / raw)
To: Daniel Vetter
Cc: intel-gfx, Jonathan Cavitt, stable, dri-devel, Rodrigo Vivi,
Matt Roper
On 10/13/2023 6:15 PM, Daniel Vetter wrote:
> 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.
> Might be good to cite the relevant git commits in the commit message to
> make this clear.
Yes, I should. It took me a while to find it. Going to add that and
resend the patch.
Thanks,
Nirmoy
> -Sima
>
>>
>> So if I am not missing anything obvious then this should be safe.
>>
>>
>> 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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH] drm/i915: Flush WC GGTT only on required platforms
@ 2023-10-16 7:12 ` Nirmoy Das
0 siblings, 0 replies; 27+ messages in thread
From: Nirmoy Das @ 2023-10-16 7:12 UTC (permalink / raw)
To: Daniel Vetter
Cc: Tvrtko Ursulin, intel-gfx, Jonathan Cavitt, stable, dri-devel,
Andi Shyti, Rodrigo Vivi, Matt Roper, John Harrison
On 10/13/2023 6:15 PM, Daniel Vetter wrote:
> 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.
> Might be good to cite the relevant git commits in the commit message to
> make this clear.
Yes, I should. It took me a while to find it. Going to add that and
resend the patch.
Thanks,
Nirmoy
> -Sima
>
>>
>> So if I am not missing anything obvious then this should be safe.
>>
>>
>> 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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH] drm/i915: Flush WC GGTT only on required platforms
@ 2023-10-16 7:12 ` Nirmoy Das
0 siblings, 0 replies; 27+ messages in thread
From: Nirmoy Das @ 2023-10-16 7:12 UTC (permalink / raw)
To: Daniel Vetter
Cc: Ville Syrjälä, Tvrtko Ursulin, intel-gfx,
Jonathan Cavitt, dri-devel, stable, Andi Shyti, Rodrigo Vivi,
Matt Roper, John Harrison
On 10/13/2023 6:15 PM, Daniel Vetter wrote:
> 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.
> Might be good to cite the relevant git commits in the commit message to
> make this clear.
Yes, I should. It took me a while to find it. Going to add that and
resend the patch.
Thanks,
Nirmoy
> -Sima
>
>>
>> So if I am not missing anything obvious then this should be safe.
>>
>>
>> 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
^ permalink raw reply [flat|nested] 27+ messages in thread