On Thu, 2025-10-09 at 11:36 +0300, Imre Deak wrote:
> 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 <[email protected]>
> > > ---
> > > .../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.
At least compute_pr_dsc_support is checking only pr_dpcd? I have done
this for early transport capability. I've been thinking it doesn't make
sense to duplicate information which is already available elsewhere
(intel_psr->*support vs. intel_dp->psr/pr dpcd).
>
> > > 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.
Ok. How it is ensured that Panel Replay is disabled/enabled accordingly
in this CRTC if those dependency CRTCs are changing?
BR,
Jouni Högander
>
> > 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)");
> >
