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

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

On Mon, 2025-09-15 at 16:19 +0200, Grzelak, Michal 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 new_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:
> v10->v11
> - update the reason from new_crtc_state instead of old_crtc_state
> [Jouni]
> 
> 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>

Hopefully you have time and interest to extend this. Anyways this one
together with your igt patch is already solving one problem for us:

Reviewed-by: Jouni Högander <jouni.hogander@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 358ab922d7a7..8e930ec197ab 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1127,6 +1127,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
> @@ -1689,6 +1690,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 01bf304c705f..a1faf8a8d184 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1582,6 +1582,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;
> @@ -1813,6 +1814,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;
>  }
>  
>  /*
> @@ -2925,6 +2927,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 = new_crtc_state-
> >no_psr_reason;
> +
>  		if (psr->enabled) {
>  			/*
>  			 * Reasons to disable:
> @@ -2973,12 +2978,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);
> @@ -4017,6 +4029,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)
> -- 
> 2.45.2
>