> -----Original Message-----
> From: Intel-xe <intel-xe-boun...@lists.freedesktop.org> On Behalf Of Michal
> Grzelak
> Sent: Thursday, 5 June 2025 12.13
> To: intel-gfx@lists.freedesktop.org; intel...@lists.freedesktop.org
> Cc: Grzelak, Michal <michal.grze...@intel.com>
> Subject: [PATCH v2 1/1] drm/i915/display: Add no_psr_reason to PSR debugfs
>
> There is no reason in debugfs why PSR has been disabled. Add no_psr_reason
> field into struct intel_psr. Write the reason, e.g. PSR
> setup timing not met, into proper PSR debugfs file.
> Clean it when PSR is activated.
>
> Signed-off-by: Michał Grzelak <michal.grze...@intel.com>
> ---
> .../drm/i915/display/intel_display_types.h | 2 +
> drivers/gpu/drm/i915/display/intel_psr.c | 38 ++++++++++++-------
> 2 files changed, 26 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 7415564d058a..7d598357a702 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1653,6 +1653,8 @@ struct intel_psr {
> bool link_ok;
>
> u8 active_non_psr_pipes;
> +
> + const char *no_psr_reason;
> };
>
> struct intel_dp {
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> b/drivers/gpu/drm/i915/display/intel_psr.c
> index ccd66bbc72f7..2c8af2a10ee1 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1577,6 +1577,7 @@ static bool _psr_compute_config(struct intel_dp
> *intel_dp,
> if (entry_setup_frames >= 0) {
> intel_dp->psr.entry_setup_frames = entry_setup_frames;
> } else {
> + intel_dp->psr.no_psr_reason = "PSR setup timing not met";
> drm_dbg_kms(display->drm,
> "PSR condition failed: PSR setup timing not met\n");
> return false;
> @@ -1812,6 +1813,7 @@ static void intel_psr_activate(struct intel_dp
> *intel_dp)
> hsw_activate_psr1(intel_dp);
>
> intel_dp->psr.active = true;
> + intel_dp->psr.no_psr_reason = NULL;
> }
>
> /*
> @@ -2904,13 +2906,21 @@ void intel_psr_pre_plane_update(struct
> intel_atomic_state *state,
> * - Region Early Transport changing
> * - Display WA #1136: skl, bxt
> */
> - if (intel_crtc_needs_modeset(new_crtc_state) ||
> - !new_crtc_state->has_psr ||
> - !new_crtc_state->active_planes ||
> - new_crtc_state->has_sel_update !=
> psr->sel_update_enabled ||
> - new_crtc_state->enable_psr2_su_region_et !=
> psr->su_region_et_enabled ||
> - new_crtc_state->has_panel_replay !=
> psr->panel_replay_enabled ||
> - (DISPLAY_VER(display) < 11 &&
> new_crtc_state->wm_level_disabled))
> + if (intel_crtc_needs_modeset(new_crtc_state))
> + psr->no_psr_reason = "New state needs modeset";
Phrasing these statements are always matter of a opinion but I would rephrase
this as following
psr->no_psr_reason = "CRTC needs modeset"
as crtc state is simply a state of the crtc and crtc is the one that needs mode
setting.
> + if (!new_crtc_state->has_psr)
> + psr->no_psr_reason = "PSR disabled in new
> state";
These reasons are listed in the comment of this section of code. However, since
has_psr is not true, the PSR is disabled whatever reason (due to lack of planes
etc.) I would simply rephrase this as
psr->no_psr_reason = "PSR disabled"
> + if (!new_crtc_state->active_planes)
> + psr->no_psr_reason = "All planes will go
> inactive";
Maybe just
psr->no_psr_reason = "All planes inactive"
> + if (new_crtc_state->has_sel_update !=
> psr->sel_update_enabled)
> + psr->no_psr_reason = "Changing between PSR
> versions";
> + if (new_crtc_state->enable_psr2_su_region_et !=
> psr->su_region_et_enabled)
> + psr->no_psr_reason = "Region Early Transport
> changing";
Since reasons above and below this starts with "Changing" and the comparison is
similar, maybe we could rephrase this similarly i.e.
psr->no_psr_reason = "Changing Region Early Transport"
> + if (new_crtc_state->has_panel_replay !=
> psr->panel_replay_enabled)
> + psr->no_psr_reason = "Changing Panel Replay
> mode";
> + if (DISPLAY_VER(display) < 11 &&
> new_crtc_state->wm_level_disabled)
> + psr->no_psr_reason = "Bspec #21664 Display WA
> #1136: skl, bxt";
> + if (psr->no_psr_reason)
> intel_psr_disable_locked(intel_dp);
> else if (new_crtc_state->wm_level_disabled)
> /* Wa_14015648006 */
> @@ -3918,12 +3928,7 @@ static void intel_psr_print_mode(struct intel_dp
> *intel_dp,
> struct seq_file *m)
> {
> struct intel_psr *psr = &intel_dp->psr;
> - const char *status, *mode, *region_et;
> -
> - if (psr->enabled)
> - status = " enabled";
> - else
> - status = "disabled";
> + const char *mode, *region_et;
>
> if (psr->panel_replay_enabled && psr->sel_update_enabled)
> mode = "Panel Replay Selective Update"; @@ -3941,7 +3946,12 @@
> static void intel_psr_print_mode(struct
> intel_dp *intel_dp,
> else
> region_et = "";
>
> - seq_printf(m, "PSR mode: %s%s%s\n", mode, status, region_et);
> + if (psr->enabled) {
> + seq_puts(m, "PSR enabled\n");
> + seq_printf(m, "PSR mode: %s%s\n", mode, region_et);
> + } else {
> + seq_printf(m, "PSR disabled: %s\n", psr->no_psr_reason);
> + }
> }
>
> static int intel_psr_status(struct seq_file *m, struct intel_dp *intel_dp)
> --
> 2.45.2
Otherwise, the patch looks good.
