Re: [PATCH v8 1/1] drm/i915/display: Add no_psr_reason to PSR debugfs

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

On Thu, 2025-08-28 at 12:49 +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.
> Extend format of debugfs file to have reason when non-NULL.
> Ensure no_psr_reason is up-to-date or NULL by resetting it
> at the beginning of intel_psr_compute_config. Clean it when
> PSR is activated.
> 
> Refactor intel_psr_post_plane_update to use no_psr_reason
> along keep_disabled.
> 
> Signed-off-by: Michał Grzelak <michal.grzelak@intel.com>
> ---
>  .../drm/i915/display/intel_display_types.h    |  2 ++
>  drivers/gpu/drm/i915/display/intel_psr.c      | 21 +++++++++++++++--
> --
>  2 files changed, 19 insertions(+), 4 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 22433fe2ee14..7c7de30f13f2 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";

One more thing I just realized. We cannot set intel_dp-
>psr.no_psr_reason directly in _psr_compute_config. That is because
compute_config may end up not using computed state -> we have intel_dp-
>psr.no_psr_reason out of sync.

It seems we need to have it in intel_crtc_state as well. Then in
intel_psr_pre_plane_update:

intel_dp->psr.no_psr_reason = crtc_state->no_psr_reason

BR,

Jouni Högander

>  		drm_dbg_kms(display->drm,
>  			    "PSR condition failed: PSR setup timing
> not met\n");
>  		return false;
> @@ -1663,6 +1664,8 @@ void intel_psr_compute_config(struct intel_dp
> *intel_dp,
>  	struct intel_crtc *crtc;
>  	u8 active_pipes = 0;
>  
> +	intel_dp->psr.no_psr_reason = NULL;
> +
>  	if (!psr_global_enabled(intel_dp)) {
>  		drm_dbg_kms(display->drm, "PSR disabled by flag\n");
>  		return;
> @@ -1810,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;
>  }
>  
>  /*
> @@ -2970,12 +2974,19 @@ void intel_psr_post_plane_update(struct
> intel_atomic_state *state,
>  		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)
> +			keep_disabled = true;
> +
> +		if (!crtc_state->active_planes) {
> +			psr->no_psr_reason = "All planes inactive";
> +			keep_disabled = true;
> +		}
>  
>  		/* 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 #1136 for
> skl, bxt";
> +			keep_disabled = true;
> +		}
>  
>  		if (!psr->enabled && !keep_disabled)
>  			intel_psr_enable_locked(intel_dp,
> crtc_state);
> @@ -3978,6 +3989,8 @@ static void intel_psr_print_mode(struct
> intel_dp *intel_dp,
>  		region_et = "";
>  
>  	seq_printf(m, "PSR mode: %s%s%s\n", mode, status,
> region_et);
> +	if (psr->no_psr_reason)
> +		seq_printf(m, "  %s\n", psr->no_psr_reason);
>  }
>  
>  static int intel_psr_status(struct seq_file *m, struct intel_dp
> *intel_dp)