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

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

[Jesse Barnes]

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.

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@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);
+	else
+		dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
+
 	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;
+	I915_WRITE(DPLL(pipe), val);
+	POSTING_READ(DPLL(pipe));
+}
+
 void vlv_wait_port_ready(struct drm_i915_private *dev_priv, int port)
 {
 	u32 port_mask;
@@ -3886,7 +3907,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 		if (encoder->post_disable)
 			encoder->post_disable(encoder);
 
-	if (!intel_pipe_has_type(crtc, INTEL_OUTPUT_DSI))
+	if (IS_VALLEYVIEW(dev) && !intel_pipe_has_type(crtc, INTEL_OUTPUT_DSI))
+		vlv_disable_pll(dev_priv, pipe);
+	else if (!IS_VALLEYVIEW(dev))
 		i9xx_disable_pll(dev_priv, pipe);
 
 	intel_crtc->active = false;
@@ -4626,8 +4649,6 @@ static void vlv_update_pll(struct intel_crtc *crtc)
 	/* Enable DPIO clock input */
 	dpll = DPLL_EXT_BUFFER_ENABLE_VLV | DPLL_REFA_CLK_ENABLE_VLV |
 		DPLL_VGA_MODE_DIS | DPLL_INTEGRATED_CLOCK_VLV;
-	if (pipe)
-		dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
 
 	dpll |= DPLL_VCO_ENABLE;
 	crtc->config.dpll_hw_state.dpll = dpll;
-- 
1.8.3.1

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

[Daniel Vetter]

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, 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@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)

I prefer explicit pipe != PIPE_B checks. We have a bunch of those already
in vlv code, and every time I read one of them my parser trips. Treating
enums as booleans is imo just evil ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Ville Syrjälä]

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.

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

> +	I915_WRITE(DPLL(pipe), val);
> +	POSTING_READ(DPLL(pipe));
> +}
> +
>  void vlv_wait_port_ready(struct drm_i915_private *dev_priv, int port)
>  {
>  	u32 port_mask;
> @@ -3886,7 +3907,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>  		if (encoder->post_disable)
>  			encoder->post_disable(encoder);
>  
> -	if (!intel_pipe_has_type(crtc, INTEL_OUTPUT_DSI))
> +	if (IS_VALLEYVIEW(dev) && !intel_pipe_has_type(crtc, INTEL_OUTPUT_DSI))
> +		vlv_disable_pll(dev_priv, pipe);
> +	else if (!IS_VALLEYVIEW(dev))
>  		i9xx_disable_pll(dev_priv, pipe);
>  
>  	intel_crtc->active = false;
> @@ -4626,8 +4649,6 @@ static void vlv_update_pll(struct intel_crtc *crtc)
>  	/* Enable DPIO clock input */
>  	dpll = DPLL_EXT_BUFFER_ENABLE_VLV | DPLL_REFA_CLK_ENABLE_VLV |
>  		DPLL_VGA_MODE_DIS | DPLL_INTEGRATED_CLOCK_VLV;
> -	if (pipe)
> -		dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
>  
>  	dpll |= DPLL_VCO_ENABLE;
>  	crtc->config.dpll_hw_state.dpll = dpll;
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

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