> Subject: Re: [PATCH v2 11/15] drm/i915/cx0: Verify C10/C20 pll dividers > > On Tue, Jan 06, 2026 at 05:04:48AM +0000, Kandpal, Suraj wrote: > > > ... > > > > > > 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. > > The overhead of the calculation is insignificant. There is also a TODO: > comment above to move it to KUnit/debug test which would remove even that > insignificant overhead. > > > 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. > > The purpose is to make sure that no PLL table entries are changed, breaking > them, after they were initially added, even if the initally added entry was > verified manually, separately. > > Additionally the function calculating the PLL clock value from the PLL > dividers > and the inverse function calculating the PLL divider values from the clock > must > be also checked and kept correct against any potential future change that > would break these functions.
LGTM, Reviewed-by: Suraj Kandpal <[email protected]> > > > Regards, > > Suraj Kandpal > > > > > } > > > > > > /** > > > -- > > > 2.34.1 > >
