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

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

[José Roberto de Souza]

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

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

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel

[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/display: Allow users to disable PSR2

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 3628 bytes --]

== Series Details ==

Series: drm/i915/display: Allow users to disable PSR2
URL   : https://patchwork.freedesktop.org/series/100658/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_11277 -> Patchwork_22389
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22389/index.html

Participating hosts (45 -> 41)
------------------------------

  Missing    (4): fi-bsw-cyan fi-icl-u2 fi-bdw-samus fi-pnv-d510 

Known issues
------------

  Here are the changes found in Patchwork_22389 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_cs_nop@fork-compute0:
    - fi-ivb-3770:        NOTRUN -> [SKIP][1] ([fdo#109271]) +17 similar issues
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22389/fi-ivb-3770/igt@amdgpu/amd_cs_nop@fork-compute0.html

  * igt@i915_selftest@live@hangcheck:
    - fi-snb-2600:        [PASS][2] -> [INCOMPLETE][3] ([i915#3921])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11277/fi-snb-2600/igt@i915_selftest@live@hangcheck.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22389/fi-snb-2600/igt@i915_selftest@live@hangcheck.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@hangcheck:
    - fi-ivb-3770:        [INCOMPLETE][4] ([i915#3303]) -> [PASS][5]
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11277/fi-ivb-3770/igt@i915_selftest@live@hangcheck.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22389/fi-ivb-3770/igt@i915_selftest@live@hangcheck.html

  
#### Warnings ####

  * igt@kms_psr@primary_page_flip:
    - fi-skl-6600u:       [FAIL][6] ([i915#4547]) -> [INCOMPLETE][7] ([i915#4547] / [i915#4838])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11277/fi-skl-6600u/igt@kms_psr@primary_page_flip.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22389/fi-skl-6600u/igt@kms_psr@primary_page_flip.html

  * igt@runner@aborted:
    - fi-skl-6600u:       [FAIL][8] ([i915#4312]) -> [FAIL][9] ([i915#2722] / [i915#4312])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11277/fi-skl-6600u/igt@runner@aborted.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22389/fi-skl-6600u/igt@runner@aborted.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#2722]: https://gitlab.freedesktop.org/drm/intel/issues/2722
  [i915#3303]: https://gitlab.freedesktop.org/drm/intel/issues/3303
  [i915#3576]: https://gitlab.freedesktop.org/drm/intel/issues/3576
  [i915#3921]: https://gitlab.freedesktop.org/drm/intel/issues/3921
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4547]: https://gitlab.freedesktop.org/drm/intel/issues/4547
  [i915#4838]: https://gitlab.freedesktop.org/drm/intel/issues/4838


Build changes
-------------

  * Linux: CI_DRM_11277 -> Patchwork_22389

  CI-20190529: 20190529
  CI_DRM_11277: a9d1ffee8dbe2c5506cccf9077eab8fe439eea46 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6355: 83ec34916bd8268bc331105cf77c4d3d3cd352be @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_22389: f980fb64e671359fad50127e8d6468cad25a85ab @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

f980fb64e671 drm/i915/display: Allow users to disable PSR2

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22389/index.html

[-- Attachment #2: Type: text/html, Size: 4414 bytes --]

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

[Souza, Jose]

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
> 

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

[Ville Syrjälä]

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ä
Intel

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

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel

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

[Souza, Jose]

+ 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

> 

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

[Rodrigo Vivi]

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.

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

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel