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

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

[Michał Grzelak]

Next version of v6 [1]. Previous version wasn't properly rebased, so CI
haven't picked up the patch. Took into consideration review from Jouni
[2].

[1] https://lore.kernel.org/intel-gfx/20250822232321.657000-1-michal.grzelak@intel.com
[2] https://lore.kernel.org/intel-gfx/3e4f40d5310b2fd4169f6befde7f7a7e611a4e09.camel@intel.com

Test-with: 20250703140451.491593-2-michal.grzelak@intel.com

Best regards,
Michał

---
Changelog:
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
- provide correct Test-with tag to the IGT patch, instead of series

v1->v2
- set other reasons than "PSR setup timing not met"
- clear no_psr_reason when activating PSR.

Michał Grzelak (1):
  drm/i915/display: Add no_psr_reason to PSR debugfs

 .../drm/i915/display/intel_display_types.h    |  2 ++
 drivers/gpu/drm/i915/display/intel_psr.c      | 31 ++++++++++---------
 2 files changed, 19 insertions(+), 14 deletions(-)

-- 
2.45.2

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

[Michał Grzelak]

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.grzelak@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;
 
 		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 (!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";
 
-		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)
 			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);
+	}
 }
 
 static int intel_psr_status(struct seq_file *m, struct intel_dp *intel_dp)
-- 
2.45.2

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

[Hogander, Jouni]

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.grzelak@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)