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

[Daniel Vetter <daniel@ffwll.ch>]

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.

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

Dropped some paste garbage by accident?

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

> +			   DPLL_INTEGRATED_CRI_CLK_VLV);
> +
>  	I915_WRITE(reg, dpll);
>  	POSTING_READ(reg);
>  	udelay(150);
> @@ -1477,6 +1484,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 */
> +	if (pipe == PIPE_B)
> +		val = 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;
> @@ -3887,7 +3908,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;
> @@ -4627,9 +4650,9 @@ 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)
> +	/* We should never disable this, set it here for state tracking */
> +	if (pipe == PIPE_B)
>  		dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
> -
>  	dpll |= DPLL_VCO_ENABLE;
>  	crtc->config.dpll_hw_state.dpll = dpll;
>  
> @@ -10296,8 +10319,15 @@ void i915_disable_vga_mem(struct drm_device *dev)
>  
>  void intel_modeset_init_hw(struct drm_device *dev)
>  {
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
>  	intel_prepare_ddi(dev);
>  
> +	/* Enable the CRI clock source so we can get at the display */
> +	if (IS_VALLEYVIEW(dev))
> +		I915_WRITE(DPLL(1), I915_READ(DPLL(1)) |

s/1/PIPE_B/g

Also there's this --in-reply-to switch for git send-email to keep
discussions on the m-l neatly tied up ;-)


> +			   DPLL_INTEGRATED_CRI_CLK_VLV);
> +
>  	intel_init_dpio(dev);
>  
>  	intel_init_clock_gating(dev);
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch