[PATCH 0/3] Enable PSR by default in certain platforms

[PATCH 0/3] Enable PSR by default in certain platforms

[Rodrigo Vivi]

For a long time we are trying to enable PSR by default everwhere
but we always end up in soem specific corner case in one
platform or another and end up not enabling in older stable platforms
that could benefit from the power saving it provides.

So, let's change the approach here and try to enable it separated.

The new blocking issue is for Skylake and Kabylake due to a vblank
wait situation with DMC firmware:
https://bugs.freedesktop.org/show_bug.cgi?id=94126
So that case will block PSR on SKL and KBL.

But  this doesn't affect at all other platforms that don't have DMC.

Also I decided to split the rest of platforms in two groups since
Valleyview/Cherryview has a complete different Hardware PSR
implementation than the rest of the platorms. Also it uses only
link standby what should be safier.

So, 1 patch to add the possibility of enable per platform, 1 patch for
VLV/CHV and 1 patch for HSW/BDW.

I hope to get SKL/KBL ready soon.

Thanks,
Rodrigo.

Rodrigo Vivi (3):
  drm/i915: Change i915.enable_psr parameter to use per platform
    default.
  drm/i915: Enable PSR by default on Valleyview and Cherryview.
  drm/i915: Enable PSR by default on Haswell and Broadwell.

 drivers/gpu/drm/i915/i915_params.c | 5 +++--
 drivers/gpu/drm/i915/intel_psr.c   | 9 +++++++++
 2 files changed, 12 insertions(+), 2 deletions(-)

-- 
2.4.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/3] drm/i915: Change i915.enable_psr parameter to use per platform default.

[Rodrigo Vivi]

This will give us flexibility to enable PSR by default independently so
issues and corner cases in one platform won't affect others were we have
it working properly.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_params.c | 5 +++--
 drivers/gpu/drm/i915/intel_psr.c   | 5 +++++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 8b9f368..1b40ee6 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -38,7 +38,7 @@ struct i915_params i915 __read_mostly = {
 	.enable_execlists = -1,
 	.enable_hangcheck = true,
 	.enable_ppgtt = -1,
-	.enable_psr = 0,
+	.enable_psr = -1,
 	.preliminary_hw_support = IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT),
 	.disable_power_well = -1,
 	.enable_ips = 1,
@@ -128,7 +128,8 @@ MODULE_PARM_DESC(enable_execlists,
 
 module_param_named_unsafe(enable_psr, i915.enable_psr, int, 0600);
 MODULE_PARM_DESC(enable_psr, "Enable PSR "
-		 "(0=disabled [default], 1=enabled - link mode chosen per-platform, 2=force link-standby mode, 3=force link-off mode)");
+		 "(0=disabled, 1=enabled - link mode chosen per-platform, 2=force link-standby mode, 3=force link-off mode) "
+		 "Default: -1 (use per-chip default)");
 
 module_param_named_unsafe(preliminary_hw_support, i915.preliminary_hw_support, int, 0600);
 MODULE_PARM_DESC(preliminary_hw_support,
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 4ab7579..655bdf6 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -778,6 +778,11 @@ void intel_psr_init(struct drm_device *dev)
 	dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ?
 		HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE;
 
+	/* Per platform default */
+	if (i915.enable_psr == -1) {
+		i915.enable_psr = 0;
+	}
+
 	/* Set link_standby x link_off defaults */
 	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 		/* HSW and BDW require workarounds that we don't implement. */
-- 
2.4.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] drm/i915: Enable PSR by default on Valleyview and Cherryview.

[Rodrigo Vivi]

With a reliable frontbuffer tracking and all instability corner cases
solved for this platform let's re-enabled PSR by default.

In case a new issue is found and PSR is the main suspect, please check
if i915.enable_psr=0 really makes your problem go away,
please report it at bugs.freedesktop.org.

In a bugzilla entry for PSR is desirable:
- dmesg (drm.debug=0xe)
- output of /sys/kernel/debug/dri/0/i915_edp_psr_status
- Platform information. Vendor, model, id, pci id.
- Graphical environment: Gnome, KDE, openbox, etc...
- Details how to reproduce.
- Also good if you could run PSR test cases of Intel-gpu-tools
- Please mention if forcing main link standby or main link off helps you.

