Re: [PATCH 1/2] drm/i915: Allow /2 CD2X divider on gen11+

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Aug 26, 2019 at 03:55:39PM -0700, Matt Roper wrote:
> The bspec has just recently been updated with new cdclk values that
> require the use of a /2 CD2X divider rather than a /1 divider.  Once we
> add the divider selection logic to ICL+ cdclk programming, we have
> pretty much the same logic we were already using on CNL, so it's simpler
> to drop icl_set_cdclk() completely and reuse cnl_set_cdclk() on gen11+
> platforms as well.
> 
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.c | 90 +++++++++-------------
>  drivers/gpu/drm/i915/i915_reg.h            |  1 +
>  2 files changed, 36 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index 939088c7d814..a56ccd0930e0 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -1659,10 +1659,23 @@ static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
>  		cnl_cdclk_pll_enable(dev_priv, vco);
>  
>  	val = divider | skl_cdclk_decimal(cdclk);
> -	if (pipe == INVALID_PIPE)
> -		val |= BXT_CDCLK_CD2X_PIPE_NONE;
> -	else
> -		val |= BXT_CDCLK_CD2X_PIPE(pipe);
> +
> +	if (INTEL_GEN(dev_priv) >= 12) {
> +		if (pipe == INVALID_PIPE)
> +			val |= ICL_CDCLK_CD2X_PIPE_NONE;
> +		else
> +			val |= BXT_CDCLK_CD2X_PIPE(pipe);

Hmm. So the mess made here with icl is now fixed. Cool. Sad they
didn't fix it on icl as well.

However I have a feeling this may end up confusing people, so maybe we
should just add TGL_CDCLK_CD2X_PIPE()?

> +	} else if (INTEL_GEN(dev_priv) >= 11) {
> +		if (pipe == INVALID_PIPE)
> +			val |= ICL_CDCLK_CD2X_PIPE_NONE;
> +		else
> +			val |= ICL_CDCLK_CD2X_PIPE(pipe);
> +	} else {
> +		if (pipe == INVALID_PIPE)
> +			val |= BXT_CDCLK_CD2X_PIPE_NONE;
> +		else
> +			val |= BXT_CDCLK_CD2X_PIPE(pipe);
> +	}
>  	I915_WRITE(CDCLK_CTL, val);
>  
>  	if (pipe != INVALID_PIPE)
> @@ -1813,51 +1826,6 @@ static int icl_calc_cdclk_pll_vco(struct drm_i915_private *dev_priv, int cdclk)
>  	return dev_priv->cdclk.hw.ref * ratio;
>  }
>  
> -static void icl_set_cdclk(struct drm_i915_private *dev_priv,
> -			  const struct intel_cdclk_state *cdclk_state,
> -			  enum pipe pipe)
> -{
> -	unsigned int cdclk = cdclk_state->cdclk;
> -	unsigned int vco = cdclk_state->vco;
> -	int ret;
> -
> -	ret = skl_pcode_request(dev_priv, SKL_PCODE_CDCLK_CONTROL,
> -				SKL_CDCLK_PREPARE_FOR_CHANGE,
> -				SKL_CDCLK_READY_FOR_CHANGE,
> -				SKL_CDCLK_READY_FOR_CHANGE, 3);
> -	if (ret) {
> -		DRM_ERROR("Failed to inform PCU about cdclk change (%d)\n",
> -			  ret);
> -		return;
> -	}
> -
> -	if (dev_priv->cdclk.hw.vco != 0 &&
> -	    dev_priv->cdclk.hw.vco != vco)
> -		cnl_cdclk_pll_disable(dev_priv);
> -
> -	if (dev_priv->cdclk.hw.vco != vco)
> -		cnl_cdclk_pll_enable(dev_priv, vco);
> -
> -	/*
> -	 * On ICL CD2X_DIV can only be 1, so we'll never end up changing the
> -	 * divider here synchronized to a pipe while CDCLK is on, nor will we
> -	 * need the corresponding vblank wait.
> -	 */
> -	I915_WRITE(CDCLK_CTL, ICL_CDCLK_CD2X_PIPE_NONE |
> -			      skl_cdclk_decimal(cdclk));
> -
> -	sandybridge_pcode_write(dev_priv, SKL_PCODE_CDCLK_CONTROL,
> -				cdclk_state->voltage_level);
> -
> -	intel_update_cdclk(dev_priv);
> -
> -	/*
> -	 * Can't read out the voltage level :(
> -	 * Let's just assume everything is as expected.
> -	 */
> -	dev_priv->cdclk.hw.voltage_level = cdclk_state->voltage_level;
> -}
> -
>  static u8 icl_calc_voltage_level(struct drm_i915_private *dev_priv, int cdclk)
>  {
>  	if (IS_ELKHARTLAKE(dev_priv)) {
> @@ -1881,6 +1849,7 @@ static void icl_get_cdclk(struct drm_i915_private *dev_priv,
>  			  struct intel_cdclk_state *cdclk_state)
>  {
>  	u32 val;
> +	int div;
>  
>  	cdclk_state->bypass = 50000;
>  
> @@ -1914,10 +1883,21 @@ static void icl_get_cdclk(struct drm_i915_private *dev_priv,
>  
>  	cdclk_state->vco = (val & BXT_DE_PLL_RATIO_MASK) * cdclk_state->ref;
>  
> -	val = I915_READ(CDCLK_CTL);
> -	WARN_ON((val & BXT_CDCLK_CD2X_DIV_SEL_MASK) != 0);
> +	val = I915_READ(CDCLK_CTL) & BXT_CDCLK_CD2X_DIV_SEL_MASK;
> +	switch (val) {
> +	case BXT_CDCLK_CD2X_DIV_SEL_1:
> +		div = 2;
> +		break;
> +	case BXT_CDCLK_CD2X_DIV_SEL_2:
> +		div = 4;
> +		break;
> +	default:
> +		MISSING_CASE(val);
> +		div = 2;
> +		break;
> +	}
>  
> -	cdclk_state->cdclk = cdclk_state->vco / 2;
> +	cdclk_state->cdclk = cdclk_state->vco / div;

cnl uses DIV_ROUND_CLOSEST(). In general this function looks rather
different to cnl_get_cdclk() even though the actual differences are
minimal. Some unification of style might be nice. Not sure if reusing
the cnl function on icl might actually make sense. Would require
a few ifs.

>  
>  out:
>  	/*
> @@ -1963,7 +1943,7 @@ static void icl_init_cdclk(struct drm_i915_private *dev_priv)
>  				icl_calc_voltage_level(dev_priv,
>  						       sanitized_state.cdclk);
>  
> -	icl_set_cdclk(dev_priv, &sanitized_state, INVALID_PIPE);
> +	cnl_set_cdclk(dev_priv, &sanitized_state, INVALID_PIPE);
>  }
>  
>  static void icl_uninit_cdclk(struct drm_i915_private *dev_priv)
> @@ -1975,7 +1955,7 @@ static void icl_uninit_cdclk(struct drm_i915_private *dev_priv)
>  	cdclk_state.voltage_level = icl_calc_voltage_level(dev_priv,
>  							   cdclk_state.cdclk);
>  
> -	icl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
> +	cnl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
>  }
>  
>  static void cnl_init_cdclk(struct drm_i915_private *dev_priv)
> @@ -2810,7 +2790,7 @@ void intel_update_rawclk(struct drm_i915_private *dev_priv)
>  void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv)
>  {
>  	if (INTEL_GEN(dev_priv) >= 11) {
> -		dev_priv->display.set_cdclk = icl_set_cdclk;
> +		dev_priv->display.set_cdclk = cnl_set_cdclk;
>  		dev_priv->display.modeset_calc_cdclk = icl_modeset_calc_cdclk;
>  	} else if (IS_CANNONLAKE(dev_priv)) {
>  		dev_priv->display.set_cdclk = cnl_set_cdclk;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index a142aa3d74e3..958dfdfb4e10 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -9671,6 +9671,7 @@ enum skl_power_gate {
>  #define  BXT_CDCLK_CD2X_PIPE(pipe)	((pipe) << 20)
>  #define  CDCLK_DIVMUX_CD_OVERRIDE	(1 << 19)
>  #define  BXT_CDCLK_CD2X_PIPE_NONE	BXT_CDCLK_CD2X_PIPE(3)
> +#define  ICL_CDCLK_CD2X_PIPE(pipe)	(_PICK(pipe, 0, 2, 6) << 19)

Could also do eg.
((((pipe) << 1) | ((pipe) & ~1)) << 19)

if we want to avoid the _PICK(). Doesn't really matter I suppose.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  #define  ICL_CDCLK_CD2X_PIPE_NONE	(7 << 19)
>  #define  BXT_CDCLK_SSA_PRECHARGE_ENABLE	(1 << 16)
>  #define  CDCLK_FREQ_DECIMAL_MASK	(0x7ff)
> -- 
> 2.20.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx