diff mbox series

drm/i915/display: Allow users to disable PSR2

Message ID 20220223194103.715109-1-jose.souza@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/display: Allow users to disable PSR2 | expand

Commit Message

Souza, Jose Feb. 23, 2022, 7:41 p.m. UTC
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(-)

Comments

Ville Syrjälä Feb. 24, 2022, 10:12 a.m. UTC | #1
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.

>  	"Default: -1 (use per-chip default)");
>  
>  i915_param_named(psr_safest_params, bool, 0400,
> -- 
> 2.35.1
Souza, Jose Feb. 24, 2022, 1:01 p.m. UTC | #2
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.

Other option is to add another parameter...

> 
> >  	"Default: -1 (use per-chip default)");
> >  
> >  i915_param_named(psr_safest_params, bool, 0400,
> > -- 
> > 2.35.1
>
Ville Syrjälä Feb. 24, 2022, 1:06 p.m. UTC | #3
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.
Ville Syrjälä Feb. 24, 2022, 1:11 p.m. UTC | #4
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.
Souza, Jose Feb. 24, 2022, 2:15 p.m. UTC | #5
+ 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

>
Rodrigo Vivi Feb. 24, 2022, 2:29 p.m. UTC | #6
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.
Ville Syrjälä Feb. 24, 2022, 2:29 p.m. UTC | #7
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

I would use the standard -1==default, 0==disable convention here.
Then we can drop the old param after a short transition period.
diff mbox series

Patch

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) "
 	"Default: -1 (use per-chip default)");
 
 i915_param_named(psr_safest_params, bool, 0400,