There are Intel-gpu-tools test cases that can be helpful to
determine if PSR is working as expected:
 kms_psr_sink_crc and kms_psr_frontbuffer_tracking.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 655bdf6..12b5a7e 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -780,7 +780,10 @@ void intel_psr_init(struct drm_device *dev)
 
 	/* Per platform default */
 	if (i915.enable_psr == -1) {
-		i915.enable_psr = 0;
+		if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
+			i915.enable_psr = 1;
+		else
+			i915.enable_psr = 0;
 	}
 
 	/* Set link_standby x link_off defaults */
-- 
2.4.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] drm/i915: Enable PSR by default on Haswell and Broadwell.

[Rodrigo Vivi]

With a reliable frontbuffer tracking and all instability corner cases
on Haswell and Broadwell solved let's re-enabled PSR by default on
these platforms.

In case a new issue is found and PSR is the main suspect, please check
if i915.enable_psr=0 really makes your problem go away. If this is the case
PSR is the culprit so after that please check if i915.enable_psr=2
or i915.enable_psr=3 solves your issue and please let us know.
There are many panels out there and not all implementations apparently
work as we would expect.

In case you needed to force it on standby or disabled or in case of any
PSR related bug please report it at bugs.freedesktop.org.
In a bugzilla entry for PSR is desirable:
- dmesg (drm.debug=0xe)
- output of /sys/kernel/debug/dri/0/i915_edp_psr_status
- Platform information. Vendor, model, id, pci id.
- Graphical environment: Gnome, KDE, openbox, etc...
- Details how to reproduce.
- Also good if you could run PSR test cases of Intel-gpu-tools
- Please mention if forcing main link standby or main link off helps you.

