Re: [PATCH v2 11/15] drm/i915/cx0: Verify C10/C20 pll dividers

[Imre Deak <imre.deak@intel.com>]

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.

> Regards,
> Suraj Kandpal
> 
> >  }
> > 
> >  /**
> > --
> > 2.34.1
>