Re: [Intel-gfx] [PATCH 13/13] drm/i915: Unify the DPLL ref clock frequency tracking

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

On Wed, Feb 26, 2020 at 10:34:55PM +0200, Imre Deak wrote:
> All platforms using the shared DPLL framework use 3 reference clocks for
> their DPLLs: SSC, non-SSC and DSI. For a more unified way across
> platforms store the frequency of these ref clocks as part of the DPLL
> global state. This also allows us to keep the HW access reading out the
> ref clock value separate from the DPLL frequency calculation that
> depends on the ref clock.
> 
> For now add only the SSC and non-SSC ref clocks, as the pre-ICL DSI code
> has its own logic for calculating DPLL parameters instead of the shared
> DPLL framework.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  .../drm/i915/display/intel_display_debugfs.c  |   5 +
>  drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 132 +++++++++++-------
>  drivers/gpu/drm/i915/i915_drv.h               |   5 +
>  3 files changed, 95 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> index d2461d7946bf..6675b7e34f0d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> @@ -920,6 +920,11 @@ static int i915_shared_dplls_info(struct seq_file *m, void *unused)
>  	int i;
>  
>  	drm_modeset_lock_all(dev);
> +
> +	seq_printf(m, "PLL refclks: non-SSC: %d kHZ, SSC: %d kHZ\n",

nit: "kHz"

> +		   dev_priv->dpll.ref_clks.nssc,
> +		   dev_priv->dpll.ref_clks.ssc);
> +
>  	for (i = 0; i < dev_priv->dpll.num_shared_dpll; i++) {
>  		struct intel_shared_dpll *pll = &dev_priv->dpll.shared_dplls[i];
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> index 7e6da58a47c9..44db46782770 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> @@ -56,6 +56,7 @@ struct intel_dpll_mgr {
>  	void (*update_active_dpll)(struct intel_atomic_state *state,
>  				   struct intel_crtc *crtc,
>  				   struct intel_encoder *encoder);
> +	void (*update_ref_clks)(struct drm_i915_private *i915);
>  	void (*dump_hw_state)(struct drm_i915_private *dev_priv,
>  			      const struct intel_dpll_hw_state *hw_state);
>  };
> @@ -886,16 +887,9 @@ static int hsw_ddi_wrpll_get_freq(struct drm_i915_private *dev_priv,
>  
>  	switch (wrpll & WRPLL_REF_MASK) {
>  	case WRPLL_REF_SPECIAL_HSW:
> -		/*
> -		 * muxed-SSC for BDW.
> -		 * non-SSC for non-ULT HSW. Check FUSE_STRAP3
> -		 * for the non-SSC reference frequency.
> -		 */
> +		/* Muxed-SSC for BDW, non-SSC for non-ULT HSW. */
>  		if (IS_HASWELL(dev_priv) && !IS_HSW_ULT(dev_priv)) {
> -			if (intel_de_read(dev_priv, FUSE_STRAP3) & HSW_REF_CLK_SELECT)
> -				refclk = 24;
> -			else
> -				refclk = 135;
> +			refclk = dev_priv->dpll.ref_clks.nssc;
>  			break;
>  		}
>  		/* fall through */
> @@ -905,10 +899,10 @@ static int hsw_ddi_wrpll_get_freq(struct drm_i915_private *dev_priv,
>  		 * code only cares about 5% accuracy, and spread is a max of
>  		 * 0.5% downspread.
>  		 */
> -		refclk = 135;
> +		refclk = dev_priv->dpll.ref_clks.ssc;
>  		break;
>  	case WRPLL_REF_LCPLL:
> -		refclk = 2700;
> +		refclk = 2700000;
>  		break;
>  	default:
>  		MISSING_CASE(wrpll);
> @@ -920,7 +914,7 @@ static int hsw_ddi_wrpll_get_freq(struct drm_i915_private *dev_priv,
>  	n = (wrpll & WRPLL_DIVIDER_FB_MASK) >> WRPLL_DIVIDER_FB_SHIFT;
>  
>  	/* Convert to KHz, p & r have a fixed point portion */
> -	return (refclk * n * 100) / (p * r) * 2;
> +	return (refclk * n / 10) / (p * r) * 2;
>  }
>  
>  static struct intel_shared_dpll *
> @@ -1049,6 +1043,16 @@ static bool hsw_get_dpll(struct intel_atomic_state *state,
>  	return true;
>  }
>  
> +static void hsw_update_dpll_ref_clks(struct drm_i915_private *i915)
> +{
> +	i915->dpll.ref_clks.ssc = 135000;
> +	/* Non-SSC is only used on non-ULT HSW. */
> +	if (intel_de_read(i915, FUSE_STRAP3) & HSW_REF_CLK_SELECT)
> +		i915->dpll.ref_clks.nssc = 24000;
> +	else
> +		i915->dpll.ref_clks.nssc = 135000;

I couldn't remember whether the PCH and CPU SSC references have the same
frquency. But looks like they do.

> +}
> +
>  static void hsw_dump_hw_state(struct drm_i915_private *dev_priv,
>  			      const struct intel_dpll_hw_state *hw_state)
>  {
> @@ -1108,6 +1112,7 @@ static const struct intel_dpll_mgr hsw_pll_mgr = {
>  	.dpll_info = hsw_plls,
>  	.get_dplls = hsw_get_dpll,
>  	.put_dplls = intel_put_dpll,
> +	.update_ref_clks = hsw_update_dpll_ref_clks,
>  	.dump_hw_state = hsw_dump_hw_state,
>  };
>  
> @@ -1523,6 +1528,7 @@ skl_ddi_calculate_wrpll(int clock /* in Hz */,
>  
>  static bool skl_ddi_hdmi_pll_dividers(struct intel_crtc_state *crtc_state)
>  {
> +	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
>  	u32 ctrl1, cfgcr1, cfgcr2;
>  	struct skl_wrpll_params wrpll_params = { 0, };
>  
> @@ -1534,7 +1540,8 @@ static bool skl_ddi_hdmi_pll_dividers(struct intel_crtc_state *crtc_state)
>  
>  	ctrl1 |= DPLL_CTRL1_HDMI_MODE(0);
>  
> -	if (!skl_ddi_calculate_wrpll(crtc_state->port_clock * 1000, 24000,
> +	if (!skl_ddi_calculate_wrpll(crtc_state->port_clock * 1000,
> +				     i915->dpll.ref_clks.nssc,
>  				     &wrpll_params))
>  		return false;
>  
> @@ -1561,7 +1568,7 @@ static int skl_ddi_wrpll_get_freq(struct drm_i915_private *i915,
>  				  const struct intel_shared_dpll *pll)
>  {
>  	const struct intel_dpll_hw_state *pll_state = &pll->state.hw_state;
> -	int ref_clock = 24000;
> +	int ref_clock = i915->dpll.ref_clks.nssc;
>  	u32 p0, p1, p2, dco_freq;
>  
>  	p0 = pll_state->cfgcr2 & DPLL_CFGCR2_PDIV_MASK;
> @@ -1751,6 +1758,12 @@ static int skl_ddi_pll_get_freq(struct drm_i915_private *i915,
>  		return skl_ddi_lcpll_get_freq(i915, pll);
>  }
>  
> +static void skl_update_dpll_ref_clks(struct drm_i915_private *i915)
> +{
> +	/* No SSC ref */
> +	i915->dpll.ref_clks.nssc = i915->cdclk.hw.ref;
> +}
> +
>  static void skl_dump_hw_state(struct drm_i915_private *dev_priv,
>  			      const struct intel_dpll_hw_state *hw_state)
>  {
> @@ -1787,6 +1800,7 @@ static const struct intel_dpll_mgr skl_pll_mgr = {
>  	.dpll_info = skl_plls,
>  	.get_dplls = skl_get_dpll,
>  	.put_dplls = intel_put_dpll,
> +	.update_ref_clks = skl_update_dpll_ref_clks,
>  	.dump_hw_state = skl_dump_hw_state,
>  };
>  
> @@ -2192,7 +2206,7 @@ static int bxt_ddi_pll_get_freq(struct drm_i915_private *i915,
>  	clock.p1 = (pll_state->ebb0 & PORT_PLL_P1_MASK) >> PORT_PLL_P1_SHIFT;
>  	clock.p2 = (pll_state->ebb0 & PORT_PLL_P2_MASK) >> PORT_PLL_P2_SHIFT;
>  
> -	return chv_calc_dpll_params(100000, &clock);
> +	return chv_calc_dpll_params(i915->dpll.ref_clks.nssc, &clock);
>  }
>  
>  static bool bxt_get_dpll(struct intel_atomic_state *state,
> @@ -2228,6 +2242,13 @@ static bool bxt_get_dpll(struct intel_atomic_state *state,
>  	return true;
>  }
>  
> +static void bxt_update_dpll_ref_clks(struct drm_i915_private *i915)
> +{
> +	i915->dpll.ref_clks.ssc = 100000;
> +	i915->dpll.ref_clks.nssc = 100000;
> +	/* DSI non-SSC ref 19.2MHz */
> +}
> +
>  static void bxt_dump_hw_state(struct drm_i915_private *dev_priv,
>  			      const struct intel_dpll_hw_state *hw_state)
>  {
> @@ -2265,6 +2286,7 @@ static const struct intel_dpll_mgr bxt_pll_mgr = {
>  	.dpll_info = bxt_plls,
>  	.get_dplls = bxt_get_dpll,
>  	.put_dplls = intel_put_dpll,
> +	.update_ref_clks = bxt_update_dpll_ref_clks,
>  	.dump_hw_state = bxt_dump_hw_state,
>  };
>  
> @@ -2508,27 +2530,13 @@ static void cnl_wrpll_params_populate(struct skl_wrpll_params *params,
>  	params->dco_fraction = dco & 0x7fff;
>  }
>  
> -int cnl_hdmi_pll_ref_clock(struct drm_i915_private *dev_priv)
> -{
> -	int ref_clock = dev_priv->cdclk.hw.ref;
> -
> -	/*
> -	 * For ICL+, the spec states: if reference frequency is 38.4,
> -	 * use 19.2 because the DPLL automatically divides that by 2.
> -	 */
> -	if (INTEL_GEN(dev_priv) >= 11 && ref_clock == 38400)
> -		ref_clock = 19200;
> -
> -	return ref_clock;
> -}
> -
>  static bool
>  cnl_ddi_calculate_wrpll(struct intel_crtc_state *crtc_state,
>  			struct skl_wrpll_params *wrpll_params)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
>  	u32 afe_clock = crtc_state->port_clock * 5;
> -	u32 ref_clock;
> +	int ref_clock = dev_priv->dpll.ref_clks.nssc;
>  	u32 dco_min = 7998000;
>  	u32 dco_max = 10000000;
>  	u32 dco_mid = (dco_min + dco_max) / 2;
> @@ -2560,9 +2568,6 @@ cnl_ddi_calculate_wrpll(struct intel_crtc_state *crtc_state,
>  		return false;
>  
>  	cnl_wrpll_get_multipliers(best_div, &pdiv, &qdiv, &kdiv);
> -
> -	ref_clock = cnl_hdmi_pll_ref_clock(dev_priv);
> -
>  	cnl_wrpll_params_populate(wrpll_params, best_dco, ref_clock,
>  				  pdiv, qdiv, kdiv);
>  
> @@ -2596,11 +2601,12 @@ static bool cnl_ddi_hdmi_pll_dividers(struct intel_crtc_state *crtc_state)
>  	return true;
>  }
>  
> -static int cnl_ddi_wrpll_get_freq(struct drm_i915_private *dev_priv,
> -				  const struct intel_shared_dpll *pll)
> +static int __cnl_ddi_wrpll_get_freq(struct drm_i915_private *dev_priv,
> +				    const struct intel_shared_dpll *pll,
> +				    int ref_clock)
>  {
>  	const struct intel_dpll_hw_state *pll_state = &pll->state.hw_state;
> -	u32 p0, p1, p2, dco_freq, ref_clock;
> +	u32 p0, p1, p2, dco_freq;
>  
>  	p0 = pll_state->cfgcr1 & DPLL_CFGCR1_PDIV_MASK;
>  	p2 = pll_state->cfgcr1 & DPLL_CFGCR1_KDIV_MASK;
> @@ -2639,8 +2645,6 @@ static int cnl_ddi_wrpll_get_freq(struct drm_i915_private *dev_priv,
>  		break;
>  	}
>  
> -	ref_clock = cnl_hdmi_pll_ref_clock(dev_priv);
> -
>  	dco_freq = (pll_state->cfgcr0 & DPLL_CFGCR0_DCO_INTEGER_MASK) *
>  		   ref_clock;
>  
> @@ -2653,6 +2657,12 @@ static int cnl_ddi_wrpll_get_freq(struct drm_i915_private *dev_priv,
>  	return dco_freq / (p0 * p1 * p2 * 5);
>  }
>  
> +static int cnl_ddi_wrpll_get_freq(struct drm_i915_private *i915,
> +				  const struct intel_shared_dpll *pll)
> +{
> +	return __cnl_ddi_wrpll_get_freq(i915, pll, i915->dpll.ref_clks.nssc);
> +}
> +
>  static bool
>  cnl_ddi_dp_set_dpll_hw_state(struct intel_crtc_state *crtc_state)
>  {
> @@ -2794,6 +2804,12 @@ static int cnl_ddi_pll_get_freq(struct drm_i915_private *i915,
>  		return cnl_ddi_lcpll_get_freq(i915, pll);
>  }
>  
> +static void cnl_update_dpll_ref_clks(struct drm_i915_private *i915)
> +{
> +	/* No SSC reference */
> +	i915->dpll.ref_clks.nssc = i915->cdclk.hw.ref;
> +}
> +
>  static void cnl_dump_hw_state(struct drm_i915_private *dev_priv,
>  			      const struct intel_dpll_hw_state *hw_state)
>  {
> @@ -2821,6 +2837,7 @@ static const struct intel_dpll_mgr cnl_pll_mgr = {
>  	.dpll_info = cnl_plls,
>  	.get_dplls = cnl_get_dpll,
>  	.put_dplls = intel_put_dpll,
> +	.update_ref_clks = cnl_update_dpll_ref_clks,
>  	.dump_hw_state = cnl_dump_hw_state,
>  };
>  
> @@ -2916,7 +2933,7 @@ static bool icl_calc_dp_combo_pll(struct intel_crtc_state *crtc_state,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
>  	const struct icl_combo_pll_params *params =
> -		dev_priv->cdclk.hw.ref == 24000 ?
> +		dev_priv->dpll.ref_clks.nssc == 24000 ?
>  		icl_dp_combo_pll_24MHz_values :
>  		icl_dp_combo_pll_19_2MHz_values;
>  	int clock = crtc_state->port_clock;
> @@ -2939,9 +2956,9 @@ static bool icl_calc_tbt_pll(struct intel_crtc_state *crtc_state,
>  	struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
>  
>  	if (INTEL_GEN(dev_priv) >= 12) {
> -		switch (dev_priv->cdclk.hw.ref) {
> +		switch (dev_priv->dpll.ref_clks.nssc) {
>  		default:
> -			MISSING_CASE(dev_priv->cdclk.hw.ref);
> +			MISSING_CASE(dev_priv->dpll.ref_clks.nssc);
>  			/* fall-through */
>  		case 19200:
>  		case 38400:
> @@ -2952,9 +2969,9 @@ static bool icl_calc_tbt_pll(struct intel_crtc_state *crtc_state,
>  			break;
>  		}
>  	} else {
> -		switch (dev_priv->cdclk.hw.ref) {
> +		switch (dev_priv->dpll.ref_clks.nssc) {
>  		default:
> -			MISSING_CASE(dev_priv->cdclk.hw.ref);
> +			MISSING_CASE(dev_priv->dpll.ref_clks.nssc);
>  			/* fall-through */
>  		case 19200:
>  		case 38400:
> @@ -3118,7 +3135,7 @@ static bool icl_calc_mg_pll_state(struct intel_crtc_state *crtc_state,
>  				  struct intel_dpll_hw_state *pll_state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
> -	int refclk_khz = dev_priv->cdclk.hw.ref;
> +	int refclk_khz = dev_priv->dpll.ref_clks.nssc;
>  	int clock = crtc_state->port_clock;
>  	u32 dco_khz, m1div, m2div_int, m2div_rem, m2div_frac;
>  	u32 iref_ndiv, iref_trim, iref_pulse_w;
> @@ -3326,7 +3343,7 @@ static int icl_ddi_mg_pll_get_freq(struct drm_i915_private *dev_priv,
>  	u32 m1, m2_int, m2_frac, div1, div2, ref_clock;
>  	u64 tmp;
>  
> -	ref_clock = dev_priv->cdclk.hw.ref;
> +	ref_clock = dev_priv->dpll.ref_clks.nssc;
>  
>  	if (INTEL_GEN(dev_priv) >= 12) {
>  		m1 = pll_state->mg_pll_div0 & DKL_PLL_DIV0_FBPREDIV_MASK;
> @@ -3478,7 +3495,16 @@ static bool icl_get_combo_phy_dpll(struct intel_atomic_state *state,
>  static int icl_ddi_combo_pll_get_freq(struct drm_i915_private *i915,
>  				      const struct intel_shared_dpll *pll)
>  {
> -	return cnl_ddi_wrpll_get_freq(i915, pll);
> +	int ref_clock = i915->dpll.ref_clks.nssc;
> +
> +	/*
> +	 * For ICL+, the spec states: if reference frequency is 38.4,
> +	 * use 19.2 because the DPLL automatically divides that by 2.
> +	 */
> +	if (ref_clock == 38400)
> +		ref_clock = 19200;

I was pondering whether it would be better to store the divided ref,
but I guess we need the original value for some other things, and it's
really the DPLL in HDMI mode that does the extra /2 for us.

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

> +
> +	return __cnl_ddi_wrpll_get_freq(i915, pll, ref_clock);
>  }
>  
>  static bool icl_get_tc_phy_dplls(struct intel_atomic_state *state,
> @@ -3629,7 +3655,7 @@ static bool mg_pll_get_hw_state(struct drm_i915_private *dev_priv,
>  	hw_state->mg_pll_tdc_coldst_bias =
>  		intel_de_read(dev_priv, MG_PLL_TDC_COLDST_BIAS(tc_port));
>  
> -	if (dev_priv->cdclk.hw.ref == 38400) {
> +	if (dev_priv->dpll.ref_clks.nssc == 38400) {
>  		hw_state->mg_pll_tdc_coldst_bias_mask = MG_PLL_TDC_COLDST_COLDSTART;
>  		hw_state->mg_pll_bias_mask = 0;
>  	} else {
> @@ -4110,6 +4136,12 @@ static void mg_pll_disable(struct drm_i915_private *dev_priv,
>  	icl_pll_disable(dev_priv, pll, enable_reg);
>  }
>  
> +static void icl_update_dpll_ref_clks(struct drm_i915_private *i915)
> +{
> +	/* No SSC ref */
> +	i915->dpll.ref_clks.nssc = i915->cdclk.hw.ref;
> +}
> +
>  static void icl_dump_hw_state(struct drm_i915_private *dev_priv,
>  			      const struct intel_dpll_hw_state *hw_state)
>  {
> @@ -4170,6 +4202,7 @@ static const struct intel_dpll_mgr icl_pll_mgr = {
>  	.get_dplls = icl_get_dplls,
>  	.put_dplls = icl_put_dplls,
>  	.update_active_dpll = icl_update_active_dpll,
> +	.update_ref_clks = icl_update_dpll_ref_clks,
>  	.dump_hw_state = icl_dump_hw_state,
>  };
>  
> @@ -4184,6 +4217,7 @@ static const struct intel_dpll_mgr ehl_pll_mgr = {
>  	.dpll_info = ehl_plls,
>  	.get_dplls = icl_get_dplls,
>  	.put_dplls = icl_put_dplls,
> +	.update_ref_clks = icl_update_dpll_ref_clks,
>  	.dump_hw_state = icl_dump_hw_state,
>  };
>  
> @@ -4212,6 +4246,7 @@ static const struct intel_dpll_mgr tgl_pll_mgr = {
>  	.get_dplls = icl_get_dplls,
>  	.put_dplls = icl_put_dplls,
>  	.update_active_dpll = icl_update_active_dpll,
> +	.update_ref_clks = icl_update_dpll_ref_clks,
>  	.dump_hw_state = icl_dump_hw_state,
>  };
>  
> @@ -4390,6 +4425,9 @@ void intel_dpll_readout_hw_state(struct drm_i915_private *i915)
>  {
>  	int i;
>  
> +	if (i915->dpll.mgr && i915->dpll.mgr->update_ref_clks)
> +		i915->dpll.mgr->update_ref_clks(i915);
> +
>  	for (i = 0; i < i915->dpll.num_shared_dpll; i++)
>  		readout_dpll_hw_state(i915, &i915->dpll.shared_dplls[i]);
>  }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index fe4eefc5e7e6..49ee3bde08f5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1059,6 +1059,11 @@ struct drm_i915_private {
>  		int num_shared_dpll;
>  		struct intel_shared_dpll shared_dplls[I915_NUM_PLLS];
>  		const struct intel_dpll_mgr *mgr;
> +
> +		struct {
> +			int nssc;
> +			int ssc;
> +		} ref_clks;
>  	} dpll;
>  
>  	struct list_head global_obj_list;
> -- 
> 2.23.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