Re: [PATCH 4/8] drm/i915/psr: Consider SCL lines when validating vblank for wake latency

["Hogander, Jouni" <jouni.hogander@intel.com>]

On Thu, 2025-10-09 at 14:30 +0530, Ankit Nautiyal wrote:
> Panel Replay and PSR2 selective update require sufficient vblank
> duration
> to accommodate wake latencies. However, the current
> wake_lines_fit_into_vblank() logic does not account for the minimum
> Set Context Latency (SCL) lines.
> 
> Separate out _intel_psr_min_set_context_latency() to compute the
> minimum
> SCL requirement based on platform and feature usage.
> 
> The alpm_config_valid() helper is updated to pass the necessary
> context for
> determining whether Panel Replay or PSR2 selective update is enabled.
> 
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> Cc: Animesh Manna <animesh.manna@intel.com>
> Cc: Jouni Högander <jouni.hogander@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_psr.c | 102 ++++++++++++++-------
> --
>  1 file changed, 61 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> b/drivers/gpu/drm/i915/display/intel_psr.c
> index 2131473cead6..212bd244beed 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1361,14 +1361,64 @@ static int
> intel_psr_entry_setup_frames(struct intel_dp *intel_dp,
>  	return entry_setup_frames;
>  }
>  
> +static
> +int _intel_psr_min_set_context_latency(const struct intel_crtc_state
> *crtc_state,
> +				       bool needs_panel_replay,
> +				       bool needs_sel_update)
> +{
> +	struct intel_display *display =
> to_intel_display(crtc_state);
> +
> +	if (!crtc_state->has_psr)
> +		return 0;
> +
> +	/* Wa_14015401596 */
> +	if (intel_vrr_possible(crtc_state) &&
> IS_DISPLAY_VER(display, 13, 14))
> +		return 1;
> +
> +	/* Rest is for SRD_STATUS needed on LunarLake and onwards */
> +	if (DISPLAY_VER(display) < 20)
> +		return 0;
> +
> +	/*
> +	 * Comment on SRD_STATUS register in Bspec for LunarLake and
> onwards:
> +	 *
> +	 * To deterministically capture the transition of the state
> machine
> +	 * going from SRDOFFACK to IDLE, the delayed V. Blank should
> be at least
> +	 * one line after the non-delayed V. Blank.
> +	 *
> +	 * Legacy TG: TRANS_SET_CONTEXT_LATENCY > 0
> +	 * VRR TG: TRANS_VRR_CTL[ VRR Guardband ] < (TRANS_VRR_VMAX[
> VRR Vmax ]
> +	 * - TRANS_VTOTAL[ Vertical Active ])
> +	 *
> +	 * SRD_STATUS is used only by PSR1 on PantherLake.
> +	 * SRD_STATUS is used by PSR1 and Panel Replay DP on
> LunarLake.
> +	 */
> +
> +	if (DISPLAY_VER(display) >= 30 && (needs_panel_replay ||
> +					   needs_sel_update))
> +		return 0;
> +	else if (DISPLAY_VER(display) < 30 && (needs_sel_update ||
> +					      
> intel_crtc_has_type(crtc_state,
> +								  
> INTEL_OUTPUT_EDP)))
> +		return 0;
> +	else
> +		return 1;
> +}
> +
>  static bool wake_lines_fit_into_vblank(struct intel_dp *intel_dp,
>  				       const struct intel_crtc_state
> *crtc_state,
> -				       bool aux_less)
> +				       bool aux_less,
> +				       bool needs_sel_update,
> +				       bool needs_panel_replay)
>  {
>  	struct intel_display *display = to_intel_display(intel_dp);
>  	int vblank = crtc_state->hw.adjusted_mode.crtc_vblank_end -
>  		crtc_state->hw.adjusted_mode.crtc_vblank_start;
>  	int wake_lines;
> +	int scl = _intel_psr_min_set_context_latency(crtc_state,
> +						    
> needs_sel_update,
> +						    
> needs_panel_replay);

Why can't you use crtc_state->set_context_latency?

> +	vblank -= scl;
>  
>  	if (aux_less)
>  		wake_lines = crtc_state-
> >alpm_state.aux_less_wake_lines;
> @@ -1390,7 +1440,9 @@ static bool wake_lines_fit_into_vblank(struct
> intel_dp *intel_dp,
>  
>  static bool alpm_config_valid(struct intel_dp *intel_dp,
>  			      struct intel_crtc_state *crtc_state,
> -			      bool aux_less)
> +			      bool aux_less,
> +			      bool needs_sel_update,
> +			      bool needs_panel_replay)
>  {
>  	struct intel_display *display = to_intel_display(intel_dp);
>  
> @@ -1400,7 +1452,8 @@ static bool alpm_config_valid(struct intel_dp
> *intel_dp,
>  		return false;
>  	}
>  
> -	if (!wake_lines_fit_into_vblank(intel_dp, crtc_state,
> aux_less)) {
> +	if (!wake_lines_fit_into_vblank(intel_dp, crtc_state,
> aux_less,
> +					needs_sel_update,
> needs_panel_replay)) {
>  		drm_dbg_kms(display->drm,
>  			    "PSR2/Panel Replay not enabled, too
> short vblank time\n");
>  		return false;
> @@ -1492,7 +1545,7 @@ static bool intel_psr2_config_valid(struct
> intel_dp *intel_dp,
>  		return false;
>  	}
>  
> -	if (!alpm_config_valid(intel_dp, crtc_state, false))
> +	if (!alpm_config_valid(intel_dp, crtc_state, false, true,
> crtc_state->has_panel_replay))

This is a bit misleading. Someone might think intel_psr2_config_valid
could be called with crtc_state->has_panel_replay == 1. Rather use
false here.

BR,

Jouni Högander

>  		return false;
>  
>  	if (!crtc_state->enable_psr2_sel_fetch &&
> @@ -1643,7 +1696,7 @@ _panel_replay_compute_config(struct intel_dp
> *intel_dp,
>  		return false;
>  	}
>  
> -	if (!alpm_config_valid(intel_dp, crtc_state, true))
> +	if (!alpm_config_valid(intel_dp, crtc_state, true, false,
> true))
>  		return false;
>  
>  	return true;
> @@ -2371,43 +2424,10 @@ void
> intel_psr_trigger_frame_change_event(struct intel_dsb *dsb,
>   */
>  int intel_psr_min_set_context_latency(const struct intel_crtc_state
> *crtc_state)
>  {
> -	struct intel_display *display =
> to_intel_display(crtc_state);
> -
> -	if (!crtc_state->has_psr)
> -		return 0;
> -
> -	/* Wa_14015401596 */
> -	if (intel_vrr_possible(crtc_state) &&
> IS_DISPLAY_VER(display, 13, 14))
> -		return 1;
> -
> -	/* Rest is for SRD_STATUS needed on LunarLake and onwards */
> -	if (DISPLAY_VER(display) < 20)
> -		return 0;
>  
> -	/*
> -	 * Comment on SRD_STATUS register in Bspec for LunarLake and
> onwards:
> -	 *
> -	 * To deterministically capture the transition of the state
> machine
> -	 * going from SRDOFFACK to IDLE, the delayed V. Blank should
> be at least
> -	 * one line after the non-delayed V. Blank.
> -	 *
> -	 * Legacy TG: TRANS_SET_CONTEXT_LATENCY > 0
> -	 * VRR TG: TRANS_VRR_CTL[ VRR Guardband ] < (TRANS_VRR_VMAX[
> VRR Vmax ]
> -	 * - TRANS_VTOTAL[ Vertical Active ])
> -	 *
> -	 * SRD_STATUS is used only by PSR1 on PantherLake.
> -	 * SRD_STATUS is used by PSR1 and Panel Replay DP on
> LunarLake.
> -	 */
> -
> -	if (DISPLAY_VER(display) >= 30 && (crtc_state-
> >has_panel_replay ||
> -					   crtc_state-
> >has_sel_update))
> -		return 0;
> -	else if (DISPLAY_VER(display) < 30 && (crtc_state-
> >has_sel_update ||
> -					      
> intel_crtc_has_type(crtc_state,
> -								  
> INTEL_OUTPUT_EDP)))
> -		return 0;
> -	else
> -		return 1;
> +	return _intel_psr_min_set_context_latency(crtc_state,
> +						  crtc_state-
> >has_panel_replay,
> +						  crtc_state-
> >has_sel_update);
>  }
>  
>  static u32 man_trk_ctl_enable_bit_get(struct intel_display *display)