Re: [PATCH] drm/i915/cx0: Convert C10 PHY PLL SSC state mismatch WARN to a debug message

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

On Mon, Dec 08, 2025 at 02:03:15PM +0200, Jani Nikula wrote:
> On Fri, 05 Dec 2025, Imre Deak <imre.deak@intel.com> wrote:
> > On C10 PHY PLLs the SSC is enabled by programming the
> > XELPDP_PORT_CLOCK_CTL / XELPDP_SSC_ENABLE_PLLB flag and the
> > PHY_C10_VDR_PLL 4..8 registers:
> >
> > - If SSC is enabled XELPDP_SSC_ENABLE_PLLB is set and the
> >   PHY_C10_VDR_PLL registers are programmed to non-zero values.
> > - If SSC is disabled XELPDP_SSC_ENABLE_PLLB is cleared and the
> >   PHY_C10_VDR_PLL registers are programmed to zeroed-out values.
> >
> > The driver's state checker verifies if the above settings are consistent,
> > i.e. if XELPDP_SSC_ENABLE_PLLB being set corresponds to the
> > PHY_C10_VDR_PLL registers being zeroed-out or not.
> >
> > On WCL the BIOS programs non-zero values to the PHY_C10_VDR_PLL 4..8
> > registers, but does not set the XELPDP_SSC_ENABLE_PLLB flag. This will
> > trigger the following PLL state check warning during driver loading:
> >
> > <4>[   44.457809] xe 0000:00:02.0: [drm] PHY B: SSC enabled state (no), doesn't match PLL configuration (SSC-enabled)
> 
> BTW I also think the message is really confusing.
> 
> "SSC enabled state (no)" vs. "PLL configuration (SSC-enabled)".
> 
> *BOTH* need to say SSC with str_enabled_disabled() and ditch the clumsy
> "SSC enabled state yes/no" and "SSC-enabled/SSC-disabled".

Makes sense, can change it to:
if (cx0pll_state->ssc_enabled != intel_c10pll_ssc_enabled(pll_state))
	drm_dbg_kms(display->drm,
		    "PHY %c: SSC state mismatch: port SSC is %s, PLL SSC is %s\n",
		    phy_name(phy),
		    str_enabled_disabled(cx0pll_state->ssc_enabled),
		    str_enabled_disabled(intel_c10pll_ssc_enabled(pll_state)));

> 
> BR,
> Jani.
> 
> 
> > <4>[   44.457833] WARNING: CPU: 4 PID: 298 at drivers/gpu/drm/i915/display/intel_cx0_phy.c:2281 intel_cx0pll_readout_hw_state+0x221/0x620 [xe]
> >
> > It's not clear whether the HW uses the PHY_C10_VDR_PLL 4..8 register
> > values if the XELPDP_SSC_ENABLE_PLLB flag is cleared, or just ignores
> > them in this case. Since the driver always programs the register values
> > according to the above, it still makes sense to verify that the
> > programming happened correctly.
> >
> > To avoid the state check WARN during driver loading due to the way BIOS
> > programs the registers, convert the WARN to a debug message.
> >
> > Cc: Mika Kahola <mika.kahola@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_cx0_phy.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > index 7bd17723e7abb..b973a9201edda 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > @@ -2278,11 +2278,12 @@ static void intel_c10pll_readout_hw_state(struct intel_encoder *encoder,
> >  	pll_state->clock = intel_c10pll_calc_port_clock(encoder, pll_state);
> >  
> >  	cx0pll_state->ssc_enabled = readout_ssc_state(encoder, true);
> > -	drm_WARN(display->drm,
> > -		 cx0pll_state->ssc_enabled != intel_c10pll_ssc_enabled(pll_state),
> > -		 "PHY %c: SSC enabled state (%s), doesn't match PLL configuration (%s)\n",
> > -		 phy_name(phy), str_yes_no(cx0pll_state->ssc_enabled),
> > -		 intel_c10pll_ssc_enabled(pll_state) ? "SSC-enabled" : "SSC-disabled");
> > +
> > +	if (cx0pll_state->ssc_enabled != intel_c10pll_ssc_enabled(pll_state))
> > +		drm_dbg_kms(display->drm,
> > +			    "PHY %c: SSC enabled state (%s), doesn't match PLL configuration (%s)\n",
> > +			    phy_name(phy), str_yes_no(cx0pll_state->ssc_enabled),
> > +			    intel_c10pll_ssc_enabled(pll_state) ? "SSC-enabled" : "SSC-disabled");
> >  }
> >  
> >  static void intel_c10_pll_program(struct intel_display *display,
> 
> -- 
> Jani Nikula, Intel