[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 1/3] drm/i915: PSR also doesn't have link_entry_time on SKL.

[Rodrigo Vivi]

This bit is also reserved on Skylake. Actually the only
platform that supports this is Haswell, so let's fix
this logic and apply this link entry time only for the
platform that supports it, i.e. Haswell.

This also changes the style to let more clear platform
differences outside the reg write. We would probably catch
this case sooner if separated, or not...

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

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 14cc2cf..9ccff30 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -276,10 +276,11 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
 	 */
 	uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
 	uint32_t val = 0x0;
-	const uint32_t link_entry_time = EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
+
+	if (IS_HASWELL(dev))
+		val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
 
 	I915_WRITE(EDP_PSR_CTL, val |
-		   (IS_BROADWELL(dev) ? 0 : link_entry_time) |
 		   max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT |
 		   idle_frames << EDP_PSR_IDLE_FRAME_SHIFT |
 		   EDP_PSR_ENABLE);
-- 
2.4.3

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

[PATCH 2/3] drm/i915: Instrument PSR parameter for possible quirks with link standby.

[Rodrigo Vivi]

Link standby support has been deprecated with 'commit 89251b177
("drm/i915: PSR: deprecate link_standby support for core platforms.")'

The reason for that is that main link in full off offers more power
savings and some platforms implementations on source side had known
bugs with link standby.

However we don't know all panels out there and we don't fully rely
on the VBT information after the case found with the commit that
made us to deprecate link standby.

So, before enable PSR by default let's instrument the PSR parameter
in a way that we can identify different panels out there that might
require or work better with link standby mode.

It is also useful to say that for backward compatibility I'm not
changing the meaning of this flag. So "0" still means disabled
and "1" means enabled with full support and maximum power savings.

Negative values are being used for debug purposes since I'd like
to save positive number for future use like PSR2 for instance.

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 24318b7..efe973b 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2567,6 +2567,10 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 				enabled = true;
 		}
 	}
+
+	seq_printf(m, "Forcing main link standby: %s\n",
+		   yesno(dev_priv->psr.link_standby));
+
 	seq_printf(m, "HW Enabled & Active bit: %s", yesno(enabled));
 
 	if (!HAS_DDI(dev))
@@ -2587,6 +2591,7 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 
 		seq_printf(m, "Performance_Counter: %u\n", psrperf);
 	}
+
 	mutex_unlock(&dev_priv->psr.lock);
 
 	intel_runtime_pm_put(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5edd393..de086f0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -969,6 +969,7 @@ struct i915_psr {
 	unsigned busy_frontbuffer_bits;
 	bool psr2_support;
 	bool aux_frame_sync;
+	bool link_standby;
 };
 
 enum intel_pch {
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 835d609..def8802 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -126,7 +126,12 @@ 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 "
+		 "(-1=force link standby for debug, 0=disabled [default], 1=enabled)"
+		 "In case you needed to force debug mode, 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.");
 
 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 9ccff30..7529f93 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -225,7 +225,12 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
 		   (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT));
 	}
 
-	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, DP_PSR_ENABLE);
+	if (dev_priv->psr.link_standby)
+		drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
+				   DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE);
+	else
+		drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
+				   DP_PSR_ENABLE);
 }
 
 static void vlv_psr_enable_source(struct intel_dp *intel_dp)
@@ -280,6 +285,9 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
 	if (IS_HASWELL(dev))
 		val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
 
+	if (dev_priv->psr.link_standby)
+		val |= EDP_PSR_LINK_STANDBY;
+
 	I915_WRITE(EDP_PSR_CTL, val |
 		   max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT |
 		   idle_frames << EDP_PSR_IDLE_FRAME_SHIFT |
@@ -763,6 +771,9 @@ 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;
 
+	if (i915.enable_psr == -1)
+		dev_priv->psr.link_standby = true;
+
 	INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work);
 	mutex_init(&dev_priv->psr.lock);
 }
-- 
2.4.3

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

[PATCH 3/3] 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.

v2: Rebased on top of psr paramenter change.

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 def8802..7dafda8 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 "
-		 "(-1=force link standby for debug, 0=disabled [default], 1=enabled)"
+		 "(-1=force link standby for debug, 0=disabled, 1=enabled [default])"
 		 "In case you needed to force debug mode, 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

[PATCH] drm/i915: Instrument PSR parameter for possible quirks with link standby.

[Rodrigo Vivi]

Link standby support has been deprecated with 'commit 89251b177
("drm/i915: PSR: deprecate link_standby support for core platforms.")'

The reason for that is that main link in full off offers more power
savings and some platforms implementations on source side had known
bugs with link standby.

However we don't know all panels out there and we don't fully rely
on the VBT information after the case found with the commit that
made us to deprecate link standby.

So, before enable PSR by default let's instrument the PSR parameter
in a way that we can identify different panels out there that might
require or work better with link standby mode.

It is also useful to say that for backward compatibility I'm not
changing the meaning of this flag. So "0" still means disabled
and "1" means enabled with full support and maximum power savings.

v2: Use positive value instead of negative for different operation mode
    as suggested by Daniel.

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 24318b7..efe973b 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2567,6 +2567,10 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 				enabled = true;
 		}
 	}
+
+	seq_printf(m, "Forcing main link standby: %s\n",
+		   yesno(dev_priv->psr.link_standby));
+
 	seq_printf(m, "HW Enabled & Active bit: %s", yesno(enabled));
 
 	if (!HAS_DDI(dev))
@@ -2587,6 +2591,7 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 
 		seq_printf(m, "Performance_Counter: %u\n", psrperf);
 	}
+
 	mutex_unlock(&dev_priv->psr.lock);
 
 	intel_runtime_pm_put(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5edd393..de086f0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -969,6 +969,7 @@ struct i915_psr {
 	unsigned busy_frontbuffer_bits;
 	bool psr2_support;
 	bool aux_frame_sync;
+	bool link_standby;
 };
 
 enum intel_pch {
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 835d609..6dd39f0 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -126,7 +126,12 @@ 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 "
+		 "(0=disabled [default], 1=link-off maximum power-savings, 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. "
+		 "It will then be included in an upcoming module version.");
 
 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 9ccff30..bcc85fd 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -225,7 +225,12 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
 		   (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT));
 	}
 
-	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, DP_PSR_ENABLE);
+	if (dev_priv->psr.link_standby)
+		drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
+				   DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE);
+	else
+		drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
+				   DP_PSR_ENABLE);
 }
 
 static void vlv_psr_enable_source(struct intel_dp *intel_dp)
@@ -280,6 +285,9 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
 	if (IS_HASWELL(dev))
 		val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
 
+	if (dev_priv->psr.link_standby)
+		val |= EDP_PSR_LINK_STANDBY;
+
 	I915_WRITE(EDP_PSR_CTL, val |
 		   max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT |
 		   idle_frames << EDP_PSR_IDLE_FRAME_SHIFT |
@@ -763,6 +771,9 @@ 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;
 
+	if (i915.enable_psr == 2)
+		dev_priv->psr.link_standby = true;
+
 	INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work);
 	mutex_init(&dev_priv->psr.lock);
 }
-- 
2.4.3

_______________________________________________
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

Re: [PATCH 2/3] drm/i915: Instrument PSR parameter for possible quirks with link standby.

[Daniel Vetter]

On Thu, Dec 10, 2015 at 08:28:23AM -0800, Rodrigo Vivi wrote:
> Link standby support has been deprecated with 'commit 89251b177
> ("drm/i915: PSR: deprecate link_standby support for core platforms.")'
> 
> The reason for that is that main link in full off offers more power
> savings and some platforms implementations on source side had known
> bugs with link standby.
> 
> However we don't know all panels out there and we don't fully rely
> on the VBT information after the case found with the commit that
> made us to deprecate link standby.
> 
> So, before enable PSR by default let's instrument the PSR parameter
> in a way that we can identify different panels out there that might
> require or work better with link standby mode.
> 
> It is also useful to say that for backward compatibility I'm not
> changing the meaning of this flag. So "0" still means disabled
> and "1" means enabled with full support and maximum power savings.
> 
> Negative values are being used for debug purposes since I'd like
> to save positive number for future use like PSR2 for instance.
> 
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |  5 +++++
>  drivers/gpu/drm/i915/i915_drv.h     |  1 +
>  drivers/gpu/drm/i915/i915_params.c  |  7 ++++++-
>  drivers/gpu/drm/i915/intel_psr.c    | 13 ++++++++++++-
>  4 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 24318b7..efe973b 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2567,6 +2567,10 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>  				enabled = true;
>  		}
>  	}
> +
> +	seq_printf(m, "Forcing main link standby: %s\n",
> +		   yesno(dev_priv->psr.link_standby));
> +
>  	seq_printf(m, "HW Enabled & Active bit: %s", yesno(enabled));
>  
>  	if (!HAS_DDI(dev))
> @@ -2587,6 +2591,7 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>  
>  		seq_printf(m, "Performance_Counter: %u\n", psrperf);
>  	}
> +
>  	mutex_unlock(&dev_priv->psr.lock);
>  
>  	intel_runtime_pm_put(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5edd393..de086f0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -969,6 +969,7 @@ struct i915_psr {
>  	unsigned busy_frontbuffer_bits;
>  	bool psr2_support;
>  	bool aux_frame_sync;
> +	bool link_standby;
>  };
>  
>  enum intel_pch {
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 835d609..def8802 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -126,7 +126,12 @@ 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 "
> +		 "(-1=force link standby for debug, 0=disabled [default], 1=enabled)"

-1 usually means debug, and values >= 1 are for different enabling modes.
Otherwise makes sense I'd say.
-Daniel

> +		 "In case you needed to force debug mode, 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.");
>  
>  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 9ccff30..7529f93 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -225,7 +225,12 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
>  		   (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT));
>  	}
>  
> -	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, DP_PSR_ENABLE);
> +	if (dev_priv->psr.link_standby)
> +		drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
> +				   DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE);
> +	else
> +		drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
> +				   DP_PSR_ENABLE);
>  }
>  
>  static void vlv_psr_enable_source(struct intel_dp *intel_dp)
> @@ -280,6 +285,9 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
>  	if (IS_HASWELL(dev))
>  		val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
>  
> +	if (dev_priv->psr.link_standby)
> +		val |= EDP_PSR_LINK_STANDBY;
> +
>  	I915_WRITE(EDP_PSR_CTL, val |
>  		   max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT |
>  		   idle_frames << EDP_PSR_IDLE_FRAME_SHIFT |
> @@ -763,6 +771,9 @@ 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;
>  
> +	if (i915.enable_psr == -1)
> +		dev_priv->psr.link_standby = true;
> +
>  	INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work);
>  	mutex_init(&dev_priv->psr.lock);
>  }
> -- 
> 2.4.3
> 

-- 
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 1/3] drm/i915: PSR also doesn't have link_entry_time on SKL.

[Paulo Zanoni]

2015-12-10 14:28 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> This bit is also reserved on Skylake. Actually the only
> platform that supports this is Haswell, so let's fix
> this logic and apply this link entry time only for the
> platform that supports it, i.e. Haswell.
>
> This also changes the style to let more clear platform
> differences outside the reg write. We would probably catch
> this case sooner if separated, or not...
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 14cc2cf..9ccff30 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -276,10 +276,11 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
>          */
>         uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
>         uint32_t val = 0x0;
> -       const uint32_t link_entry_time = EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
> +
> +       if (IS_HASWELL(dev))
> +               val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;

It's kinda weird that we used the variable "val" for nothing. At least
we're using it now :). <insert bikeshed suggestion about either using
it for storing all bits or just kill it>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
>         I915_WRITE(EDP_PSR_CTL, val |
> -                  (IS_BROADWELL(dev) ? 0 : link_entry_time) |
>                    max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT |
>                    idle_frames << EDP_PSR_IDLE_FRAME_SHIFT |
>                    EDP_PSR_ENABLE);
> --
> 2.4.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 1/3] drm/i915: PSR also doesn't have link_entry_time on SKL.

[Vivi, Rodrigo]

On Fri, 2015-12-11 at 17:09 -0200, Paulo Zanoni wrote:
> 2015-12-10 14:28 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> > This bit is also reserved on Skylake. Actually the only
> > platform that supports this is Haswell, so let's fix
> > this logic and apply this link entry time only for the
> > platform that supports it, i.e. Haswell.
> > 
> > This also changes the style to let more clear platform
> > differences outside the reg write. We would probably catch
> > this case sooner if separated, or not...

> > 
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 14cc2cf..9ccff30 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -276,10 +276,11 @@ static void hsw_psr_enable_source(struct 
> > intel_dp *intel_dp)
> >          */
> >         uint32_t idle_frames = max(6, dev_priv
> > ->vbt.psr.idle_frames);
> >         uint32_t val = 0x0;
> > -       const uint32_t link_entry_time = 
> > EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
> > +
> > +       if (IS_HASWELL(dev))
> > +               val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
> 
> It's kinda weird that we used the variable "val" for nothing. At 
> least
> we're using it now :).

Yes, indeed...  in the past we had some conditional stuff there that we
were removing one by one and val end up empty...
first version that I was doing here had a patch 1 to remove it and
second one with this change here... However I had to add link standby
condition back and I decided to keep val.

>  <insert bikeshed suggestion about either using
> it for storing all bits or just kill it>

hm.. I like the val to check conditionals and the fixed ones inside the
write macro, but I see your point of a standardization... 

> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

thanks

> 
> > 
> >         I915_WRITE(EDP_PSR_CTL, val |
> > -                  (IS_BROADWELL(dev) ? 0 : link_entry_time) |
> >                    max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT |
> >                    idle_frames << EDP_PSR_IDLE_FRAME_SHIFT |
> >                    EDP_PSR_ENABLE);
> > --
> > 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: Instrument PSR parameter for possible quirks with link standby.

[Paulo Zanoni]

2015-12-11 6:49 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> Link standby support has been deprecated with 'commit 89251b177
> ("drm/i915: PSR: deprecate link_standby support for core platforms.")'
>
> The reason for that is that main link in full off offers more power
> savings and some platforms implementations on source side had known
> bugs with link standby.

I also read that for link_standby to work we need a workaround that
involves single frame update support, but that's impossible on Haswell
and would require 3 additional workarounds on Broadwell, which I'm
assuming we don't implement yet. So why are we bringing this back if
we know it won't work?

>
> However we don't know all panels out there and we don't fully rely
> on the VBT information after the case found with the commit that
> made us to deprecate link standby.

Well, we kinda rely on the VBT. From what I see inside
intel_psr_match_conditions(), if the VBT requests link standby mode
and we're not VLV/CHV, we disable PSR (even if the user passes
i915.enable_psr=2, which sounds like another problem).

>
> So, before enable PSR by default let's instrument the PSR parameter
> in a way that we can identify different panels out there that might
> require or work better with link standby mode.
>
> It is also useful to say that for backward compatibility I'm not
> changing the meaning of this flag. So "0" still means disabled
> and "1" means enabled with full support and maximum power savings.
>
> v2: Use positive value instead of negative for different operation mode
>     as suggested by Daniel.
>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |  5 +++++
>  drivers/gpu/drm/i915/i915_drv.h     |  1 +
>  drivers/gpu/drm/i915/i915_params.c  |  7 ++++++-
>  drivers/gpu/drm/i915/intel_psr.c    | 13 ++++++++++++-
>  4 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 24318b7..efe973b 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2567,6 +2567,10 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>                                 enabled = true;
>                 }
>         }
> +
> +       seq_printf(m, "Forcing main link standby: %s\n",
> +                  yesno(dev_priv->psr.link_standby));
> +
>         seq_printf(m, "HW Enabled & Active bit: %s", yesno(enabled));
>
>         if (!HAS_DDI(dev))
> @@ -2587,6 +2591,7 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>
>                 seq_printf(m, "Performance_Counter: %u\n", psrperf);
>         }
> +

Bad chunk.


