Re: [PATCH] drm/i915: split intel_ddi_pll_mode_set in 2 pieces

[Damien Lespiau <damien.lespiau@intel.com>]

On Mon, Nov 25, 2013 at 03:27:08PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> The first piece, intel_ddi_pll_select, finds a PLL and assigns it to
> the CRTC, but doesn't write any register. It can also fail in case it
> doesn't find a PLL.
> 
> The second piece, intel_ddi_pll_enable, uses the information stored by
> intel_ddi_pll_select to actually enable the PLL by writing to its
> register. This function can't fail. We also have some refcount sanity
> checks here.
> 
> The idea is that one day we'll remove all the functions that touch
> registers from haswell_crtc_mode_set to haswell_crtc_enable, so we'll
> call intel_ddi_pll_select at haswell_crtc_mode_set and then call
> intel_ddi_pll_enable at haswell_crtc_enable. Since I'm already
> touching this code, let's take care of this particular split today.
> 
> v2: - Clock on the debug message is in KHz
>     - Add missing POSTING_READ
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>


Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

/*
 * As a side note, CodingStyle stipulates this is the preferred style
 * for multi-line comments. Really a small detail to keep in my in the
 * future :)
 */

-- 
Damien

> ---
>  drivers/gpu/drm/i915/intel_ddi.c     | 102 ++++++++++++++++++++++++++++-------
>  drivers/gpu/drm/i915/intel_display.c |   3 +-
>  drivers/gpu/drm/i915/intel_drv.h     |   3 +-
>  3 files changed, 88 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 731a919..4263ec3 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -696,21 +696,21 @@ intel_ddi_calculate_wrpll(int clock /* in Hz */,
>  	*n2_out = best.n2;
>  	*p_out = best.p;
>  	*r2_out = best.r2;
> -
> -	DRM_DEBUG_KMS("WRPLL: %dHz refresh rate with p=%d, n2=%d r2=%d\n",
> -		      clock, *p_out, *n2_out, *r2_out);
>  }
>  
> -bool intel_ddi_pll_mode_set(struct drm_crtc *crtc)
> +/* Tries to find a PLL for the CRTC. If it finds, it increases the refcount and
> + * stores it in intel_crtc->ddi_pll_sel, so other mode sets won't be able to
> + * steal the selected PLL. You need to call intel_ddi_pll_enable to actually
> + * enable the PLL. */
> +bool intel_ddi_pll_select(struct intel_crtc *intel_crtc)
>  {
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct drm_crtc *crtc = &intel_crtc->base;
>  	struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc);
>  	struct drm_encoder *encoder = &intel_encoder->base;
>  	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
>  	struct intel_ddi_plls *plls = &dev_priv->ddi_plls;
>  	int type = intel_encoder->type;
>  	enum pipe pipe = intel_crtc->pipe;
> -	uint32_t reg, val;
>  	int clock = intel_crtc->config.port_clock;
>  
>  	intel_ddi_put_crtc_pll(crtc);
> @@ -734,10 +734,8 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc)
>  			return false;
>  		}
>  
> -		/* We don't need to turn any PLL on because we'll use LCPLL. */
> -		return true;
> -
>  	} else if (type == INTEL_OUTPUT_HDMI) {
> +		uint32_t reg, val;
>  		unsigned p, n2, r2;
>  
>  		intel_ddi_calculate_wrpll(clock * 1000, &r2, &n2, &p);
> @@ -767,6 +765,9 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc)
>  			return false;
>  		}
>  
> +		DRM_DEBUG_KMS("WRPLL: %dKHz refresh rate with p=%d, n2=%d r2=%d\n",
> +			      clock, p, n2, r2);
> +
>  		if (reg == WRPLL_CTL1) {
>  			plls->wrpll1_refcount++;
>  			intel_crtc->ddi_pll_sel = PORT_CLK_SEL_WRPLL1;
> @@ -780,29 +781,94 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc)
>  			DRM_DEBUG_KMS("Using SPLL on pipe %c\n",
>  				      pipe_name(pipe));
>  			plls->spll_refcount++;
> -			reg = SPLL_CTL;
>  			intel_crtc->ddi_pll_sel = PORT_CLK_SEL_SPLL;
>  		} else {
>  			DRM_ERROR("SPLL already in use\n");
>  			return false;
>  		}
>  
> -		WARN(I915_READ(reg) & SPLL_PLL_ENABLE,
> -		     "SPLL already enabled\n");
> -
> -		val = SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | SPLL_PLL_SSC;
> -
>  	} else {
>  		WARN(1, "Invalid DDI encoder type %d\n", type);
>  		return false;
>  	}
>  
> -	I915_WRITE(reg, val);
> -	udelay(20);
> -
>  	return true;
>  }
>  
> +/* To be called after intel_ddi_pll_select(). That one selects the PLL to be
> + * used, this one actually enables the PLL. */
> +void intel_ddi_pll_enable(struct intel_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_ddi_plls *plls = &dev_priv->ddi_plls;
> +	int clock = crtc->config.port_clock;
> +	uint32_t reg, cur_val, new_val;
> +	int refcount;
> +	const char *pll_name;
> +	uint32_t enable_bit = (1 << 31);
> +	unsigned int p, n2, r2;
> +
> +	BUILD_BUG_ON(enable_bit != SPLL_PLL_ENABLE);
> +	BUILD_BUG_ON(enable_bit != WRPLL_PLL_ENABLE);
> +
> +	switch (crtc->ddi_pll_sel) {
> +	case PORT_CLK_SEL_LCPLL_2700:
> +	case PORT_CLK_SEL_LCPLL_1350:
> +	case PORT_CLK_SEL_LCPLL_810:
> +		/* LCPLL should always be enabled at this point of the mode set
> +		 * sequence, so nothing to do. */
> +		return;
> +
> +	case PORT_CLK_SEL_SPLL:
> +		pll_name = "SPLL";
> +		reg = SPLL_CTL;
> +		refcount = plls->spll_refcount;
> +		new_val = SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz |
> +			  SPLL_PLL_SSC;
> +		break;
> +
> +	case PORT_CLK_SEL_WRPLL1:
> +	case PORT_CLK_SEL_WRPLL2:
> +		if (crtc->ddi_pll_sel == PORT_CLK_SEL_WRPLL1) {
> +			pll_name = "WRPLL1";
> +			reg = WRPLL_CTL1;
> +			refcount = plls->wrpll1_refcount;
> +		} else {
> +			pll_name = "WRPLL2";
> +			reg = WRPLL_CTL2;
> +			refcount = plls->wrpll2_refcount;
> +		}
> +
> +		intel_ddi_calculate_wrpll(clock * 1000, &r2, &n2, &p);
> +
> +		new_val = WRPLL_PLL_ENABLE | WRPLL_PLL_SELECT_LCPLL_2700 |
> +			  WRPLL_DIVIDER_REFERENCE(r2) |
> +			  WRPLL_DIVIDER_FEEDBACK(n2) | WRPLL_DIVIDER_POST(p);
> +
> +		break;
> +
> +	case PORT_CLK_SEL_NONE:
> +		WARN(1, "Bad selected pll: PORT_CLK_SEL_NONE\n");
> +		return;
> +	default:
> +		WARN(1, "Bad selected pll: 0x%08x\n", crtc->ddi_pll_sel);
> +		return;
> +	}
> +
> +	cur_val = I915_READ(reg);
> +
> +	WARN(refcount < 1, "Bad %s refcount: %d\n", pll_name, refcount);
> +	if (refcount == 1) {
> +		WARN(cur_val & enable_bit, "%s already enabled\n", pll_name);
> +		I915_WRITE(reg, new_val);
> +		POSTING_READ(reg);
> +		udelay(20);
> +	} else {
> +		WARN((cur_val & enable_bit) == 0, "%s disabled\n", pll_name);
> +	}
> +}
> +
>  void intel_ddi_set_pipe_settings(struct drm_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e85d838..667f641 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6880,8 +6880,9 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>  	int plane = intel_crtc->plane;
>  	int ret;
>  
> -	if (!intel_ddi_pll_mode_set(crtc))
> +	if (!intel_ddi_pll_select(intel_crtc))
>  		return -EINVAL;
> +	intel_ddi_pll_enable(intel_crtc);
>  
>  	if (intel_crtc->config.has_dp_encoder)
>  		intel_dp_set_m_n(intel_crtc);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0231281..76f52b6 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -612,7 +612,8 @@ void intel_ddi_disable_transcoder_func(struct drm_i915_private *dev_priv,
>  void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc);
>  void intel_ddi_disable_pipe_clock(struct intel_crtc *intel_crtc);
>  void intel_ddi_setup_hw_pll_state(struct drm_device *dev);
> -bool intel_ddi_pll_mode_set(struct drm_crtc *crtc);
> +bool intel_ddi_pll_select(struct intel_crtc *crtc);
> +void intel_ddi_pll_enable(struct intel_crtc *crtc);
>  void intel_ddi_put_crtc_pll(struct drm_crtc *crtc);
>  void intel_ddi_set_pipe_settings(struct drm_crtc *crtc);
>  void intel_ddi_prepare_link_retrain(struct drm_encoder *encoder);
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx