| Message ID | 1422438304-5328-1-git-send-email-mika.kuoppala@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jan 28, 2015 at 11:45:04AM +0200, Mika Kuoppala wrote: > intel_uncore_early_sanitize() will reset the forcewake registers. When > forcewake domains were introduced, the domain init was done after the > sanitization of the forcewake registers. And as the resetting of > registers use the domain accessors, we tried to reset the forcewake > registers with unitialized forcewake domains and failed. > > Fix this by sanitizing after all the domains have been initialized. > On ivb we need special care as there we need early forcewake access to > determine the final configuration for the forcewake domain. > > This regression was introduced in > > commit 05a2fb157e44a53c79133805d30eaada43911941 > Author: Mika Kuoppala <mika.kuoppala@linux.intel.com> > Date: Mon Jan 19 16:20:43 2015 +0200 > > drm/i915: Consolidate forcewake code > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88805 > Reported-by: Olof Johansson <olof@lixom.net> > Tested-by: Darren Hart <dvhart@linux.intel.com> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > --- > drivers/gpu/drm/i915/intel_uncore.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index b3951f2..c438ca4 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -72,6 +72,7 @@ assert_device_not_suspended(struct drm_i915_private *dev_priv) > static inline void > fw_domain_reset(const struct intel_uncore_forcewake_domain *d) > { > + WARN_ON(d->reg_set == 0); > __raw_i915_write32(d->i915, d->reg_set, d->val_reset); > } > > @@ -166,6 +167,8 @@ fw_domains_reset(struct drm_i915_private *dev_priv, enum forcewake_domains fw_do > struct intel_uncore_forcewake_domain *d; > enum forcewake_domain_id id; > > + WARN_ON(dev_priv->uncore.fw_domains == 0); > + > for_each_fw_domain_mask(d, fw_domains, dev_priv, id) > fw_domain_reset(d); > > @@ -987,8 +990,7 @@ static void fw_domain_init(struct drm_i915_private *dev_priv, > void intel_uncore_init(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > - > - __intel_uncore_early_sanitize(dev, false); > + bool sanitize_done = false; I felt this looks quite clumsy. The only reason why you want to restrict calling __intel_uncore_early_sanitize() is that it does ellc_size detection and has a DRM_INFO right? I think you want to pull that out of __intel_uncore_early_santize() into intel_uncore_init() itself (or better, it's own intel_uncore_ellc_detect()). ellc_size detection has nothing to do with sanitizing register state. Then it should be simple to enough to sanitize twice, once with a comment in the code explaining how we verify that FORCEWAKE_MT is enabled by a manual forcewaked read of ECOBUS. -Chris
On Wed, Jan 28, 2015 at 10:17:39AM +0000, Chris Wilson wrote: > On Wed, Jan 28, 2015 at 11:45:04AM +0200, Mika Kuoppala wrote: > > intel_uncore_early_sanitize() will reset the forcewake registers. When > > forcewake domains were introduced, the domain init was done after the > > sanitization of the forcewake registers. And as the resetting of > > registers use the domain accessors, we tried to reset the forcewake > > registers with unitialized forcewake domains and failed. > > > > Fix this by sanitizing after all the domains have been initialized. > > On ivb we need special care as there we need early forcewake access to > > determine the final configuration for the forcewake domain. > > > > This regression was introduced in > > > > commit 05a2fb157e44a53c79133805d30eaada43911941 > > Author: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > Date: Mon Jan 19 16:20:43 2015 +0200 > > > > drm/i915: Consolidate forcewake code > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88805 > > Reported-by: Olof Johansson <olof@lixom.net> > > Tested-by: Darren Hart <dvhart@linux.intel.com> > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > > --- > > drivers/gpu/drm/i915/intel_uncore.c | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > > index b3951f2..c438ca4 100644 > > --- a/drivers/gpu/drm/i915/intel_uncore.c > > +++ b/drivers/gpu/drm/i915/intel_uncore.c > > @@ -72,6 +72,7 @@ assert_device_not_suspended(struct drm_i915_private *dev_priv) > > static inline void > > fw_domain_reset(const struct intel_uncore_forcewake_domain *d) > > { > > + WARN_ON(d->reg_set == 0); > > __raw_i915_write32(d->i915, d->reg_set, d->val_reset); > > } > > > > @@ -166,6 +167,8 @@ fw_domains_reset(struct drm_i915_private *dev_priv, enum forcewake_domains fw_do > > struct intel_uncore_forcewake_domain *d; > > enum forcewake_domain_id id; > > > > + WARN_ON(dev_priv->uncore.fw_domains == 0); > > + > > for_each_fw_domain_mask(d, fw_domains, dev_priv, id) > > fw_domain_reset(d); > > > > @@ -987,8 +990,7 @@ static void fw_domain_init(struct drm_i915_private *dev_priv, > > void intel_uncore_init(struct drm_device *dev) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > - > > - __intel_uncore_early_sanitize(dev, false); > > + bool sanitize_done = false; > > I felt this looks quite clumsy. The only reason why you want to restrict > calling __intel_uncore_early_sanitize() is that it does ellc_size > detection and has a DRM_INFO right? > > I think you want to pull that out of __intel_uncore_early_santize() into > intel_uncore_init() itself (or better, it's own > intel_uncore_ellc_detect()). ellc_size detection has nothing to do with > sanitizing register state. > > Then it should be simple to enough to sanitize twice, once with a > comment in the code explaining how we verify that FORCEWAKE_MT is > enabled by a manual forcewaked read of ECOBUS. Also, why are we not calling fw_domain_reset() from fw_domain_init()? That would be enough to avoid the early santize required for ivb, right? -Chris
Chris Wilson <chris@chris-wilson.co.uk> writes: > On Wed, Jan 28, 2015 at 10:17:39AM +0000, Chris Wilson wrote: >> On Wed, Jan 28, 2015 at 11:45:04AM +0200, Mika Kuoppala wrote: >> > intel_uncore_early_sanitize() will reset the forcewake registers. When >> > forcewake domains were introduced, the domain init was done after the >> > sanitization of the forcewake registers. And as the resetting of >> > registers use the domain accessors, we tried to reset the forcewake >> > registers with unitialized forcewake domains and failed. >> > >> > Fix this by sanitizing after all the domains have been initialized. >> > On ivb we need special care as there we need early forcewake access to >> > determine the final configuration for the forcewake domain. >> > >> > This regression was introduced in >> > >> > commit 05a2fb157e44a53c79133805d30eaada43911941 >> > Author: Mika Kuoppala <mika.kuoppala@linux.intel.com> >> > Date: Mon Jan 19 16:20:43 2015 +0200 >> > >> > drm/i915: Consolidate forcewake code >> > >> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88805 >> > Reported-by: Olof Johansson <olof@lixom.net> >> > Tested-by: Darren Hart <dvhart@linux.intel.com> >> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> >> > --- >> > drivers/gpu/drm/i915/intel_uncore.c | 11 +++++++++-- >> > 1 file changed, 9 insertions(+), 2 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c >> > index b3951f2..c438ca4 100644 >> > --- a/drivers/gpu/drm/i915/intel_uncore.c >> > +++ b/drivers/gpu/drm/i915/intel_uncore.c >> > @@ -72,6 +72,7 @@ assert_device_not_suspended(struct drm_i915_private *dev_priv) >> > static inline void >> > fw_domain_reset(const struct intel_uncore_forcewake_domain *d) >> > { >> > + WARN_ON(d->reg_set == 0); >> > __raw_i915_write32(d->i915, d->reg_set, d->val_reset); >> > } >> > >> > @@ -166,6 +167,8 @@ fw_domains_reset(struct drm_i915_private *dev_priv, enum forcewake_domains fw_do >> > struct intel_uncore_forcewake_domain *d; >> > enum forcewake_domain_id id; >> > >> > + WARN_ON(dev_priv->uncore.fw_domains == 0); >> > + >> > for_each_fw_domain_mask(d, fw_domains, dev_priv, id) >> > fw_domain_reset(d); >> > >> > @@ -987,8 +990,7 @@ static void fw_domain_init(struct drm_i915_private *dev_priv, >> > void intel_uncore_init(struct drm_device *dev) >> > { >> > struct drm_i915_private *dev_priv = dev->dev_private; >> > - >> > - __intel_uncore_early_sanitize(dev, false); >> > + bool sanitize_done = false; >> >> I felt this looks quite clumsy. The only reason why you want to restrict >> calling __intel_uncore_early_sanitize() is that it does ellc_size >> detection and has a DRM_INFO right? >> >> I think you want to pull that out of __intel_uncore_early_santize() into >> intel_uncore_init() itself (or better, it's own >> intel_uncore_ellc_detect()). ellc_size detection has nothing to do with >> sanitizing register state. >> >> Then it should be simple to enough to sanitize twice, once with a >> comment in the code explaining how we verify that FORCEWAKE_MT is >> enabled by a manual forcewaked read of ECOBUS. > > Also, why are we not calling fw_domain_reset() from fw_domain_init()? > That would be enough to avoid the early santize required for ivb, > right? Agreed here. That was my plan originally, doing the sanitize inside in domain inits. But I wanted to fix this particular item by trying to be as close as possible to the previous init/forcewake ordering on all gens. Reasoning is that I would like to see this stabilize a short while before introducing further changes. I burned my fingers already touching these, so they need to heal :) Ok if this is for future work? -Mika
On Wed, Jan 28, 2015 at 12:59:52PM +0200, Mika Kuoppala wrote: > Chris Wilson <chris@chris-wilson.co.uk> writes: > > Also, why are we not calling fw_domain_reset() from fw_domain_init()? > > That would be enough to avoid the early santize required for ivb, > > right? > > Agreed here. That was my plan originally, doing the sanitize inside > in domain inits. But I wanted to fix this particular item by trying to > be as close as possible to the previous init/forcewake ordering on all gens. > > Reasoning is that I would like to see this stabilize a short while > before introducing further changes. I burned my fingers already touching > these, so they need to heal :) > > Ok if this is for future work? The bug is in -next, so take advantage of the guinea pigs... -Chris
Chris Wilson <chris@chris-wilson.co.uk> writes: > On Wed, Jan 28, 2015 at 12:59:52PM +0200, Mika Kuoppala wrote: >> Chris Wilson <chris@chris-wilson.co.uk> writes: >> > Also, why are we not calling fw_domain_reset() from fw_domain_init()? >> > That would be enough to avoid the early santize required for ivb, >> > right? >> >> Agreed here. That was my plan originally, doing the sanitize inside >> in domain inits. But I wanted to fix this particular item by trying to >> be as close as possible to the previous init/forcewake ordering on all gens. >> >> Reasoning is that I would like to see this stabilize a short while >> before introducing further changes. I burned my fingers already touching >> these, so they need to heal :) >> >> Ok if this is for future work? > > The bug is in -next, so take advantage of the guinea pigs... v2 in: 1422449006-4028-1-git-send-email-mika.kuoppala@intel.com -Mika
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5673
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV 353/353 353/353
ILK -3 355/355 352/355
SNB 400/422 400/422
IVB +1-1 485/487 485/487
BYT 296/296 296/296
HSW +1 507/508 508/508
BDW 401/402 401/402
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*ILK igt_drv_suspend_debugfs-reader PASS(3, M26) DMESG_WARN(1, M26)
*ILK igt_gem_unfence_active_buffers PASS(3, M26) DMESG_WARN(1, M26)
*ILK igt_gem_workarounds_suspend-resume PASS(3, M26) DMESG_WARN(1, M26)
IVB igt_gem_storedw_batches_loop_normal DMESG_WARN(6, M34M4)PASS(16, M34M4M21) PASS(1, M34)
IVB igt_gem_storedw_batches_loop_secure-dispatch DMESG_WARN(1, M34)PASS(7, M34M4) DMESG_WARN(1, M34)
HSW igt_gem_pwrite_pread_snooped-pwrite-blt-cpu_mmap-performance DMESG_WARN(1, M40)PASS(20, M40M20) PASS(1, M40)
Note: You need to pay more attention to line start with '*'
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index b3951f2..c438ca4 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -72,6 +72,7 @@ assert_device_not_suspended(struct drm_i915_private *dev_priv) static inline void fw_domain_reset(const struct intel_uncore_forcewake_domain *d) { + WARN_ON(d->reg_set == 0); __raw_i915_write32(d->i915, d->reg_set, d->val_reset); } @@ -166,6 +167,8 @@ fw_domains_reset(struct drm_i915_private *dev_priv, enum forcewake_domains fw_do struct intel_uncore_forcewake_domain *d; enum forcewake_domain_id id; + WARN_ON(dev_priv->uncore.fw_domains == 0); + for_each_fw_domain_mask(d, fw_domains, dev_priv, id) fw_domain_reset(d); @@ -987,8 +990,7 @@ static void fw_domain_init(struct drm_i915_private *dev_priv, void intel_uncore_init(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; - - __intel_uncore_early_sanitize(dev, false); + bool sanitize_done = false; if (IS_GEN9(dev)) { dev_priv->uncore.funcs.force_wake_get = fw_domains_get; @@ -1037,6 +1039,8 @@ void intel_uncore_init(struct drm_device *dev) fw_domain_init(dev_priv, FW_DOMAIN_ID_RENDER, FORCEWAKE_MT, FORCEWAKE_MT_ACK); + __intel_uncore_early_sanitize(dev, false); + sanitize_done = true; mutex_lock(&dev->struct_mutex); fw_domains_get_with_thread_status(dev_priv, FORCEWAKE_ALL); ecobus = __raw_i915_read32(dev_priv, ECOBUS); @@ -1058,6 +1062,9 @@ void intel_uncore_init(struct drm_device *dev) FORCEWAKE, FORCEWAKE_ACK); } + if (sanitize_done == false) + __intel_uncore_early_sanitize(dev, false); + switch (INTEL_INFO(dev)->gen) { default: MISSING_CASE(INTEL_INFO(dev)->gen);
intel_uncore_early_sanitize() will reset the forcewake registers. When forcewake domains were introduced, the domain init was done after the sanitization of the forcewake registers. And as the resetting of registers use the domain accessors, we tried to reset the forcewake registers with unitialized forcewake domains and failed. Fix this by sanitizing after all the domains have been initialized. On ivb we need special care as there we need early forcewake access to determine the final configuration for the forcewake domain. This regression was introduced in commit 05a2fb157e44a53c79133805d30eaada43911941 Author: Mika Kuoppala <mika.kuoppala@linux.intel.com> Date: Mon Jan 19 16:20:43 2015 +0200 drm/i915: Consolidate forcewake code Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88805 Reported-by: Olof Johansson <olof@lixom.net> Tested-by: Darren Hart <dvhart@linux.intel.com> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> --- drivers/gpu/drm/i915/intel_uncore.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)