There are Intel-gpu-tools test cases that can be helpful to
determine if PSR is working as expected:
kms_psr_sink_crc and kms_psr_frontbuffer_tracking.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 12b5a7e..0b42ada 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -780,7 +780,8 @@ void intel_psr_init(struct drm_device *dev)
 
 	/* Per platform default */
 	if (i915.enable_psr == -1) {
-		if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
+		if (IS_HASWELL(dev) || IS_BROADWELL(dev) ||
+		    IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
 			i915.enable_psr = 1;
 		else
 			i915.enable_psr = 0;
-- 
2.4.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for Enable PSR by default in certain platforms

[Patchwork]

== Summary ==

Series 3382v1 Enable PSR by default in certain platforms
http://patchwork.freedesktop.org/api/1.0/series/3382/revisions/1/mbox/

Test gem_sync:
        Subgroup basic-bsd:
                dmesg-fail -> PASS       (ilk-hp8440p)
        Subgroup basic-vebox:
                dmesg-fail -> PASS       (hsw-brixbox)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                dmesg-warn -> PASS       (ilk-hp8440p) UNSTABLE
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> PASS       (ivb-t430s)
Test pm_rpm:
        Subgroup basic-rte:
                pass       -> DMESG-WARN (byt-nuc) UNSTABLE

bdw-nuci7        total:162  pass:152  dwarn:0   dfail:0   fail:0   skip:10 
bdw-ultra        total:165  pass:152  dwarn:0   dfail:0   fail:0   skip:13 
bsw-nuc-2        total:165  pass:135  dwarn:1   dfail:0   fail:0   skip:29 
byt-nuc          total:165  pass:140  dwarn:1   dfail:0   fail:0   skip:24 
hsw-brixbox      total:165  pass:151  dwarn:0   dfail:0   fail:0   skip:14 
hsw-gt2          total:165  pass:154  dwarn:0   dfail:0   fail:1   skip:10 
ilk-hp8440p      total:165  pass:116  dwarn:0   dfail:0   fail:1   skip:48 
ivb-t430s        total:165  pass:150  dwarn:0   dfail:0   fail:1   skip:14 
skl-i5k-2        total:165  pass:150  dwarn:0   dfail:0   fail:0   skip:15 
snb-dellxps      total:165  pass:142  dwarn:0   dfail:0   fail:1   skip:22 
snb-x220t        total:165  pass:142  dwarn:0   dfail:0   fail:2   skip:21 

Results at /archive/results/CI_IGT_test/Patchwork_1403/

a4474d338aa8156348cebe58a329a18c8560da1e drm-intel-nightly: 2016y-02m-15d-17h-27m-11s UTC integration manifest
94582cca769d3ba53753adf60ac312af7f09f8a0 drm/i915: Enable PSR by default on Haswell and Broadwell.
809b92bd00ec26a5e637579e7dc8b6b32958fd74 drm/i915: Enable PSR by default on Valleyview and Cherryview.
28196cda57aebc08a0582ae79d5d05bc086a98aa drm/i915: Change i915.enable_psr parameter to use per platform default.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Change i915.enable_psr parameter to use per platform default.

[Jani Nikula]

On Fri, 12 Feb 2016, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> This will give us flexibility to enable PSR by default independently so
> issues and corner cases in one platform won't affect others were we have
> it working properly.
>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_params.c | 5 +++--
>  drivers/gpu/drm/i915/intel_psr.c   | 5 +++++
>  2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 8b9f368..1b40ee6 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -38,7 +38,7 @@ struct i915_params i915 __read_mostly = {
>  	.enable_execlists = -1,
>  	.enable_hangcheck = true,
>  	.enable_ppgtt = -1,
> -	.enable_psr = 0,
> +	.enable_psr = -1,
>  	.preliminary_hw_support = IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT),
>  	.disable_power_well = -1,
>  	.enable_ips = 1,
> @@ -128,7 +128,8 @@ MODULE_PARM_DESC(enable_execlists,
>  
>  module_param_named_unsafe(enable_psr, i915.enable_psr, int, 0600);
>  MODULE_PARM_DESC(enable_psr, "Enable PSR "
> -		 "(0=disabled [default], 1=enabled - link mode chosen per-platform, 2=force link-standby mode, 3=force link-off mode)");
> +		 "(0=disabled, 1=enabled - link mode chosen per-platform, 2=force link-standby mode, 3=force link-off mode) "
> +		 "Default: -1 (use per-chip default)");
>  
>  module_param_named_unsafe(preliminary_hw_support, i915.preliminary_hw_support, int, 0600);
>  MODULE_PARM_DESC(preliminary_hw_support,
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 4ab7579..655bdf6 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -778,6 +778,11 @@ void intel_psr_init(struct drm_device *dev)
>  	dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ?
>  		HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE;
>  
> +	/* Per platform default */
> +	if (i915.enable_psr == -1) {
> +		i915.enable_psr = 0;
> +	}

I don't like changing module parameters in-kernel, because them changing
may be surprising to the user (and possibly developers debugging
issues). We do this anyway, but AFAICT only for read only
parameters. It's perhaps less surprising in that case.

Now, i915.enable_psr has permissions 0600, so you can change it
dynamically through sysfs. Maybe it's a good debugging thing, I don't
know. But I also notice some parts of the PSR functionality (link
standby) can only be forced with i915.enable_psr during module load, and
not dynamically.

So I think you need to either a) change enable_psr permission to 0400
and give up the ability to dynamically toggle the feature, or b) fix up
the current link standby force and not change the parameter value
in-kernel in this patch.

BR,
Jani.


> +
>  	/* Set link_standby x link_off defaults */
>  	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>  		/* HSW and BDW require workarounds that we don't implement. */

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Change i915.enable_psr parameter to use per platform default.

[Zanoni, Paulo R]

Em Ter, 2016-02-16 às 12:27 +0200, Jani Nikula escreveu:
> On Fri, 12 Feb 2016, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > This will give us flexibility to enable PSR by default
> > independently so
> > issues and corner cases in one platform won't affect others were we
> > have
> > it working properly.
> > 
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_params.c | 5 +++--
> >  drivers/gpu/drm/i915/intel_psr.c   | 5 +++++
> >  2 files changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_params.c
> > b/drivers/gpu/drm/i915/i915_params.c
> > index 8b9f368..1b40ee6 100644
> > --- a/drivers/gpu/drm/i915/i915_params.c
> > +++ b/drivers/gpu/drm/i915/i915_params.c
> > @@ -38,7 +38,7 @@ struct i915_params i915 __read_mostly = {
> >  	.enable_execlists = -1,
> >  	.enable_hangcheck = true,
> >  	.enable_ppgtt = -1,
> > -	.enable_psr = 0,
> > +	.enable_psr = -1,
> >  	.preliminary_hw_support =
> > IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT),
> >  	.disable_power_well = -1,
> >  	.enable_ips = 1,
> > @@ -128,7 +128,8 @@ MODULE_PARM_DESC(enable_execlists,
> >  
> >  module_param_named_unsafe(enable_psr, i915.enable_psr, int, 0600);
> >  MODULE_PARM_DESC(enable_psr, "Enable PSR "
> > -		 "(0=disabled [default], 1=enabled - link mode
> > chosen per-platform, 2=force link-standby mode, 3=force link-off
> > mode)");
> > +		 "(0=disabled, 1=enabled - link mode chosen per-
> > platform, 2=force link-standby mode, 3=force link-off mode) "
> > +		 "Default: -1 (use per-chip default)");
> >  
> >  module_param_named_unsafe(preliminary_hw_support,
> > i915.preliminary_hw_support, int, 0600);
> >  MODULE_PARM_DESC(preliminary_hw_support,
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 4ab7579..655bdf6 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -778,6 +778,11 @@ void intel_psr_init(struct drm_device *dev)
> >  	dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ?
> >  		HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE;
> >  
> > +	/* Per platform default */
> > +	if (i915.enable_psr == -1) {
> > +		i915.enable_psr = 0;
> > +	}
> 
> I don't like changing module parameters in-kernel, because them
> changing
> may be surprising to the user (and possibly developers debugging
> issues). We do this anyway, but AFAICT only for read only
> parameters. It's perhaps less surprising in that case.
> 
> Now, i915.enable_psr has permissions 0600, so you can change it
> dynamically through sysfs. Maybe it's a good debugging thing, I don't
> know. But I also notice some parts of the PSR functionality (link
> standby) can only be forced with i915.enable_psr during module load,
> and
> not dynamically.
> 
> So I think you need to either a) change enable_psr permission to 0400
> and give up the ability to dynamically toggle the feature, or b) fix
> up
> the current link standby force and not change the parameter value
> in-kernel in this patch.

