Re: [PATCH v2] drm/i915/psr: clear the Panel Replay error status register

[Hogander, Jouni]

On Tue, 2026-09-01 at 16:59 -0400, Jake Steinman wrote:
> psr_get_status_and_error_status() selects the DPCD offset to read the
> error
> status from based on whether Panel Replay is enabled:
> 
> 	offset = intel_dp->psr.panel_replay_enabled ?
> 		 DP_PANEL_REPLAY_ERROR_STATUS : DP_PSR_ERROR_STATUS;
> 
> but intel_psr_short_pulse() acknowledges it unconditionally to the
> PSR
> register. Under Panel Replay the error is therefore read from
> DP_PANEL_REPLAY_ERROR_STATUS (0x2020) and the acknowledgement written
> to
> DP_PSR_ERROR_STATUS (0x2006). DP_PANEL_REPLAY_ERROR_STATUS is never
> written
> anywhere in the tree; it appears only in the read above and in its
> own #define.
> 
> The sink's Panel Replay error latch can consequently never be
> cleared. Once it
> latches, every subsequent short pulse re-reads the same errors, so
> PSR is
> disabled with sink_not_reliable set permanently, and until a short
> pulse
> arrives the driver keeps Panel Replay enabled while the sink is
> reporting
> errors it cannot see.
> 
> Observed on a Dell XPS 16 DA16260 (Panther Lake, Arc B390, display
> version
> 30.00) with the eDP Panel Replay quirk from commit cb8d155b0806
> removed
> locally so the feature could be exercised. The sink reports a
> persistently
> latched error:
> 
> 	Sink PANEL-REPLAY status: 0x2 [active, display from RFB]
> 	Sink PANEL-REPLAY error status: 0x1:
> 		PANEL-REPLAY Link CRC error
> 
> which survives across reads indefinitely, while dmesg stays silent
> and Panel
> Replay Selective Update remains enabled.
> 
> Use the same conditional offset when clearing. psr-
> >panel_replay_enabled
> cannot be used at that point because intel_psr_disable_locked()
> clears it
> earlier in the same function whenever an error was detected, which is
> exactly
> the case that needs the Panel Replay offset, so save it beforehand.
> 
> v2: use a copy of panel_replay_enabled taken before
>     intel_psr_disable_locked() clears it. In v1 the condition was
> evaluated
>     after the disable, so it selected DP_PSR_ERROR_STATUS in the
> error path
>     and the patch was a no-op there. Caught by Sashiko AI review.
> 
> Signed-off-by: Jake Steinman <j@xxxxxxxxxxxxxxxx>

Reviewed-by: Jouni Högander <jouni.hogander@xxxxxxxxx>

> ---
>  drivers/gpu/drm/i915/display/intel_psr.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -3840,6 +3840,7 @@
>  	struct intel_display *display = to_intel_display(intel_dp);
>  	struct intel_psr *psr = &intel_dp->psr;
>  	u8 status, error_status;
> +	bool panel_replay_enabled;
>  	const u8 errors = DP_PSR_RFB_STORAGE_ERROR |
>  			  DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR |
>  			  DP_PSR_LINK_CRC_ERROR;
> @@ -3860,6 +3861,12 @@
>  		goto exit;
>  	}
>  
> +	/*
> +	 * Save this before intel_psr_disable_locked() clears it;
> the error
> +	 * status is acknowledged to a different DPCD address
> depending on it.
> +	 */
> +	panel_replay_enabled = psr->panel_replay_enabled;
> +
>  	if ((!psr->panel_replay_enabled && status ==
> DP_PSR_SINK_INTERNAL_ERROR) ||
>  	    (error_status & errors)) {
>  		intel_psr_disable_locked(intel_dp);
> @@ -3885,7 +3892,10 @@
>  			"PSR_ERROR_STATUS unhandled errors %x\n",
>  			error_status & ~errors);
>  	/* clear status register */
> -	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_ERROR_STATUS,
> error_status);
> +	drm_dp_dpcd_writeb(&intel_dp->aux,
> +			   panel_replay_enabled ?
> +			   DP_PANEL_REPLAY_ERROR_STATUS :
> DP_PSR_ERROR_STATUS,
> +			   error_status);
>  
>  	if (!psr->panel_replay_enabled) {
>  		psr_alpm_check(intel_dp);