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@xxxxxxxxxxxxxxx> > > 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@xxxxxxxxx> > > Tested-by: Darren Hart <dvhart@xxxxxxxxxxxxxxx> > > Signed-off-by: Mika Kuoppala <mika.kuoppala@xxxxxxxxx> > > --- > > 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, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
