[PATCH 0/3] PSR link standby x link force off changes.

[PATCH 0/3] PSR link standby x link force off changes.

[Rodrigo Vivi]

Durring last attempt to enable PSR we found one Skylake that required
link standby mode and on Skylake we don't have the link standby
issues we faced on HSW and BDW. So let's put link standby support back
and instrument i915.enable_psr to force link standby and link force off
for debug purposes.

More details about the changes are on individual commits.

I intend to send PSR enable by default soon as soon our QA gives the ok
it is working well, besides my own local tests here.

Thanks,
Rodrigo.

Rodrigo Vivi (3):
  drm/i915: PSR simplify port and link standby checks.
  drm/i915: Add PSR main link standby support back
  drm/i915: Instrument PSR parameter for debuging with link standby x
    link off.

 drivers/gpu/drm/i915/i915_debugfs.c |  4 +++
 drivers/gpu/drm/i915/i915_drv.h     |  1 +
 drivers/gpu/drm/i915/i915_params.c  |  3 ++-
 drivers/gpu/drm/i915/intel_psr.c    | 54 ++++++++++++++++++++++++++++++-------
 4 files changed, 52 insertions(+), 10 deletions(-)

-- 
2.4.3

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

[PATCH 1/3] drm/i915: PSR simplify port and link standby checks.

[Rodrigo Vivi]

Current code not just block link_standby for non DDI platforms but also
block PSR from work on other ports B/C/D/E.

So, besides change any behaviour let's just fix the mess a bit here and
reuse HSW check to block the other ports and reduce the second if only to
link stadnby request.

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

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 9ccff30..3c80b86 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -304,8 +304,15 @@ static bool intel_psr_match_conditions(struct intel_dp *intel_dp)
 
 	dev_priv->psr.source_ok = false;
 
-	if (IS_HASWELL(dev) && dig_port->port != PORT_A) {
-		DRM_DEBUG_KMS("HSW ties PSR to DDI A (eDP)\n");
+	/*
+	 * HSW spec explicitly says PSR is tied to port A.
+	 * BDW+ platforms with DDI implementation of PSR have different
+	 * PSR registers per transcoder and we only implement transcoder EDP
+	 * ones. Since by Display design transcoder EDP is tied to port A
+	 * we can safely escape based on the port A.
+	 */
+	if (HAS_DDI(dev) && dig_port->port != PORT_A) {
+		DRM_DEBUG_KMS("PSR condition failed: Port not supported\n");
 		return false;
 	}
 
@@ -328,7 +335,7 @@ static bool intel_psr_match_conditions(struct intel_dp *intel_dp)
 	}
 
 	if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev) &&
-	    ((dev_priv->vbt.psr.full_link) || (dig_port->port != PORT_A))) {
+	    dev_priv->vbt.psr.full_link) {
 		DRM_DEBUG_KMS("PSR condition failed: Link Standby requested/needed but not supported on this platform\n");
 		return false;
 	}
-- 
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: Add PSR main link standby support back

[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 on HSW and BDW implementations on source side had known
bugs with link standby.

However that same HSD report only mentions BDW and HSW and tells that
a fix was going to new platforms. Since on Skylake link standby
didn't cause the bad blank flickering screens seen on HSW and BDW
let's respect VBT again for this and future platforms.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c |  4 ++++
 drivers/gpu/drm/i915/i915_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_psr.c    | 26 +++++++++++++++++++-------
 3 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 863012a..ec0c2a05e 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2583,6 +2583,10 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 				enabled = true;
 		}
 	}
+
+	seq_printf(m, "Main link in standby mode: %s\n",
+		   yesno(dev_priv->psr.link_standby));
+
 	seq_printf(m, "HW Enabled & Active bit: %s", yesno(enabled));
 
 	if (!HAS_DDI(dev))
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c921ad8..65a2cd0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1007,6 +1007,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/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 3c80b86..b99a105 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 |
@@ -334,12 +342,6 @@ static bool intel_psr_match_conditions(struct intel_dp *intel_dp)
 		return false;
 	}
 
-	if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev) &&
-	    dev_priv->vbt.psr.full_link) {
-		DRM_DEBUG_KMS("PSR condition failed: Link Standby requested/needed but not supported on this platform\n");
-		return false;
-	}
-
 	dev_priv->psr.source_ok = true;
 	return true;
 }
@@ -770,6 +772,16 @@ 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 (IS_HASWELL(dev) || IS_BROADWELL(dev))
+		/* HSW and BDW require workarounds that we don't implement. */
+		dev_priv->psr.link_standby = false;
+	else if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
+		/* On VLV and CHV only standby mode is supported. */
+		dev_priv->psr.link_standby = true;
+	else
+		/* For new platforms let's respect VBT back again */
+		dev_priv->psr.link_standby = dev_priv->vbt.psr.full_link;
+
 	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: Instrument PSR parameter for debuging with link standby x link off.

[Rodrigo Vivi]

Unfortunately we don't know all panels and platforms out there and we
found internal prototypes without VBT proper set but where only
link in standby worked well.

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.

v3: As Paulo suggested use 2 to force link standby and 3 to force link
    fully on. Also split the link_standby introduction in a separated patch.

v4: Use DRM_ERROR for link off request on platfors that don't support and
    Remove the quirk promisse.

Cc: Jani Nikula <jani.nikula@intel.com>
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_params.c |  3 ++-
 drivers/gpu/drm/i915/intel_psr.c   | 17 +++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 8d90c25..8b9f368 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -127,7 +127,8 @@ 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=enabled - link mode chosen per-platform, 2=force link-standby mode, 3=force link-off mode)");
 
 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 b99a105..4ab7579 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -329,6 +329,12 @@ static bool intel_psr_match_conditions(struct intel_dp *intel_dp)
 		return false;
 	}
 
+	if ((IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) &&
+	    !dev_priv->psr.link_standby) {
+		DRM_ERROR("PSR condition failed: Link off requested but not supported on this platform\n");
+		return false;
+	}
+
 	if (IS_HASWELL(dev) &&
 	    I915_READ(HSW_STEREO_3D_CTL(intel_crtc->config->cpu_transcoder)) &
 		      S3D_ENABLE) {
@@ -772,6 +778,7 @@ 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;
 
+	/* Set link_standby x link_off defaults */
 	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 		/* HSW and BDW require workarounds that we don't implement. */
 		dev_priv->psr.link_standby = false;
@@ -782,6 +789,16 @@ void intel_psr_init(struct drm_device *dev)
 		/* For new platforms let's respect VBT back again */
 		dev_priv->psr.link_standby = dev_priv->vbt.psr.full_link;
 
+	/* Override link_standby x link_off defaults */
+	if (i915.enable_psr == 2 && !dev_priv->psr.link_standby) {
+		DRM_DEBUG_KMS("PSR: Forcing link standby\n");
+		dev_priv->psr.link_standby = true;
+	}
+	if (i915.enable_psr == 3 && dev_priv->psr.link_standby) {
+		DRM_DEBUG_KMS("PSR: Forcing main link off\n");
+		dev_priv->psr.link_standby = false;
+	}
+
 	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

Re: [PATCH 3/3] drm/i915: Instrument PSR parameter for debuging with link standby x link off.

[Zanoni, Paulo R]

Em Seg, 2016-02-01 às 10:49 -0800, Rodrigo Vivi escreveu:
> Unfortunately we don't know all panels and platforms out there and we
> found internal prototypes without VBT proper set but where only
> link in standby worked well.
> 
> 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.
> 
> v3: As Paulo suggested use 2 to force link standby and 3 to force
> link
>     fully on. Also split the link_standby introduction in a separated
> patch.
> 
> v4: Use DRM_ERROR for link off request on platfors that don't support
> and
>     Remove the quirk promisse.

"platforms" and "promise"

For the 3 patches:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> 
> Cc: Jani Nikula <jani.nikula@intel.com>
> 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_params.c |  3 ++-
>  drivers/gpu/drm/i915/intel_psr.c   | 17 +++++++++++++++++
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_params.c
> b/drivers/gpu/drm/i915/i915_params.c
> index 8d90c25..8b9f368 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -127,7 +127,8 @@ 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=enabled - link mode
> chosen per-platform, 2=force link-standby mode, 3=force link-off
> mode)");
>  
>  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 b99a105..4ab7579 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -329,6 +329,12 @@ static bool intel_psr_match_conditions(struct
> intel_dp *intel_dp)
>  		return false;
>  	}
>  
> +	if ((IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) &&
> +	    !dev_priv->psr.link_standby) {
> +		DRM_ERROR("PSR condition failed: Link off requested
> but not supported on this platform\n");
> +		return false;
> +	}
> +
>  	if (IS_HASWELL(dev) &&
>  	    I915_READ(HSW_STEREO_3D_CTL(intel_crtc->config-
> >cpu_transcoder)) &
>  		      S3D_ENABLE) {
> @@ -772,6 +778,7 @@ 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;
>  
> +	/* Set link_standby x link_off defaults */
>  	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>  		/* HSW and BDW require workarounds that we don't
> implement. */
>  		dev_priv->psr.link_standby = false;
> @@ -782,6 +789,16 @@ void intel_psr_init(struct drm_device *dev)
>  		/* For new platforms let's respect VBT back again */
>  		dev_priv->psr.link_standby = dev_priv-
> >vbt.psr.full_link;
>  
> +	/* Override link_standby x link_off defaults */
> +	if (i915.enable_psr == 2 && !dev_priv->psr.link_standby) {
> +		DRM_DEBUG_KMS("PSR: Forcing link standby\n");
> +		dev_priv->psr.link_standby = true;
> +	}
> +	if (i915.enable_psr == 3 && dev_priv->psr.link_standby) {
> +		DRM_DEBUG_KMS("PSR: Forcing main link off\n");
> +		dev_priv->psr.link_standby = false;
> +	}
> +
>  	INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work);
>  	mutex_init(&dev_priv->psr.lock);
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for PSR link standby x link force off changes.

[Patchwork]

== Summary ==

Series 2991v1 PSR link standby x link force off changes.
2016-02-01T18:49:22.338593 http://patchwork.freedesktop.org/api/1.0/series/2991/revisions/1/mbox/
Applying: drm/i915: PSR simplify port and link standby checks.
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/intel_psr.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/intel_psr.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/intel_psr.c
Patch failed at 0001 drm/i915: PSR simplify port and link standby checks.

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