Re: [PATCH 1/3] drm/i915/ltphy: Remove state verification for LT PHY fields

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 12/31/2025 10:53 AM, Suraj Kandpal wrote:
> Remove LT PHY State verification for all VDR register fields other
> than VDR0_CONFIG and VDR2_CONFIG since those are the only reliable

Nit pick:

I think shorter sentence will be easier to comprehend. Let the sentence 
end after VDR2_CONFIG:

`than VDR0_CONFIG and VDR2_CONFIG. VDR0_CONFIG and VDR2_CONFIG are the 
only reliable`

> shadow register that hold on to its value in case there is PSR/PR

Nit pick:

shadow registers that hold on to their values even with the PSR/PR features


> which can cause power gating internally of the PHY.

Hmm.. it seems that there are few things implicit in the message.

I guess it would be easier if we mention the current practice first, 
followed by the problem faced with the current practice.

After that the solution of only comparing the VDR{0,2}_CONFIG can be 
explained.

but that is my personal opinion.

>
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>


With the commit message improved:

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>


> ---
>   drivers/gpu/drm/i915/display/intel_lt_phy.c | 30 +++++----------------
>   1 file changed, 7 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_lt_phy.c b/drivers/gpu/drm/i915/display/intel_lt_phy.c
> index 939c8975fd4c..9501ac861712 100644
> --- a/drivers/gpu/drm/i915/display/intel_lt_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_lt_phy.c
> @@ -2259,8 +2259,6 @@ void intel_lt_phy_pll_state_verify(struct intel_atomic_state *state,
>   	struct intel_encoder *encoder;
>   	struct intel_lt_phy_pll_state pll_hw_state = {};
>   	const struct intel_lt_phy_pll_state *pll_sw_state = &new_crtc_state->dpll_hw_state.ltpll;
> -	int clock;
> -	int i, j;
>   
>   	if (DISPLAY_VER(display) < 35)
>   		return;
> @@ -2275,33 +2273,19 @@ void intel_lt_phy_pll_state_verify(struct intel_atomic_state *state,
>   
>   	encoder = intel_get_crtc_new_encoder(state, new_crtc_state);
>   	intel_lt_phy_pll_readout_hw_state(encoder, new_crtc_state, &pll_hw_state);
> -	clock = intel_lt_phy_calc_port_clock(encoder, new_crtc_state);
>   
>   	dig_port = enc_to_dig_port(encoder);
>   	if (intel_tc_port_in_tbt_alt_mode(dig_port))
>   		return;
>   
> -	INTEL_DISPLAY_STATE_WARN(display, pll_hw_state.clock != clock,
> -				 "[CRTC:%d:%s] mismatch in LT PHY: Register CLOCK (expected %d, found %d)",
> +	INTEL_DISPLAY_STATE_WARN(display, pll_hw_state.config[0] != pll_sw_state->config[0],
> +				 "[CRTC:%d:%s] mismatch in LT PHY PLL CONFIG 0: (expected 0x%04x, found 0x%04x)",
>   				 crtc->base.base.id, crtc->base.name,
> -				 pll_sw_state->clock, pll_hw_state.clock);
> -
> -	for (i = 0; i < 3; i++) {
> -		INTEL_DISPLAY_STATE_WARN(display, pll_hw_state.config[i] != pll_sw_state->config[i],
> -					 "[CRTC:%d:%s] mismatch in LT PHY PLL CONFIG%d: (expected 0x%04x, found 0x%04x)",
> -					 crtc->base.base.id, crtc->base.name, i,
> -					 pll_sw_state->config[i], pll_hw_state.config[i]);
> -	}
> -
> -	for (i = 0; i <= 12; i++) {
> -		for (j = 3; j >= 0; j--)
> -			INTEL_DISPLAY_STATE_WARN(display,
> -						 pll_hw_state.data[i][j] !=
> -						 pll_sw_state->data[i][j],
> -						 "[CRTC:%d:%s] mismatch in LT PHY PLL DATA[%d][%d]: (expected 0x%04x, found 0x%04x)",
> -						 crtc->base.base.id, crtc->base.name, i, j,
> -						 pll_sw_state->data[i][j], pll_hw_state.data[i][j]);
> -	}
> +				 pll_sw_state->config[0], pll_hw_state.config[0]);
> +	INTEL_DISPLAY_STATE_WARN(display, pll_hw_state.config[2] != pll_sw_state->config[2],
> +				 "[CRTC:%d:%s] mismatch in LT PHY PLL CONFIG 2: (expected 0x%04x, found 0x%04x)",
> +				 crtc->base.base.id, crtc->base.name,
> +				 pll_sw_state->config[2], pll_hw_state.config[2]);
>   }
>   
>   void intel_xe3plpd_pll_enable(struct intel_encoder *encoder,