Re: [PATCH 27/31] drm/i915: move i9xx dpll enabling into crtc enable function

[Imre Deak <imre.deak@intel.com>]

On Wed, 2013-06-05 at 13:34 +0200, Daniel Vetter wrote:
> Now that we have the proper pipe config to track this, we don't need
> to write any registers any more.
> 
> v2: Drop a few now unnecessary local variables and switch the enable
> function to take a struct intel_crtc * to simply arguments.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 102 +++++++++++++----------------------
>  1 file changed, 37 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b9be047..b6f5e48 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1298,32 +1298,48 @@ static void vlv_enable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
>  	udelay(150); /* wait for warmup */
>  }
>  
> -static void i9xx_enable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
> +static void i9xx_enable_pll(struct intel_crtc *crtc)
>  {
> -	int reg;
> -	u32 val;
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int reg = DPLL(crtc->pipe);
> +	u32 dpll = crtc->config.dpll_hw_state.dpll;
>  
> -	assert_pipe_disabled(dev_priv, pipe);
> +	assert_pipe_disabled(dev_priv, crtc->pipe);
>  
>  	/* No really, not for ILK+ */
> -	BUG_ON(!IS_VALLEYVIEW(dev_priv->dev) && dev_priv->info->gen >= 5);
> +	BUG_ON(!IS_VALLEYVIEW(dev) && dev_priv->info->gen >= 5);
>  
>  	/* PLL is protected by panel, make sure we can write it */
> -	if (IS_MOBILE(dev_priv->dev) && !IS_I830(dev_priv->dev))
> -		assert_panel_unlocked(dev_priv, pipe);
> +	if (IS_MOBILE(dev) && !IS_I830(dev))
> +		assert_panel_unlocked(dev_priv, crtc->pipe);
>  
> -	reg = DPLL(pipe);
> -	val = I915_READ(reg);
> -	val |= DPLL_VCO_ENABLE;
> +	I915_WRITE(reg, dpll);
> +
> +	/* Wait for the clocks to stabilize. */
> +	POSTING_READ(reg);
> +	udelay(150);
> +
> +	if (INTEL_INFO(dev)->gen >= 4) {
> +		I915_WRITE(DPLL_MD(crtc->pipe),
> +			   crtc->config.dpll_hw_state.dpll_md);
> +	} else {
> +		/* The pixel multiplier can only be updated once the
> +		 * DPLL is enabled and the clocks are stable.
> +		 *
> +		 * So write it again.
> +		 */
> +		I915_WRITE(reg, dpll);

It's ok but isn't really needed any more as now we write dpll right
after this with the same value.

> +	}
>  
>  	/* We do this three times for luck */
> -	I915_WRITE(reg, val);
> +	I915_WRITE(reg, dpll);
>  	POSTING_READ(reg);
>  	udelay(150); /* wait for warmup */
> -	I915_WRITE(reg, val);
> +	I915_WRITE(reg, dpll);
>  	POSTING_READ(reg);
>  	udelay(150); /* wait for warmup */
> -	I915_WRITE(reg, val);
> +	I915_WRITE(reg, dpll);
>  	POSTING_READ(reg);
>  	udelay(150); /* wait for warmup */
>  }
> @@ -3591,7 +3607,11 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>  	intel_crtc->active = true;
>  	intel_update_watermarks(dev);
>  
> -	i9xx_enable_pll(dev_priv, pipe);
> +	for_each_encoder_on_crtc(dev, crtc, encoder)
> +		if (encoder->pre_pll_enable)
> +			encoder->pre_pll_enable(encoder);
> +
> +	i9xx_enable_pll(intel_crtc);
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		if (encoder->pre_enable)
> @@ -4429,8 +4449,6 @@ static void i9xx_update_pll(struct intel_crtc *crtc,
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_encoder *encoder;
> -	int pipe = crtc->pipe;
>  	u32 dpll;
>  	bool is_sdvo;
>  	struct dpll *clock = &crtc->config.dpll;
> @@ -4494,37 +4512,14 @@ static void i9xx_update_pll(struct intel_crtc *crtc,
>  	dpll |= DPLL_VCO_ENABLE;
>  	crtc->config.dpll_hw_state.dpll = dpll;
>  
> -	I915_WRITE(DPLL(pipe), dpll & ~DPLL_VCO_ENABLE);
> -	POSTING_READ(DPLL(pipe));
> -	udelay(150);
> -
> -	for_each_encoder_on_crtc(dev, &crtc->base, encoder)
> -		if (encoder->pre_pll_enable)
> -			encoder->pre_pll_enable(encoder);
> -
> -	if (crtc->config.has_dp_encoder)
> -		intel_dp_set_m_n(crtc);
> -
> -	I915_WRITE(DPLL(pipe), dpll);
> -
> -	/* Wait for the clocks to stabilize. */
> -	POSTING_READ(DPLL(pipe));
> -	udelay(150);
> -
>  	if (INTEL_INFO(dev)->gen >= 4) {
>  		u32 dpll_md = (crtc->config.pixel_multiplier - 1)
>  			<< DPLL_MD_UDI_MULTIPLIER_SHIFT;
>  		crtc->config.dpll_hw_state.dpll_md = dpll_md;
> -
> -		I915_WRITE(DPLL_MD(pipe), dpll_md);
> -	} else {
> -		/* The pixel multiplier can only be updated once the
> -		 * DPLL is enabled and the clocks are stable.
> -		 *
> -		 * So write it again.
> -		 */
> -		I915_WRITE(DPLL(pipe), dpll);
>  	}
> +
> +	if (crtc->config.has_dp_encoder)
> +		intel_dp_set_m_n(crtc);
>  }
>  
>  static void i8xx_update_pll(struct intel_crtc *crtc,
> @@ -4533,8 +4528,6 @@ static void i8xx_update_pll(struct intel_crtc *crtc,
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_encoder *encoder;
> -	int pipe = crtc->pipe;
>  	u32 dpll;
>  	struct dpll *clock = &crtc->config.dpll;
>  
> @@ -4561,27 +4554,6 @@ static void i8xx_update_pll(struct intel_crtc *crtc,
>  
>  	dpll |= DPLL_VCO_ENABLE;
>  	crtc->config.dpll_hw_state.dpll = dpll;
> -
> -	I915_WRITE(DPLL(pipe), dpll & ~DPLL_VCO_ENABLE);
> -	POSTING_READ(DPLL(pipe));
> -	udelay(150);
> -
> -	for_each_encoder_on_crtc(dev, &crtc->base, encoder)
> -		if (encoder->pre_pll_enable)
> -			encoder->pre_pll_enable(encoder);
> -
> -	I915_WRITE(DPLL(pipe), dpll);
> -
> -	/* Wait for the clocks to stabilize. */
> -	POSTING_READ(DPLL(pipe));
> -	udelay(150);
> -
> -	/* The pixel multiplier can only be updated once the
> -	 * DPLL is enabled and the clocks are stable.
> -	 *
> -	 * So write it again.
> -	 */
> -	I915_WRITE(DPLL(pipe), dpll);
>  }
>  
>  static void intel_set_pipe_timings(struct intel_crtc *intel_crtc)