> -----Original Message-----
> From: Intel-gfx <[email protected]> On Behalf Of Mika
> Kahola
> Sent: Wednesday, December 17, 2025 8:50 PM
> To: [email protected]; [email protected]
> Cc: Kahola, Mika <[email protected]>
> Subject: [PATCH v2 11/15] drm/i915/cx0: Verify C10/C20 pll dividers
>
> Add verification for pll table dividers. The port clock is computed based on
> pll
> tables and, for hdmi case, the algorithmic model is applied to calculate pll
> dividers.
> If port clock differs more than +-1 kHz from expected value an drm_warn() is
> thrown and pll divider differences are printed out for debugging purposes.
>
> v2:
> - Move clock derivation from dividers in intel_cx0pll_enable()
> earlier in the patchset.
> - Keep intel_cx0_pll_power_save_wa() in intel_dpll_sanitize_state()
> - Use tables[i].name != NULL as a terminating condition.
> - Drop duplicate intel_cx0pll_clock_matches() declaration in header.
> - Use state vs. params term consistently in intel_c10pll_verify_clock()
> and intel_c20pll_verify_clock().
>
> Signed-off-by: Mika Kahola <[email protected]>
> ---
> drivers/gpu/drm/i915/display/intel_cx0_phy.c | 121 ++++++++++++++++++
> drivers/gpu/drm/i915/display/intel_cx0_phy.h | 1 +
> drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 9 +-
> 3 files changed, 130 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> index ea807191cb4f..a8c37a14d427 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> @@ -3824,3 +3824,124 @@ void intel_cx0_pll_power_save_wa(struct
> intel_display *display)
> intel_cx0pll_disable(encoder);
> }
> }
> +
> +static void intel_c10pll_verify_clock(struct intel_display *display,
> + int precomputed_clock,
> + const char *pll_state_name,
> + const struct intel_c10pll_state
> *pll_state,
> + bool is_precomputed_state)
> +{
> + struct drm_printer p;
> + int clock;
> +
> + clock = intel_c10pll_calc_port_clock(pll_state);
> +
> + if (intel_cx0pll_clock_matches(clock, precomputed_clock))
> + return;
> +
> + drm_warn(display->drm,
> + "PLL state %s (%s): clock difference too high: computed %d,
> pre-computed %d\n",
> + pll_state_name,
> + is_precomputed_state ? "precomputed" : "computed",
> + clock, precomputed_clock);
> +
> + if (!drm_debug_enabled(DRM_UT_KMS))
> + return;
> +
> + p = drm_dbg_printer(display->drm, DRM_UT_KMS, NULL);
> +
> + drm_printf(&p, "PLL state %s (%s):\n",
> + pll_state_name,
> + is_precomputed_state ? "precomputed" : "computed");
> + intel_c10pll_dump_hw_state(&p, pll_state); }
> +
> +static void intel_c10pll_verify_params(struct intel_display *display,
> + const struct intel_cx0pll_params
> *pll_params) {
> + struct intel_c10pll_state pll_state;
> +
> + intel_c10pll_verify_clock(display, pll_params->clock_rate,
> +pll_params->name, pll_params->c10, true);
> +
> + if (!pll_params->is_hdmi)
> + return;
> +
> + intel_snps_hdmi_pll_compute_c10pll(&pll_state,
> +pll_params->clock_rate);
> +
> + intel_c10pll_verify_clock(display, pll_params->clock_rate,
> +pll_params->name, &pll_state, false); }
> +
> +static void intel_c20pll_verify_clock(struct intel_display *display,
> + int precomputed_clock,
> + const char *pll_state_name,
> + const struct intel_c20pll_state
> *pll_state,
> + bool is_precomputed_state)
> +{
> + struct drm_printer p;
> + int clock;
> +
> + clock = intel_c20pll_calc_port_clock(pll_state);
> +
> + if (intel_cx0pll_clock_matches(clock, precomputed_clock))
> + return;
> +
> + drm_warn(display->drm,
> + "PLL state %s (%s): clock difference too high: computed %d,
> pre-computed %d\n",
> + pll_state_name,
> + is_precomputed_state ? "precomputed" : "computed",
> + clock, precomputed_clock);
> +
> + if (!drm_debug_enabled(DRM_UT_KMS))
> + return;
> +
> + p = drm_dbg_printer(display->drm, DRM_UT_KMS, NULL);
> +
> + drm_printf(&p, "PLL state %s (%s):\n",
> + pll_state_name,
> + is_precomputed_state ? "precomputed" : "computed");
> + intel_c20pll_dump_hw_state(&p, pll_state); }
> +
> +static void intel_c20pll_verify_params(struct intel_display *display,
> + const struct intel_cx0pll_params
> *pll_params) {
> + struct intel_c20pll_state pll_state;
> +
> + intel_c20pll_verify_clock(display, pll_params->clock_rate,
> +pll_params->name, pll_params->c20, true);
> +
> + if (!pll_params->is_hdmi)
> + return;
> +
> + if (intel_c20_compute_hdmi_tmds_pll(display, pll_params-
> >clock_rate, &pll_state) != 0)
> + return;
> +
> + intel_c20pll_verify_clock(display, pll_params->clock_rate,
> +pll_params->name, &pll_state, false); }
> +
> +static void intel_cx0pll_verify_tables(struct intel_display *display,
> + const struct intel_cx0pll_params *tables)
> {
> + int i;
> +
> + for (i = 0; tables[i].name; i++) {
> + if (tables[i].is_c10)
> + intel_c10pll_verify_params(display, &tables[i]);
> + else
> + intel_c20pll_verify_params(display, &tables[i]);
> + }
> +}
> +
> +void intel_cx0pll_verify_plls(struct intel_display *display) {
> + /* C10 */
> + intel_cx0pll_verify_tables(display, mtl_c10_edp_tables);
> + intel_cx0pll_verify_tables(display, mtl_c10_dp_tables);
> + intel_cx0pll_verify_tables(display, mtl_c10_hdmi_tables);
> +
> + /* C20 */
> + intel_cx0pll_verify_tables(display, xe2hpd_c20_edp_tables);
> + intel_cx0pll_verify_tables(display, mtl_c20_dp_tables);
> + intel_cx0pll_verify_tables(display, xe2hpd_c20_dp_tables);
> + intel_cx0pll_verify_tables(display, xe3lpd_c20_dp_edp_tables);
> + intel_cx0pll_verify_tables(display, mtl_c20_hdmi_tables); }
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> b/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> index 3d9c580eb562..c0ac67f7b11f 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> @@ -78,6 +78,7 @@ bool intel_mtl_tbt_pll_readout_hw_state(struct
> intel_display *display,
> struct intel_dpll_hw_state
> *hw_state); int intel_mtl_tbt_calc_port_clock(struct intel_encoder
> *encoder);
>
> +void intel_cx0pll_verify_plls(struct intel_display *display);
> void intel_cx0_pll_power_save_wa(struct intel_display *display); void
> intel_lnl_mac_transmit_lfps(struct intel_encoder *encoder,
> const struct intel_crtc_state *crtc_state);
> diff
> --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> index 9aa84a430f09..7127bc2a0898 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> @@ -4613,7 +4613,7 @@ void intel_dpll_init(struct intel_display *display)
> dpll_mgr = &pch_pll_mgr;
>
> if (!dpll_mgr)
> - return;
> + goto out_verify;
>
> dpll_info = dpll_mgr->dpll_info;
>
> @@ -4632,6 +4632,13 @@ void intel_dpll_init(struct intel_display *display)
>
> display->dpll.mgr = dpll_mgr;
> display->dpll.num_dpll = i;
> +
> +out_verify:
> + /*
> + * TODO: Convert these to a KUnit test or dependent on a kconfig
> + * debug option.
> + */
> + intel_cx0pll_verify_plls(display);
According to me having this done during every boot does not make sense, maybe
as a test it may but here having the driver
Spend time doing these calculations for every table for all the rates seems
like a waste.
Specially when you take into account that all these tables are static which
means you have the values and the algorithm beforehand
And before adding the static tables you can get this the algorithm tested
against the table and fix it accordingly.
Also it should be the responsibility of anyone who adds any other static table
to see if the clock matches.
Regards,
Suraj Kandpal
> }
>
> /**
> --
> 2.34.1