Just as a reminder: igt relies on the 0600 permissions so it can run
PSR tests even while PSR is disabled by default. This was done because
we're trying to fix the features, so removing them from QA/CI testing
would only allow people to introduce unnoticed regressions. When I did
this, I was also expecting that at some point our pre-merge testing
would be running a huge chunk of IGT.

> 
> BR,
> Jani.
> 
> 
> > +
> >  	/* Set link_standby x link_off defaults */
> >  	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> >  		/* HSW and BDW require workarounds that we don't
> > implement. */
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/3] Enable PSR by default in certain platforms

[Daniel Vetter]

On Fri, Feb 12, 2016 at 04:08:10AM -0800, Rodrigo Vivi wrote:
> For a long time we are trying to enable PSR by default everwhere
> but we always end up in soem specific corner case in one
> platform or another and end up not enabling in older stable platforms
> that could benefit from the power saving it provides.
> 
> So, let's change the approach here and try to enable it separated.
> 
> The new blocking issue is for Skylake and Kabylake due to a vblank
> wait situation with DMC firmware:
> https://bugs.freedesktop.org/show_bug.cgi?id=94126
> So that case will block PSR on SKL and KBL.
> 
> But  this doesn't affect at all other platforms that don't have DMC.
> 
> Also I decided to split the rest of platforms in two groups since
> Valleyview/Cherryview has a complete different Hardware PSR
> implementation than the rest of the platorms. Also it uses only
> link standby what should be safier.
> 
> So, 1 patch to add the possibility of enable per platform, 1 patch for
> VLV/CHV and 1 patch for HSW/BDW.
> 
> I hope to get SKL/KBL ready soon.
> 
> Thanks,
> Rodrigo.
> 
> Rodrigo Vivi (3):
>   drm/i915: Change i915.enable_psr parameter to use per platform
>     default.
>   drm/i915: Enable PSR by default on Valleyview and Cherryview.
>   drm/i915: Enable PSR by default on Haswell and Broadwell.

I don't really have a real opinion on the issue on patch 1 and don't mind
merging the patch as-is.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> on the entire series.

> 
>  drivers/gpu/drm/i915/i915_params.c | 5 +++--
>  drivers/gpu/drm/i915/intel_psr.c   | 9 +++++++++
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> -- 
> 2.4.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Change i915.enable_psr parameter to use per platform default.

[Vivi, Rodrigo]

