gen8_ggtt_invalidate() is only needed for limitted 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.
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)
Hi Nirmoy,
On Fri, Oct 13, 2023 at 12:31:40PM +0200, Nirmoy Das wrote:
gen8_ggtt_invalidate() is only needed for limitted set of platforms
/limitted/limited/
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.
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
Acked-by: Andi Shyti andi.shyti@linux.intel.com
Andi
On 10/13/2023 12:35 PM, Andi Shyti wrote:
Hi Nirmoy,
On Fri, Oct 13, 2023 at 12:31:40PM +0200, Nirmoy Das wrote:
gen8_ggtt_invalidate() is only needed for limitted set of platforms
/limitted/limited/
Added " autocmd FileType gitcommit setlocal spell" to my vim config. Wish I knew about it
before.
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.
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
Acked-by: Andi Shyti andi.shyti@linux.intel.com
Thanks,
Nirmoy
Andi
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?
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
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
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
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);-- 2.41.0
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);-- 2.41.0
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);-- 2.41.0
linux-stable-mirror@lists.linaro.org
-
Andi Shyti -
Daniel Vetter -
Nirmoy Das -
Ville Syrjälä