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

[Imre Deak <imre.deak@intel.com>]

On Thu, Oct 09, 2025 at 06:56:13AM +0300, Jouni Hogander wrote:
> 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?

This cap depends on other conditions as well, besides the flag in
pr_dpcd[] so I think it should be determined once, already where all
those other conditions are checked as well, in
_panel_replay_init_dpcd(). The other sink panel replay/psr caps are also
decoded there, so it would be strange to not do this at the same place
for the panel replay DSC caps.

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

Okay, will change this.

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

The panel replay in DSC mode setting depends on the DSC state, computed
separately wrt. the PSR/panel replay state and also depends on the state
computed for other CRTCs than what is tracked by crtc_state. I would
like to verify at this point - after everything is computed - that all
these states are as expected.

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