On Mon, Jun 10, 2013 at 04:34:05PM +0200, Daniel Vetter wrote:
> On Mon, Jun 10, 2013 at 12:11 PM, Ville Syrj?l?
> <ville.syrjala at linux.intel.com> wrote:
> > 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 at 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);
>
> That one's gonna misfire every time since we set pll->on = true only
> at the end of the function in this case.
>
> > and maybe also:
> > + assert_pch_pll_disabled(dev_priv, pll, NULL);
>
> This one should already be in the platform-specific pll->enable hook.
> It's added in "drm/i915: enable/disable hooks for shared dplls"
>
> > Or maybe just kill 'pll->on' as it seems totally redundant.
>
> Yeah, I've considered that but independently checking pll->on with the
> hw state and pll->active with the number of crtc using it looked
> neated. I guess we could rip out pll->on as a follow-up though.
>
> > 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().
>
> The totally paranoid hw state cross checker does that at the very end
> of each modeset. The checks in here are simply to assert a bunch of
> edge transitions. And like I've said, I think I pretty much have it
> all covered.
Before we set pll->on to true, pll->on should be false, no?
--
Ville Syrj?l?
Intel OTC
