Re: [PATCH] drm/i915/vlv: untangle integrated clock source handling v3

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Sep 30, 2013 at 03:20:44PM -0700, Jesse Barnes wrote:
> On Tue, 1 Oct 2013 00:19:55 +0300
> Ville Syrjälä <ville.syrjala@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@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).

Ah so it is. But you're also still setting it in vlv_enable_pll(),
which is no longer necessary.

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

-- 
Ville Syrjälä
Intel OTC