On Tue, 2016-02-16 at 11:49 +0000, Zanoni, Paulo R wrote:
> Em Ter, 2016-02-16 às 12:27 +0200, Jani Nikula escreveu:
> > On Fri, 12 Feb 2016, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > > This will give us flexibility to enable PSR by default
> > > independently so
> > > issues and corner cases in one platform won't affect others were 
> > > we
> > > have
> > > it working properly.
> > > 
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_params.c | 5 +++--
> > >  drivers/gpu/drm/i915/intel_psr.c   | 5 +++++
> > >  2 files changed, 8 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_params.c
> > > b/drivers/gpu/drm/i915/i915_params.c
> > > index 8b9f368..1b40ee6 100644
> > > --- a/drivers/gpu/drm/i915/i915_params.c
> > > +++ b/drivers/gpu/drm/i915/i915_params.c
> > > @@ -38,7 +38,7 @@ struct i915_params i915 __read_mostly = {
> > >  	.enable_execlists = -1,
> > >  	.enable_hangcheck = true,
> > >  	.enable_ppgtt = -1,
> > > -	.enable_psr = 0,
> > > +	.enable_psr = -1,
> > >  	.preliminary_hw_support =
> > > IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT),
> > >  	.disable_power_well = -1,
> > >  	.enable_ips = 1,
> > > @@ -128,7 +128,8 @@ MODULE_PARM_DESC(enable_execlists,
> > >  
> > >  module_param_named_unsafe(enable_psr, i915.enable_psr, int, 
> > > 0600);
> > >  MODULE_PARM_DESC(enable_psr, "Enable PSR "
> > > -		 "(0=disabled [default], 1=enabled - link mode
> > > chosen per-platform, 2=force link-standby mode, 3=force link-off
> > > mode)");
> > > +		 "(0=disabled, 1=enabled - link mode chosen per-
> > > platform, 2=force link-standby mode, 3=force link-off mode) "
> > > +		 "Default: -1 (use per-chip default)");
> > >  
> > >  module_param_named_unsafe(preliminary_hw_support,
> > > i915.preliminary_hw_support, int, 0600);
> > >  MODULE_PARM_DESC(preliminary_hw_support,
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 4ab7579..655bdf6 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -778,6 +778,11 @@ void intel_psr_init(struct drm_device *dev)
> > >  	dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ?
> > >  		HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE;
> > >  
> > > +	/* Per platform default */
> > > +	if (i915.enable_psr == -1) {
> > > +		i915.enable_psr = 0;
> > > +	}
> > 
> > I don't like changing module parameters in-kernel, because them
> > changing
> > may be surprising to the user (and possibly developers debugging
> > issues). We do this anyway, but AFAICT only for read only
> > parameters. It's perhaps less surprising in that case.


To be honest I shared this feeling. So when I was looking to the code
to do this change I tried different ways but all of them for me were
ugly. So in the end this one here seemed for me the cleanest way.

In the end -1 means driver will choose and nowadays driver is choosing
0, if we were enabling by default for all platforms driver would choose
1. So we are just moving driver's choice from the params to the
psr_init function. 

Also I didn't want to impact igt functionality at this point that
relies on the ability of changing the paramenter at runtime. If I was
adding another variable to control that I would have to change igt as
well.

> > 
> > Now, i915.enable_psr has permissions 0600, so you can change it
> > dynamically through sysfs. Maybe it's a good debugging thing, I 
> > don't
> > know. But I also notice some parts of the PSR functionality (link
> > standby) can only be forced with i915.enable_psr during module 
> > load,
> > and
> > not dynamically.

Yeap, the dynamic part is needed for igt that relies on it for
enabling/disabling psr. 
Since sysfs interface wasn't accepted yet I decided not changing this b
ehavior.
I also noticed that force-standby and force-link-off were just
available at boot time but this is all we need for now so while we
don't have the sysfs interface in place I believe we can live with this
difference.

> > 
> > So I think you need to either a) change enable_psr permission to 
> > 0400
> > and give up the ability to dynamically toggle the feature, or b) 
> > fix
> > up
> > the current link standby force and not change the parameter value
> > in-kernel in this patch.
> 
> Just as a reminder: igt relies on the 0600 permissions so it can run
> PSR tests even while PSR is disabled by default. This was done 
> because
> we're trying to fix the features, so removing them from QA/CI testing
> would only allow people to introduce unnoticed regressions. When I 
> did
> this, I was also expecting that at some point our pre-merge testing
> would be running a huge chunk of IGT.

Exactly. At this point it would require IGT changes as well that would
block stuff or, worst, not noticing new regressions or bugs.

I'm confident that sysfs changes will land soon for power stuff as we
agreed on design reviews so powertop could take advantage of
controlling and showing all of our power related stuff. So when that
properly lands we can re-visit this parameter and change PSR paramenter
and IGT properly.

> 
> > 
> > BR,
> > Jani.
> > 
> > 
> > > +
> > >  	/* Set link_standby x link_off defaults */
> > >  	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > >  		/* HSW and BDW require workarounds that we don't
> > > implement. */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx