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.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
