[Intel-gfx] [PATCH] drm/i915/vlv: untangle integrated clock source handling v3

[Jesse Barnes]

On Sat, 28 Sep 2013 11:54:21 +0200
Daniel Vetter <daniel at ffwll.ch> wrote:

> On Fri, Sep 27, 2013 at 04:02:29PM -0700, Jesse Barnes wrote:
> > The global integrated clock source bit resides in DPLL B on VLV, but we
> > were treating it as a per-pipe resource.  It needs to be set whenever
> > any PLL is active, so pull setting the bit out of vlv_update_pll and
> > into vlv_enable_pll.  Also add a vlv_disable_pll to prevent disabling it
> > when pipe B shuts down.
> > 
> > I'm guessing on the references here, I expect this to bite any config
> > where multiple displays are active or displays are moved from pipe to
> > pipe.
> > 
> > v2: re-add bits in vlv_update_pll to keep from confusing the state checker
> > v3: use enum pipe checks (Daniel)
> >     set CRI clock source early (Ville)
> >     consistently set CRI clock source everywhere (Ville)
> 
> Btw do we care about the additional power consumption of having that clock
> running all the time? My long-term plan/idea for these display refclocks
> was to enable/disable them in the ->modeset_global_resources callback so
> that we only ever enable what we actually need. Haswell has this somewhat
> implemented already implicitly through the pc8+ power refcounting.
> 
> Just a "have you thought about this" comment.

Yes I have.  But I don't think this actually enables a clock, just
allows it to flow through to the PLLs and DPIO.  Shutting down the
display power well will probably take care of it though, so I'm not too
worried about it now (plus like Ville said, this may be required to
access some display reg anyway).

> 
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=67245
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=69693
> > Signed-off-by: Jesse Barnes <jbarnes at virtuousgeek.org>
> > 
> > int clock
> 
> Dropped some paste garbage by accident?

Oh you don't think it add anything to the changelog? :p

> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 36 +++++++++++++++++++++++++++++++++---
> >  1 file changed, 33 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 9a83236..1c76a26 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1387,6 +1387,13 @@ static void vlv_enable_pll(struct intel_crtc *crtc)
> >  	if (IS_MOBILE(dev_priv->dev) && !IS_I830(dev_priv->dev))
> >  		assert_panel_unlocked(dev_priv, crtc->pipe);
> >  
> > +	/* Make sure the integrated clock source is enabled */
> > +	if (crtc->pipe == PIPE_B)
> > +		dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
> > +	else
> > +		I915_WRITE(DPLL(1), I915_READ(DPLL(1)) |
> 
> s/1/PIPE_B/g

I call "too late" on this one. :)  You wanted me to avoid enum cast to
bool in your previous reply, but didn't suggest that I use enums as
ints here.  Plus I like this way better... so you can change it when
you apply if you feel strongly about it.

> Also there's this --in-reply-to switch for git send-email to keep
> discussions on the m-l neatly tied up ;-)

Yeah need to fix my send-email on this new machine, it doesn't prompt
me for anything anymore, which is a pain (and why that other patch got
sent out).

-- 
Jesse Barnes, Intel Open Source Technology Center