Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@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? 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
