Re: [PATCH 16/19] drm/i915: sanity check power well sw state against hw state

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Feb 18, 2014 at 07:59:05PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 18, 2014 at 07:37:01PM +0200, Imre Deak wrote:
> > On Tue, 2014-02-18 at 18:55 +0200, Ville Syrjälä wrote:
> > > On Tue, Feb 18, 2014 at 12:02:17AM +0200, Imre Deak wrote:
> > > > Suggested by Daniel.
> > > > 
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_pm.c | 33 ++++++++++++++++++++++++++++++---
> > > >  1 file changed, 30 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > index e81e7de..21ccf89 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > @@ -5338,6 +5338,24 @@ static void hsw_power_well_disable(struct drm_i915_private *dev_priv,
> > > >  	hsw_enable_package_c8(dev_priv);
> > > >  }
> > > >  
> > > > +static void check_power_well_state(struct drm_i915_private *dev_priv,
> > > > +				   struct i915_power_well *power_well)
> > > > +{
> > > > +	bool enabled;
> > > > +
> > > > +	if (!power_well->ops->is_enabled)
> > > > +		return;
> > > > +
> > > > +	enabled = power_well->ops->is_enabled(dev_priv, power_well);
> > > > +
> > > > +	if (enabled != (power_well->count > 0 || !i915.disable_power_well)) {
> > > 
> > > Doesn't i915.disable_power_well==true mean "leave power wells always
> > > enabled"? So I think the '!' needs to be removed.
> > 
> > No, i915.disable_power_well==true means disable power wells when the
> > refcount goes to 0. Perhaps not the best name/semantics for this kind of
> > option, the default for it should be 0 and mean normal operation, which
> > is to disable power wells when possible.
> 
> Oh I had the impression it was the other way around, but you're right.
> Seems I keep getting confused by this thing. It has happened before and
> I'm guessing it will happen again after I've forgotten the details
> again.

I concur that a module option rename pass is in order. Maybe once this has
all settled a bit ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch