| Message ID | 1488244327-23363-1-git-send-email-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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
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 >
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); \
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(-)