Re: [PATCH 2/2] drm/i915/dp: Fix panel replay when DSC is enabled

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

On Wed, 2025-10-08 at 12:41 +0300, Imre Deak wrote:
> Prevent enabling panel replay if the sink doesn't support this due to
> DSC being enabled.
> 
> Panel replay has two modes, updating full frames or only selected
> regions of the frame. If the sink doesn't support PR in full frame
> update mode with DSC prevent PR completely if DSC is enabled. If the
> sink doesn't support PR only in the selective update mode while DSC
> is
> enabled, it will still support PR in the full frame update mode, so
> only
> prevent selective updates in this case.
> 
> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/14869
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  .../drm/i915/display/intel_display_types.h    |  9 ++
>  drivers/gpu/drm/i915/display/intel_dp.c       |  2 +
>  drivers/gpu/drm/i915/display/intel_psr.c      | 93
> ++++++++++++++++++-
>  3 files changed, 99 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index ca5a87772e93a..e0c5abab3b380 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -955,6 +955,12 @@ struct intel_csc_matrix {
>  	u16 postoff[3];
>  };
>  
> +enum intel_panel_replay_dsc_support {
> +	INTEL_DP_PANEL_REPLAY_DSC_NOT_SUPPORTED,
> +	INTEL_DP_PANEL_REPLAY_DSC_FULL_FRAME_ONLY,
> +	INTEL_DP_PANEL_REPLAY_DSC_SELECTIVE_UPDATE,
> +};
> +

How about relying on pr_dpcd in _panel_replay_compute_config and
intel_sel_update_config_valid with some helpers?

