[PATCH 1/4] drm/i915: PSR Fix standby logic for PSR on non DDI-A for certain platforms.

[PATCH 1/4] drm/i915: PSR Fix standby logic for PSR on non DDI-A for certain platforms.

[Rodrigo Vivi]

Current platforms that support PSR on other port than A only support link
standby mode.

The logic here was wrong since 'commit 89251b177b ("drm/i915: PSR:
deprecate link_standby support for core platforms.")

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

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 9ccff30..4a9c062 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -327,9 +327,9 @@ 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) || (dig_port->port != PORT_A))) {
-		DRM_DEBUG_KMS("PSR condition failed: Link Standby requested/needed but not supported on this platform\n");
+	if (HAS_DDI(dev) && !dev_priv->vbt.psr.full_link &&
+	    dig_port->port != PORT_A) {
+		DRM_DEBUG_KMS("PSR condition failed: Link Off requested/needed but not supported on this port\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/4] 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 I got a SKL working really
well with link in standby mode let's respect VBT again for this
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    | 27 +++++++++++++++++++++++++--
 3 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 24318b7..c15434b 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, "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 9124085..4b4ae3a 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/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 4a9c062..b84ec80 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 |
@@ -327,7 +335,7 @@ static bool intel_psr_match_conditions(struct intel_dp *intel_dp)
 		return false;
 	}
 
-	if (HAS_DDI(dev) && !dev_priv->vbt.psr.full_link &&
+	if (HAS_DDI(dev) && !dev_priv->psr.link_standby &&
 	    dig_port->port != PORT_A) {
 		DRM_DEBUG_KMS("PSR condition failed: Link Off requested/needed but not supported on this port\n");
 		return false;
@@ -763,6 +771,21 @@ 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))
+		/*
+		 * On HSW and BDW Source implementation as an issue with PSR
+		 * exit that requires an workaround that doesn't exist on HSW
+		 * and that on BDW requires single frame update that we don't
+		 * implement because it needs 3 or 4 extra workarounds.
+		 */
+		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/4] drm/i915: Instrument PSR parameter for possible quirks with link standby.

[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.

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 |  7 ++++++-
 drivers/gpu/drm/i915/intel_psr.c   | 17 +++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 835d609..f78ddf3 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=enabled - link mode chosen per-platform, 2=force link-standby mode, 3=force link-off mode)"
+		 "In case you needed to force any different option, 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 b84ec80..c3c2bb8 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -335,6 +335,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_DEBUG_KMS("PSR condition failed: Link off requested/needed but not supported on this platform\n");
+		return false;
+	}
+
 	if (HAS_DDI(dev) && !dev_priv->psr.link_standby &&
 	    dig_port->port != PORT_A) {
 		DRM_DEBUG_KMS("PSR condition failed: Link Off requested/needed but not supported on this port\n");
@@ -771,6 +777,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))
 		/*
 		 * On HSW and BDW Source implementation as an issue with PSR
@@ -786,6 +793,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

[PATCH 4/4] 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
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, 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.
v4: Rebase after changing previous patch.

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 f78ddf3..2730435 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=enabled - link mode chosen per-platform, 2=force link-standby mode, 3=force link-off mode)"
+		 "(0=disabled, 1=enabled [default] - link mode chosen per-platform, 2=force link-standby mode, 3=force link-off mode)"
 		 "In case you needed to force any different option, 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 1/4] drm/i915: PSR Fix standby logic for PSR on non DDI-A for certain platforms.

[Zanoni, Paulo R]

Em Sex, 2015-12-11 às 08:39 -0800, Rodrigo Vivi escreveu:
> Current platforms that support PSR on other port than A only support
> link
> standby mode.
> 
> The logic here was wrong since 'commit 89251b177b ("drm/i915: PSR:
> deprecate link_standby support for core platforms.")

What's wrong/broken with the logic?

> 
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 9ccff30..4a9c062 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -327,9 +327,9 @@ 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) || (dig_port->port !=
> PORT_A))) {
> -		DRM_DEBUG_KMS("PSR condition failed: Link Standby
> requested/needed but not supported on this platform\n");
> +	if (HAS_DDI(dev) && !dev_priv->vbt.psr.full_link &&
> +	    dig_port->port != PORT_A) {
> +		DRM_DEBUG_KMS("PSR condition failed: Link Off
> requested/needed but not supported on this port\n");
>  		return false;

I spent a long time looking at this patch, trying to understand what
exactly changed. You didn't really mention which specific case was
wrong and why. Please explicitly mention the case and platform, also
replacing "certain platforms" for something more precise.

The code mentions "full_link", "link standby" and "link off", but
there's nothing clarifying the relationship between them. It seems
"full_link" means "link standby", but I suppose we could put a comment
somewhere clarifying this. We previously had dev_priv-
>psr.link_standby, but we don't have this anymore.

It seems that with the previous code we only supported link off on port
A, nothing else. This is reflected both by the check you're changing
and by hsw_psr_enable_source(). Now we're only going to reject link off
for ports B/C/D/E. This means that this patch is both enabling ports
B/C/D/E _and_ enabling link standby support on port A. Maybe we could
do this through two separate patches: one for the port A new feature,
the other for the new ports. This means:
 - We're now accepting link standby on every port, but we don't have
the code to set link standby on port A.
 - We're now accepting port B/C/D/E on DDI platforms, but our code
currently hardcodes PSR to the register associated with transcoder eDP.

Also, according to 89251b177b58, we don't want link standby for
HSW/BDW, but we're accepting this now. So this patch is a half-revert
of that commit.

By the way, the spec talks in terms of transcoders and our code talks
in terms of ports. I know there was some direct relationship between
them at some point, but as we add more platforms, we increase the
chance that our implicit assertions between the relationship of
transcoders and ports may not be valid anymore. So please either add
some assertions, or some huge comment, or just change the code to check
transcoders instead of ports.

To conclude: I still can't see what's wrong with the current code.


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

Re: [PATCH 2/4] drm/i915: Add PSR main link standby support back

[Zanoni, Paulo R]

Em Sex, 2015-12-11 às 08:39 -0800, Rodrigo Vivi escreveu:
> 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 I got a SKL working really
> well 

Please define what "working really well" means, especially when
comparing against the full-off mode that's supposed to be even better
:)


> with link in standby mode let's respect VBT again for this

I see this patch fixes some of the issues I pointed in the review of
patch 1/4. This patch should definitely be applied _before_ the patch
where we tell intel_psr_match_conditions() to start accepting link
standby mode.

> 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    | 27 +++++++++++++++++++++++++--
>  3 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 24318b7..c15434b 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, "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 9124085..4b4ae3a 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/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 4a9c062..b84ec80 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 |
> @@ -327,7 +335,7 @@ static bool intel_psr_match_conditions(struct
> intel_dp *intel_dp)
>  		return false;
>  	}
>  
> -	if (HAS_DDI(dev) && !dev_priv->vbt.psr.full_link &&
> +	if (HAS_DDI(dev) && !dev_priv->psr.link_standby &&

Since I think this patch should come before patch 1, then I suppose
you'll need to rebase this chunk.


>  	    dig_port->port != PORT_A) {
>  		DRM_DEBUG_KMS("PSR condition failed: Link Off
> requested/needed but not supported on this port\n");
>  		return false;
> @@ -763,6 +771,21 @@ 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))
> +		/*
> +		 * On HSW and BDW Source implementation as an issue
> with PSR
> +		 * exit that requires an workaround that doesn't
> exist on HSW
> +		 * and that on BDW requires single frame update that
> we don't
> +		 * implement because it needs 3 or 4 extra
> workarounds.
> +		 */

Bikeshed: since we don't mention the exact problems anywhere, we could
just go with something like "HSW and BDW require workarounds that we
don't implement".

Besides the patch ordering issue, everything else looks correct.

> +		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);
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Zanoni, Paulo R]

Em Sex, 2015-12-11 às 08:39 -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.
> 
> 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 |  7 ++++++-
>  drivers/gpu/drm/i915/intel_psr.c   | 17 +++++++++++++++++
>  2 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_params.c
> b/drivers/gpu/drm/i915/i915_params.c
> index 835d609..f78ddf3 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=enabled - link mode
> chosen per-platform, 2=force link-standby mode, 3=force link-off
> mode)"
> +		 "In case you needed to force any different option,
> 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.");

Are we making a promise here? Isn't that dangerous? :P
I'd just tell the users to open bug reports.
(I'm not requiring you to change anything here, but something something
lawyers something)

>  
>  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 b84ec80..c3c2bb8 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -335,6 +335,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) {

s/dev_priv->psr.link_standby/!dev_priv->psr.link_standby/


Also, I'm not sure if this chunk belongs here or at intel_psr_init(),
since it effectively disables PSR. This means that i915.enable_psr=3
disables PSR on VLV/CHV. But maybe we shouldn't care since users
shouldn't be using the option anyway. On the other hand, users may
start claiming that i915.enable_psr=X "fixed PSR" for them while
effectively it just disabled PSR, so perhaps DRM_ERROR would be better.
Anyway, I'm not requesting any change, just pointing things in case you
or someone else has any idea, but maybe I'd go with DRM_ERROR since
users usually don't know which platform supports what, so the loud
message may help them.

Another check which we seem to be missing is "if (HAS_DDI(dev_priv) &&
transcoder != TRANSCODER_EDP && !dev_priv->psr.link_standby)", but this
depends on the result of the discussion of patch 1.

Everything else looks good, but it would be nice to see the opinions of
maintainers here since they always have something to say about new
i915.ko options.


> +		DRM_DEBUG_KMS("PSR condition failed: Link off
> requested/needed but not supported on this platform\n");
> +		return false;
> +	}
> +
>  	if (HAS_DDI(dev) && !dev_priv->psr.link_standby &&
>  	    dig_port->port != PORT_A) {
>  		DRM_DEBUG_KMS("PSR condition failed: Link Off
> requested/needed but not supported on this port\n");
> @@ -771,6 +777,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))
>  		/*
>  		 * On HSW and BDW Source implementation as an issue
> with PSR
> @@ -786,6 +793,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

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

[Thulasimani, Sivakumar]

On 1/20/2016 10:32 PM, Zanoni, Paulo R wrote:
> Em Sex, 2015-12-11 às 08:39 -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.
:) if it is internal i assume someone has to set the vbt ,we encountered 
an issue
sometime back that blamed vbt as incorrect only to later learn that
the person who created the setup didn't care to configure the VBT.
>>
>> 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.
>>
>> 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 |  7 ++++++-
>>   drivers/gpu/drm/i915/intel_psr.c   | 17 +++++++++++++++++
>>   2 files changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_params.c
>> b/drivers/gpu/drm/i915/i915_params.c
>> index 835d609..f78ddf3 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=enabled - link mode
>> chosen per-platform, 2=force link-standby mode, 3=force link-off
>> mode)"
>> +		 "In case you needed to force any different option,
>> 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.");
> Are we making a promise here? Isn't that dangerous? :P
> I'd just tell the users to open bug reports.
> (I'm not requiring you to change anything here, but something something
> lawyers something)
>
>>   
>>   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 b84ec80..c3c2bb8 100644
>> --- a/drivers/gpu/drm/i915/intel_psr.c
>> +++ b/drivers/gpu/drm/i915/intel_psr.c
>> @@ -335,6 +335,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) {
IS_VALLEYVIEW() will return true for both valleyview and cherryview so
the above check for cherryview can be removed.
> s/dev_priv->psr.link_standby/!dev_priv->psr.link_standby/
>
>
> Also, I'm not sure if this chunk belongs here or at intel_psr_init(),
> since it effectively disables PSR. This means that i915.enable_psr=3
> disables PSR on VLV/CHV. But maybe we shouldn't care since users
> shouldn't be using the option anyway. On the other hand, users may
> start claiming that i915.enable_psr=X "fixed PSR" for them while
> effectively it just disabled PSR, so perhaps DRM_ERROR would be better.
> Anyway, I'm not requesting any change, just pointing things in case you
> or someone else has any idea, but maybe I'd go with DRM_ERROR since
> users usually don't know which platform supports what, so the loud
> message may help them.
i agree, psr_match_conditions should check for parameters that can change
dynamically post boot to decide if we can enable psr or not,
if link_standby cannot be changed post boot we should check for it in init
  so we can avoid psr being enabled in the first place.
> Another check which we seem to be missing is "if (HAS_DDI(dev_priv) &&
> transcoder != TRANSCODER_EDP && !dev_priv->psr.link_standby)", but this
> depends on the result of the discussion of patch 1.
>
> Everything else looks good, but it would be nice to see the opinions of
> maintainers here since they always have something to say about new
> i915.ko options.
>
>
>> +		DRM_DEBUG_KMS("PSR condition failed: Link off
>> requested/needed but not supported on this platform\n");
>> +		return false;
>> +	}
>> +
sorry i came late to this thread, but can you point me to some issues for
link off in CHT/VLV ? we have enabled link off in CHT Android and it seems
to be working fine. we can check again if we have missed something.
>>   	if (HAS_DDI(dev) && !dev_priv->psr.link_standby &&
>>   	    dig_port->port != PORT_A) {
>>   		DRM_DEBUG_KMS("PSR condition failed: Link Off
>> requested/needed but not supported on this port\n");
>> @@ -771,6 +777,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))
>>   		/*
>>   		 * On HSW and BDW Source implementation as an issue
>> with PSR
>> @@ -786,6 +793,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

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

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

[Zanoni, Paulo R]

Em Qui, 2016-01-21 às 08:35 +0530, Thulasimani, Sivakumar escreveu:
> 
> On 1/20/2016 10:32 PM, Zanoni, Paulo R wrote:
> > Em Sex, 2015-12-11 às 08:39 -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.
> :) if it is internal i assume someone has to set the vbt ,we
> encountered 
> an issue
> sometime back that blamed vbt as incorrect only to later learn that
> the person who created the setup didn't care to configure the VBT.

But in order to discover if the VBT is incorrect, the parameter can be
used.

> > > 
> > > 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.
> > > 
> > > 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 |  7 ++++++-
> > >   drivers/gpu/drm/i915/intel_psr.c   | 17 +++++++++++++++++
> > >   2 files changed, 23 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_params.c
> > > b/drivers/gpu/drm/i915/i915_params.c
> > > index 835d609..f78ddf3 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=enabled - link mode
> > > chosen per-platform, 2=force link-standby mode, 3=force link-off
> > > mode)"
> > > +		 "In case you needed to force any different
> > > option,
> > > 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.");
> > Are we making a promise here? Isn't that dangerous? :P
> > I'd just tell the users to open bug reports.
> > (I'm not requiring you to change anything here, but something
> > something
> > lawyers something)
> > 
> > >   
> > >   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 b84ec80..c3c2bb8 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -335,6 +335,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) {
> IS_VALLEYVIEW() will return true for both valleyview and cherryview
> so
> the above check for cherryview can be removed.

Not anymore:
commit 666a45379e2c29bc16e60648e5ad8f6f8b7fa6ce  
Author: Wayne Boyer <wayne.boyer@intel.com>      
Date:   Wed Dec 9 12:29:35 2015 -0800            
                                                 
    drm/i915: Separate cherryview from valleyview

> > s/dev_priv->psr.link_standby/!dev_priv->psr.link_standby/
> > 
> > 
> > Also, I'm not sure if this chunk belongs here or at
> > intel_psr_init(),
> > since it effectively disables PSR. This means that
> > i915.enable_psr=3
> > disables PSR on VLV/CHV. But maybe we shouldn't care since users
> > shouldn't be using the option anyway. On the other hand, users may
> > start claiming that i915.enable_psr=X "fixed PSR" for them while
> > effectively it just disabled PSR, so perhaps DRM_ERROR would be
> > better.
> > Anyway, I'm not requesting any change, just pointing things in case
> > you
> > or someone else has any idea, but maybe I'd go with DRM_ERROR since
> > users usually don't know which platform supports what, so the loud
> > message may help them.
> i agree, psr_match_conditions should check for parameters that can
> change
> dynamically post boot to decide if we can enable psr or not,
> if link_standby cannot be changed post boot we should check for it in
> init
>   so we can avoid psr being enabled in the first place.
> > Another check which we seem to be missing is "if (HAS_DDI(dev_priv)
> > &&
> > transcoder != TRANSCODER_EDP && !dev_priv->psr.link_standby)", but
> > this
> > depends on the result of the discussion of patch 1.
> > 
> > Everything else looks good, but it would be nice to see the
> > opinions of
> > maintainers here since they always have something to say about new
> > i915.ko options.
> > 
> > 
> > > +		DRM_DEBUG_KMS("PSR condition failed: Link off
> > > requested/needed but not supported on this platform\n");
> > > +		return false;
> > > +	}
> > > +
> sorry i came late to this thread, but can you point me to some issues
> for
> link off in CHT/VLV ? we have enabled link off in CHT Android and it
> seems
> to be working fine. we can check again if we have missed something.

The issue is that the upstream Kernel does not appear to support it,
since vlv_psr_enable_sink() always sets DP_PSR_MAIN_LINK_ACTIVE. But I
didn't check if VLV_PSRCTL is properly setting link active too.


> > >   	if (HAS_DDI(dev) && !dev_priv->psr.link_standby &&
> > >   	    dig_port->port != PORT_A) {
> > >   		DRM_DEBUG_KMS("PSR condition failed: Link Off
> > > requested/needed but not supported on this port\n");
> > > @@ -771,6 +777,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))
> > >   		/*
> > >   		 * On HSW and BDW Source implementation as an
> > > issue
> > > with PSR
> > > @@ -786,6 +793,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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Thulasimani, Sivakumar]

On 1/21/2016 5:26 PM, Zanoni, Paulo R wrote:
> Em Qui, 2016-01-21 às 08:35 +0530, Thulasimani, Sivakumar escreveu:
>> On 1/20/2016 10:32 PM, Zanoni, Paulo R wrote:
>>> Em Sex, 2015-12-11 às 08:39 -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.
>> :) if it is internal i assume someone has to set the vbt ,we
>> encountered
>> an issue
>> sometime back that blamed vbt as incorrect only to later learn that
>> the person who created the setup didn't care to configure the VBT.
> But in order to discover if the VBT is incorrect, the parameter can be
> used.
>
>>>> 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.
>>>>
>>>> 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 |  7 ++++++-
>>>>    drivers/gpu/drm/i915/intel_psr.c   | 17 +++++++++++++++++
>>>>    2 files changed, 23 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_params.c
>>>> b/drivers/gpu/drm/i915/i915_params.c
>>>> index 835d609..f78ddf3 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=enabled - link mode
>>>> chosen per-platform, 2=force link-standby mode, 3=force link-off
>>>> mode)"
>>>> +		 "In case you needed to force any different
>>>> option,
>>>> 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.");
>>> Are we making a promise here? Isn't that dangerous? :P
>>> I'd just tell the users to open bug reports.
>>> (I'm not requiring you to change anything here, but something
>>> something
>>> lawyers something)
>>>
>>>>    
>>>>    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 b84ec80..c3c2bb8 100644
>>>> --- a/drivers/gpu/drm/i915/intel_psr.c
>>>> +++ b/drivers/gpu/drm/i915/intel_psr.c
>>>> @@ -335,6 +335,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) {
>> IS_VALLEYVIEW() will return true for both valleyview and cherryview
>> so
>> the above check for cherryview can be removed.
> Not anymore:
> commit 666a45379e2c29bc16e60648e5ad8f6f8b7fa6ce
> Author: Wayne Boyer <wayne.boyer@intel.com>
> Date:   Wed Dec 9 12:29:35 2015 -0800
>                                                   
>      drm/i915: Separate cherryview from valleyview
Thanks for commit id of patch :). jumping on upstream every
now and then will lead to missing a lot of changes happening.
>
>>> s/dev_priv->psr.link_standby/!dev_priv->psr.link_standby/
>>>
>>>
>>> Also, I'm not sure if this chunk belongs here or at
>>> intel_psr_init(),
>>> since it effectively disables PSR. This means that
>>> i915.enable_psr=3
>>> disables PSR on VLV/CHV. But maybe we shouldn't care since users
>>> shouldn't be using the option anyway. On the other hand, users may
>>> start claiming that i915.enable_psr=X "fixed PSR" for them while
>>> effectively it just disabled PSR, so perhaps DRM_ERROR would be
>>> better.
>>> Anyway, I'm not requesting any change, just pointing things in case
>>> you
>>> or someone else has any idea, but maybe I'd go with DRM_ERROR since
>>> users usually don't know which platform supports what, so the loud
>>> message may help them.
>> i agree, psr_match_conditions should check for parameters that can
>> change
>> dynamically post boot to decide if we can enable psr or not,
>> if link_standby cannot be changed post boot we should check for it in
>> init
>>    so we can avoid psr being enabled in the first place.
>>> Another check which we seem to be missing is "if (HAS_DDI(dev_priv)
>>> &&
>>> transcoder != TRANSCODER_EDP && !dev_priv->psr.link_standby)", but
>>> this
>>> depends on the result of the discussion of patch 1.
>>>
>>> Everything else looks good, but it would be nice to see the
>>> opinions of
>>> maintainers here since they always have something to say about new
>>> i915.ko options.
>>>
>>>
>>>> +		DRM_DEBUG_KMS("PSR condition failed: Link off
>>>> requested/needed but not supported on this platform\n");
>>>> +		return false;
>>>> +	}
>>>> +
>> sorry i came late to this thread, but can you point me to some issues
>> for
>> link off in CHT/VLV ? we have enabled link off in CHT Android and it
>> seems
>> to be working fine. we can check again if we have missed something.
> The issue is that the upstream Kernel does not appear to support it,
> since vlv_psr_enable_sink() always sets DP_PSR_MAIN_LINK_ACTIVE. But I
> didn't check if VLV_PSRCTL is properly setting link active too.
>
We too initially failed to get link off working, the main culprit being 
link training.
Our LT is too slow (~10ms) to bring up the panel from link off. we had 
to write an
optimized version of LT that will train the link back again. ideally the 
code
to reuse last trained values is similar to what we tried (try EQ 
directly since we
expect both CR & EQ to be passing with those values)  it should help in 
allowing
link off on CHV & VLV.
>>>>    	if (HAS_DDI(dev) && !dev_priv->psr.link_standby &&
>>>>    	    dig_port->port != PORT_A) {
>>>>    		DRM_DEBUG_KMS("PSR condition failed: Link Off
>>>> requested/needed but not supported on this port\n");
>>>> @@ -771,6 +777,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))
>>>>    		/*
>>>>    		 * On HSW and BDW Source implementation as an
>>>> issue
>>>> with PSR
>>>> @@ -786,6 +793,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

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

Re: [PATCH 1/4] drm/i915: PSR Fix standby logic for PSR on non DDI-A for certain platforms.

[Vivi, Rodrigo]

On Wed, 2016-01-20 at 14:11 +0000, Zanoni, Paulo R wrote:
> Em Sex, 2015-12-11 às 08:39 -0800, Rodrigo Vivi escreveu:
> > Current platforms that support PSR on other port than A only 
> > support
> > link
> > standby mode.
> > 
> > The logic here was wrong since 'commit 89251b177b ("drm/i915: PSR:
> > deprecate link_standby support for core platforms.")
> 
> What's wrong/broken with the logic?

If we request link_standby via VBT and we aren't on TransEDP-at-DDI-A
we are nowadays avoiding PSR

With this patch if full_link disabled (== link off == main link off ==
not in link standby mode) and we are not using DDIA than we block PSR
and lets get PSR on the links_standby on non portA that we are
currently blocking...

> 
> > 
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 9ccff30..4a9c062 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -327,9 +327,9 @@ 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) || (dig_port->port !=
> > PORT_A))) {
> > -		DRM_DEBUG_KMS("PSR condition failed: Link Standby
> > requested/needed but not supported on this platform\n");
> > +	if (HAS_DDI(dev) && !dev_priv->vbt.psr.full_link &&
> > +	    dig_port->port != PORT_A) {
> > +		DRM_DEBUG_KMS("PSR condition failed: Link Off
> > requested/needed but not supported on this port\n");
> >  		return false;
> 
> I spent a long time looking at this patch, trying to understand what
> exactly changed. You didn't really mention which specific case was
> wrong and why. Please explicitly mention the case and platform, also
> replacing "certain platforms" for something more precise.

Your comments made me to keep looking to my own patch a long time as
well trying to understand what I had done... 

> 
> The code mentions "full_link", "link standby" and "link off", but
> there's nothing clarifying the relationship between them. It seems
> "full_link" means "link standby", but I suppose we could put a 
> comment
> somewhere clarifying this. We previously had dev_priv-
> > psr.link_standby, but we don't have this anymore.

yeah, this is indeed confusing names and also was the reason that I
staid a long time also confused by my patch.
I will try to explain a bit here and put this comment somewhere in the
code and in the commit message.

We have 2 situations:
1 - Main/Full link needs to be enabled the whole time in stand by mode
to allow source-sink communication when exiting PSR.
2 - All links can be disabled, go off on PSR entry. 

This is optimal case for power savings, but depend on the panel
implementation it can fail badly. So if we identify this earlier during
product development OEMs can set this VBT bit to force the link
standby.

So, full_link == standby == link on
link-off == ! stand-by == ! full link

The confusion happens because VESA uses one way and our spec another
and VBT another.


> 
> It seems that with the previous code we only supported link off on 
> port
> A, nothing else. This is reflected both by the check you're changing
> and by hsw_psr_enable_source(). Now we're only going to reject link 
> off
> for ports B/C/D/E. 

nope the previous one support link off for ports B/C/D/E and skip on
standy, and this is exactly the issue.

> This means that this patch is both enabling ports
> B/C/D/E _and_ enabling link standby support on port A. Maybe we could
> do this through two separate patches: one for the port A new feature,
> the other for the new ports. This means:
>  - We're now accepting link standby on every port, but we don't have
> the code to set link standby on port A.

Well, this was already the case on BDW+... for HSW PSR is tied to DDI-A
according spec and this restriction was removed on BDW by adding the
IS_HASWELL(dev) to the pre-existing port != DDIA

>  - We're now accepting port B/C/D/E on DDI platforms, but our code
> currently hardcodes PSR to the register associated with transcoder 
> eDP.

Oh wait, you are absolutely right here! We do not block PSR on this
function, but we just use transcoder_EDP one...

Now I believe we should just remove this entire block and that
IS_HASWELL and get tied again to DDI_A and transcoder_EDP before a
rework that would make that really work. With a low priority since we
never really target platforms with 2 eDPs or eDP on other port than not
TRANS_EDP+DDIA.

What do you think?

> 
> Also, according to 89251b177b58, we don't want link standby for
> HSW/BDW, but we're accepting this now. So this patch is a half-revert
> of that commit.

not really... because the logic was inverted on that commit causing all
the confusion...

> 
> By the way, the spec talks in terms of transcoders and our code talks
> in terms of ports. I know there was some direct relationship between
> them at some point, but as we add more platforms, we increase the
> chance that our implicit assertions between the relationship of
> transcoders and ports may not be valid anymore. So please either add
> some assertions, or some huge comment, or just change the code to 
> check
> transcoders instead of ports.

well, initial specs were also talking in terms of trans_edp + DDI_A or
! DDI_A, etc. But based on registers divided by transcoder we would
need the full rework to allow multiple PSR in different transcoders...
Also another question is where would we get a machine like this to
validate?! 

So, let's fully tie it to transcoder_edp for now?

> 
> To conclude: I still can't see what's wrong with the current code.

I hope it is clear now, but we can talk more tomorrow about it on irc.

Thank you very much for all highlights here!

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

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

[Jani Nikula]

On Fri, 11 Dec 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> 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.
>
> 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 |  7 ++++++-
>  drivers/gpu/drm/i915/intel_psr.c   | 17 +++++++++++++++++
>  2 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 835d609..f78ddf3 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=enabled - link mode chosen per-platform, 2=force link-standby mode, 3=force link-off mode)"

Since this parameter is _unsafe, I'm fine with adding the alternatives.

> +		 "In case you needed to force any different option, 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.");

However I object to the implied plan of quirking this in the driver. I
do not think quirking should be perceived as a viable option for
anything. It's a maintainability nightmare.

BR,
Jani.



>  
>  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 b84ec80..c3c2bb8 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -335,6 +335,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_DEBUG_KMS("PSR condition failed: Link off requested/needed but not supported on this platform\n");
> +		return false;
> +	}
> +
>  	if (HAS_DDI(dev) && !dev_priv->psr.link_standby &&
>  	    dig_port->port != PORT_A) {
>  		DRM_DEBUG_KMS("PSR condition failed: Link Off requested/needed but not supported on this port\n");
> @@ -771,6 +777,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))
>  		/*
>  		 * On HSW and BDW Source implementation as an issue with PSR
> @@ -786,6 +793,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);
>  }

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