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

[Jesse Barnes]

On Fri, 27 Sep 2013 23:09:26 +0300
Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:

> On Fri, Sep 27, 2013 at 12:22:11PM -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,
> 
> Actually AFAIU the cri clock has to be running even if we just attempt
> to do any register access to the phy.

Ah hm, that explains the comment below.  So yeah stuffing this into
init_hw would be good, and just keeping it on all the time.

> 
> > 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.
> > 
> > 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>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 27 ++++++++++++++++++++++++---
> >  1 file changed, 24 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 0eeba84..def2473 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1386,6 +1386,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 to use the integrated clock source */
> > +	if (!crtc->pipe)
> > +		I915_WRITE(DPLL(1), I915_READ(DPLL(1)) |
> > +			   DPLL_INTEGRATED_CRI_CLK_VLV);
> 
> We may need the cri clock before this, so I would just put this part
> into some modeset hw init function.
> 
> > +	else
> > +		dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
> 
> And obviously this part has to stay here, or we could do a 
>  'I915_READ(DPLL(1)) & DPLL_INTEGRATED_CRI_CLK_VLV)' to extract the
> current setting from the hardware like you do in vlv_disable_pll().
> In any case I think doing it the same way for both enable and
> disable would be good.
> 
> Or we could leave setting of this bit up to vlv_update_pll() like it was
> before.
> 
> > +
> >  	I915_WRITE(reg, dpll);
> >  	POSTING_READ(reg);
> >  	udelay(150);
> > @@ -1476,6 +1483,20 @@ static void i9xx_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
> >  	POSTING_READ(DPLL(pipe));
> >  }
> >  
> > +static void vlv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
> > +{
> > +	u32 val = 0;
> > +
> > +	/* Make sure the pipe isn't still relying on us */
> > +	assert_pipe_disabled(dev_priv, pipe);
> > +
> > +	/* Leave integrated clock source enabled for the other pipe */
> > +	if (pipe)
> > +		val = I915_READ(DPLL(pipe)) & DPLL_INTEGRATED_CRI_CLK_VLV;
> 
> Right. As stated we need to either hardcode the bit on (if we assume 1
> is the right answer always) or we read it back like you do.

Yeah I'll just keep it enabled all the time.

The other bit I was missing is that the hw state tracking needs to have
the bit set, or it'll complain about a DPLL mismatch.

I'll post a fixed up one with both your and Daniel's changes applied.

-- 
Jesse Barnes, Intel Open Source Technology Center