Re: [PATCH 03/31] drm/i915: lock down pch pll accouting some more

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Jun 07, 2013 at 11:13:32PM +0200, Daniel Vetter wrote:
> On Fri, Jun 07, 2013 at 11:46:08PM +0300, Ville Syrjälä wrote:
> > On Fri, Jun 07, 2013 at 10:03:20PM +0200, Daniel Vetter wrote:
> > > On Fri, Jun 07, 2013 at 07:32:56PM +0300, Ville Syrjälä wrote:
> > > > On Wed, Jun 05, 2013 at 01:34:05PM +0200, Daniel Vetter wrote:
> > > > > Before I start to make a complete mess out of this, crank up
> > > > > the paranoia level a bit.
> > > > > 
> > > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_display.c | 9 ++++++++-
> > > > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > > index 56fb6ed..39e977f 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > @@ -1440,6 +1440,7 @@ static void intel_disable_pch_pll(struct intel_crtc *intel_crtc)
> > > > >  	}
> > > > >  
> > > > >  	assert_pch_pll_enabled(dev_priv, pll, NULL);
> > > > > +	WARN_ON(!pll->on);
> > > > >  	if (--pll->active)
> > > > >  		return;
> > > > 
> > > > Maybe a WARN_ON(pll->on) near the end of ironlake_enable_pch_pll() too?
> > > 
> > > At the very end we set on = true, and the only non-error early return
> > > (when the active refcount is > 0 to begin with) has alreay a
> > > WARN_ON(!pll->on). Shouldn't that be good enough?
> > 
> > Well I was just thinking that since we have this dual bookeeping w/
> > active and on, maybe we want to warn if things go out of sync.
> 
> Now I'm confused. I've tried to explain why I think we already have full
> checking of pll->on in enable_shared_dpll ... Can you maybe show in a diff
> where you'd want to add more?

Something like this:

	if (pll->active++) {
		WARN_ON(!pll->on);
		assert_pch_pll_enabled(dev_priv, pll, NULL);
		return;
	}
+	WARN_ON(pll->on);

and maybe also:
+	assert_pch_pll_disabled(dev_priv, pll, NULL);


Or maybe just kill 'pll->on' as it seems totally redundant.

Also maybe we could move most of the asserts and WARNs to some
central location. Currently there are quite a few early return paths
from the pll enable/disable functions, and I don't think we perform the
same checks for all of the branches. So maybe we could just have one
function that would cross-check pll->on, pll->active and the real hardware
state. We could call said function just before and after
enable/disable_pch_pll().

-- 
Ville Syrjälä
Intel OTC