Re: [CI 14/32] drm/i915/display: Print additional Cx0 PLL HW state

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

On Tue, Nov 11, 2025 at 01:16:46PM +0200, Jani Nikula wrote:
> On Tue, 11 Nov 2025, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > On Fri, 31 Oct 2025, Mika Kahola <mika.kahola@intel.com> wrote:
> >> From: Imre Deak <imre.deak@intel.com>
> >>
> >> Print all the Cx0 PLL state in the PLL state dumper.
> >>
> >> Signed-off-by: Imre Deak <imre.deak@intel.com>
> >> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/display/intel_cx0_phy.c | 18 +++++++++++++++---
> >>  1 file changed, 15 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >> index 3418fc560faf..1e68a73c2ca8 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >> @@ -2311,8 +2311,8 @@ static void intel_c10pll_dump_hw_state(struct intel_display *display,
> >>  	unsigned int multiplier, tx_clk_div;
> >>  
> >>  	fracen = hw_state->pll[0] & C10_PLL0_FRACEN;
> >> -	drm_dbg_kms(display->drm, "c10pll_hw_state: fracen: %s, ",
> >> -		    str_yes_no(fracen));
> >> +	drm_dbg_kms(display->drm, "c10pll_hw_state: clock: %d, fracen: %s, ",
> >> +		    hw_state->clock, str_yes_no(fracen));
> >>  
> >>  	if (fracen) {
> >>  		frac_quot = hw_state->pll[12] << 8 | hw_state->pll[11];
> >> @@ -2835,7 +2835,7 @@ static void intel_c20pll_dump_hw_state(struct intel_display *display,
> >>  {
> >>  	int i;
> >>  
> >> -	drm_dbg_kms(display->drm, "c20pll_hw_state:\n");
> >> +	drm_dbg_kms(display->drm, "c20pll_hw_state clock: %d:\n", hw_state->clock);
> >>  	drm_dbg_kms(display->drm,
> >>  		    "tx[0] = 0x%.4x, tx[1] = 0x%.4x, tx[2] = 0x%.4x\n",
> >>  		    hw_state->tx[0], hw_state->tx[1], hw_state->tx[2]);
> >> @@ -2851,12 +2851,24 @@ static void intel_c20pll_dump_hw_state(struct intel_display *display,
> >>  		for (i = 0; i < ARRAY_SIZE(hw_state->mplla); i++)
> >>  			drm_dbg_kms(display->drm, "mplla[%d] = 0x%.4x\n", i,
> >>  				    hw_state->mplla[i]);
> >> +
> >> +		/* For full coverage, also print the additional PLL B entry. */
> >> +		WARN_ON(i + 1 != ARRAY_SIZE(hw_state->mpllb));
> >
> > Why? What if we hit this? At the very least please use
> > drm_WARN_ON(). What does the comment have to do with the WARN?

The WARN verifies that the additional entry to include in the print
exists and it is the only entry to print.

> Besides after the loop i == ARRAY_SIZE(hw_state->mplla), i.e. the whole
> thing can be
> 
> 	BUILD_BUG_ON(ARRAY_SIZE(hw_state->mplla) + 1 != ARRAY_SIZE(hw_state->mpllb));

Yes, BUILD_BUG_ON() is better here, no reason for delaying the check
until runtime.

> >
> >> +		drm_dbg_kms(display->drm, "mpllb[%d] = 0x%.4x\n", i, hw_state->mpllb[i]);
> >>  	}
> >> +
> >> +	drm_dbg_kms(display->drm, "vdr: custom width: 0x%02x, serdes rate: 0x%02x, hdmi rate: 0x%02x\n",
> >> +		    hw_state->vdr.custom_width, hw_state->vdr.serdes_rate, hw_state->vdr.hdmi_rate);
> >>  }
> >>  
> >>  void intel_cx0pll_dump_hw_state(struct intel_display *display,
> >>  				const struct intel_cx0pll_state *hw_state)
> >>  {
> >> +	drm_dbg_kms(display->drm,
> >> +		    "cx0pll_hw_state: lane_count: %d, ssc_enabled: %s, use_c10: %s, tbt_mode: %s\n",
> >> +		    hw_state->lane_count, str_yes_no(hw_state->ssc_enabled),
> >> +		    str_yes_no(hw_state->use_c10), str_yes_no(hw_state->tbt_mode));
> >> +
> >>  	if (hw_state->use_c10)
> >>  		intel_c10pll_dump_hw_state(display, &hw_state->c10);
> >>  	else
> 
> -- 
> Jani Nikula, Intel