Re: [Intel-gfx] [PATCH] drm/i915/display: Allow users to disable PSR2

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Feb 24, 2022 at 02:15:44PM +0000, Souza, Jose wrote:
> + Rodrigo
> 
> On Thu, 2022-02-24 at 15:11 +0200, Ville Syrjälä wrote:
> > On Thu, Feb 24, 2022 at 03:06:30PM +0200, Ville Syrjälä wrote:
> > > On Thu, Feb 24, 2022 at 01:01:24PM +0000, Souza, Jose wrote:
> > > > On Thu, 2022-02-24 at 12:12 +0200, Ville Syrjälä wrote:
> > > > > On Wed, Feb 23, 2022 at 11:41:03AM -0800, José Roberto de Souza wrote:
> > > > > > Some users are suffering with PSR2 issues that are under debug or
> > > > > > issues that were root caused to panel firmware, to make life of those
> > > > > > users easier here adding a option to disable PSR1 with kernel
> > > > > > parameter.
> > > > > > 
> > > > > > Using the same enable_psr that is current used to turn PSR1 and PSR2
> > > > > > off or on and adding a new value to only disable PSR2.
> > > > > > The previous valid values did not had their behavior changed.
> > > > > > 
> > > > > > Link: https://gitlab.freedesktop.org/drm/intel/-/issues/4951
> > > > > > Cc: Jouni Högander <jouni.hogander@intel.com>
> > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/display/intel_psr.c | 4 ++++
> > > > > >  drivers/gpu/drm/i915/i915_params.c       | 2 +-
> > > > > >  2 files changed, 5 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > index 2e0b092f4b6be..fc6b684bb7bec 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > @@ -100,11 +100,15 @@ static bool psr_global_enabled(struct intel_dp *intel_dp)
> > > > > >  
> > > > > >  static bool psr2_global_enabled(struct intel_dp *intel_dp)
> > > > > >  {
> > > > > > +	struct drm_i915_private *i915 = dp_to_i915(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 (i915->params.enable_psr == 2)
> > > > > > +			return false;
> > > > > >  		return true;
> > > > > >  	}
> > > > > >  }
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> > > > > > index eea355c2fc28a..a9b97e6eb3df0 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_params.c
> > > > > > +++ b/drivers/gpu/drm/i915/i915_params.c
> > > > > > @@ -94,7 +94,7 @@ i915_param_named_unsafe(enable_hangcheck, bool, 0400,
> > > > > >  
> > > > > >  i915_param_named_unsafe(enable_psr, int, 0400,
> > > > > >  	"Enable PSR "
> > > > > > -	"(0=disabled, 1=enabled) "
> > > > > > +	"(0=disabled, 1=enable up to PSR2 if supported, 2=enable up to PSR1) "
> > > > > 
> > > > > That seems very unintuitive. I would just make it 1==PSR1 and 2==PSR2.
> > > > 
> > > > This will break current behavior.
> > > 
> > > It's a modparam. We routinely break those since they are not meant
> > > to used by normal users as any kind of permanent "make my machine
> > > work" knob.
> > 
> > But I guess if we want to make it a bit less painful your idea of a new
> > modparam might work. + deprecate the old param and remove after one or
> > two kernel releases.
> 
> Was thinking about a new one to limit the version of PSR:
> 
> enable_psr_version
> default = 0(per-chip default), 1 = up to PSR1, 2 = PSR2
> 
> > 
> 

My trauma with PSR makes me to ask if we really wants this option.
Is PSR1 really more stable at this point? Shouldn't we just disable everything
while we fix the PSR2 and avoid touching the parameter?

If we are confident that PSR1 is better than simply disabling, then
I like the idea of a new parameter that overrides the previous one so
we can delete in a couple months and keep only one.

i915.psr
default=-1 (per-chip default), 0=disabled, 1=psr1, 2=psr2

but I also would be okay breaking a parameter that is marked as unstable
and which main use externally is to disable the psr (i915.enable_psr=0).

Thanks,
Rodrigo.