On Thu, Feb 27, 2020 at 08:13:29PM +0200, Ville Syrjälä wrote:
> 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@xxxxxxxxx>
> > ---
> > .../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.
Yes, according to bspec for both HSW and BDW.
>
> > +}
> > +
> > 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.
Yes in case the original ref is 38.4MHz, DPLL uses 19.2MHz as reference
both for DP and HDMI, while MG-PLLs use 38.4MHz as reference in that
case. And we store the ref globally not per-PLL.
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> > +
> > + 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@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