>  struct intel_crtc_state {
>  	/*
>  	 * uapi (drm) state. This is the software state shown to
> userspace.
> @@ -1133,6 +1139,8 @@ struct intel_crtc_state {
>  	bool has_panel_replay;
>  	bool wm_level_disabled;
>  	bool pkg_c_latency_used;
> +	/* Only used for state verification. */
> +	enum intel_panel_replay_dsc_support
> panel_replay_dsc_support;
>  	u32 dc3co_exitline;
>  	u16 su_y_granularity;
>  	u8 active_non_psr_pipes;
> @@ -1691,6 +1699,7 @@ struct intel_psr {
>  	bool source_panel_replay_support;
>  	bool sink_panel_replay_support;
>  	bool sink_panel_replay_su_support;
> +	enum intel_panel_replay_dsc_support
> sink_panel_replay_dsc_support;
>  	bool panel_replay_enabled;
>  	u32 dc3co_exitline;
>  	u32 dc3co_exit_delay;
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 8a2fc1fcb5d04..116d7720f427b 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -6043,6 +6043,8 @@ intel_dp_detect(struct drm_connector
> *_connector,
>  		memset(connector->dp.dsc_dpcd, 0, sizeof(connector-
> >dp.dsc_dpcd));
>  		intel_dp->psr.sink_panel_replay_support = false;
>  		intel_dp->psr.sink_panel_replay_su_support = false;
> +		intel_dp->psr.sink_panel_replay_dsc_support =
> +			INTEL_DP_PANEL_REPLAY_DSC_NOT_SUPPORTED;
>  
>  		intel_dp_mst_disconnect(intel_dp);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> b/drivers/gpu/drm/i915/display/intel_psr.c
> index f7115969b4c5e..080a86f8dc80b 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -29,6 +29,7 @@
>  #include <drm/drm_vblank.h>
>  
>  #include "i915_reg.h"
> +#include "i915_utils.h"
>  #include "intel_alpm.h"
>  #include "intel_atomic.h"
>  #include "intel_crtc.h"
> @@ -50,6 +51,7 @@
>  #include "intel_snps_phy.h"
>  #include "intel_step.h"
>  #include "intel_vblank.h"
> +#include "intel_vdsc.h"
>  #include "intel_vrr.h"
>  #include "skl_universal_plane.h"
>  
> @@ -580,6 +582,44 @@ static void intel_dp_get_su_granularity(struct
> intel_dp *intel_dp)
>  	intel_dp->psr.su_y_granularity = y;
>  }
>  
> +static enum intel_panel_replay_dsc_support
> +compute_pr_dsc_support(struct intel_dp *intel_dp)
> +{
> +	u8 pr_dsc_mode;
> +	u8 val;
> +
> +	val = intel_dp-
> >pr_dpcd[INTEL_PR_DPCD_INDEX(DP_PANEL_REPLAY_CAP_CAPABILITY)];
> +	pr_dsc_mode =
> REG_FIELD_GET8(DP_PANEL_REPLAY_DSC_DECODE_CAPABILITY_IN_PR_MASK,
> val);
> +
> +	switch (pr_dsc_mode) {
> +	case DP_DSC_DECODE_CAPABILITY_IN_PR_FULL_FRAME_ONLY:
> +		return INTEL_DP_PANEL_REPLAY_DSC_FULL_FRAME_ONLY;
> +	case DP_DSC_DECODE_CAPABILITY_IN_PR_SUPPORTED:
> +		return INTEL_DP_PANEL_REPLAY_DSC_SELECTIVE_UPDATE;
> +	default:
> +		MISSING_CASE(pr_dsc_mode);
> +		fallthrough;
> +	case DP_DSC_DECODE_CAPABILITY_IN_PR_NOT_SUPPORTED:
> +	case DP_DSC_DECODE_CAPABILITY_IN_PR_RESERVED:
> +		return INTEL_DP_PANEL_REPLAY_DSC_NOT_SUPPORTED;
> +	}
> +}
> +
> +static const char *panel_replay_dsc_support_str(enum
> intel_panel_replay_dsc_support dsc_support)
> +{
> +	switch (dsc_support) {
> +	case INTEL_DP_PANEL_REPLAY_DSC_NOT_SUPPORTED:
> +		return "not supported";
> +	case INTEL_DP_PANEL_REPLAY_DSC_FULL_FRAME_ONLY:
> +		return "full frame only";
> +	case INTEL_DP_PANEL_REPLAY_DSC_SELECTIVE_UPDATE:
> +		return "selective update";
> +	default:
> +		MISSING_CASE(dsc_support);
> +		return "n/a";
> +	};
> +}
> +
>  static void _panel_replay_init_dpcd(struct intel_dp *intel_dp)
>  {
>  	struct intel_display *display = to_intel_display(intel_dp);
> @@ -615,10 +655,13 @@ static void _panel_replay_init_dpcd(struct
> intel_dp *intel_dp)
>  	    DP_PANEL_REPLAY_SU_SUPPORT)
>  		intel_dp->psr.sink_panel_replay_su_support = true;
>  
> +	intel_dp->psr.sink_panel_replay_dsc_support =
> compute_pr_dsc_support(intel_dp);
> +
>  	drm_dbg_kms(display->drm,
> -		    "Panel replay %sis supported by panel\n",
> +		    "Panel replay %sis supported by panel (in DSC
> mode: %s)\n",
>  		    intel_dp->psr.sink_panel_replay_su_support ?
> -		    "selective_update " : "");
> +		    "selective_update " : "",
> +		    panel_replay_dsc_support_str(intel_dp-
> >psr.sink_panel_replay_dsc_support));
>  }
>  
>  static void _psr_init_dpcd(struct intel_dp *intel_dp)
> @@ -1535,9 +1578,21 @@ static bool
> intel_sel_update_config_valid(struct intel_dp *intel_dp,
>  		goto unsupported;
>  	}
>  
> -	if (crtc_state->has_panel_replay && (DISPLAY_VER(display) <
> 14 ||
> -					     !intel_dp-
> >psr.sink_panel_replay_su_support))
> -		goto unsupported;
> +	if (crtc_state->has_panel_replay) {
> +		if (DISPLAY_VER(display) < 14)
> +			goto unsupported;
> +
> +		if (!intel_dp->psr.sink_panel_replay_su_support)
> +			goto unsupported;
> +
> +		if (intel_dsc_is_enabled_on_link(crtc_state) &&
> +		    intel_dp->psr.sink_panel_replay_dsc_support !=
> +		    INTEL_DP_PANEL_REPLAY_DSC_SELECTIVE_UPDATE) {
> +			drm_dbg_kms(display->drm,
> +				    "Selective update with PR not
> enabled because it's not

"Panel Replay" used everywhere else.

> supported with DSC\n");
> +			goto unsupported;
> +		}
> +	}
>  
>  	if (crtc_state->crc_enabled) {
>  		drm_dbg_kms(display->drm,
> @@ -1614,6 +1669,14 @@ _panel_replay_compute_config(struct intel_dp
> *intel_dp,
>  		return false;
>  	}
>  
> +	if (intel_dsc_is_enabled_on_link(crtc_state) &&
> +	    intel_dp->psr.sink_panel_replay_dsc_support ==
> +	    INTEL_DP_PANEL_REPLAY_DSC_NOT_SUPPORTED) {
> +		drm_dbg_kms(display->drm,
> +			    "Panel Replay not enabled because it's
> not supported with DSC\n");
> +		return false;
> +	}
> +
>  	if (!intel_dp_is_edp(intel_dp))
>  		return true;
>  
> @@ -1694,6 +1757,8 @@ void intel_psr_compute_config(struct intel_dp
> *intel_dp,
>  		return;
>  	}
>  
> +	/* Only used for state verification. */
> +	crtc_state->panel_replay_dsc_support = intel_dp-
> >psr.sink_panel_replay_dsc_support;
>  	crtc_state->has_panel_replay =
> _panel_replay_compute_config(intel_dp,
>  								   
> crtc_state,
>  								   
> conn_state);
> @@ -2951,6 +3016,20 @@ void intel_psr_pre_plane_update(struct
> intel_atomic_state *state,
>  	}
>  }
>  
> +static void
> +verify_panel_replay_dsc_state(const struct intel_crtc_state
> *crtc_state)
> +{
> +	struct intel_display *display =
> to_intel_display(crtc_state);
> +
> +	if (!crtc_state->has_panel_replay)
> +		return;
> +
> +	drm_WARN_ON(display->drm,
> +		    intel_dsc_is_enabled_on_link(crtc_state) &&
> +		    crtc_state->panel_replay_dsc_support ==
> +		    INTEL_DP_PANEL_REPLAY_DSC_NOT_SUPPORTED);
> +}
> +
>  void intel_psr_post_plane_update(struct intel_atomic_state *state,
>  				 struct intel_crtc *crtc)
>  {
> @@ -2962,6 +3041,8 @@ void intel_psr_post_plane_update(struct
> intel_atomic_state *state,
>  	if (!crtc_state->has_psr)
>  		return;
>  
> +	verify_panel_replay_dsc_state(crtc_state);

I'm not convinced adding this verify is valuable. There are so many
restrictions taken into account in _compute_configs and I don't see why
DSC would be somehow different. Maybe our IGT tests should just take
this into account?

Also support parameter you added into crtc_state can be dropped if you
leave this out.

> +
>  	for_each_intel_encoder_mask_with_psr(state->base.dev,
> encoder,
>  					     crtc_state-
> >uapi.encoder_mask) {
>  		struct intel_dp *intel_dp =
> enc_to_intel_dp(encoder);
> @@ -3991,6 +4072,8 @@ static void intel_psr_sink_capability(struct
> intel_dp *intel_dp,
>  	seq_printf(m, ", Panel Replay = %s", str_yes_no(psr-
> >sink_panel_replay_support));
>  	seq_printf(m, ", Panel Replay Selective Update = %s",
>  		   str_yes_no(psr->sink_panel_replay_su_support));
> +	seq_printf(m, ", PR DSC support = %s",

"Panel Replay" used everywhere else.

BR,

Jouni Högander

> +		   panel_replay_dsc_support_str(psr-
> >sink_panel_replay_dsc_support));
>  	if (intel_dp-
> >pr_dpcd[INTEL_PR_DPCD_INDEX(DP_PANEL_REPLAY_CAP_SUPPORT)] &
>  	    DP_PANEL_REPLAY_EARLY_TRANSPORT_SUPPORT)
>  		seq_printf(m, " (Early Transport)");