On Fri, 09 Feb 2024, Ville Syrjala <ville.syrj...@linux.intel.com> wrote: > From: Ville Syrjälä <ville.syrj...@linux.intel.com> > > Chunk up the humenguous dpll_hw_state comparison check into per-platform > variants, implemented in the dpll_mgr. This is step one in allowing > each platform (or perhaps even PLL) type to have a custom hw state > structure instead of having to smash it all into one. > > Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++++------- > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 95 +++++++++++++++++++ > drivers/gpu/drm/i915/display/intel_dpll_mgr.h | 3 + > 3 files changed, 141 insertions(+), 35 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > b/drivers/gpu/drm/i915/display/intel_display.c > index 1d381fa96c84..66ee6749fdae 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -4907,6 +4907,36 @@ pipe_config_mismatch(bool fastset, const struct > intel_crtc *crtc, > va_end(args); > } > > +static void > +pipe_config_pll_mismatch(bool fastset, > + const struct intel_crtc *crtc, > + const char *name, > + const struct intel_dpll_hw_state *a, > + const struct intel_dpll_hw_state *b) > +{ > + struct drm_i915_private *i915 = to_i915(crtc->base.dev); > + > + if (fastset) { > + if (!drm_debug_enabled(DRM_UT_KMS)) > + return; > + > + drm_dbg_kms(&i915->drm, > + "[CRTC:%d:%s] fastset requirement not met in %s\n", > + crtc->base.base.id, crtc->base.name, name); > + drm_dbg_kms(&i915->drm, "expected:\n"); > + intel_dpll_dump_hw_state(i915, a); > + drm_dbg_kms(&i915->drm, "found:\n"); > + intel_dpll_dump_hw_state(i915, b); > + } else { > + drm_err(&i915->drm, "[CRTC:%d:%s] mismatch in %s buffer\n", > + crtc->base.base.id, crtc->base.name, name); > + drm_err(&i915->drm, "expected:\n"); > + intel_dpll_dump_hw_state(i915, a); > + drm_err(&i915->drm, "found:\n"); > + intel_dpll_dump_hw_state(i915, b); > + } > +}
As follow-up, would be great to see this move towards drm_printer based approach, similar to pipe_config_dp_vsc_sdp_mismatch(). Reduces duplication. BR, Jani. > + > static bool fastboot_enabled(struct drm_i915_private *dev_priv) > { > /* Enable fastboot by default on Skylake and newer */ > @@ -5016,7 +5046,17 @@ intel_pipe_config_compare(const struct > intel_crtc_state *current_config, > } \ > } while (0) > > -#define PIPE_CONF_CHECK_TIMINGS(name) do { \ > +#define PIPE_CONF_CHECK_PLL(name) do { \ > + if (!intel_dpll_compare_hw_state(dev_priv, ¤t_config->name, \ > + &pipe_config->name)) { \ > + pipe_config_pll_mismatch(fastset, crtc, __stringify(name), \ > + ¤t_config->name, \ > + &pipe_config->name); \ > + ret = false; \ > + } \ > +} while (0) > + > +#define PIPE_CONF_CHECK_TIMINGS(name) do { \ > PIPE_CONF_CHECK_I(name.crtc_hdisplay); \ > PIPE_CONF_CHECK_I(name.crtc_htotal); \ > PIPE_CONF_CHECK_I(name.crtc_hblank_start); \ > @@ -5223,40 +5263,8 @@ intel_pipe_config_compare(const struct > intel_crtc_state *current_config, > PIPE_CONF_CHECK_P(shared_dpll); > > /* FIXME convert everything over the dpll_mgr */ > - if (dev_priv->display.dpll.mgr || HAS_GMCH(dev_priv)) { > - PIPE_CONF_CHECK_X(dpll_hw_state.dpll); > - PIPE_CONF_CHECK_X(dpll_hw_state.dpll_md); > - PIPE_CONF_CHECK_X(dpll_hw_state.fp0); > - PIPE_CONF_CHECK_X(dpll_hw_state.fp1); > - PIPE_CONF_CHECK_X(dpll_hw_state.wrpll); > - PIPE_CONF_CHECK_X(dpll_hw_state.spll); > - PIPE_CONF_CHECK_X(dpll_hw_state.ctrl1); > - PIPE_CONF_CHECK_X(dpll_hw_state.cfgcr1); > - PIPE_CONF_CHECK_X(dpll_hw_state.cfgcr2); > - PIPE_CONF_CHECK_X(dpll_hw_state.cfgcr0); > - PIPE_CONF_CHECK_X(dpll_hw_state.div0); > - PIPE_CONF_CHECK_X(dpll_hw_state.ebb0); > - PIPE_CONF_CHECK_X(dpll_hw_state.ebb4); > - PIPE_CONF_CHECK_X(dpll_hw_state.pll0); > - PIPE_CONF_CHECK_X(dpll_hw_state.pll1); > - PIPE_CONF_CHECK_X(dpll_hw_state.pll2); > - PIPE_CONF_CHECK_X(dpll_hw_state.pll3); > - PIPE_CONF_CHECK_X(dpll_hw_state.pll6); > - PIPE_CONF_CHECK_X(dpll_hw_state.pll8); > - PIPE_CONF_CHECK_X(dpll_hw_state.pll9); > - PIPE_CONF_CHECK_X(dpll_hw_state.pll10); > - PIPE_CONF_CHECK_X(dpll_hw_state.pcsdw12); > - PIPE_CONF_CHECK_X(dpll_hw_state.mg_refclkin_ctl); > - PIPE_CONF_CHECK_X(dpll_hw_state.mg_clktop2_coreclkctl1); > - PIPE_CONF_CHECK_X(dpll_hw_state.mg_clktop2_hsclkctl); > - PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_div0); > - PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_div1); > - PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_lf); > - PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_frac_lock); > - PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_ssc); > - PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_bias); > - PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_tdc_coldst_bias); > - } > + if (dev_priv->display.dpll.mgr || HAS_GMCH(dev_priv)) > + PIPE_CONF_CHECK_PLL(dpll_hw_state); > > PIPE_CONF_CHECK_X(dsi_pll.ctrl); > PIPE_CONF_CHECK_X(dsi_pll.div); > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > index c6cc7465b92c..ff480f171f75 100644 > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > @@ -109,6 +109,8 @@ struct intel_dpll_mgr { > void (*update_ref_clks)(struct drm_i915_private *i915); > void (*dump_hw_state)(struct drm_i915_private *i915, > const struct intel_dpll_hw_state *hw_state); > + bool (*compare_hw_state)(const struct intel_dpll_hw_state *a, > + const struct intel_dpll_hw_state *b); > }; > > static void > @@ -644,6 +646,15 @@ static void ibx_dump_hw_state(struct drm_i915_private > *i915, > hw_state->fp1); > } > > +static bool ibx_compare_hw_state(const struct intel_dpll_hw_state *a, > + const struct intel_dpll_hw_state *b) > +{ > + return a->dpll == b->dpll && > + a->dpll_md == b->dpll_md && > + a->fp0 == b->fp0 && > + a->fp1 == b->fp1; > +} > + > static const struct intel_shared_dpll_funcs ibx_pch_dpll_funcs = { > .enable = ibx_pch_dpll_enable, > .disable = ibx_pch_dpll_disable, > @@ -662,6 +673,7 @@ static const struct intel_dpll_mgr pch_pll_mgr = { > .get_dplls = ibx_get_dpll, > .put_dplls = intel_put_dpll, > .dump_hw_state = ibx_dump_hw_state, > + .compare_hw_state = ibx_compare_hw_state, > }; > > static void hsw_ddi_wrpll_enable(struct drm_i915_private *i915, > @@ -1220,6 +1232,13 @@ static void hsw_dump_hw_state(struct drm_i915_private > *i915, > hw_state->wrpll, hw_state->spll); > } > > +static bool hsw_compare_hw_state(const struct intel_dpll_hw_state *a, > + const struct intel_dpll_hw_state *b) > +{ > + return a->wrpll == b->wrpll && > + a->spll == b->spll; > +} > + > static const struct intel_shared_dpll_funcs hsw_ddi_wrpll_funcs = { > .enable = hsw_ddi_wrpll_enable, > .disable = hsw_ddi_wrpll_disable, > @@ -1278,6 +1297,7 @@ static const struct intel_dpll_mgr hsw_pll_mgr = { > .put_dplls = intel_put_dpll, > .update_ref_clks = hsw_update_dpll_ref_clks, > .dump_hw_state = hsw_dump_hw_state, > + .compare_hw_state = hsw_compare_hw_state, > }; > > struct skl_dpll_regs { > @@ -1929,6 +1949,14 @@ static void skl_dump_hw_state(struct drm_i915_private > *i915, > hw_state->cfgcr2); > } > > +static bool skl_compare_hw_state(const struct intel_dpll_hw_state *a, > + const struct intel_dpll_hw_state *b) > +{ > + return a->ctrl1 == b->ctrl1 && > + a->cfgcr1 == b->cfgcr1 && > + a->cfgcr2 == b->cfgcr2; > +} > + > static const struct intel_shared_dpll_funcs skl_ddi_pll_funcs = { > .enable = skl_ddi_pll_enable, > .disable = skl_ddi_pll_disable, > @@ -1959,6 +1987,7 @@ static const struct intel_dpll_mgr skl_pll_mgr = { > .put_dplls = intel_put_dpll, > .update_ref_clks = skl_update_dpll_ref_clks, > .dump_hw_state = skl_dump_hw_state, > + .compare_hw_state = skl_compare_hw_state, > }; > > static void bxt_ddi_pll_enable(struct drm_i915_private *i915, > @@ -2392,6 +2421,21 @@ static void bxt_dump_hw_state(struct drm_i915_private > *i915, > hw_state->pcsdw12); > } > > +static bool bxt_compare_hw_state(const struct intel_dpll_hw_state *a, > + const struct intel_dpll_hw_state *b) > +{ > + return a->ebb0 == b->ebb0 && > + a->ebb4 == b->ebb4 && > + a->pll0 == b->pll0 && > + a->pll1 == b->pll1 && > + a->pll2 == b->pll2 && > + a->pll3 == b->pll3 && > + a->pll6 == b->pll6 && > + a->pll8 == b->pll8 && > + a->pll10 == b->pll10 && > + a->pcsdw12 == b->pcsdw12; > +} > + > static const struct intel_shared_dpll_funcs bxt_ddi_pll_funcs = { > .enable = bxt_ddi_pll_enable, > .disable = bxt_ddi_pll_disable, > @@ -2413,6 +2457,7 @@ static const struct intel_dpll_mgr bxt_pll_mgr = { > .put_dplls = intel_put_dpll, > .update_ref_clks = bxt_update_dpll_ref_clks, > .dump_hw_state = bxt_dump_hw_state, > + .compare_hw_state = bxt_compare_hw_state, > }; > > static void icl_wrpll_get_multipliers(int bestdiv, int *pdiv, > @@ -4005,6 +4050,25 @@ static void icl_dump_hw_state(struct drm_i915_private > *i915, > hw_state->mg_pll_tdc_coldst_bias); > } > > +static bool icl_compare_hw_state(const struct intel_dpll_hw_state *a, > + const struct intel_dpll_hw_state *b) > +{ > + /* FIXME split combo vs. mg more thoroughly */ > + return a->cfgcr0 == b->cfgcr0 && > + a->cfgcr1 == b->cfgcr1 && > + a->div0 == b->div0 && > + a->mg_refclkin_ctl == b->mg_refclkin_ctl && > + a->mg_clktop2_coreclkctl1 == b->mg_clktop2_coreclkctl1 && > + a->mg_clktop2_hsclkctl == b->mg_clktop2_hsclkctl && > + a->mg_pll_div0 == b->mg_pll_div0 && > + a->mg_pll_div1 == b->mg_pll_div1 && > + a->mg_pll_lf == b->mg_pll_lf && > + a->mg_pll_frac_lock == b->mg_pll_frac_lock && > + a->mg_pll_ssc == b->mg_pll_ssc && > + a->mg_pll_bias == b->mg_pll_bias && > + a->mg_pll_tdc_coldst_bias == b->mg_pll_tdc_coldst_bias; > +} > + > static const struct intel_shared_dpll_funcs combo_pll_funcs = { > .enable = combo_pll_enable, > .disable = combo_pll_disable, > @@ -4046,6 +4110,7 @@ static const struct intel_dpll_mgr icl_pll_mgr = { > .update_active_dpll = icl_update_active_dpll, > .update_ref_clks = icl_update_dpll_ref_clks, > .dump_hw_state = icl_dump_hw_state, > + .compare_hw_state = icl_compare_hw_state, > }; > > static const struct dpll_info ehl_plls[] = { > @@ -4063,6 +4128,7 @@ static const struct intel_dpll_mgr ehl_pll_mgr = { > .put_dplls = icl_put_dplls, > .update_ref_clks = icl_update_dpll_ref_clks, > .dump_hw_state = icl_dump_hw_state, > + .compare_hw_state = icl_compare_hw_state, > }; > > static const struct intel_shared_dpll_funcs dkl_pll_funcs = { > @@ -4094,6 +4160,7 @@ static const struct intel_dpll_mgr tgl_pll_mgr = { > .update_active_dpll = icl_update_active_dpll, > .update_ref_clks = icl_update_dpll_ref_clks, > .dump_hw_state = icl_dump_hw_state, > + .compare_hw_state = icl_compare_hw_state, > }; > > static const struct dpll_info rkl_plls[] = { > @@ -4110,6 +4177,7 @@ static const struct intel_dpll_mgr rkl_pll_mgr = { > .put_dplls = icl_put_dplls, > .update_ref_clks = icl_update_dpll_ref_clks, > .dump_hw_state = icl_dump_hw_state, > + .compare_hw_state = icl_compare_hw_state, > }; > > static const struct dpll_info dg1_plls[] = { > @@ -4127,6 +4195,7 @@ static const struct intel_dpll_mgr dg1_pll_mgr = { > .put_dplls = icl_put_dplls, > .update_ref_clks = icl_update_dpll_ref_clks, > .dump_hw_state = icl_dump_hw_state, > + .compare_hw_state = icl_compare_hw_state, > }; > > static const struct dpll_info adls_plls[] = { > @@ -4144,6 +4213,7 @@ static const struct intel_dpll_mgr adls_pll_mgr = { > .put_dplls = icl_put_dplls, > .update_ref_clks = icl_update_dpll_ref_clks, > .dump_hw_state = icl_dump_hw_state, > + .compare_hw_state = icl_compare_hw_state, > }; > > static const struct dpll_info adlp_plls[] = { > @@ -4166,6 +4236,7 @@ static const struct intel_dpll_mgr adlp_pll_mgr = { > .update_active_dpll = icl_update_active_dpll, > .update_ref_clks = icl_update_dpll_ref_clks, > .dump_hw_state = icl_dump_hw_state, > + .compare_hw_state = icl_compare_hw_state, > }; > > /** > @@ -4462,6 +4533,30 @@ void intel_dpll_dump_hw_state(struct drm_i915_private > *i915, > } > } > > +/** > + * intel_dpll_compare_hw_state - compare the two states > + * @i915: i915 drm device > + * @a: first DPLL hw state > + * @b: second DPLL hw state > + * > + * Compare DPLL hw states @a and @b. > + * > + * Returns: true if the states are equal, false if the differ > + */ > +bool intel_dpll_compare_hw_state(struct drm_i915_private *i915, > + const struct intel_dpll_hw_state *a, > + const struct intel_dpll_hw_state *b) > +{ > + if (i915->display.dpll.mgr) { > + return i915->display.dpll.mgr->compare_hw_state(a, b); > + } else { > + /* fallback for platforms that don't use the shared dpll > + * infrastructure > + */ > + return ibx_compare_hw_state(a, b); > + } > +} > + > static void > verify_single_dpll_state(struct drm_i915_private *i915, > struct intel_shared_dpll *pll, > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > index 616afe861b46..cc0e1386309d 100644 > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > @@ -378,6 +378,9 @@ void intel_dpll_sanitize_state(struct drm_i915_private > *i915); > > void intel_dpll_dump_hw_state(struct drm_i915_private *i915, > const struct intel_dpll_hw_state *hw_state); > +bool intel_dpll_compare_hw_state(struct drm_i915_private *i915, > + const struct intel_dpll_hw_state *a, > + const struct intel_dpll_hw_state *b); > enum intel_dpll_id icl_tc_port_to_pll_id(enum tc_port tc_port); > bool intel_dpll_is_combophy(enum intel_dpll_id id); -- Jani Nikula, Intel