>         mutex_unlock(&dev_priv->psr.lock);
>
>         intel_runtime_pm_put(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5edd393..de086f0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -969,6 +969,7 @@ struct i915_psr {
>         unsigned busy_frontbuffer_bits;
>         bool psr2_support;
>         bool aux_frame_sync;
> +       bool link_standby;
>  };
>
>  enum intel_pch {
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 835d609..6dd39f0 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -126,7 +126,12 @@ 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 "
> +                "(0=disabled [default], 1=link-off maximum power-savings, 2=link-standby mode)"

It's not clear which value should be tried by the user in case he
wants to try PSR, but I don't think this matters, right?

Also, on VLV/CHV, i915.enable_psr=1 will still use link standby mode
(check vlv_psr_enable_sink()). So if we keep this patch, maybe we
should do:
0 = disabled
1 = enabled (let the Kernel choose the link mode)
2 = force link off
3 = force link standby

Although I'm not sure how we're supposed to proceed in case someone
sets i915.enable_psr=2 on VLV with my suggestion.


> +                "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.");
>
>  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 9ccff30..bcc85fd 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -225,7 +225,12 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
>                    (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT));
>         }
>
> -       drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, DP_PSR_ENABLE);
> +       if (dev_priv->psr.link_standby)
> +               drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
> +                                  DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE);
> +       else
> +               drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
> +                                  DP_PSR_ENABLE);
>  }
>
>  static void vlv_psr_enable_source(struct intel_dp *intel_dp)
> @@ -280,6 +285,9 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
>         if (IS_HASWELL(dev))
>                 val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
>
> +       if (dev_priv->psr.link_standby)
> +               val |= EDP_PSR_LINK_STANDBY;
> +
>         I915_WRITE(EDP_PSR_CTL, val |
>                    max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT |
>                    idle_frames << EDP_PSR_IDLE_FRAME_SHIFT |
> @@ -763,6 +771,9 @@ 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;
>
> +       if (i915.enable_psr == 2)
> +               dev_priv->psr.link_standby = true;
> +
>         INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work);
>         mutex_init(&dev_priv->psr.lock);
>  }
> --
> 2.4.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH] drm/i915: Instrument PSR parameter for possible quirks with link standby.

[Vivi, Rodrigo]

On Fri, 2015-12-11 at 17:55 -0200, Paulo Zanoni wrote:
> 2015-12-11 6:49 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> > Link standby support has been deprecated with 'commit 89251b177
> > ("drm/i915: PSR: deprecate link_standby support for core 
> > platforms.")'
> > 
> > The reason for that is that main link in full off offers more power
> > savings and some platforms implementations on source side had known
> > bugs with link standby.
> 
> I also read that for link_standby to work we need a workaround that
> involves single frame update support, but that's impossible on 
> Haswell
> and would require 3 additional workarounds on Broadwell, which I'm
> assuming we don't implement yet. So why are we bringing this back if
> we know it won't work?

I found a Skylake that although VBT doesn't tell anything about link
standby the machine works better on link standby.

But maybe if HSW || BDW we should just disable PSR at if link_standby
needed and if SKL we can force one or another.

> 
> > 
> > However we don't know all panels out there and we don't fully rely
> > on the VBT information after the case found with the commit that
> > made us to deprecate link standby.
> 
> Well, we kinda rely on the VBT. From what I see inside
> intel_psr_match_conditions(), if the VBT requests link standby mode
> and we're not VLV/CHV, we disable PSR (even if the user passes
> i915.enable_psr=2, which sounds like another problem).

Oh, I had forgotten about this one. Let'me re-work that in a way that
we continue relying VBT but provide a way to force one behaviour or
another.

> 
> > 
> > So, before enable PSR by default let's instrument the PSR parameter
> > in a way that we can identify different panels out there that might
> > require or work better with link standby mode.
> > 
> > It is also useful to say that for backward compatibility I'm not
> > changing the meaning of this flag. So "0" still means disabled
> > and "1" means enabled with full support and maximum power savings.
> > 
> > v2: Use positive value instead of negative for different operation 
> > mode
> >     as suggested by Daniel.
> > 
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c |  5 +++++
> >  drivers/gpu/drm/i915/i915_drv.h     |  1 +
> >  drivers/gpu/drm/i915/i915_params.c  |  7 ++++++-
> >  drivers/gpu/drm/i915/intel_psr.c    | 13 ++++++++++++-
> >  4 files changed, 24 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 24318b7..efe973b 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2567,6 +2567,10 @@ static int i915_edp_psr_status(struct 
> > seq_file *m, void *data)
> >                                 enabled = true;
> >                 }
> >         }
> > +
> > +       seq_printf(m, "Forcing main link standby: %s\n",
> > +                  yesno(dev_priv->psr.link_standby));
> > +
> >         seq_printf(m, "HW Enabled & Active bit: %s", 
> > yesno(enabled));
> > 
> >         if (!HAS_DDI(dev))
> > @@ -2587,6 +2591,7 @@ static int i915_edp_psr_status(struct 
> > seq_file *m, void *data)
> > 
> >                 seq_printf(m, "Performance_Counter: %u\n", 
> > psrperf);
> >         }
> > +
> 
> Bad chunk.

ops!

> 
> 
> >         mutex_unlock(&dev_priv->psr.lock);
> > 
> >         intel_runtime_pm_put(dev_priv);
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 5edd393..de086f0 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -969,6 +969,7 @@ struct i915_psr {
> >         unsigned busy_frontbuffer_bits;
> >         bool psr2_support;
> >         bool aux_frame_sync;
> > +       bool link_standby;
> >  };
> > 
> >  enum intel_pch {
> > diff --git a/drivers/gpu/drm/i915/i915_params.c 
> > b/drivers/gpu/drm/i915/i915_params.c
> > index 835d609..6dd39f0 100644
> > --- a/drivers/gpu/drm/i915/i915_params.c
> > +++ b/drivers/gpu/drm/i915/i915_params.c
> > @@ -126,7 +126,12 @@ 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 "
> > +                "(0=disabled [default], 1=link-off maximum power
> > -savings, 2=link-standby mode)"
> 
> It's not clear which value should be tried by the user in case he
> wants to try PSR, but I don't think this matters, right?
> 
> Also, on VLV/CHV, i915.enable_psr=1 will still use link standby mode
> (check vlv_psr_enable_sink()). So if we keep this patch, maybe we
> should do:
> 0 = disabled
> 1 = enabled (let the Kernel choose the link mode)
> 2 = force link off
> 3 = force link standby

Great idea! Thanks!

> Although I'm not sure how we're supposed to proceed in case someone
> sets i915.enable_psr=2 on VLV with my suggestion.

force what user wants anyway... we warn these are unsafe options ;)

> 
> 
> > +                "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.");
> > 
> >  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 9ccff30..bcc85fd 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -225,7 +225,12 @@ static void hsw_psr_enable_sink(struct 
> > intel_dp *intel_dp)
> >                    (aux_clock_divider << 
> > DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT));
> >         }
> > 
> > -       drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 
> > DP_PSR_ENABLE);
> > +       if (dev_priv->psr.link_standby)
> > +               drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
> > +                                  DP_PSR_ENABLE | 
> > DP_PSR_MAIN_LINK_ACTIVE);
> > +       else
> > +               drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
> > +                                  DP_PSR_ENABLE);
> >  }
> > 
> >  static void vlv_psr_enable_source(struct intel_dp *intel_dp)
> > @@ -280,6 +285,9 @@ static void hsw_psr_enable_source(struct 
> > intel_dp *intel_dp)
> >         if (IS_HASWELL(dev))
> >                 val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
> > 
> > +       if (dev_priv->psr.link_standby)
> > +               val |= EDP_PSR_LINK_STANDBY;
> > +
> >         I915_WRITE(EDP_PSR_CTL, val |
> >                    max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT |
> >                    idle_frames << EDP_PSR_IDLE_FRAME_SHIFT |
> > @@ -763,6 +771,9 @@ 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;
> > 
> > +       if (i915.enable_psr == 2)
> > +               dev_priv->psr.link_standby = true;
> > +
> >         INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work);
> >         mutex_init(&dev_priv->psr.lock);
> >  }
> > --
> > 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