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

[Ville Syrjälä]

On Sat, Sep 28, 2013 at 08:05:14AM -0700, Jesse Barnes wrote:
> 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).

Yeah I think this doesn't actually enable the clock. IIRC there's some
DPIO refclock stuff in ccu, so we may be able to turn it off there if
needed.

But what happened to the idea to set this bit in init_hw or somesuch
place and just preserving it for pipe B in the PLL funcs?

> 
> > 
> > > 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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC