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

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

On 10/10/2025 12:10 PM, Hogander, Jouni wrote:
> 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?

This check wake_lines_fit_into_vblank() is called during 
encoder->compute_config() path (specifically in psr_compute_config()).

At this point of time set_context_latency is not computed. It is 
computed later in intel_crtc_compute_config().

There is some more discussion about it in : 
https://lore.kernel.org/all/aOVOJp2zeN1eCp7O@intel.com/

Perhaps I should have mentioned this in cover-letter.



>
>> +	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.

Hmm makes sense we are checking for psr2_config_valid() only when 
crtc_state->has_panel_replay is false.

Thanks for pointing this out, will fix this.


Regards,

Ankit

>
> 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)