Re: [PATCH 13/14] drm/i915/display: Add member to store AS SDP transmission time

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

On 2/26/2026 10:30 PM, Ville Syrjälä wrote:
> On Mon, Feb 23, 2026 at 07:14:30PM +0530, Ankit Nautiyal wrote:
>> AS SDP can be sent at two different positions T1 and T2.
>> These depend on the Panel Replay configuration and Adaptive Sync SDP
>> modes as per DP v2.1.
>> Currently we have configurations where SDP needs to be sent at T1 only.
>> However, to make way for supporting more PR and AS SDP configurations,
>> add a new member to store AS SDP transmission time in crtc_state.
>>
>> This is filled with T1 for now during panel_replay_compute_config() and
>> is used to set the MMIO register PR_ALPM and DPCD Panel_Replay_Config3
>> DPCD offsets.
>>
>> readout for this new member needs to be added along with other related
>> members.
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_alpm.c     | 21 ++++++++++++++++++-
>>   .../drm/i915/display/intel_display_types.h    |  1 +
>>   drivers/gpu/drm/i915/display/intel_psr.c      |  7 +++++++
>>   3 files changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c b/drivers/gpu/drm/i915/display/intel_alpm.c
>> index b3334bc4d0f9..6eecd5ce4fad 100644
>> --- a/drivers/gpu/drm/i915/display/intel_alpm.c
>> +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
>> @@ -365,6 +365,25 @@ void intel_alpm_lobf_compute_config(struct intel_dp *intel_dp,
>>   	crtc_state->has_lobf = true;
>>   }
>>   
>> +static int get_pr_alpm_as_sdp_transmission_time(const struct intel_crtc_state *crtc_state)
>> +{
>> +	struct intel_display *display = to_intel_display(crtc_state);
>> +
>> +	switch (crtc_state->pr_as_sdp_transmission) {
>> +	case AS_SDP_SETUP_TIME_T1:
>> +		return PR_ALPM_CTL_ADAPTIVE_SYNC_SDP_POSITION_T1;
>> +	case AS_SDP_SETUP_TIME_DYNAMIC:
>> +		return PR_ALPM_CTL_ADAPTIVE_SYNC_SDP_POSITION_T1_OR_T2;
>> +	case AS_SDP_SETUP_TIME_T2:
>> +		return PR_ALPM_CTL_ADAPTIVE_SYNC_SDP_POSITION_T2;
>> +	default:
>> +		drm_dbg_kms(display->drm,
>> +			    "Missing case %d for AS SDP Position, going with T1 by default\n",
>> +			    crtc_state->pr_as_sdp_transmission);
> MISSING_CASE()
>
> There's no point in adding eloquent debug messages/etc into dead code.

Roger that. I think we were avoiding MISSING_CASE() some time back 
during transitioning to struct display, but that was long back.

Will use MISSING_CASE() here.


>
>> +		return AS_SDP_SETUP_TIME_T1;
>> +	}
>> +}
>> +
>>   static void lnl_alpm_configure(struct intel_dp *intel_dp,
>>   			       const struct intel_crtc_state *crtc_state)
>>   {
>> @@ -388,7 +407,7 @@ static void lnl_alpm_configure(struct intel_dp *intel_dp,
>>   			ALPM_CTL_AUX_LESS_WAKE_TIME(crtc_state->alpm_state.aux_less_wake_lines);
>>   
>>   		if (intel_dp->as_sdp_supported) {
>> -			u32 pr_alpm_ctl = PR_ALPM_CTL_ADAPTIVE_SYNC_SDP_POSITION_T1;
>> +			u32 pr_alpm_ctl = get_pr_alpm_as_sdp_transmission_time(crtc_state);
>>   
>>   			if (crtc_state->link_off_after_as_sdp_when_pr_active)
>>   				pr_alpm_ctl |= PR_ALPM_CTL_ALLOW_LINK_OFF_BETWEEN_AS_SDP_AND_SU;
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
>> index e8e4af03a6a6..9065bf8bd6a5 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>> @@ -1181,6 +1181,7 @@ struct intel_crtc_state {
>>   	bool has_panel_replay;
>>   	bool link_off_after_as_sdp_when_pr_active;
>>   	bool disable_as_sdp_when_pr_active;
>> +	int pr_as_sdp_transmission;
> I don't see much point of adding that if you only ever set
> it to one fixed value. Though I suppose the point was to make
> sure the DPCD and PR_ALPM_CTL register values stay in sync.
> We could also achieve that by replacing that crtc_state member
> with eg. just a simple function.

The idea was to get this computed in compute_config phase. Currently 
this is a fixed value T1, but if we ever want to change (e.g. 
dynamically change to t2) we might need to store it and perhaps will be 
used to configure other things.

But all this is premature optimization and as you said, we could just 
have a simple function.

>
>>   	bool wm_level_disabled;
>>   	bool pkg_c_latency_used;
>>   	/* Only used for state verification. */
>> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
>> index e0e6ddbfaa2d..74242c93db87 100644
>> --- a/drivers/gpu/drm/i915/display/intel_psr.c
>> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>> @@ -779,6 +779,7 @@ static void _panel_replay_enable_sink(struct intel_dp *intel_dp,
>>   		DP_PANEL_REPLAY_RFB_STORAGE_ERROR_EN |
>>   		DP_PANEL_REPLAY_ACTIVE_FRAME_CRC_ERROR_EN;
>>   	u8 panel_replay_config2 = DP_PANEL_REPLAY_CRC_VERIFICATION;
>> +	u8 panel_replay_config3 = crtc_state->pr_as_sdp_transmission;
>>   
>>   	if (crtc_state->has_sel_update)
>>   		val |= DP_PANEL_REPLAY_SU_ENABLE;
>> @@ -794,6 +795,9 @@ static void _panel_replay_enable_sink(struct intel_dp *intel_dp,
>>   
>>   	drm_dp_dpcd_writeb(&intel_dp->aux, PANEL_REPLAY_CONFIG2,
>>   			   panel_replay_config2);
>> +
>> +	drm_dp_dpcd_writeb(&intel_dp->aux, PANEL_REPLAY_CONFIG3,
>> +			   panel_replay_config3);
> This is starting to look like someone should make this a burst write.

I can send a separate patch for this.


Regards,

Ankit

>
>>   }
>>   
>>   static void _psr_enable_sink(struct intel_dp *intel_dp,
>> @@ -1771,6 +1775,9 @@ static bool _panel_replay_compute_config(struct intel_crtc_state *crtc_state,
>>   	crtc_state->link_off_after_as_sdp_when_pr_active = compute_link_off_after_as_sdp_when_pr_active(connector);
>>   	crtc_state->disable_as_sdp_when_pr_active = compute_disable_as_sdp_when_pr_active(connector);
>>   
>> +	/* For now we use T1 as the transmission time */
>> +	crtc_state->pr_as_sdp_transmission = AS_SDP_SETUP_TIME_T1;
>> +
>>   	if (!intel_dp_is_edp(intel_dp))
>>   		return true;
>>   
>> -- 
>> 2.45.2