[Intel-gfx] [PATCH] drm/i915: Do uncore early sanitize after domain init

[Chris Wilson]

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 at 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 at lixom.net>
> > Tested-by: Darren Hart <dvhart at linux.intel.com>
> > Signed-off-by: Mika Kuoppala <mika.kuoppala at 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, Intel Open Source Technology Centre