Re: [PATCH] drm/i915: Do uncore early sanitize after domain init

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

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