Re: [PATCH 2/2] drm/i915/psr: Do not disable Panel Replay if PSR2 is disabled

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

On Mon, 2025-07-07 at 11:01 -0400, Rodrigo Vivi wrote:
> On Mon, Jul 07, 2025 at 01:47:33PM +0300, Jouni Högander wrote:
> > Currently disabling PSR2 via enable_psr module parameter causes
> > Panel
> > Replay being disabled as well. This patch changes this by still
> > allowing
> > Panel Replay even if PSR2 is disabled.
> > 
> > After this patch enable_psr module parameter values are:
> > 
> > -1 = PSR1 : yes, PSR2 = yes, Panel Replay : yes
> >  0 = PSR1 : no,  PSR2 = no,  Panel Replay : no
> >  1 = PSR1 : yes, PSR2 = no,  Panel Replay : yes
> >  2 = PSR1 : yes, PSR2 = yes, Panel Replay : no
> > 
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > ---
> >  .../drm/i915/display/intel_display_params.c   |  3 +--
> >  drivers/gpu/drm/i915/display/intel_psr.c      | 20 +++++++++++++--
> > ----
> >  2 files changed, 15 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c
> > b/drivers/gpu/drm/i915/display/intel_display_params.c
> > index 75316247ee8a..1ba17ea40bba 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_params.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_params.c
> > @@ -116,8 +116,7 @@ intel_display_param_named_unsafe(enable_fbc,
> > int, 0400,
> >  	"(default: -1 (use per-chip default))");
> >  
> >  intel_display_param_named_unsafe(enable_psr, int, 0400,
> > -	"Enable PSR "
> > -	"(0=disabled, 1=enable up to PSR1, 2=enable up to PSR2) "
> > +	"Enable PSR (0=disabled, 1=disable PSR2, 2=disable Panel
> > Replay)"
> 
> What about a bit mask?
> 
> PSR1 = BIT0
> PSR2 = BIT1 (PSR2 infers PSR1 enabled)
> PANEL_REPLAY = BIT2 (also infers PSR1(and 2?) enabled)
> 
> (Peraps even bit3 for early transport?)
> 
> This is backwards compatible because
> 
> 0 = disabled,
> 1 = up to psr1,
> 2 = up to psr2, (no panel replay)
> 3 = up to psr2, (same as 2)
> 4 = panel replay on
> 5 = same as 5

Original problem behind my patch is enable_psr=1 having impact on Panel
Replay. First I thought I Fix this by just ignoring enable_psr on Panel
Replay. That would mean adding enable_panel_replay parameter as well. I
didn't want to do that. We have user wanting to have PSR2 specifically
disabled but still use Panel Replay.
->
I wanted to have enable_psr legacy values working as before without
having impact on Panel Replay. To have this I choose this meaning to
values (bits):

0 = disable PSR/Panel Replay completely
1 (BIT0) = disable PSR2 (allow PSR1/Panel Replay) (up to psr1)
2 (BIT1) = disable Panel Replay (allow PSR1/PSR2) (up to psr2, (no
panel replay))

I left Early Transport out here as disabling it means disabling Panel
Replay as well. I also didn't want to make this parameter too
extensive/complex as we have separate debug interface to bisect PSR
features (/sys/kernel/debug/dri/*/i915_edp_psr_debug). Using this
disable bit approach is also easy to extent in future as needed.

Your idea is following better meaning of "enable_psr".

BR,

Jouni Högander

> 
> >  	"Default: -1 (use per-chip default)");
> >  
> >  intel_display_param_named(psr_safest_params, bool, 0400,
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index a2b5688f0c82..3215a11baa66 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -254,13 +254,15 @@ static bool psr2_global_enabled(struct
> > intel_dp *intel_dp)
> >  {
> >  	struct intel_display *display =
> > to_intel_display(intel_dp);
> >  
> > +	return display->params.enable_psr != 1;
> > +}
> > +
> > +static bool sel_update_global_enabled(struct intel_dp *intel_dp)
> > +{
> >  	switch (intel_dp->psr.debug & I915_PSR_DEBUG_MODE_MASK) {
> > -	case I915_PSR_DEBUG_DISABLE:
> >  	case I915_PSR_DEBUG_FORCE_PSR1:
> >  		return false;
> >  	default:
> > -		if (display->params.enable_psr == 1)
> > -			return false;
> >  		return true;
> >  	}
> >  }
> > @@ -269,7 +271,7 @@ static bool panel_replay_global_enabled(struct
> > intel_dp *intel_dp)
> >  {
> >  	struct intel_display *display =
> > to_intel_display(intel_dp);
> >  
> > -	if ((display->params.enable_psr != -1) ||
> > +	if (display->params.enable_psr == 2 ||
> >  	    (intel_dp->psr.debug &
> > I915_PSR_DEBUG_PANEL_REPLAY_DISABLE))
> >  		return false;
> >  	return true;
> > @@ -1415,6 +1417,12 @@ static bool intel_psr2_config_valid(struct
> > intel_dp *intel_dp,
> >  	if (!intel_dp->psr.sink_psr2_support)
> >  		return false;
> >  
> > +	if (!psr2_global_enabled(intel_dp)) {
> > +		drm_dbg_kms(display->drm,
> > +			    "PSR2 disabled by flag\n");
> > +		return false;
> > +	}
> > +
> >  	/* JSL and EHL only supports eDP 1.3 */
> >  	if (display->platform.jasperlake || display-
> > >platform.elkhartlake) {
> >  		drm_dbg_kms(display->drm, "PSR2 not supported by
> > phy\n");
> > @@ -1517,7 +1525,7 @@ static bool
> > intel_sel_update_config_valid(struct intel_dp *intel_dp,
> >  		goto unsupported;
> >  	}
> >  
> > -	if (!psr2_global_enabled(intel_dp)) {
> > +	if (!sel_update_global_enabled(intel_dp)) {
> >  		drm_dbg_kms(display->drm,
> >  			    "Selective update disabled by
> > flag\n");
> >  		goto unsupported;
> > @@ -1664,7 +1672,7 @@ void intel_psr_compute_config(struct intel_dp
> > *intel_dp,
> >  	u8 active_pipes = 0;
> >  
> >  	if (!psr_global_enabled(intel_dp)) {
> > -		drm_dbg_kms(display->drm, "PSR disabled by
> > flag\n");
> > +		drm_dbg_kms(display->drm, "PSR/Panel Replay
> > disabled by flag\n");
> >  		return;
> >  	}
> >  
> > -- 
> > 2.43.0
> >