Re: [v4 3/4] drm/i915/display: Reprogram AS SDP skip frames on seamless VRR transitions

[Dibin Moolakadan Subrahmanian <dibin.moolakadan.subrahmanian@intel.com>]

On 9/1/2026 3:04 AM, Uma Shankar wrote:
> The AS SDP skip-frame count is written to PR_ALPM_CTL only from
> lnl_alpm_configure(), which runs from intel_psr_enable_locked() on a
> Panel Replay disabled->enabled transition. VRR, however, can be enabled
> and disabled seamlessly - without a modeset and without cycling Panel
> Replay (intel_crtc_vrr_enabling()/disabling() in the pipe update path).
>
> As a result, when a panel comes up with VRR off the non-zero skip count
> is programmed, and when VRR is later turned on seamlessly PR stays
> enabled, lnl_alpm_configure() is not re-invoked, and the stale skip
> count is left in the register. This also leaves the coupled AS SDP
> transmission / DC3CO idle-protocol bits inconsistent with the DC3co
> state, which is recomputed on every commit.
>
> Factor the PR_ALPM_CTL AS SDP programming out of lnl_alpm_configure()
> into intel_alpm_configure_pr_as_sdp() and expose
> intel_alpm_pr_as_sdp_update(), which recomputes those fields for the
> current VRR state. Call it from the seamless VRR enable and disable
> sites (non-modeset only; a modeset re-runs PR enable anyway) so the skip
> counter always matches whether VRR is actively driving the refresh rate.
>
> v2: Fixed Sashiko review comments
>
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_alpm.c    | 119 +++++++++++++------
>   drivers/gpu/drm/i915/display/intel_alpm.h    |   1 +
>   drivers/gpu/drm/i915/display/intel_display.c |  18 +++
>   3 files changed, 100 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c b/drivers/gpu/drm/i915/display/intel_alpm.c
> index 0a33a89975bc..c784e77f610b 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> @@ -427,6 +427,85 @@ bool intel_alpm_pr_as_sdp_skip_frames_enabled(struct intel_dp *intel_dp,
>   	return intel_pr_as_sdp_skip_frames(intel_dp) > 0;
>   }
>   
> +/*
> + * Program the AS SDP portion of PR_ALPM_CTL: the transmission position, the
> + * skip-frame counter and the coupled AS SDP transmission / DC3CO idle-protocol
> + * bits. This is a full recompute of those fields (the base value is built from
> + * scratch, not read back), so it can be called both at PR enable time and when
> + * VRR is toggled seamlessly - which changes whether periodic AS SDP is used.
> + *
> + * Caller must hold intel_dp->alpm.lock.
> + */
> +static void intel_alpm_configure_pr_as_sdp(struct intel_dp *intel_dp,
> +					   const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_display *display = to_intel_display(intel_dp);
> +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> +	u32 pr_alpm_ctl = get_pr_alpm_as_sdp_transmission_time(crtc_state);
> +	u32 skip_frames = 0;
> +
> +	if (intel_alpm_pr_as_sdp_skip_frames_enabled(intel_dp, crtc_state))
> +		skip_frames = intel_pr_as_sdp_skip_frames(intel_dp);
> +
> +	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;
> +
> +	/*
> +	 * Skip frames needs the AS SDP to keep flowing during PR active, so it
> +	 * is mutually exclusive with disabling AS SDP transmission in active and
> +	 * with the DC3CO idle protocol.
> +	 */
> +	if (skip_frames) {
> +		pr_alpm_ctl |= PR_ALPM_CTL_AS_SDP_SKIP_FRAMES(skip_frames);
> +		pr_alpm_ctl &= ~PR_ALPM_CTL_AS_SDP_TRANSMISSION_IN_ACTIVE_DISABLE;
> +		pr_alpm_ctl &= ~PR_ALPM_CTL_USE_DC3CO_IDLE_PROTOCOL;
> +	} else {
> +		pr_alpm_ctl &= ~PR_ALPM_CTL_AS_SDP_SKIP_FRAMES_MASK;
> +
> +		if (crtc_state->disable_as_sdp_when_pr_active)
> +			pr_alpm_ctl |= PR_ALPM_CTL_AS_SDP_TRANSMISSION_IN_ACTIVE_DISABLE;
> +
> +		if (intel_display_power_dc3co_allowed(display))
> +			pr_alpm_ctl |= PR_ALPM_CTL_USE_DC3CO_IDLE_PROTOCOL;
> +	}
> +
> +	intel_de_write(display, PR_ALPM_CTL(display, cpu_transcoder), pr_alpm_ctl);
> +}
> +
> +/*
> + * VRR can be enabled or disabled seamlessly, i.e. without a modeset and without
> + * cycling Panel Replay, so the AS SDP skip-frame programming done at PR enable
> + * time would otherwise go stale across a VRR toggle. Reprogram it here so the
> + * skip counter (and the coupled bits) matches the new VRR state.
> + */
> +void intel_alpm_pr_as_sdp_update(const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_display *display = to_intel_display(crtc_state);
> +	struct intel_encoder *encoder;
> +
> +	/* AS SDP skip frames field only exists on Xe3p_LPD+ */
> +	if (DISPLAY_VER(display) < 35)
> +		return;
> +
> +	for_each_intel_encoder_mask(display->drm, encoder,
> +				    crtc_state->uapi.encoder_mask) {
> +		struct intel_dp *intel_dp;
> +
> +		if (!intel_encoder_is_dp(encoder))
> +			continue;
> +
> +		intel_dp = enc_to_intel_dp(encoder);
> +
> +		if (!intel_dp->as_sdp_supported ||
> +		    !intel_alpm_is_alpm_aux_less(intel_dp, crtc_state))
> +			continue;
> +
> +		mutex_lock(&intel_dp->alpm.lock);
> +		intel_alpm_configure_pr_as_sdp(intel_dp, crtc_state);
> +		mutex_unlock(&intel_dp->alpm.lock);
> +	}
> +}
> +
>   static void lnl_alpm_configure(struct intel_dp *intel_dp,
>   			       const struct intel_crtc_state *crtc_state)
>   {
> @@ -449,44 +528,8 @@ static void lnl_alpm_configure(struct intel_dp *intel_dp,
>   			ALPM_CTL_AUX_LESS_SLEEP_HOLD_TIME_50_SYMBOLS |
>   			ALPM_CTL_AUX_LESS_WAKE_TIME(crtc_state->alpm_state.aux_less_wake_lines);
>   
> -		if (intel_dp->as_sdp_supported) {
> -			u32 pr_alpm_ctl = get_pr_alpm_as_sdp_transmission_time(crtc_state);
> -			u32 skip_frames = 0;
> -
> -			/*
> -			 * AS SDP skip frames field only exists on Xe3p_LPD+, and
> -			 * periodic AS SDP is a Panel Replay feature that is only
> -			 * used when VRR is not actively driving the refresh rate.
> -			 */
> -			if (DISPLAY_VER(display) >= 35 && crtc_state->has_panel_replay &&
> -			    !crtc_state->vrr.enable)
> -				skip_frames = intel_pr_as_sdp_skip_frames(intel_dp);
> -
> -			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;
> -
> -			/*
> -			 * Skip frames needs the AS SDP to keep flowing during PR
> -			 * active, so it is mutually exclusive with disabling AS SDP
> -			 * transmission in active and with the DC3CO idle protocol.
> -			 */
> -			if (skip_frames) {
> -				pr_alpm_ctl |= PR_ALPM_CTL_AS_SDP_SKIP_FRAMES(skip_frames);
> -				pr_alpm_ctl &= ~PR_ALPM_CTL_AS_SDP_TRANSMISSION_IN_ACTIVE_DISABLE;
> -				pr_alpm_ctl &= ~PR_ALPM_CTL_USE_DC3CO_IDLE_PROTOCOL;
> -			} else {
> -				pr_alpm_ctl &= ~PR_ALPM_CTL_AS_SDP_SKIP_FRAMES_MASK;
> -
> -				if (crtc_state->disable_as_sdp_when_pr_active)
> -					pr_alpm_ctl |= PR_ALPM_CTL_AS_SDP_TRANSMISSION_IN_ACTIVE_DISABLE;
> -
> -				if (intel_display_power_dc3co_allowed(display))
> -					pr_alpm_ctl |= PR_ALPM_CTL_USE_DC3CO_IDLE_PROTOCOL;
> -			}
> -
> -			intel_de_write(display, PR_ALPM_CTL(display, cpu_transcoder),
> -				       pr_alpm_ctl);
> -		}
> +		if (intel_dp->as_sdp_supported)
> +			intel_alpm_configure_pr_as_sdp(intel_dp, crtc_state);
>   
>   	} else {
>   		alpm_ctl = ALPM_CTL_EXTENDED_FAST_WAKE_ENABLE |
> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.h b/drivers/gpu/drm/i915/display/intel_alpm.h
> index 328920027f1c..f8f605d94f96 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.h
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.h
> @@ -36,6 +36,7 @@ bool intel_alpm_is_alpm_aux_less(struct intel_dp *intel_dp,
>   				 const struct intel_crtc_state *crtc_state);
>   bool intel_alpm_pr_as_sdp_skip_frames_enabled(struct intel_dp *intel_dp,
>   					      const struct intel_crtc_state *crtc_state);
> +void intel_alpm_pr_as_sdp_update(const struct intel_crtc_state *crtc_state);
>   void intel_alpm_disable(struct intel_dp *intel_dp);
>   bool intel_alpm_get_error(struct intel_dp *intel_dp);
>   void intel_alpm_lobf_compute_config_late(struct intel_dp *intel_dp,
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index fc30a455bed3..9151ea6c15ab 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -1232,6 +1232,14 @@ static void intel_pre_plane_update(struct intel_atomic_state *state,
>   		intel_vrr_disable(old_crtc_state);
>   		intel_vrr_dcb_reset(old_crtc_state, crtc);
>   		intel_crtc_update_active_timings(old_crtc_state, false);
> +
> +		/*
> +		 * VRR is being disabled seamlessly (no modeset, Panel Replay
> +		 * stays enabled), so re-apply the AS SDP skip-frame programming
> +		 * for the new (VRR off) state.
> +		 */
> +		if (!intel_crtc_needs_modeset(new_crtc_state))
> +			intel_alpm_pr_as_sdp_update(new_crtc_state);
>   	}
>   
>   	if (audio_disabling(old_crtc_state, new_crtc_state))
> @@ -6971,6 +6979,16 @@ static void intel_update_crtc(struct intel_atomic_state *state,
>   		intel_crtc_update_active_timings(new_crtc_state,
>   						 new_crtc_state->vrr.enable);
>   
> +	/*
> +	 * VRR is being enabled seamlessly (no modeset, Panel Replay stays
> +	 * enabled), so re-apply the AS SDP skip-frame programming for the new
> +	 * (VRR on) state. Done here, outside the vblank-evasion critical section
> +	 * (which runs with interrupts disabled), because it takes alpm.lock.
> +	 */
> +	if (intel_crtc_vrr_enabling(state, crtc) &&
> +	    !intel_crtc_needs_modeset(new_crtc_state))
> +		intel_alpm_pr_as_sdp_update(new_crtc_state);
> +
>   	if (new_crtc_state->vrr.dc_balance.enable)
>   		intel_vrr_dcb_increment_flip_count(new_crtc_state, crtc);
>   

LGTM.
Reviewed-by: Dibin Moolakadan Subrahmanian <dibin.moolakadan.subrahmanian@intel.com>