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

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

On Wed, 2025-09-03 at 12:31 +0200, Michał Grzelak wrote:
> There is no reason in debugfs why PSR has been disabled. Currently,
> without this information, IGT tests cannot decide whether PSR has
> been disabled on purpose or was it abnormal behavior. Because of it,
> the status of the test cannot be decided correctly.
> 
> Add no_psr_reason field into struct intel_psr. Add no_psr_reason
> into struct intel_crtc_state to prevent staying out of sync when
> _psr_compute_config is not using computed state. Write the reason,
> e.g. PSR setup timing not met, into proper PSR debugfs file. Update
> the reason from old_crtc_state in intel_psr_pre_plane_update. Extend
> format of debugfs file to have reason when it is non-NULL. Clean the
> reason when PSR is activated.
> 
> Refactor intel_psr_post_plane_update to use no_psr_reason along
> keep_disabled.
> 
> Changelog:
> v9->v10
> - log reason into intel_crtc_state instead of intel_dp->psr [Jouni]
> - remove clearing no_psr_reason in intel_psr_compute_config [Jouni]
> - change update of no_psr_reason into more readable form [Jouni]
> 
> v8->v9
> - add no_psr_reason into struct intel_crtc_state [Jouni]
> - update the reason in intel_psr_pre_plane_update [Jouni]
> - elaborate on motivation of the feature in commit message
> [Sebastian]
> - copy changelog to commit message [Sebastian]
> 
> v7->v8
> - reset no_psr_reason at the begin of intel_psr_compute_config
> [Jouni]
> - restore keep_disabled [Jouni]
> - drop setting "Sink not reliable" [Jouni]
> - add WA number [Jouni]
> - if non-NULL, write no_psr_reason after PSR mode [Jouni]
> 
> v6->v7
> - rebase onto new drm-tip
> 
> v5->v6
> - move setting no_psr_reason to intel_psr_post_plane_update [Jouni]
> - remove setting no_psr_reason when disabling PSR is temporary
> [Jouni]
> 
> v4->v5
> - fix indentation errors from checkpatch
> 
> v3->v4
> - change format of logging workaround #1136
> 
> v2->v3
> - change reason description to be more specific [Mika]
> - remove BSpecs number & WA number from being written into
> no_psr_reason
> - replace spaces with tabs
> 
> v1->v2
> - set other reasons than "PSR setup timing not met"
> - clear no_psr_reason when activating PSR.
> 
> Reviewed-by: Sebastian Brzezinka <sebastian.brzezinka@intel.com>
> Signed-off-by: Michał Grzelak <michal.grzelak@intel.com>
> ---
>  .../drm/i915/display/intel_display_types.h    |  3 +++
>  drivers/gpu/drm/i915/display/intel_psr.c      | 22 +++++++++++++++--
> --
>  2 files changed, 21 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..e165a471aa6d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1125,6 +1125,7 @@ struct intel_crtc_state {
>  	u32 dc3co_exitline;
>  	u16 su_y_granularity;
>  	u8 active_non_psr_pipes;
> +	const char *no_psr_reason;
>  
>  	/*
>  	 * Frequency the dpll for the port should run at. Differs
> from the
> @@ -1687,6 +1688,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..ebc56fd958ac 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 {
> +		crtc_state->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;
>  }
>  
>  /*
> @@ -2922,6 +2924,9 @@ void intel_psr_pre_plane_update(struct
> intel_atomic_state *state,
>  
>  		mutex_lock(&psr->lock);
>  
> +		if (!new_crtc_state->has_psr)
> +			psr->no_psr_reason = old_crtc_state-
> >no_psr_reason;

I think you should use new_crtc_state->no_psr_reason.

BR,

Jouni Högander

> +
>  		if (psr->enabled) {
>  			/*
>  			 * Reasons to disable:
> @@ -2970,12 +2975,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 +3990,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)