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

[Jesse Barnes]

On Tue, 1 Oct 2013 00:19:55 +0300
Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:

> 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?

It's being set early and preserved in the v3 version (well overwritten,
but that's the same number of lines).

Daniel and I briefly discussed having a global settings mask for the
PLL checker and global_resources too, but that would be even more lines
than this simple approach.

Jesse