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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 13 May 2025, Michał Grzelak <michal.grzelak@intel.com> 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.
>
> Signed-off-by: Michał Grzelak <michal.grzelak@intel.com>
> ---
>  .../gpu/drm/i915/display/intel_display_types.h    |  2 ++
>  drivers/gpu/drm/i915/display/intel_psr.c          | 15 ++++++++-------
>  2 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 7415564d058a..7d598357a702 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1653,6 +1653,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 ccd66bbc72f7..f0fdff236fef 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1577,6 +1577,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";

What about other reasons?

Where is intel_dp->psr.no_psr_reason cleared? Now this remains set until
the end of time...

>  		drm_dbg_kms(display->drm,
>  			    "PSR condition failed: PSR setup timing not met\n");
>  		return false;
> @@ -3918,12 +3919,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";
> @@ -3941,7 +3937,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);

Since psr->no_psr_reason isn't set for all reasons, this may be NULL. It
won't oops, but it'll likely look like "PSR disabled: (null)".

BR,
Jani.

> +	}
>  }
>  
>  static int intel_psr_status(struct seq_file *m, struct intel_dp *intel_dp)

-- 
Jani Nikula, Intel