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

[Hogander, Jouni]

Hello Michal,

Some comment inline below.

On Tue, 2025-06-24 at 18:48 +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.
> 
> Signed-off-by: Michał Grzelak <michal.grzelak at 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 30c7315fc25e..8d8202bb06c8 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1661,6 +1661,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 8bee2f592ae7..3fe91c5816ce 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1591,6 +1591,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;
> @@ -1820,6 +1821,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;
>  }
>  
>  /*
> @@ -2918,13 +2920,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 = "CRTC needs
> modeset";

No need to handle this as post_plane_update will re-enable PSR in case
crtc_state->has_psr == true. I don't think we need to keep
psr_no_reason up-to-date over modeset.

> +			if (!new_crtc_state->has_psr)
> +				psr->no_psr_reason = "PSR disabled";

I think this should be handled in psr_compute_config. As you are doing
with that "PSR setup timing not met" Anyways setting it here as "PSR
disabled" would overwrite existing "PSR setup timing not met".

> +			if (!new_crtc_state->active_planes)
> +				psr->no_psr_reason = "All planes
> inactive";

I would rather set this (and everything else which is not handled in
psr_compute_config) in post_plane_update.

> +			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 = "Changing
> Region Early Transport";
> +			if (new_crtc_state->has_panel_replay != psr-
> >panel_replay_enabled)
> +				psr->no_psr_reason = "Changing Panel
> Replay mode";

No need to handle these as PSR will be disabled only for changing the
mode and then enabled again after that.

> +			if (DISPLAY_VER(display) < 11 &&
> new_crtc_state->wm_level_disabled)
> +				psr->no_psr_reason = "Workaround for
> skl, bxt";

This also to post_plane_update.

BR,

Jouni Högander

> +			if (psr->no_psr_reason)
>  				intel_psr_disable_locked(intel_dp);
>  			else if (new_crtc_state->wm_level_disabled)
>  				/* Wa_14015648006 */
> @@ -3932,12 +3942,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";
> @@ -3955,7 +3960,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)