[PATCH] drm/i915: Force uncached PPAT for debugging purposes.

[PATCH] drm/i915: Force uncached PPAT for debugging purposes.

[Rodrigo Vivi]

Many screen corruptions and hangs in the past were somehow
related to the caches. In many situations forcing the uncached
was useful at least to narrow down the issue by confirming it
was cache related.

Instead of having to hardcode it everytime that we suspect on
this table let's provide a mechanism to disable these
cache leves on this private table (PPAT).

Cc: Ben Widawsky <benjamin.widawsky@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 6 +++++-
 drivers/gpu/drm/i915/i915_params.c  | 5 +++++
 drivers/gpu/drm/i915/i915_params.h  | 1 +
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index e0c9542..df9f71e 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2645,7 +2645,10 @@ static int ggtt_probe_common(struct i915_ggtt *ggtt, u64 size)
  * writing this data shouldn't be harmful even in those cases. */
 static void bdw_setup_private_ppat(struct drm_i915_private *dev_priv)
 {
-	u64 pat;
+	u64 pat = 0;
+
+	if (i915.uncached_pat)
+		goto out;
 
 	pat = GEN8_PPAT(0, GEN8_PPAT_WB | GEN8_PPAT_LLC)     | /* for normal objects, no eLLC */
 	      GEN8_PPAT(1, GEN8_PPAT_WC | GEN8_PPAT_LLCELLC) | /* for something pointing to ptes? */
@@ -2672,6 +2675,7 @@ static void bdw_setup_private_ppat(struct drm_i915_private *dev_priv)
 		 */
 		pat = GEN8_PPAT(0, GEN8_PPAT_UC);
 
+out:
 	/* XXX: spec defines this as 2 distinct registers. It's unclear if a 64b
 	 * write would work. */
 	I915_WRITE(GEN8_PRIVATE_PAT_LO, pat);
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 2e9645e..505afda 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -37,6 +37,7 @@ struct i915_params i915 __read_mostly = {
 	.enable_fbc = -1,
 	.enable_execlists = -1,
 	.enable_hangcheck = true,
+	.uncached_pat = false,
 	.enable_ppgtt = -1,
 	.enable_psr = -1,
 	.alpha_support = IS_ENABLED(CONFIG_DRM_I915_ALPHA_SUPPORT),
@@ -130,6 +131,10 @@ struct i915_params i915 __read_mostly = {
 	"WARNING: Disabling this can cause system wide hangs. "
 	"(default: true)");
 
+module_param_named_unsafe(uncached_pat, i915.uncached_pat, bool, 0400);
+MODULE_PARM_DESC(uncached_pat,
+	"Force Uncached Private PPAT for debugging purposes. (default:false)");
+
 module_param_named_unsafe(enable_ppgtt, i915.enable_ppgtt, int, 0400);
 MODULE_PARM_DESC(enable_ppgtt,
 	"Override PPGTT usage. "
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 55d47ee..2453431 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -54,6 +54,7 @@
 	func(bool, alpha_support); \
 	func(bool, enable_cmd_parser); \
 	func(bool, enable_hangcheck); \
+	func(bool, uncached_pat); \
 	func(bool, fastboot); \
 	func(bool, prefault_disable); \
 	func(bool, load_detect_test); \
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Force uncached PPAT for debugging purposes.

[Ben Widawsky]

On 17-02-27 17:12:07, Rodrigo Vivi wrote:
>Many screen corruptions and hangs in the past were somehow
>related to the caches. In many situations forcing the uncached
>was useful at least to narrow down the issue by confirming it
>was cache related.
>
>Instead of having to hardcode it everytime that we suspect on
>this table let's provide a mechanism to disable these
>cache leves on this private table (PPAT).
>
>Cc: Ben Widawsky <benjamin.widawsky@intel.com>
>Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

I think this is a cool idea, it could equally be achieved by modifying the PTE
encoding function. In my head, modifying the pte encode makes more sense since
it applies to all legacy context, advanced context, and ggtt entries.

>---
> drivers/gpu/drm/i915/i915_gem_gtt.c | 6 +++++-
> drivers/gpu/drm/i915/i915_params.c  | 5 +++++
> drivers/gpu/drm/i915/i915_params.h  | 1 +
> 3 files changed, 11 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>index e0c9542..df9f71e 100644
>--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>@@ -2645,7 +2645,10 @@ static int ggtt_probe_common(struct i915_ggtt *ggtt, u64 size)
>  * writing this data shouldn't be harmful even in those cases. */
> static void bdw_setup_private_ppat(struct drm_i915_private *dev_priv)
> {
>-	u64 pat;
>+	u64 pat = 0;
>+
>+	if (i915.uncached_pat)
>+		goto out;
>
> 	pat = GEN8_PPAT(0, GEN8_PPAT_WB | GEN8_PPAT_LLC)     | /* for normal objects, no eLLC */
> 	      GEN8_PPAT(1, GEN8_PPAT_WC | GEN8_PPAT_LLCELLC) | /* for something pointing to ptes? */
>@@ -2672,6 +2675,7 @@ static void bdw_setup_private_ppat(struct drm_i915_private *dev_priv)
> 		 */
> 		pat = GEN8_PPAT(0, GEN8_PPAT_UC);
>
>+out:
> 	/* XXX: spec defines this as 2 distinct registers. It's unclear if a 64b
> 	 * write would work. */
> 	I915_WRITE(GEN8_PRIVATE_PAT_LO, pat);
>diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
>index 2e9645e..505afda 100644
>--- a/drivers/gpu/drm/i915/i915_params.c
>+++ b/drivers/gpu/drm/i915/i915_params.c
>@@ -37,6 +37,7 @@ struct i915_params i915 __read_mostly = {
> 	.enable_fbc = -1,
> 	.enable_execlists = -1,
> 	.enable_hangcheck = true,
>+	.uncached_pat = false,
> 	.enable_ppgtt = -1,
> 	.enable_psr = -1,
> 	.alpha_support = IS_ENABLED(CONFIG_DRM_I915_ALPHA_SUPPORT),
>@@ -130,6 +131,10 @@ struct i915_params i915 __read_mostly = {
> 	"WARNING: Disabling this can cause system wide hangs. "
> 	"(default: true)");
>
>+module_param_named_unsafe(uncached_pat, i915.uncached_pat, bool, 0400);
>+MODULE_PARM_DESC(uncached_pat,
>+	"Force Uncached Private PPAT for debugging purposes. (default:false)");
>+
> module_param_named_unsafe(enable_ppgtt, i915.enable_ppgtt, int, 0400);
> MODULE_PARM_DESC(enable_ppgtt,
> 	"Override PPGTT usage. "
>diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
>index 55d47ee..2453431 100644
>--- a/drivers/gpu/drm/i915/i915_params.h
>+++ b/drivers/gpu/drm/i915/i915_params.h
>@@ -54,6 +54,7 @@
> 	func(bool, alpha_support); \
> 	func(bool, enable_cmd_parser); \
> 	func(bool, enable_hangcheck); \
>+	func(bool, uncached_pat); \
> 	func(bool, fastboot); \
> 	func(bool, prefault_disable); \
> 	func(bool, load_detect_test); \
>-- 
>1.9.1
>

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Force uncached PPAT for debugging purposes.

[Patchwork]

== Series Details ==

Series: drm/i915: Force uncached PPAT for debugging purposes.
URL   : https://patchwork.freedesktop.org/series/20334/
State : success

== Summary ==

Series 20334v1 drm/i915: Force uncached PPAT for debugging purposes.
https://patchwork.freedesktop.org/api/1.0/series/20334/revisions/1/mbox/

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:108  pass:95   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29 

1a8bd0fb40e5d02f827f925b7702ed6f64fadce2 drm-tip: 2017y-02m-27d-22h-04m-19s UTC integration manifest
0560c81 drm/i915: Force uncached PPAT for debugging purposes.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3991/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Force uncached PPAT for debugging purposes.

[Chris Wilson]

On Mon, Feb 27, 2017 at 05:18:08PM -0800, Ben Widawsky wrote:
> On 17-02-27 17:12:07, Rodrigo Vivi wrote:
> >Many screen corruptions and hangs in the past were somehow
> >related to the caches. In many situations forcing the uncached
> >was useful at least to narrow down the issue by confirming it
> >was cache related.
> >
> >Instead of having to hardcode it everytime that we suspect on
> >this table let's provide a mechanism to disable these
> >cache leves on this private table (PPAT).
> >
> >Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> >Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> >Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> I think this is a cool idea, it could equally be achieved by modifying the PTE
> encoding function. In my head, modifying the pte encode makes more sense since
> it applies to all legacy context, advanced context, and ggtt entries.

Changing the mocs table is not the same as changing the PTE. Both are
useful for different types of bugs.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Force uncached PPAT for debugging purposes.

[Vivi, Rodrigo]

On Tue, 2017-02-28 at 09:05 +0000, Chris Wilson wrote:
> On Mon, Feb 27, 2017 at 05:18:08PM -0800, Ben Widawsky wrote:
> > On 17-02-27 17:12:07, Rodrigo Vivi wrote:
> > >Many screen corruptions and hangs in the past were somehow
> > >related to the caches. In many situations forcing the uncached
> > >was useful at least to narrow down the issue by confirming it
> > >was cache related.
> > >
> > >Instead of having to hardcode it everytime that we suspect on
> > >this table let's provide a mechanism to disable these
> > >cache leves on this private table (PPAT).
> > >
> > >Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> > >Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > >Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > 
> > I think this is a cool idea, it could equally be achieved by modifying the PTE
> > encoding function. In my head, modifying the pte encode makes more sense since
> > it applies to all legacy context, advanced context, and ggtt entries.
> 
> Changing the mocs table is not the same as changing the PTE. Both are
> useful for different types of bugs.

Indeed.

Although I believe Ben mentioned the pat_sel inside PTE. But if this is
the case I checked here and it seems that it is spread in different
functions here so I believe the simplest and safiest place to do is here
inside the PPAT setup.

> -Chris
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx