Hello Michal,
Addition to my comments inline below:
You should reset no_psr_reason to NULL at the begin of
intel_psr_compute_config. Just to ensure it's up-to-date or NULL.
BR,
Jouni Högander
On Mon, 2025-08-25 at 12:33 +0200, Michał Grzelak wrote:
> 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.
>
> Refactor intel_psr_post_plane_update by using no_psr_reason
> instead of boolean keep_disabled.
>
> 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 | 31 ++++++++++-------
> --
> 2 files changed, 19 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 fd9d2527889b..0f8332ce1aa0 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1687,6 +1687,8 @@ struct intel_psr {
> bool pkg_c_latency_used;
>
> 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 eec4f7dc2d66..4143dfc0c27c 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1579,6 +1579,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;
> @@ -1810,6 +1811,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;
> }
>
> /*
> @@ -2962,28 +2964,29 @@ void intel_psr_post_plane_update(struct
> intel_atomic_state *state,
> crtc_state-
> >uapi.encoder_mask) {
> struct intel_dp *intel_dp =
> enc_to_intel_dp(encoder);
> struct intel_psr *psr = &intel_dp->psr;
> - bool keep_disabled = false;
I don't think you should change this to rely on debug data. Debug data
is supposed to just present what is the state and not affect to what is
the state.
>
> mutex_lock(&psr->lock);
>
> drm_WARN_ON(display->drm,
> psr->enabled && !crtc_state-
> >active_planes);
>
> - keep_disabled |= psr->sink_not_reliable;
> - keep_disabled |= !crtc_state->active_planes;
> + if (psr->sink_not_reliable)
> + psr->no_psr_reason = "Sink not reliable";
If you are adding this here, then I think you should have it in
intel_psr_compute_config as well:
void intel_psr_compute_config(struct intel_dp *intel_dp,
struct intel_crtc_state *crtc_state,
struct drm_connector_state *conn_state)
{
...
if (intel_dp->psr.sink_not_reliable) {
drm_dbg_kms(display->drm,
"PSR sink implementation is not
reliable\n");
return;
}
...
I would just drop this change at this point. You already have this
information anyways:
static int intel_psr_status(struct seq_file *m, struct intel_dp
*intel_dp)
...
if (!psr->enabled) {
seq_printf(m, "PSR sink not reliable: %s\n",
str_yes_no(psr->sink_not_reliable));
goto unlock;
}
...
> + if (!crtc_state->active_planes)
> + psr->no_psr_reason = "All planes inactive";
>
> /* Display WA #1136: skl, bxt */
> - keep_disabled |= DISPLAY_VER(display) < 11 &&
> - crtc_state->wm_level_disabled;
> + if (DISPLAY_VER(display) < 11 && crtc_state-
> >wm_level_disabled)
> + psr->no_psr_reason = "Workaround for skl,
> bxt";
Maybe add also that WA number?
>
> - if (!psr->enabled && !keep_disabled)
> + if (!psr->enabled && !psr->no_psr_reason)
> intel_psr_enable_locked(intel_dp,
> crtc_state);
> else if (psr->enabled && !crtc_state-
> >wm_level_disabled)
> /* Wa_14015648006 */
> wm_optimization_wa(intel_dp, crtc_state);
>
> /* Force a PSR exit when enabling CRC to avoid CRC
> timeouts */
> - if (crtc_state->crc_enabled && psr->enabled)
> + if (psr->enabled && crtc_state->crc_enabled)
This change is unrelated.
> intel_psr_force_update(intel_dp);
>
> /*
> @@ -3953,12 +3956,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";
> @@ -3976,7 +3974,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);
> + }
Do you have specific reason to change this format? You could keep the
format and then just add no_psr_reason (when it's available) into
braces:
PSR mode: disabled (PSR setup timing not met)
Then if no_psr_reason is not filled with a reason then it could be just
as it is currently always:
PSR mode: disabled
Then if think you want/need to change the format you should have it in
separate patch having commit message explaining why you want to do
that.
> }
>
> static int intel_psr_status(struct seq_file *m, struct intel_dp
> *intel_dp)
