[PATCH] drm/i915: Enable PSR by default.

[PATCH] drm/i915: Enable PSR by default.

[Rodrigo Vivi]

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

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_params.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 835d609..461c326 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -37,7 +37,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,
@@ -126,7 +126,7 @@ MODULE_PARM_DESC(enable_execlists,
 	"(-1=auto [default], 0=disabled, 1=enabled)");
 
 module_param_named_unsafe(enable_psr, i915.enable_psr, int, 0600);
-MODULE_PARM_DESC(enable_psr, "Enable PSR (default: false)");
+MODULE_PARM_DESC(enable_psr, "Enable PSR (default: true)");
 
 module_param_named_unsafe(preliminary_hw_support, i915.preliminary_hw_support, int, 0600);
 MODULE_PARM_DESC(preliminary_hw_support,
-- 
2.4.3

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

Re: [PATCH] drm/i915: Enable PSR by default.

[Daniel Vetter]

On Wed, Dec 02, 2015 at 08:08:30AM -0800, Rodrigo Vivi wrote:
> With a reliable frontbuffer tracking and all instability corner cases
> solved let's re-enabled PSR by default on all supported platforms.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Hm, one thing we're missing here still I think is that right now there's
not yet a BAT for psr. At least I didn't spot anything in the CI overview.
Is there a super-basic PSR testcase (just checking that we go into psr
self-refresh is imo good enough), similar to what we have with runtime PM?

Also I guess we need to get the dp aux series from you in first. With BAT
and outstanding series merged:

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Awesome work!

Cheers, Daniel
> ---
>  drivers/gpu/drm/i915/i915_params.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 835d609..461c326 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -37,7 +37,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,
> @@ -126,7 +126,7 @@ MODULE_PARM_DESC(enable_execlists,
>  	"(-1=auto [default], 0=disabled, 1=enabled)");
>  
>  module_param_named_unsafe(enable_psr, i915.enable_psr, int, 0600);
> -MODULE_PARM_DESC(enable_psr, "Enable PSR (default: false)");
> +MODULE_PARM_DESC(enable_psr, "Enable PSR (default: true)");
>  
>  module_param_named_unsafe(preliminary_hw_support, i915.preliminary_hw_support, int, 0600);
>  MODULE_PARM_DESC(preliminary_hw_support,
> -- 
> 2.4.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Enable PSR by default.

[Vivi, Rodrigo]

On Fri, 2015-12-04 at 16:31 +0100, Daniel Vetter wrote:
> On Wed, Dec 02, 2015 at 08:08:30AM -0800, Rodrigo Vivi wrote:
> > With a reliable frontbuffer tracking and all instability corner 
> > cases
> > solved let's re-enabled PSR by default on all supported platforms.
> > 
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> Hm, one thing we're missing here still I think is that right now 
> there's
> not yet a BAT for psr.

Yes, agree.

>  At least I didn't spot anything in the CI overview.
> Is there a super-basic PSR testcase (just checking that we go into 
> psr
> self-refresh is imo good enough), similar to what we have with 
> runtime PM?

we had that in the past but in the end I removed. Maybe get it back and
call basic. But also we could have few cases like page_flip, mmap_gtt
and mmap_cpu as BAT as well. What do you think?

> 
> Also I guess we need to get the dp aux series from you in first.

Actually I believe the only required patch there is that one that check
for message size. Would you accept split that patch from that series
for now with a FIXME?
I promisse I won't give up on killing intel_dp_dpcd_read_wake ;)

>  With BAT
> and outstanding series merged:
> 
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Awesome work!
> 
> Cheers, Daniel

Thank you very much!
Rodrigo.

> > ---
> >  drivers/gpu/drm/i915/i915_params.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_params.c 
> > b/drivers/gpu/drm/i915/i915_params.c
> > index 835d609..461c326 100644
> > --- a/drivers/gpu/drm/i915/i915_params.c
> > +++ b/drivers/gpu/drm/i915/i915_params.c
> > @@ -37,7 +37,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,
> > @@ -126,7 +126,7 @@ MODULE_PARM_DESC(enable_execlists,
> >  	"(-1=auto [default], 0=disabled, 1=enabled)");
> >  
> >  module_param_named_unsafe(enable_psr, i915.enable_psr, int, 0600);
> > -MODULE_PARM_DESC(enable_psr, "Enable PSR (default: false)");
> > +MODULE_PARM_DESC(enable_psr, "Enable PSR (default: true)");
> >  
> >  module_param_named_unsafe(preliminary_hw_support, 
> > i915.preliminary_hw_support, int, 0600);
> >  MODULE_PARM_DESC(preliminary_hw_support,
> > -- 
> > 2.4.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Enable PSR by default.

[Daniel Vetter]

On Fri, Dec 04, 2015 at 04:22:20PM +0000, Vivi, Rodrigo wrote:
> On Fri, 2015-12-04 at 16:31 +0100, Daniel Vetter wrote:
> > On Wed, Dec 02, 2015 at 08:08:30AM -0800, Rodrigo Vivi wrote:
> > > With a reliable frontbuffer tracking and all instability corner 
> > > cases
> > > solved let's re-enabled PSR by default on all supported platforms.
> > > 
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > 
> > Hm, one thing we're missing here still I think is that right now 
> > there's
> > not yet a BAT for psr.
> 
> Yes, agree.
> 
> >  At least I didn't spot anything in the CI overview.
> > Is there a super-basic PSR testcase (just checking that we go into 
> > psr
> > self-refresh is imo good enough), similar to what we have with 
> > runtime PM?
> 
> we had that in the past but in the end I removed. Maybe get it back and
> call basic. But also we could have few cases like page_flip, mmap_gtt
> and mmap_cpu as BAT as well. What do you think?

BAT is already taking a while to run, and any modeset tests we add take
away more time. I think just a basic one should be good enough for BAT.
Imo BAT really should be just the "is this total crap?" test. We will
still run full igt (at least that's my goal) on any patch that goes in,
with the goal to run all the fast tests (less than 1 minute or so)
pre-merge. In this glorious future BAT will only decide whether more tests
should even be started.

> > Also I guess we need to get the dp aux series from you in first.
> 
> Actually I believe the only required patch there is that one that check
> for message size. Would you accept split that patch from that series
> for now with a FIXME?
> I promisse I won't give up on killing intel_dp_dpcd_read_wake ;)

Makes sense I'd say.

Cheers, Daniel
> 
> >  With BAT
> > and outstanding series merged:
> > 
> > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > Awesome work!
> > 
> > Cheers, Daniel
> 
> Thank you very much!
> Rodrigo.
> 
> > > ---
> > >  drivers/gpu/drm/i915/i915_params.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_params.c 
> > > b/drivers/gpu/drm/i915/i915_params.c
> > > index 835d609..461c326 100644
> > > --- a/drivers/gpu/drm/i915/i915_params.c
> > > +++ b/drivers/gpu/drm/i915/i915_params.c
> > > @@ -37,7 +37,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,
> > > @@ -126,7 +126,7 @@ MODULE_PARM_DESC(enable_execlists,
> > >  	"(-1=auto [default], 0=disabled, 1=enabled)");
> > >  
> > >  module_param_named_unsafe(enable_psr, i915.enable_psr, int, 0600);
> > > -MODULE_PARM_DESC(enable_psr, "Enable PSR (default: false)");
> > > +MODULE_PARM_DESC(enable_psr, "Enable PSR (default: true)");
> > >  
> > >  module_param_named_unsafe(preliminary_hw_support, 
> > > i915.preliminary_hw_support, int, 0600);
> > >  MODULE_PARM_DESC(preliminary_hw_support,
> > > -- 
> > > 2.4.3
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Enable PSR by default.

[Vivi, Rodrigo]

On Fri, 2015-12-04 at 17:54 +0100, Daniel Vetter wrote:
> On Fri, Dec 04, 2015 at 04:22:20PM +0000, Vivi, Rodrigo wrote:
> > On Fri, 2015-12-04 at 16:31 +0100, Daniel Vetter wrote:
> > > On Wed, Dec 02, 2015 at 08:08:30AM -0800, Rodrigo Vivi wrote:
> > > > With a reliable frontbuffer tracking and all instability corner 
> > > > cases
> > > > solved let's re-enabled PSR by default on all supported 
> > > > platforms.
> > > > 
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > 
> > > Hm, one thing we're missing here still I think is that right now 
> > > there's
> > > not yet a BAT for psr.
> > 
> > Yes, agree.
> > 
> > >  At least I didn't spot anything in the CI overview.
> > > Is there a super-basic PSR testcase (just checking that we go 
> > > into 
> > > psr
> > > self-refresh is imo good enough), similar to what we have with 
> > > runtime PM?
> > 
> > we had that in the past but in the end I removed. Maybe get it back 
> > and
> > call basic. But also we could have few cases like page_flip, 
> > mmap_gtt
> > and mmap_cpu as BAT as well. What do you think?
> 
> BAT is already taking a while to run, and any modeset tests we add 
> take
> away more time. I think just a basic one should be good enough for 
> BAT.
> Imo BAT really should be just the "is this total crap?" test. We will
> still run full igt (at least that's my goal) on any patch that goes 
> in,
> with the goal to run all the fast tests (less than 1 minute or so)
> pre-merge. In this glorious future BAT will only decide whether more 
> tests
> should even be started.

Makes sense.

> 
> > > Also I guess we need to get the dp aux series from you in first.
> > 
> > Actually I believe the only required patch there is that one that 
> > check
> > for message size. Would you accept split that patch from that 
> > series
> > for now with a FIXME?
> > I promisse I won't give up on killing intel_dp_dpcd_read_wake ;)
> 
> Makes sense I'd say.

Thanks!

> 
> Cheers, Daniel
> > 
> > >  With BAT
> > > and outstanding series merged:
> > > 
> > > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > 
> > > Awesome work!
> > > 
> > > Cheers, Daniel
> > 
> > Thank you very much!
> > Rodrigo.
> > 
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_params.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_params.c 
> > > > b/drivers/gpu/drm/i915/i915_params.c
> > > > index 835d609..461c326 100644
> > > > --- a/drivers/gpu/drm/i915/i915_params.c
> > > > +++ b/drivers/gpu/drm/i915/i915_params.c
> > > > @@ -37,7 +37,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,
> > > > @@ -126,7 +126,7 @@ MODULE_PARM_DESC(enable_execlists,
> > > >  	"(-1=auto [default], 0=disabled, 1=enabled)");
> > > >  
> > > >  module_param_named_unsafe(enable_psr, i915.enable_psr, int, 
> > > > 0600);
> > > > -MODULE_PARM_DESC(enable_psr, "Enable PSR (default: false)");
> > > > +MODULE_PARM_DESC(enable_psr, "Enable PSR (default: true)");
> > > >  
> > > >  module_param_named_unsafe(preliminary_hw_support, 
> > > > i915.preliminary_hw_support, int, 0600);
> > > >  MODULE_PARM_DESC(preliminary_hw_support,
> > > > -- 
> > > > 2.4.3
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Enable PSR by default.

[Rodrigo Vivi]

With a reliable frontbuffer tracking and all instability corner cases
solved let's re-enabled PSR by default on all supported 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
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, please
report PCI device ID, subsystem vendor and subsystem device ID
to intel-gfx@lists.freedesktop.org, if your machine needs it.
It will then be included in an upcoming module version.

Also, if the standby mode doesn't help a bugzilla entry is desirable
containing:
- dmesg (drm.debug=0xe)
- 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

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.

v2: Rebased on top of psr paramenter change.

v3: Rebase and improve commit message.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_params.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 6dd39f0..edec23c 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -37,7 +37,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,
@@ -127,7 +127,7 @@ 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=link-off maximum power-savings, 2=link-standby mode)"
+		 "(0=disabled, 1=link-off maximum power-savings [default], 2=link-standby mode)"
 		 "In case you needed to force it on standby or disabled, please "
 		 "report PCI device ID, subsystem vendor and subsystem device ID "
 		 "to intel-gfx@lists.freedesktop.org, if your machine needs it. "
-- 
2.4.3

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