[PATCH 0/7] drm/i915: CHV DPIO power gating stuff

[PATCH 0/7] drm/i915: CHV DPIO power gating stuff

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

This series implementes DPIO lane power gating for CHV. In addition it
also removes the hack that links both common lane power wells together
so each one can be individually power gated.

There's also some stuff to deal with the DPIO_PHY_CONTROL register
corruption, which is a prerequisite for the actual power gating features
since they will need to frob that register.

I also tossed in the PHY lane stagger patch which has been sitting
around in my trees approximately forever.

And finally I went ahead and cleaned up some of the mess with the CHV
power well definitions which were added before I knew just how the
Punit power well stuff changed from VLV.

This series needs Clint's interpair skew issue fix [1] as otherwise
some lingering link training issues will become worse.

The series (and Clin't fix) is available in a git repo here:
git://github.com/vsyrjala/linux.git chv_dpio_powergating

[1] http://lists.freedesktop.org/archives/intel-gfx/2015-April/064266.html

Ville Syrjälä (7):
  drm/i915: Implement chv display PHY lane stagger setup
  drm/i915: Work around DISPLAY_PHY_CONTROL register corruption on CHV
  Revert "drm/i915: Hack to tie both common lanes together on chv"
  drm/i915: Use the default 600ns LDO programming sequence delay
  drm/i915: Only wait for required lanes in vlv_wait_port_ready()
  drm/i915: Implement PHY lane power gating for CHV
  drm/i915: Throw out WIP CHV power well definitions

 drivers/gpu/drm/i915/i915_drv.h         |   2 +
 drivers/gpu/drm/i915/i915_reg.h         |  28 ++++-
 drivers/gpu/drm/i915/intel_display.c    |  10 +-
 drivers/gpu/drm/i915/intel_dp.c         |  47 ++++++-
 drivers/gpu/drm/i915/intel_drv.h        |   5 +-
 drivers/gpu/drm/i915/intel_hdmi.c       |  44 ++++++-
 drivers/gpu/drm/i915/intel_runtime_pm.c | 210 +++++++++++++++-----------------
 7 files changed, 218 insertions(+), 128 deletions(-)

-- 
2.0.5

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

[PATCH 1/7] drm/i915: Implement chv display PHY lane stagger setup

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Set up the chv display PHY lane stagger registers according to
"Programming Guide for 1273 CHV eDP/DP/HDMI Display PHY" v1.04

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h   | 13 +++++++++++++
 drivers/gpu/drm/i915/intel_dp.c   | 35 +++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_hdmi.c | 35 +++++++++++++++++++++++++++++++++--
 3 files changed, 79 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index c6adf2d..cfbd5a6 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -949,6 +949,7 @@ enum skl_disp_power_wells {
 
 #define _VLV_PCS_DW11_CH0		0x822c
 #define _VLV_PCS_DW11_CH1		0x842c
+#define   DPIO_TX2_STAGGER_MASK(x)	((x)<<24)
 #define   DPIO_LANEDESKEW_STRAP_OVRD	(1<<3)
 #define   DPIO_LEFT_TXFIFO_RST_MASTER	(1<<1)
 #define   DPIO_RIGHT_TXFIFO_RST_MASTER	(1<<0)
@@ -961,8 +962,20 @@ enum skl_disp_power_wells {
 #define VLV_PCS01_DW11(ch) _PORT(ch, _VLV_PCS01_DW11_CH0, _VLV_PCS01_DW11_CH1)
 #define VLV_PCS23_DW11(ch) _PORT(ch, _VLV_PCS23_DW11_CH0, _VLV_PCS23_DW11_CH1)
 
+#define _VLV_PCS01_DW12_CH0		0x0230
+#define _VLV_PCS23_DW12_CH0		0x0430
+#define _VLV_PCS01_DW12_CH1		0x2630
+#define _VLV_PCS23_DW12_CH1		0x2830
+#define VLV_PCS01_DW12(ch) _PORT(ch, _VLV_PCS01_DW12_CH0, _VLV_PCS01_DW12_CH1)
+#define VLV_PCS23_DW12(ch) _PORT(ch, _VLV_PCS23_DW12_CH0, _VLV_PCS23_DW12_CH1)
+
 #define _VLV_PCS_DW12_CH0		0x8230
 #define _VLV_PCS_DW12_CH1		0x8430
+#define   DPIO_TX2_STAGGER_MULT(x)	((x)<<20)
+#define   DPIO_TX1_STAGGER_MULT(x)	((x)<<16)
+#define   DPIO_TX1_STAGGER_MASK(x)	((x)<<8)
+#define   DPIO_LANESTAGGER_STRAP_OVRD	(1<<6)
+#define   DPIO_LANESTAGGER_STRAP(x)	((x)<<0)
 #define VLV_PCS_DW12(ch) _PORT(ch, _VLV_PCS_DW12_CH0, _VLV_PCS_DW12_CH1)
 
 #define _VLV_PCS_DW14_CH0		0x8238
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f106763..7401fa3 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2707,7 +2707,7 @@ static void chv_pre_enable_dp(struct intel_encoder *encoder)
 		to_intel_crtc(encoder->base.crtc);
 	enum dpio_channel ch = vlv_dport_to_channel(dport);
 	int pipe = intel_crtc->pipe;
-	int data, i;
+	int data, i, stagger;
 	u32 val;
 
 	mutex_lock(&dev_priv->dpio_lock);
@@ -2747,7 +2747,38 @@ static void chv_pre_enable_dp(struct intel_encoder *encoder)
 	}
 
 	/* Data lane stagger programming */
-	/* FIXME: Fix up value only after power analysis */
+	if (intel_crtc->config->port_clock > 270000)
+		stagger = 0x18;
+	else if (intel_crtc->config->port_clock > 135000)
+		stagger = 0xd;
+	else if (intel_crtc->config->port_clock > 67500)
+		stagger = 0x7;
+	else if (intel_crtc->config->port_clock > 33750)
+		stagger = 0x4;
+	else
+		stagger = 0x2;
+
+	val = vlv_dpio_read(dev_priv, pipe, VLV_PCS01_DW11(ch));
+	val |= DPIO_TX2_STAGGER_MASK(0x1f);
+	vlv_dpio_write(dev_priv, pipe, VLV_PCS01_DW11(ch), val);
+
+	val = vlv_dpio_read(dev_priv, pipe, VLV_PCS23_DW11(ch));
+	val |= DPIO_TX2_STAGGER_MASK(0x1f);
+	vlv_dpio_write(dev_priv, pipe, VLV_PCS23_DW11(ch), val);
+
+	vlv_dpio_write(dev_priv, pipe, VLV_PCS01_DW12(ch),
+		       DPIO_LANESTAGGER_STRAP(stagger) |
+		       DPIO_LANESTAGGER_STRAP_OVRD |
+		       DPIO_TX1_STAGGER_MASK(0x1f) |
+		       DPIO_TX1_STAGGER_MULT(6) |
+		       DPIO_TX2_STAGGER_MULT(0));
+
+	vlv_dpio_write(dev_priv, pipe, VLV_PCS23_DW12(ch),
+		       DPIO_LANESTAGGER_STRAP(stagger) |
+		       DPIO_LANESTAGGER_STRAP_OVRD |
+		       DPIO_TX1_STAGGER_MASK(0x1f) |
+		       DPIO_TX1_STAGGER_MULT(7) |
+		       DPIO_TX2_STAGGER_MULT(5));
 
 	mutex_unlock(&dev_priv->dpio_lock);
 
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 3cef326..a24e2c8 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1482,7 +1482,7 @@ static void chv_hdmi_pre_enable(struct intel_encoder *encoder)
 		&intel_crtc->config->base.adjusted_mode;
 	enum dpio_channel ch = vlv_dport_to_channel(dport);
 	int pipe = intel_crtc->pipe;
-	int data, i;
+	int data, i, stagger;
 	u32 val;
 
 	mutex_lock(&dev_priv->dpio_lock);
@@ -1522,7 +1522,38 @@ static void chv_hdmi_pre_enable(struct intel_encoder *encoder)
 	}
 
 	/* Data lane stagger programming */
-	/* FIXME: Fix up value only after power analysis */
+	if (intel_crtc->config->port_clock > 270000)
+		stagger = 0x18;
+	else if (intel_crtc->config->port_clock > 135000)
+		stagger = 0xd;
+	else if (intel_crtc->config->port_clock > 67500)
+		stagger = 0x7;
+	else if (intel_crtc->config->port_clock > 33750)
+		stagger = 0x4;
+	else
+		stagger = 0x2;
+
+	val = vlv_dpio_read(dev_priv, pipe, VLV_PCS01_DW11(ch));
+	val |= DPIO_TX2_STAGGER_MASK(0x1f);
+	vlv_dpio_write(dev_priv, pipe, VLV_PCS01_DW11(ch), val);
+
+	val = vlv_dpio_read(dev_priv, pipe, VLV_PCS23_DW11(ch));
+	val |= DPIO_TX2_STAGGER_MASK(0x1f);
+	vlv_dpio_write(dev_priv, pipe, VLV_PCS23_DW11(ch), val);
+
+	vlv_dpio_write(dev_priv, pipe, VLV_PCS01_DW12(ch),
+		       DPIO_LANESTAGGER_STRAP(stagger) |
+		       DPIO_LANESTAGGER_STRAP_OVRD |
+		       DPIO_TX1_STAGGER_MASK(0x1f) |
+		       DPIO_TX1_STAGGER_MULT(6) |
+		       DPIO_TX2_STAGGER_MULT(0));
+
+	vlv_dpio_write(dev_priv, pipe, VLV_PCS23_DW12(ch),
+		       DPIO_LANESTAGGER_STRAP(stagger) |
+		       DPIO_LANESTAGGER_STRAP_OVRD |
+		       DPIO_TX1_STAGGER_MASK(0x1f) |
+		       DPIO_TX1_STAGGER_MULT(7) |
+		       DPIO_TX2_STAGGER_MULT(5));
 
 	/* Clear calc init */
 	val = vlv_dpio_read(dev_priv, pipe, VLV_PCS01_DW10(ch));
-- 
2.0.5

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

Re: [PATCH 1/7] drm/i915: Implement chv display PHY lane stagger setup

[Deepak S]

On Friday 10 April 2015 08:51 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Set up the chv display PHY lane stagger registers according to
> "Programming Guide for 1273 CHV eDP/DP/HDMI Display PHY" v1.04
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_reg.h   | 13 +++++++++++++
>   drivers/gpu/drm/i915/intel_dp.c   | 35 +++++++++++++++++++++++++++++++++--
>   drivers/gpu/drm/i915/intel_hdmi.c | 35 +++++++++++++++++++++++++++++++++--
>   3 files changed, 79 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index c6adf2d..cfbd5a6 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -949,6 +949,7 @@ enum skl_disp_power_wells {
>   
>   #define _VLV_PCS_DW11_CH0		0x822c
>   #define _VLV_PCS_DW11_CH1		0x842c
> +#define   DPIO_TX2_STAGGER_MASK(x)	((x)<<24)
>   #define   DPIO_LANEDESKEW_STRAP_OVRD	(1<<3)
>   #define   DPIO_LEFT_TXFIFO_RST_MASTER	(1<<1)
>   #define   DPIO_RIGHT_TXFIFO_RST_MASTER	(1<<0)
> @@ -961,8 +962,20 @@ enum skl_disp_power_wells {
>   #define VLV_PCS01_DW11(ch) _PORT(ch, _VLV_PCS01_DW11_CH0, _VLV_PCS01_DW11_CH1)
>   #define VLV_PCS23_DW11(ch) _PORT(ch, _VLV_PCS23_DW11_CH0, _VLV_PCS23_DW11_CH1)
>   
> +#define _VLV_PCS01_DW12_CH0		0x0230
> +#define _VLV_PCS23_DW12_CH0		0x0430
> +#define _VLV_PCS01_DW12_CH1		0x2630
> +#define _VLV_PCS23_DW12_CH1		0x2830
> +#define VLV_PCS01_DW12(ch) _PORT(ch, _VLV_PCS01_DW12_CH0, _VLV_PCS01_DW12_CH1)
> +#define VLV_PCS23_DW12(ch) _PORT(ch, _VLV_PCS23_DW12_CH0, _VLV_PCS23_DW12_CH1)
> +
>   #define _VLV_PCS_DW12_CH0		0x8230
>   #define _VLV_PCS_DW12_CH1		0x8430
> +#define   DPIO_TX2_STAGGER_MULT(x)	((x)<<20)
> +#define   DPIO_TX1_STAGGER_MULT(x)	((x)<<16)
> +#define   DPIO_TX1_STAGGER_MASK(x)	((x)<<8)
> +#define   DPIO_LANESTAGGER_STRAP_OVRD	(1<<6)
> +#define   DPIO_LANESTAGGER_STRAP(x)	((x)<<0)
>   #define VLV_PCS_DW12(ch) _PORT(ch, _VLV_PCS_DW12_CH0, _VLV_PCS_DW12_CH1)
>   
>   #define _VLV_PCS_DW14_CH0		0x8238
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f106763..7401fa3 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2707,7 +2707,7 @@ static void chv_pre_enable_dp(struct intel_encoder *encoder)
>   		to_intel_crtc(encoder->base.crtc);
>   	enum dpio_channel ch = vlv_dport_to_channel(dport);
>   	int pipe = intel_crtc->pipe;
> -	int data, i;
> +	int data, i, stagger;
>   	u32 val;
>   
>   	mutex_lock(&dev_priv->dpio_lock);
> @@ -2747,7 +2747,38 @@ static void chv_pre_enable_dp(struct intel_encoder *encoder)
>   	}
>   
>   	/* Data lane stagger programming */
> -	/* FIXME: Fix up value only after power analysis */
> +	if (intel_crtc->config->port_clock > 270000)
> +		stagger = 0x18;
> +	else if (intel_crtc->config->port_clock > 135000)
> +		stagger = 0xd;
> +	else if (intel_crtc->config->port_clock > 67500)
> +		stagger = 0x7;
> +	else if (intel_crtc->config->port_clock > 33750)
> +		stagger = 0x4;
> +	else
> +		stagger = 0x2;
> +
> +	val = vlv_dpio_read(dev_priv, pipe, VLV_PCS01_DW11(ch));
> +	val |= DPIO_TX2_STAGGER_MASK(0x1f);
> +	vlv_dpio_write(dev_priv, pipe, VLV_PCS01_DW11(ch), val);
> +
> +	val = vlv_dpio_read(dev_priv, pipe, VLV_PCS23_DW11(ch));
> +	val |= DPIO_TX2_STAGGER_MASK(0x1f);
> +	vlv_dpio_write(dev_priv, pipe, VLV_PCS23_DW11(ch), val);
> +
> +	vlv_dpio_write(dev_priv, pipe, VLV_PCS01_DW12(ch),
> +		       DPIO_LANESTAGGER_STRAP(stagger) |
> +		       DPIO_LANESTAGGER_STRAP_OVRD |
> +		       DPIO_TX1_STAGGER_MASK(0x1f) |
> +		       DPIO_TX1_STAGGER_MULT(6) |
> +		       DPIO_TX2_STAGGER_MULT(0));
> +
> +	vlv_dpio_write(dev_priv, pipe, VLV_PCS23_DW12(ch),
> +		       DPIO_LANESTAGGER_STRAP(stagger) |
> +		       DPIO_LANESTAGGER_STRAP_OVRD |
> +		       DPIO_TX1_STAGGER_MASK(0x1f) |
> +		       DPIO_TX1_STAGGER_MULT(7) |
> +		       DPIO_TX2_STAGGER_MULT(5));
>   
>   	mutex_unlock(&dev_priv->dpio_lock);
>   
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 3cef326..a24e2c8 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1482,7 +1482,7 @@ static void chv_hdmi_pre_enable(struct intel_encoder *encoder)
>   		&intel_crtc->config->base.adjusted_mode;
>   	enum dpio_channel ch = vlv_dport_to_channel(dport);
>   	int pipe = intel_crtc->pipe;
> -	int data, i;
> +	int data, i, stagger;
>   	u32 val;
>   
>   	mutex_lock(&dev_priv->dpio_lock);
> @@ -1522,7 +1522,38 @@ static void chv_hdmi_pre_enable(struct intel_encoder *encoder)
>   	}
>   
>   	/* Data lane stagger programming */
> -	/* FIXME: Fix up value only after power analysis */
> +	if (intel_crtc->config->port_clock > 270000)
> +		stagger = 0x18;
> +	else if (intel_crtc->config->port_clock > 135000)
> +		stagger = 0xd;
> +	else if (intel_crtc->config->port_clock > 67500)
> +		stagger = 0x7;
> +	else if (intel_crtc->config->port_clock > 33750)
> +		stagger = 0x4;
> +	else
> +		stagger = 0x2;
> +
> +	val = vlv_dpio_read(dev_priv, pipe, VLV_PCS01_DW11(ch));
> +	val |= DPIO_TX2_STAGGER_MASK(0x1f);
> +	vlv_dpio_write(dev_priv, pipe, VLV_PCS01_DW11(ch), val);
> +
> +	val = vlv_dpio_read(dev_priv, pipe, VLV_PCS23_DW11(ch));
> +	val |= DPIO_TX2_STAGGER_MASK(0x1f);
> +	vlv_dpio_write(dev_priv, pipe, VLV_PCS23_DW11(ch), val);
> +
> +	vlv_dpio_write(dev_priv, pipe, VLV_PCS01_DW12(ch),
> +		       DPIO_LANESTAGGER_STRAP(stagger) |
> +		       DPIO_LANESTAGGER_STRAP_OVRD |
> +		       DPIO_TX1_STAGGER_MASK(0x1f) |
> +		       DPIO_TX1_STAGGER_MULT(6) |
> +		       DPIO_TX2_STAGGER_MULT(0));
> +
> +	vlv_dpio_write(dev_priv, pipe, VLV_PCS23_DW12(ch),
> +		       DPIO_LANESTAGGER_STRAP(stagger) |
> +		       DPIO_LANESTAGGER_STRAP_OVRD |
> +		       DPIO_TX1_STAGGER_MASK(0x1f) |
> +		       DPIO_TX1_STAGGER_MULT(7) |
> +		       DPIO_TX2_STAGGER_MULT(5));
>   
>   	/* Clear calc init */
>   	val = vlv_dpio_read(dev_priv, pipe, VLV_PCS01_DW10(ch));

Patch does what spec says :)
Reviewed-by:  Deepak S <deepak.s@linux.intel.com>

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

[PATCH 2/7] drm/i915: Work around DISPLAY_PHY_CONTROL register corruption on CHV

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Sometimes (exactly when is a bit unclear) DISPLAY_PHY_CONTROL appears to
get corrupted. The values I've managed to read from it seem to have some
pattern but vary quite a lot. The corruption doesn't seem to just happen
when the register is accessed, but can also happen spontaneosly during
modeset. When this happens during a modeset things go south and the
display doesn't light up.

I've managed to hit the problemn when toggling HDMI on port D on and
off. When things get corrupted the display doesn't light up, but as soon
as I manually write the correct value to the register the display comes
up.

First I was suspicious that we ourselves accidentally overwrite it with
garbage, but didn't catch anything with the reg_rw tracepoint. Also I
sprinkled check all over the modeset path to see exactly when the
corruption happens, and eg. the read back value was fine just before
intel_dp_set_m(), and corrupted immediately after it. I also made my
check function repair the register value whenever it was wrong, and with
this approach the corruption repeated several times during the modeset
operation, always seeming to trigger in the same exact calls to the
check function, while other calls to the function never caught anything.

So far I've not seen this problem occurring when carefully avoiding all
read accesses to DISPLAY_PHY_CONTROL. Not sure if that's just pure luck
or an actual workaround, but we can hope it works. So let's avoid reading
the register and instead track the desired value of the register in dev_priv.

v2: Read out the power well state to determine initial register value
v3: Use DPIO_CHx names instead of raw numbers

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |  2 ++
 drivers/gpu/drm/i915/i915_reg.h         |  5 ++++-
 drivers/gpu/drm/i915/intel_runtime_pm.c | 36 ++++++++++++++++++++++++++++-----
 3 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 822f259..288c3fc 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1754,6 +1754,8 @@ struct drm_i915_private {
 
 	u32 fdi_rx_config;
 
+	u32 chv_phy_control;
+
 	u32 suspend_count;
 	struct i915_suspend_saved_registers regfile;
 	struct vlv_s0ix_state vlv_s0ix_state;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index cfbd5a6..98588d5 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1887,7 +1887,10 @@ enum skl_disp_power_wells {
 #define DPIO_PHY_STATUS			(VLV_DISPLAY_BASE + 0x6240)
 #define   DPLL_PORTD_READY_MASK		(0xf)
 #define DISPLAY_PHY_CONTROL (VLV_DISPLAY_BASE + 0x60100)
-#define   PHY_COM_LANE_RESET_DEASSERT(phy) (1 << (phy))
+#define   PHY_CH_SU_PSR				0x1
+#define   PHY_CH_DEEP_PSR			0x7
+#define   PHY_CH_POWER_MODE(mode, phy, ch)	((mode) << (6*(phy)+3*(ch)+2))
+#define   PHY_COM_LANE_RESET_DEASSERT(phy)	(1 << (phy))
 #define DISPLAY_PHY_STATUS (VLV_DISPLAY_BASE + 0x60104)
 #define   PHY_POWERGOOD(phy)	(((phy) == DPIO_PHY0) ? (1<<31) : (1<<30))
 
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index ce00e69..b73671f 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -666,8 +666,8 @@ static void chv_dpio_cmn_power_well_enable(struct drm_i915_private *dev_priv,
 	if (wait_for(I915_READ(DISPLAY_PHY_STATUS) & PHY_POWERGOOD(phy), 1))
 		DRM_ERROR("Display PHY %d is not power up\n", phy);
 
-	I915_WRITE(DISPLAY_PHY_CONTROL, I915_READ(DISPLAY_PHY_CONTROL) |
-		   PHY_COM_LANE_RESET_DEASSERT(phy));
+	dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(phy);
+	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
 }
 
 static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
@@ -687,8 +687,8 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
 		assert_pll_disabled(dev_priv, PIPE_C);
 	}
 
-	I915_WRITE(DISPLAY_PHY_CONTROL, I915_READ(DISPLAY_PHY_CONTROL) &
-		   ~PHY_COM_LANE_RESET_DEASSERT(phy));
+	dev_priv->chv_phy_control &= ~PHY_COM_LANE_RESET_DEASSERT(phy);
+	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
 
 	vlv_set_power_well(dev_priv, power_well, false);
 }
@@ -1401,6 +1401,30 @@ static void intel_power_domains_resume(struct drm_i915_private *dev_priv)
 	mutex_unlock(&power_domains->lock);
 }
 
+static void chv_phy_control_init(struct drm_i915_private *dev_priv)
+{
+	struct i915_power_well *cmn_bc =
+		lookup_power_well(dev_priv, PUNIT_POWER_WELL_DPIO_CMN_BC);
+	struct i915_power_well *cmn_d =
+		lookup_power_well(dev_priv, PUNIT_POWER_WELL_DPIO_CMN_D);
+
+	/*
+	 * DISPLAY_PHY_CONTROL can get corrupted if read. As a
+	 * workaround never ever read DISPLAY_PHY_CONTROL, and
+	 * instead maintain a shadow copy ourselves. Use the actual
+	 * power well state to reconstruct the expected initial
+	 * value.
+	 */
+	dev_priv->chv_phy_control =
+		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH0) |
+		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH1) |
+		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY1, DPIO_CH0);
+	if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc))
+		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY0);
+	if (cmn_d->ops->is_enabled(dev_priv, cmn_d))
+		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY1);
+}
+
 static void vlv_cmnlane_wa(struct drm_i915_private *dev_priv)
 {
 	struct i915_power_well *cmn =
@@ -1443,7 +1467,9 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv)
 
 	power_domains->initializing = true;
 
-	if (IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev)) {
+	if (IS_CHERRYVIEW(dev)) {
+		chv_phy_control_init(dev_priv);
+	} else if (IS_VALLEYVIEW(dev)) {
 		mutex_lock(&power_domains->lock);
 		vlv_cmnlane_wa(dev_priv);
 		mutex_unlock(&power_domains->lock);
-- 
2.0.5

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

Re: [PATCH 2/7] drm/i915: Work around DISPLAY_PHY_CONTROL register corruption on CHV

[Deepak S]

On Friday 10 April 2015 08:51 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Sometimes (exactly when is a bit unclear) DISPLAY_PHY_CONTROL appears to
> get corrupted. The values I've managed to read from it seem to have some
> pattern but vary quite a lot. The corruption doesn't seem to just happen
> when the register is accessed, but can also happen spontaneosly during
> modeset. When this happens during a modeset things go south and the
> display doesn't light up.
>
> I've managed to hit the problemn when toggling HDMI on port D on and
> off. When things get corrupted the display doesn't light up, but as soon
> as I manually write the correct value to the register the display comes
> up.
>
> First I was suspicious that we ourselves accidentally overwrite it with
> garbage, but didn't catch anything with the reg_rw tracepoint. Also I
> sprinkled check all over the modeset path to see exactly when the
> corruption happens, and eg. the read back value was fine just before
> intel_dp_set_m(), and corrupted immediately after it. I also made my
> check function repair the register value whenever it was wrong, and with
> this approach the corruption repeated several times during the modeset
> operation, always seeming to trigger in the same exact calls to the
> check function, while other calls to the function never caught anything.
>
> So far I've not seen this problem occurring when carefully avoiding all
> read accesses to DISPLAY_PHY_CONTROL. Not sure if that's just pure luck
> or an actual workaround, but we can hope it works. So let's avoid reading
> the register and instead track the desired value of the register in dev_priv.
>
> v2: Read out the power well state to determine initial register value
> v3: Use DPIO_CHx names instead of raw numbers

Even reading once DISPLAY_PHY_CONTROL layer is getting corrupted?
I saw similar issues on my setup. On some platform access phy is causing system behave inconsistently  :(

> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h         |  2 ++
>   drivers/gpu/drm/i915/i915_reg.h         |  5 ++++-
>   drivers/gpu/drm/i915/intel_runtime_pm.c | 36 ++++++++++++++++++++++++++++-----
>   3 files changed, 37 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 822f259..288c3fc 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1754,6 +1754,8 @@ struct drm_i915_private {
>   
>   	u32 fdi_rx_config;
>   
> +	u32 chv_phy_control;
> +
>   	u32 suspend_count;
>   	struct i915_suspend_saved_registers regfile;
>   	struct vlv_s0ix_state vlv_s0ix_state;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index cfbd5a6..98588d5 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1887,7 +1887,10 @@ enum skl_disp_power_wells {
>   #define DPIO_PHY_STATUS			(VLV_DISPLAY_BASE + 0x6240)
>   #define   DPLL_PORTD_READY_MASK		(0xf)
>   #define DISPLAY_PHY_CONTROL (VLV_DISPLAY_BASE + 0x60100)
> -#define   PHY_COM_LANE_RESET_DEASSERT(phy) (1 << (phy))
> +#define   PHY_CH_SU_PSR				0x1
> +#define   PHY_CH_DEEP_PSR			0x7

PHY_CH_DEEP_PSR defined but not used in this patch?

other than this, patch does what it says.
Reviewed-by:  Deepak S<deepak.s@linux.intel.com>

> +#define   PHY_CH_POWER_MODE(mode, phy, ch)	((mode) << (6*(phy)+3*(ch)+2))
> +#define   PHY_COM_LANE_RESET_DEASSERT(phy)	(1 << (phy))
>   #define DISPLAY_PHY_STATUS (VLV_DISPLAY_BASE + 0x60104)
>   #define   PHY_POWERGOOD(phy)	(((phy) == DPIO_PHY0) ? (1<<31) : (1<<30))
>   
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index ce00e69..b73671f 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -666,8 +666,8 @@ static void chv_dpio_cmn_power_well_enable(struct drm_i915_private *dev_priv,
>   	if (wait_for(I915_READ(DISPLAY_PHY_STATUS) & PHY_POWERGOOD(phy), 1))
>   		DRM_ERROR("Display PHY %d is not power up\n", phy);
>   
> -	I915_WRITE(DISPLAY_PHY_CONTROL, I915_READ(DISPLAY_PHY_CONTROL) |
> -		   PHY_COM_LANE_RESET_DEASSERT(phy));
> +	dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(phy);
> +	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
>   }
>   
>   static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
> @@ -687,8 +687,8 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
>   		assert_pll_disabled(dev_priv, PIPE_C);
>   	}
>   
> -	I915_WRITE(DISPLAY_PHY_CONTROL, I915_READ(DISPLAY_PHY_CONTROL) &
> -		   ~PHY_COM_LANE_RESET_DEASSERT(phy));
> +	dev_priv->chv_phy_control &= ~PHY_COM_LANE_RESET_DEASSERT(phy);
> +	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
>   
>   	vlv_set_power_well(dev_priv, power_well, false);
>   }
> @@ -1401,6 +1401,30 @@ static void intel_power_domains_resume(struct drm_i915_private *dev_priv)
>   	mutex_unlock(&power_domains->lock);
>   }
>   
> +static void chv_phy_control_init(struct drm_i915_private *dev_priv)
> +{
> +	struct i915_power_well *cmn_bc =
> +		lookup_power_well(dev_priv, PUNIT_POWER_WELL_DPIO_CMN_BC);
> +	struct i915_power_well *cmn_d =
> +		lookup_power_well(dev_priv, PUNIT_POWER_WELL_DPIO_CMN_D);
> +
> +	/*
> +	 * DISPLAY_PHY_CONTROL can get corrupted if read. As a
> +	 * workaround never ever read DISPLAY_PHY_CONTROL, and
> +	 * instead maintain a shadow copy ourselves. Use the actual
> +	 * power well state to reconstruct the expected initial
> +	 * value.
> +	 */
> +	dev_priv->chv_phy_control =
> +		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH0) |
> +		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH1) |
> +		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY1, DPIO_CH0);
> +	if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc))
> +		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY0);
> +	if (cmn_d->ops->is_enabled(dev_priv, cmn_d))
> +		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY1);
> +}
> +
>   static void vlv_cmnlane_wa(struct drm_i915_private *dev_priv)
>   {
>   	struct i915_power_well *cmn =
> @@ -1443,7 +1467,9 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv)
>   
>   	power_domains->initializing = true;
>   
> -	if (IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev)) {
> +	if (IS_CHERRYVIEW(dev)) {
> +		chv_phy_control_init(dev_priv);
> +	} else if (IS_VALLEYVIEW(dev)) {
>   		mutex_lock(&power_domains->lock);
>   		vlv_cmnlane_wa(dev_priv);
>   		mutex_unlock(&power_domains->lock);

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

Re: [PATCH 2/7] drm/i915: Work around DISPLAY_PHY_CONTROL register corruption on CHV

[Ville Syrjälä]

On Fri, May 08, 2015 at 06:24:42PM +0530, Deepak S wrote:
> 
> 
> On Friday 10 April 2015 08:51 PM, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Sometimes (exactly when is a bit unclear) DISPLAY_PHY_CONTROL appears to
> > get corrupted. The values I've managed to read from it seem to have some
> > pattern but vary quite a lot. The corruption doesn't seem to just happen
> > when the register is accessed, but can also happen spontaneosly during
> > modeset. When this happens during a modeset things go south and the
> > display doesn't light up.
> >
> > I've managed to hit the problemn when toggling HDMI on port D on and
> > off. When things get corrupted the display doesn't light up, but as soon
> > as I manually write the correct value to the register the display comes
> > up.
> >
> > First I was suspicious that we ourselves accidentally overwrite it with
> > garbage, but didn't catch anything with the reg_rw tracepoint. Also I
> > sprinkled check all over the modeset path to see exactly when the
> > corruption happens, and eg. the read back value was fine just before
> > intel_dp_set_m(), and corrupted immediately after it. I also made my
> > check function repair the register value whenever it was wrong, and with
> > this approach the corruption repeated several times during the modeset
> > operation, always seeming to trigger in the same exact calls to the
> > check function, while other calls to the function never caught anything.
> >
> > So far I've not seen this problem occurring when carefully avoiding all
> > read accesses to DISPLAY_PHY_CONTROL. Not sure if that's just pure luck
> > or an actual workaround, but we can hope it works. So let's avoid reading
> > the register and instead track the desired value of the register in dev_priv.
> >
> > v2: Read out the power well state to determine initial register value
> > v3: Use DPIO_CHx names instead of raw numbers
> 
> Even reading once DISPLAY_PHY_CONTROL layer is getting corrupted?

Not always. I think it somehow depends on what other register accesses
happen around it. So there is perhaps some magic sequence that might allow
reading it, but I decided that it's better to be safe and never read it.

> I saw similar issues on my setup. On some platform access phy is causing system behave inconsistently  :(
> 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_drv.h         |  2 ++
> >   drivers/gpu/drm/i915/i915_reg.h         |  5 ++++-
> >   drivers/gpu/drm/i915/intel_runtime_pm.c | 36 ++++++++++++++++++++++++++++-----
> >   3 files changed, 37 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 822f259..288c3fc 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1754,6 +1754,8 @@ struct drm_i915_private {
> >   
> >   	u32 fdi_rx_config;
> >   
> > +	u32 chv_phy_control;
> > +
> >   	u32 suspend_count;
> >   	struct i915_suspend_saved_registers regfile;
> >   	struct vlv_s0ix_state vlv_s0ix_state;
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index cfbd5a6..98588d5 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -1887,7 +1887,10 @@ enum skl_disp_power_wells {
> >   #define DPIO_PHY_STATUS			(VLV_DISPLAY_BASE + 0x6240)
> >   #define   DPLL_PORTD_READY_MASK		(0xf)
> >   #define DISPLAY_PHY_CONTROL (VLV_DISPLAY_BASE + 0x60100)
> > -#define   PHY_COM_LANE_RESET_DEASSERT(phy) (1 << (phy))
> > +#define   PHY_CH_SU_PSR				0x1
> > +#define   PHY_CH_DEEP_PSR			0x7
> 
> PHY_CH_DEEP_PSR defined but not used in this patch?

Just wanted to define it since it's the only other valid value, and the
doc situation is crap. I've not played around with PSR so I'm not
entirely sure how these would be used in practise. My gut is telling me
SU_PSR might be used with link standby and DEEP_PSR with link off, but
that's just a hunch at this point.

You'll see in later patches that we'll start using the override bits to
force the power state, so I think at that point these don't even matter.
But I suppose when we enter PSR we should drop the override bits to
allow the hardware to manage the power states based on the PSR mode
selected.

> 
> other than this, patch does what it says.
> Reviewed-by:  Deepak S<deepak.s@linux.intel.com>
> 
> > +#define   PHY_CH_POWER_MODE(mode, phy, ch)	((mode) << (6*(phy)+3*(ch)+2))
> > +#define   PHY_COM_LANE_RESET_DEASSERT(phy)	(1 << (phy))
> >   #define DISPLAY_PHY_STATUS (VLV_DISPLAY_BASE + 0x60104)
> >   #define   PHY_POWERGOOD(phy)	(((phy) == DPIO_PHY0) ? (1<<31) : (1<<30))
> >   
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index ce00e69..b73671f 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -666,8 +666,8 @@ static void chv_dpio_cmn_power_well_enable(struct drm_i915_private *dev_priv,
> >   	if (wait_for(I915_READ(DISPLAY_PHY_STATUS) & PHY_POWERGOOD(phy), 1))
> >   		DRM_ERROR("Display PHY %d is not power up\n", phy);
> >   
> > -	I915_WRITE(DISPLAY_PHY_CONTROL, I915_READ(DISPLAY_PHY_CONTROL) |
> > -		   PHY_COM_LANE_RESET_DEASSERT(phy));
> > +	dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(phy);
> > +	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
> >   }
> >   
> >   static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
> > @@ -687,8 +687,8 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
> >   		assert_pll_disabled(dev_priv, PIPE_C);
> >   	}
> >   
> > -	I915_WRITE(DISPLAY_PHY_CONTROL, I915_READ(DISPLAY_PHY_CONTROL) &
> > -		   ~PHY_COM_LANE_RESET_DEASSERT(phy));
> > +	dev_priv->chv_phy_control &= ~PHY_COM_LANE_RESET_DEASSERT(phy);
> > +	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
> >   
> >   	vlv_set_power_well(dev_priv, power_well, false);
> >   }
> > @@ -1401,6 +1401,30 @@ static void intel_power_domains_resume(struct drm_i915_private *dev_priv)
> >   	mutex_unlock(&power_domains->lock);
> >   }
> >   
> > +static void chv_phy_control_init(struct drm_i915_private *dev_priv)
> > +{
> > +	struct i915_power_well *cmn_bc =
> > +		lookup_power_well(dev_priv, PUNIT_POWER_WELL_DPIO_CMN_BC);
> > +	struct i915_power_well *cmn_d =
> > +		lookup_power_well(dev_priv, PUNIT_POWER_WELL_DPIO_CMN_D);
> > +
> > +	/*
> > +	 * DISPLAY_PHY_CONTROL can get corrupted if read. As a
> > +	 * workaround never ever read DISPLAY_PHY_CONTROL, and
> > +	 * instead maintain a shadow copy ourselves. Use the actual
> > +	 * power well state to reconstruct the expected initial
> > +	 * value.
> > +	 */
> > +	dev_priv->chv_phy_control =
> > +		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH0) |
> > +		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH1) |
> > +		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY1, DPIO_CH0);
> > +	if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc))
> > +		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY0);
> > +	if (cmn_d->ops->is_enabled(dev_priv, cmn_d))
> > +		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY1);
> > +}
> > +
> >   static void vlv_cmnlane_wa(struct drm_i915_private *dev_priv)
> >   {
> >   	struct i915_power_well *cmn =
> > @@ -1443,7 +1467,9 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv)
> >   
> >   	power_domains->initializing = true;
> >   
> > -	if (IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev)) {
> > +	if (IS_CHERRYVIEW(dev)) {
> > +		chv_phy_control_init(dev_priv);
> > +	} else if (IS_VALLEYVIEW(dev)) {
> >   		mutex_lock(&power_domains->lock);
> >   		vlv_cmnlane_wa(dev_priv);
> >   		mutex_unlock(&power_domains->lock);
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915: Work around DISPLAY_PHY_CONTROL register corruption on CHV

[Deepak S]

On Friday 08 May 2015 06:49 PM, Ville Syrjälä wrote:
> On Fri, May 08, 2015 at 06:24:42PM +0530, Deepak S wrote:
>>
>> On Friday 10 April 2015 08:51 PM, ville.syrjala@linux.intel.com wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> Sometimes (exactly when is a bit unclear) DISPLAY_PHY_CONTROL appears to
>>> get corrupted. The values I've managed to read from it seem to have some
>>> pattern but vary quite a lot. The corruption doesn't seem to just happen
>>> when the register is accessed, but can also happen spontaneosly during
>>> modeset. When this happens during a modeset things go south and the
>>> display doesn't light up.
>>>
>>> I've managed to hit the problemn when toggling HDMI on port D on and
>>> off. When things get corrupted the display doesn't light up, but as soon
>>> as I manually write the correct value to the register the display comes
>>> up.
>>>
>>> First I was suspicious that we ourselves accidentally overwrite it with
>>> garbage, but didn't catch anything with the reg_rw tracepoint. Also I
>>> sprinkled check all over the modeset path to see exactly when the
>>> corruption happens, and eg. the read back value was fine just before
>>> intel_dp_set_m(), and corrupted immediately after it. I also made my
>>> check function repair the register value whenever it was wrong, and with
>>> this approach the corruption repeated several times during the modeset
>>> operation, always seeming to trigger in the same exact calls to the
>>> check function, while other calls to the function never caught anything.
>>>
>>> So far I've not seen this problem occurring when carefully avoiding all
>>> read accesses to DISPLAY_PHY_CONTROL. Not sure if that's just pure luck
>>> or an actual workaround, but we can hope it works. So let's avoid reading
>>> the register and instead track the desired value of the register in dev_priv.
>>>
>>> v2: Read out the power well state to determine initial register value
>>> v3: Use DPIO_CHx names instead of raw numbers
>> Even reading once DISPLAY_PHY_CONTROL layer is getting corrupted?
> Not always. I think it somehow depends on what other register accesses
> happen around it. So there is perhaps some magic sequence that might allow
> reading it, but I decided that it's better to be safe and never read it.
>
>> I saw similar issues on my setup. On some platform access phy is causing system behave inconsistently  :(
>>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/i915_drv.h         |  2 ++
>>>    drivers/gpu/drm/i915/i915_reg.h         |  5 ++++-
>>>    drivers/gpu/drm/i915/intel_runtime_pm.c | 36 ++++++++++++++++++++++++++++-----
>>>    3 files changed, 37 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 822f259..288c3fc 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -1754,6 +1754,8 @@ struct drm_i915_private {
>>>    
>>>    	u32 fdi_rx_config;
>>>    
>>> +	u32 chv_phy_control;
>>> +
>>>    	u32 suspend_count;
>>>    	struct i915_suspend_saved_registers regfile;
>>>    	struct vlv_s0ix_state vlv_s0ix_state;
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>> index cfbd5a6..98588d5 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -1887,7 +1887,10 @@ enum skl_disp_power_wells {
>>>    #define DPIO_PHY_STATUS			(VLV_DISPLAY_BASE + 0x6240)
>>>    #define   DPLL_PORTD_READY_MASK		(0xf)
>>>    #define DISPLAY_PHY_CONTROL (VLV_DISPLAY_BASE + 0x60100)
>>> -#define   PHY_COM_LANE_RESET_DEASSERT(phy) (1 << (phy))
>>> +#define   PHY_CH_SU_PSR				0x1
>>> +#define   PHY_CH_DEEP_PSR			0x7
>> PHY_CH_DEEP_PSR defined but not used in this patch?
> Just wanted to define it since it's the only other valid value, and the
> doc situation is crap. I've not played around with PSR so I'm not
> entirely sure how these would be used in practise. My gut is telling me
> SU_PSR might be used with link standby and DEEP_PSR with link off, but
> that's just a hunch at this point.

right ville, DEEP_PSR is related to link off. I will try to find the docs related to this.

> You'll see in later patches that we'll start using the override bits to
> force the power state, so I think at that point these don't even matter.
> But I suppose when we enter PSR we should drop the override bits to
> allow the hardware to manage the power states based on the PSR mode
> selected.
>
>> other than this, patch does what it says.
>> Reviewed-by:  Deepak S<deepak.s@linux.intel.com>
>>
>>> +#define   PHY_CH_POWER_MODE(mode, phy, ch)	((mode) << (6*(phy)+3*(ch)+2))
>>> +#define   PHY_COM_LANE_RESET_DEASSERT(phy)	(1 << (phy))
>>>    #define DISPLAY_PHY_STATUS (VLV_DISPLAY_BASE + 0x60104)
>>>    #define   PHY_POWERGOOD(phy)	(((phy) == DPIO_PHY0) ? (1<<31) : (1<<30))
>>>    
>>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>> index ce00e69..b73671f 100644
>>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>> @@ -666,8 +666,8 @@ static void chv_dpio_cmn_power_well_enable(struct drm_i915_private *dev_priv,
>>>    	if (wait_for(I915_READ(DISPLAY_PHY_STATUS) & PHY_POWERGOOD(phy), 1))
>>>    		DRM_ERROR("Display PHY %d is not power up\n", phy);
>>>    
>>> -	I915_WRITE(DISPLAY_PHY_CONTROL, I915_READ(DISPLAY_PHY_CONTROL) |
>>> -		   PHY_COM_LANE_RESET_DEASSERT(phy));
>>> +	dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(phy);
>>> +	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
>>>    }
>>>    
>>>    static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
>>> @@ -687,8 +687,8 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
>>>    		assert_pll_disabled(dev_priv, PIPE_C);
>>>    	}
>>>    
>>> -	I915_WRITE(DISPLAY_PHY_CONTROL, I915_READ(DISPLAY_PHY_CONTROL) &
>>> -		   ~PHY_COM_LANE_RESET_DEASSERT(phy));
>>> +	dev_priv->chv_phy_control &= ~PHY_COM_LANE_RESET_DEASSERT(phy);
>>> +	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
>>>    
>>>    	vlv_set_power_well(dev_priv, power_well, false);
>>>    }
>>> @@ -1401,6 +1401,30 @@ static void intel_power_domains_resume(struct drm_i915_private *dev_priv)
>>>    	mutex_unlock(&power_domains->lock);
>>>    }
>>>    
>>> +static void chv_phy_control_init(struct drm_i915_private *dev_priv)
>>> +{
>>> +	struct i915_power_well *cmn_bc =
>>> +		lookup_power_well(dev_priv, PUNIT_POWER_WELL_DPIO_CMN_BC);
>>> +	struct i915_power_well *cmn_d =
>>> +		lookup_power_well(dev_priv, PUNIT_POWER_WELL_DPIO_CMN_D);
>>> +
>>> +	/*
>>> +	 * DISPLAY_PHY_CONTROL can get corrupted if read. As a
>>> +	 * workaround never ever read DISPLAY_PHY_CONTROL, and
>>> +	 * instead maintain a shadow copy ourselves. Use the actual
>>> +	 * power well state to reconstruct the expected initial
>>> +	 * value.
>>> +	 */
>>> +	dev_priv->chv_phy_control =
>>> +		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH0) |
>>> +		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH1) |
>>> +		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY1, DPIO_CH0);
>>> +	if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc))
>>> +		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY0);
>>> +	if (cmn_d->ops->is_enabled(dev_priv, cmn_d))
>>> +		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY1);
>>> +}
>>> +
>>>    static void vlv_cmnlane_wa(struct drm_i915_private *dev_priv)
>>>    {
>>>    	struct i915_power_well *cmn =
>>> @@ -1443,7 +1467,9 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv)
>>>    
>>>    	power_domains->initializing = true;
>>>    
>>> -	if (IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev)) {
>>> +	if (IS_CHERRYVIEW(dev)) {
>>> +		chv_phy_control_init(dev_priv);
>>> +	} else if (IS_VALLEYVIEW(dev)) {
>>>    		mutex_lock(&power_domains->lock);
>>>    		vlv_cmnlane_wa(dev_priv);
>>>    		mutex_unlock(&power_domains->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 2/7] drm/i915: Work around DISPLAY_PHY_CONTROL register corruption on CHV

[Daniel Vetter]

On Fri, May 08, 2015 at 04:19:13PM +0300, Ville Syrjälä wrote:
> On Fri, May 08, 2015 at 06:24:42PM +0530, Deepak S wrote:
> > 
> > 
> > On Friday 10 April 2015 08:51 PM, ville.syrjala@linux.intel.com wrote:
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index cfbd5a6..98588d5 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -1887,7 +1887,10 @@ enum skl_disp_power_wells {
> > >   #define DPIO_PHY_STATUS			(VLV_DISPLAY_BASE + 0x6240)
> > >   #define   DPLL_PORTD_READY_MASK		(0xf)
> > >   #define DISPLAY_PHY_CONTROL (VLV_DISPLAY_BASE + 0x60100)
> > > -#define   PHY_COM_LANE_RESET_DEASSERT(phy) (1 << (phy))
> > > +#define   PHY_CH_SU_PSR				0x1
> > > +#define   PHY_CH_DEEP_PSR			0x7
> > 
> > PHY_CH_DEEP_PSR defined but not used in this patch?
> 
> Just wanted to define it since it's the only other valid value, and the
> doc situation is crap. I've not played around with PSR so I'm not
> entirely sure how these would be used in practise. My gut is telling me
> SU_PSR might be used with link standby and DEEP_PSR with link off, but
> that's just a hunch at this point.

Yeah adding all #defines in the a patch even if not all used is imo good
practice. Merged the first 3 patches to dinq, thanks.
-Daniel
-- 
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

[PATCH 3/7] Revert "drm/i915: Hack to tie both common lanes together on chv"

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

With recent hardware/firmware there don't appear to be any glitches
on the other PHY when we toggle the cmnreset for the other PHY. So
detangle the cmnlane power wells from one another and let them be
controlled independently.

This reverts commit 3dd7b97458e8aa2d8985b46622d226fa635071e7.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index b73671f..1f800f8 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1181,23 +1181,13 @@ static struct i915_power_well chv_power_wells[] = {
 #endif
 	{
 		.name = "dpio-common-bc",
-		/*
-		 * XXX: cmnreset for one PHY seems to disturb the other.
-		 * As a workaround keep both powered on at the same
-		 * time for now.
-		 */
-		.domains = CHV_DPIO_CMN_BC_POWER_DOMAINS | CHV_DPIO_CMN_D_POWER_DOMAINS,
+		.domains = CHV_DPIO_CMN_BC_POWER_DOMAINS,
 		.data = PUNIT_POWER_WELL_DPIO_CMN_BC,
 		.ops = &chv_dpio_cmn_power_well_ops,
 	},
 	{
 		.name = "dpio-common-d",
-		/*
-		 * XXX: cmnreset for one PHY seems to disturb the other.
-		 * As a workaround keep both powered on at the same
-		 * time for now.
-		 */
-		.domains = CHV_DPIO_CMN_BC_POWER_DOMAINS | CHV_DPIO_CMN_D_POWER_DOMAINS,
+		.domains = CHV_DPIO_CMN_D_POWER_DOMAINS,
 		.data = PUNIT_POWER_WELL_DPIO_CMN_D,
 		.ops = &chv_dpio_cmn_power_well_ops,
 	},
-- 
2.0.5

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

Re: [PATCH 3/7] Revert "drm/i915: Hack to tie both common lanes together on chv"

[Deepak S]

On Friday 10 April 2015 08:51 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> With recent hardware/firmware there don't appear to be any glitches
> on the other PHY when we toggle the cmnreset for the other PHY. So
> detangle the cmnlane power wells from one another and let them be
> controlled independently.
>
> This reverts commit 3dd7b97458e8aa2d8985b46622d226fa635071e7.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/intel_runtime_pm.c | 14 ++------------
>   1 file changed, 2 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index b73671f..1f800f8 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1181,23 +1181,13 @@ static struct i915_power_well chv_power_wells[] = {
>   #endif
>   	{
>   		.name = "dpio-common-bc",
> -		/*
> -		 * XXX: cmnreset for one PHY seems to disturb the other.
> -		 * As a workaround keep both powered on at the same
> -		 * time for now.
> -		 */
> -		.domains = CHV_DPIO_CMN_BC_POWER_DOMAINS | CHV_DPIO_CMN_D_POWER_DOMAINS,
> +		.domains = CHV_DPIO_CMN_BC_POWER_DOMAINS,
>   		.data = PUNIT_POWER_WELL_DPIO_CMN_BC,
>   		.ops = &chv_dpio_cmn_power_well_ops,
>   	},
>   	{
>   		.name = "dpio-common-d",
> -		/*
> -		 * XXX: cmnreset for one PHY seems to disturb the other.
> -		 * As a workaround keep both powered on at the same
> -		 * time for now.
> -		 */
> -		.domains = CHV_DPIO_CMN_BC_POWER_DOMAINS | CHV_DPIO_CMN_D_POWER_DOMAINS,
> +		.domains = CHV_DPIO_CMN_D_POWER_DOMAINS,
>   		.data = PUNIT_POWER_WELL_DPIO_CMN_D,
>   		.ops = &chv_dpio_cmn_power_well_ops,
>   	},

Right, Issue is fixed with latest FW.
Reviewed-by:  Deepak S<deepak.s@linux.intel.com>

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

[PATCH 4/7] drm/i915: Use the default 600ns LDO programming sequence delay

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Not sure which LDO programming sequence delay should be used for the CHV
PHY, but the spec says that 600ns is "Used by default for initial
bringup", and the BIOS seems to use that, so let's do the same.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h         | 4 ++++
 drivers/gpu/drm/i915/intel_runtime_pm.c | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 98588d5..977bad6 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1887,6 +1887,10 @@ enum skl_disp_power_wells {
 #define DPIO_PHY_STATUS			(VLV_DISPLAY_BASE + 0x6240)
 #define   DPLL_PORTD_READY_MASK		(0xf)
 #define DISPLAY_PHY_CONTROL (VLV_DISPLAY_BASE + 0x60100)
+#define   PHY_LDO_DELAY_0NS			0x0
+#define   PHY_LDO_DELAY_200NS			0x1
+#define   PHY_LDO_DELAY_600NS			0x2
+#define   PHY_LDO_SEQ_DELAY(delay, phy)		((delay) << (2*(phy)+23))
 #define   PHY_CH_SU_PSR				0x1
 #define   PHY_CH_DEEP_PSR			0x7
 #define   PHY_CH_POWER_MODE(mode, phy, ch)	((mode) << (6*(phy)+3*(ch)+2))
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 1f800f8..5cd8a51 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1406,6 +1406,8 @@ static void chv_phy_control_init(struct drm_i915_private *dev_priv)
 	 * value.
 	 */
 	dev_priv->chv_phy_control =
+		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY0) |
+		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY1) |
 		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH0) |
 		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH1) |
 		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY1, DPIO_CH0);
-- 
2.0.5

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

Re: [PATCH 4/7] drm/i915: Use the default 600ns LDO programming sequence delay

[Deepak S]

On Friday 10 April 2015 08:51 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Not sure which LDO programming sequence delay should be used for the CHV
> PHY, but the spec says that 600ns is "Used by default for initial
> bringup", and the BIOS seems to use that, so let's do the same.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_reg.h         | 4 ++++
>   drivers/gpu/drm/i915/intel_runtime_pm.c | 2 ++
>   2 files changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 98588d5..977bad6 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1887,6 +1887,10 @@ enum skl_disp_power_wells {
>   #define DPIO_PHY_STATUS			(VLV_DISPLAY_BASE + 0x6240)
>   #define   DPLL_PORTD_READY_MASK		(0xf)
>   #define DISPLAY_PHY_CONTROL (VLV_DISPLAY_BASE + 0x60100)
> +#define   PHY_LDO_DELAY_0NS			0x0
> +#define   PHY_LDO_DELAY_200NS			0x1

PHY_LDO_DELAY_0NS & PHY_LDO_DELAY_200NS not used right?
Should we keep the definitions?

> +#define   PHY_LDO_DELAY_600NS			0x2
> +#define   PHY_LDO_SEQ_DELAY(delay, phy)		((delay) << (2*(phy)+23))
>   #define   PHY_CH_SU_PSR				0x1
>   #define   PHY_CH_DEEP_PSR			0x7
>   #define   PHY_CH_POWER_MODE(mode, phy, ch)	((mode) << (6*(phy)+3*(ch)+2))
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 1f800f8..5cd8a51 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1406,6 +1406,8 @@ static void chv_phy_control_init(struct drm_i915_private *dev_priv)
>   	 * value.
>   	 */
>   	dev_priv->chv_phy_control =
> +		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY0) |
> +		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY1) |
>   		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH0) |
>   		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH1) |
>   		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY1, DPIO_CH0);

I think we need to squash this patch to previous one?
[Intel-gfx] [PATCH 1/7] drm/i915: Implement chv display PHY lane stagger setup
http://www.spinics.net/lists/intel-gfx/msg64481.html

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

Re: [PATCH 4/7] drm/i915: Use the default 600ns LDO programming sequence delay

[Ville Syrjälä]

On Fri, May 08, 2015 at 06:31:23PM +0530, Deepak S wrote:
> 
> 
> On Friday 10 April 2015 08:51 PM, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Not sure which LDO programming sequence delay should be used for the CHV
> > PHY, but the spec says that 600ns is "Used by default for initial
> > bringup", and the BIOS seems to use that, so let's do the same.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_reg.h         | 4 ++++
> >   drivers/gpu/drm/i915/intel_runtime_pm.c | 2 ++
> >   2 files changed, 6 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 98588d5..977bad6 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -1887,6 +1887,10 @@ enum skl_disp_power_wells {
> >   #define DPIO_PHY_STATUS			(VLV_DISPLAY_BASE + 0x6240)
> >   #define   DPLL_PORTD_READY_MASK		(0xf)
> >   #define DISPLAY_PHY_CONTROL (VLV_DISPLAY_BASE + 0x60100)
> > +#define   PHY_LDO_DELAY_0NS			0x0
> > +#define   PHY_LDO_DELAY_200NS			0x1
> 
> PHY_LDO_DELAY_0NS & PHY_LDO_DELAY_200NS not used right?
> Should we keep the definitions?

I generally like to keep a bit of extra for VLV/CHV due to the bad doc
situation.

> 
> > +#define   PHY_LDO_DELAY_600NS			0x2
> > +#define   PHY_LDO_SEQ_DELAY(delay, phy)		((delay) << (2*(phy)+23))
> >   #define   PHY_CH_SU_PSR				0x1
> >   #define   PHY_CH_DEEP_PSR			0x7
> >   #define   PHY_CH_POWER_MODE(mode, phy, ch)	((mode) << (6*(phy)+3*(ch)+2))
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 1f800f8..5cd8a51 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -1406,6 +1406,8 @@ static void chv_phy_control_init(struct drm_i915_private *dev_priv)
> >   	 * value.
> >   	 */
> >   	dev_priv->chv_phy_control =
> > +		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY0) |
> > +		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY1) |
> >   		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH0) |
> >   		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH1) |
> >   		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY1, DPIO_CH0);
> 
> I think we need to squash this patch to previous one?
> [Intel-gfx] [PATCH 1/7] drm/i915: Implement chv display PHY lane stagger setup
> http://www.spinics.net/lists/intel-gfx/msg64481.html

Well, IIRC I never saw any real issues with the 0ns delay either, with
or without the lane stagger setup. So not much point in squashing IMO.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/7] drm/i915: Use the default 600ns LDO programming sequence delay

[Deepak S]

On Friday 08 May 2015 06:52 PM, Ville Syrjälä wrote:
> On Fri, May 08, 2015 at 06:31:23PM +0530, Deepak S wrote:
>>
>> On Friday 10 April 2015 08:51 PM, ville.syrjala@linux.intel.com wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> Not sure which LDO programming sequence delay should be used for the CHV
>>> PHY, but the spec says that 600ns is "Used by default for initial
>>> bringup", and the BIOS seems to use that, so let's do the same.
>>>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/i915_reg.h         | 4 ++++
>>>    drivers/gpu/drm/i915/intel_runtime_pm.c | 2 ++
>>>    2 files changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>> index 98588d5..977bad6 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -1887,6 +1887,10 @@ enum skl_disp_power_wells {
>>>    #define DPIO_PHY_STATUS			(VLV_DISPLAY_BASE + 0x6240)
>>>    #define   DPLL_PORTD_READY_MASK		(0xf)
>>>    #define DISPLAY_PHY_CONTROL (VLV_DISPLAY_BASE + 0x60100)
>>> +#define   PHY_LDO_DELAY_0NS			0x0
>>> +#define   PHY_LDO_DELAY_200NS			0x1
>> PHY_LDO_DELAY_0NS & PHY_LDO_DELAY_200NS not used right?
>> Should we keep the definitions?
> I generally like to keep a bit of extra for VLV/CHV due to the bad doc
> situation.
>
>>> +#define   PHY_LDO_DELAY_600NS			0x2
>>> +#define   PHY_LDO_SEQ_DELAY(delay, phy)		((delay) << (2*(phy)+23))
>>>    #define   PHY_CH_SU_PSR				0x1
>>>    #define   PHY_CH_DEEP_PSR			0x7
>>>    #define   PHY_CH_POWER_MODE(mode, phy, ch)	((mode) << (6*(phy)+3*(ch)+2))
>>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>> index 1f800f8..5cd8a51 100644
>>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>> @@ -1406,6 +1406,8 @@ static void chv_phy_control_init(struct drm_i915_private *dev_priv)
>>>    	 * value.
>>>    	 */
>>>    	dev_priv->chv_phy_control =
>>> +		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY0) |
>>> +		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY1) |
>>>    		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH0) |
>>>    		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH1) |
>>>    		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY1, DPIO_CH0);
>> I think we need to squash this patch to previous one?
>> [Intel-gfx] [PATCH 1/7] drm/i915: Implement chv display PHY lane stagger setup
>> http://www.spinics.net/lists/intel-gfx/msg64481.html
> Well, IIRC I never saw any real issues with the 0ns delay either, with
> or without the lane stagger setup. So not much point in squashing IMO.
>

Thanks for the clarification :)
Reviewed-by:  Deepak S<deepak.s@linux.intel.com>

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

[PATCH 5/7] drm/i915: Only wait for required lanes in vlv_wait_port_ready()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently vlv_wait_port_ready() waits for all four lanes on the
appropriate channel. This no longer works on CHV when the unused
lanes may be power gated. So pass in a mask of lanes that the
caller is expecting to be ready.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 10 ++++++----
 drivers/gpu/drm/i915/intel_dp.c      |  4 +++-
 drivers/gpu/drm/i915/intel_drv.h     |  3 ++-
 drivers/gpu/drm/i915/intel_hdmi.c    |  4 ++--
 4 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7bfe2af..2aa8055 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1819,7 +1819,8 @@ static void chv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
 }
 
 void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
-		struct intel_digital_port *dport)
+			 struct intel_digital_port *dport,
+			 unsigned int expected_mask)
 {
 	u32 port_mask;
 	int dpll_reg;
@@ -1832,6 +1833,7 @@ void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
 	case PORT_C:
 		port_mask = DPLL_PORTC_READY_MASK;
 		dpll_reg = DPLL(0);
+		expected_mask <<= 4;
 		break;
 	case PORT_D:
 		port_mask = DPLL_PORTD_READY_MASK;
@@ -1841,9 +1843,9 @@ void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
 		BUG();
 	}
 
-	if (wait_for((I915_READ(dpll_reg) & port_mask) == 0, 1000))
-		WARN(1, "timed out waiting for port %c ready: 0x%08x\n",
-		     port_name(dport->port), I915_READ(dpll_reg));
+	if (wait_for((I915_READ(dpll_reg) & port_mask) == expected_mask, 1000))
+		WARN(1, "timed out waiting for port %c ready: got 0x%x, expected 0x%x\n",
+		     port_name(dport->port), I915_READ(dpll_reg) & port_mask, expected_mask);
 }
 
 static void intel_prepare_shared_dpll(struct intel_crtc *crtc)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 7401fa3..ac38fd8 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2472,6 +2472,7 @@ static void intel_enable_dp(struct intel_encoder *encoder)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
 	uint32_t dp_reg = I915_READ(intel_dp->output_reg);
+	unsigned int lane_mask = 0x0;
 
 	if (WARN_ON(dp_reg & DP_PORT_EN))
 		return;
@@ -2490,7 +2491,8 @@ static void intel_enable_dp(struct intel_encoder *encoder)
 	pps_unlock(intel_dp);
 
 	if (IS_VALLEYVIEW(dev))
-		vlv_wait_port_ready(dev_priv, dp_to_dig_port(intel_dp));
+		vlv_wait_port_ready(dev_priv, dp_to_dig_port(intel_dp),
+				    lane_mask);
 
 	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
 	intel_dp_start_link_train(intel_dp);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index efa53d5..3ec829a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -951,7 +951,8 @@ intel_wait_for_vblank(struct drm_device *dev, int pipe)
 }
 int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp);
 void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
-			 struct intel_digital_port *dport);
+			 struct intel_digital_port *dport,
+			 unsigned int expected_mask);
 bool intel_get_load_detect_pipe(struct drm_connector *connector,
 				struct drm_display_mode *mode,
 				struct intel_load_detect_pipe *old,
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index a24e2c8..24b0aa1 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1319,7 +1319,7 @@ static void vlv_hdmi_pre_enable(struct intel_encoder *encoder)
 
 	intel_enable_hdmi(encoder);
 
-	vlv_wait_port_ready(dev_priv, dport);
+	vlv_wait_port_ready(dev_priv, dport, 0x0);
 }
 
 static void vlv_hdmi_pre_pll_enable(struct intel_encoder *encoder)
@@ -1636,7 +1636,7 @@ static void chv_hdmi_pre_enable(struct intel_encoder *encoder)
 
 	intel_enable_hdmi(encoder);
 
-	vlv_wait_port_ready(dev_priv, dport);
+	vlv_wait_port_ready(dev_priv, dport, 0x0);
 }
 
 static void intel_hdmi_destroy(struct drm_connector *connector)
-- 
2.0.5

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

Re: [PATCH 5/7] drm/i915: Only wait for required lanes in vlv_wait_port_ready()

[Deepak S]

On Friday 10 April 2015 08:51 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Currently vlv_wait_port_ready() waits for all four lanes on the
> appropriate channel. This no longer works on CHV when the unused
> lanes may be power gated. So pass in a mask of lanes that the
> caller is expecting to be ready.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 10 ++++++----
>   drivers/gpu/drm/i915/intel_dp.c      |  4 +++-
>   drivers/gpu/drm/i915/intel_drv.h     |  3 ++-
>   drivers/gpu/drm/i915/intel_hdmi.c    |  4 ++--
>   4 files changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7bfe2af..2aa8055 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1819,7 +1819,8 @@ static void chv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
>   }
>   
>   void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
> -		struct intel_digital_port *dport)
> +			 struct intel_digital_port *dport,
> +			 unsigned int expected_mask)
>   {
>   	u32 port_mask;
>   	int dpll_reg;
> @@ -1832,6 +1833,7 @@ void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
>   	case PORT_C:
>   		port_mask = DPLL_PORTC_READY_MASK;
>   		dpll_reg = DPLL(0);
> +		expected_mask <<= 4;
>   		break;
>   	case PORT_D:
>   		port_mask = DPLL_PORTD_READY_MASK;
> @@ -1841,9 +1843,9 @@ void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
>   		BUG();
>   	}
>   
> -	if (wait_for((I915_READ(dpll_reg) & port_mask) == 0, 1000))
> -		WARN(1, "timed out waiting for port %c ready: 0x%08x\n",
> -		     port_name(dport->port), I915_READ(dpll_reg));
> +	if (wait_for((I915_READ(dpll_reg) & port_mask) == expected_mask, 1000))
> +		WARN(1, "timed out waiting for port %c ready: got 0x%x, expected 0x%x\n",
> +		     port_name(dport->port), I915_READ(dpll_reg) & port_mask, expected_mask);
>   }
>   
>   static void intel_prepare_shared_dpll(struct intel_crtc *crtc)
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 7401fa3..ac38fd8 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2472,6 +2472,7 @@ static void intel_enable_dp(struct intel_encoder *encoder)
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
>   	uint32_t dp_reg = I915_READ(intel_dp->output_reg);
> +	unsigned int lane_mask = 0x0;
>   
>   	if (WARN_ON(dp_reg & DP_PORT_EN))
>   		return;
> @@ -2490,7 +2491,8 @@ static void intel_enable_dp(struct intel_encoder *encoder)
>   	pps_unlock(intel_dp);
>   
>   	if (IS_VALLEYVIEW(dev))
> -		vlv_wait_port_ready(dev_priv, dp_to_dig_port(intel_dp));
> +		vlv_wait_port_ready(dev_priv, dp_to_dig_port(intel_dp),
> +				    lane_mask);
>   
>   	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
>   	intel_dp_start_link_train(intel_dp);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index efa53d5..3ec829a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -951,7 +951,8 @@ intel_wait_for_vblank(struct drm_device *dev, int pipe)
>   }
>   int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp);
>   void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
> -			 struct intel_digital_port *dport);
> +			 struct intel_digital_port *dport,
> +			 unsigned int expected_mask);
>   bool intel_get_load_detect_pipe(struct drm_connector *connector,
>   				struct drm_display_mode *mode,
>   				struct intel_load_detect_pipe *old,
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index a24e2c8..24b0aa1 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1319,7 +1319,7 @@ static void vlv_hdmi_pre_enable(struct intel_encoder *encoder)
>   
>   	intel_enable_hdmi(encoder);
>   
> -	vlv_wait_port_ready(dev_priv, dport);
> +	vlv_wait_port_ready(dev_priv, dport, 0x0);
>   }
>   
>   static void vlv_hdmi_pre_pll_enable(struct intel_encoder *encoder)
> @@ -1636,7 +1636,7 @@ static void chv_hdmi_pre_enable(struct intel_encoder *encoder)
>   
>   	intel_enable_hdmi(encoder);
>   
> -	vlv_wait_port_ready(dev_priv, dport);
> +	vlv_wait_port_ready(dev_priv, dport, 0x0);
>   }
>   
>   static void intel_hdmi_destroy(struct drm_connector *connector)

Looks good.
Reviewed-by:  Deepak S<deepak.s@linux.intel.com>

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

Re: [PATCH 5/7] drm/i915: Only wait for required lanes in vlv_wait_port_ready()

[Daniel Vetter]

On Fri, May 08, 2015 at 07:23:42PM +0530, Deepak S wrote:
> 
> 
> On Friday 10 April 2015 08:51 PM, ville.syrjala@linux.intel.com wrote:
> >From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> >Currently vlv_wait_port_ready() waits for all four lanes on the
> >appropriate channel. This no longer works on CHV when the unused
> >lanes may be power gated. So pass in a mask of lanes that the
> >caller is expecting to be ready.
> >
> >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >---
> >  drivers/gpu/drm/i915/intel_display.c | 10 ++++++----
> >  drivers/gpu/drm/i915/intel_dp.c      |  4 +++-
> >  drivers/gpu/drm/i915/intel_drv.h     |  3 ++-
> >  drivers/gpu/drm/i915/intel_hdmi.c    |  4 ++--
> >  4 files changed, 13 insertions(+), 8 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >index 7bfe2af..2aa8055 100644
> >--- a/drivers/gpu/drm/i915/intel_display.c
> >+++ b/drivers/gpu/drm/i915/intel_display.c
> >@@ -1819,7 +1819,8 @@ static void chv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
> >  }
> >  void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
> >-		struct intel_digital_port *dport)
> >+			 struct intel_digital_port *dport,
> >+			 unsigned int expected_mask)
> >  {
> >  	u32 port_mask;
> >  	int dpll_reg;
> >@@ -1832,6 +1833,7 @@ void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
> >  	case PORT_C:
> >  		port_mask = DPLL_PORTC_READY_MASK;
> >  		dpll_reg = DPLL(0);
> >+		expected_mask <<= 4;
> >  		break;
> >  	case PORT_D:
> >  		port_mask = DPLL_PORTD_READY_MASK;
> >@@ -1841,9 +1843,9 @@ void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
> >  		BUG();
> >  	}
> >-	if (wait_for((I915_READ(dpll_reg) & port_mask) == 0, 1000))
> >-		WARN(1, "timed out waiting for port %c ready: 0x%08x\n",
> >-		     port_name(dport->port), I915_READ(dpll_reg));
> >+	if (wait_for((I915_READ(dpll_reg) & port_mask) == expected_mask, 1000))
> >+		WARN(1, "timed out waiting for port %c ready: got 0x%x, expected 0x%x\n",
> >+		     port_name(dport->port), I915_READ(dpll_reg) & port_mask, expected_mask);
> >  }
> >  static void intel_prepare_shared_dpll(struct intel_crtc *crtc)
> >diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >index 7401fa3..ac38fd8 100644
> >--- a/drivers/gpu/drm/i915/intel_dp.c
> >+++ b/drivers/gpu/drm/i915/intel_dp.c
> >@@ -2472,6 +2472,7 @@ static void intel_enable_dp(struct intel_encoder *encoder)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
> >  	uint32_t dp_reg = I915_READ(intel_dp->output_reg);
> >+	unsigned int lane_mask = 0x0;
> >  	if (WARN_ON(dp_reg & DP_PORT_EN))
> >  		return;
> >@@ -2490,7 +2491,8 @@ static void intel_enable_dp(struct intel_encoder *encoder)
> >  	pps_unlock(intel_dp);
> >  	if (IS_VALLEYVIEW(dev))
> >-		vlv_wait_port_ready(dev_priv, dp_to_dig_port(intel_dp));
> >+		vlv_wait_port_ready(dev_priv, dp_to_dig_port(intel_dp),
> >+				    lane_mask);
> >  	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> >  	intel_dp_start_link_train(intel_dp);
> >diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >index efa53d5..3ec829a 100644
> >--- a/drivers/gpu/drm/i915/intel_drv.h
> >+++ b/drivers/gpu/drm/i915/intel_drv.h
> >@@ -951,7 +951,8 @@ intel_wait_for_vblank(struct drm_device *dev, int pipe)
> >  }
> >  int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp);
> >  void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
> >-			 struct intel_digital_port *dport);
> >+			 struct intel_digital_port *dport,
> >+			 unsigned int expected_mask);
> >  bool intel_get_load_detect_pipe(struct drm_connector *connector,
> >  				struct drm_display_mode *mode,
> >  				struct intel_load_detect_pipe *old,
> >diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> >index a24e2c8..24b0aa1 100644
> >--- a/drivers/gpu/drm/i915/intel_hdmi.c
> >+++ b/drivers/gpu/drm/i915/intel_hdmi.c
> >@@ -1319,7 +1319,7 @@ static void vlv_hdmi_pre_enable(struct intel_encoder *encoder)
> >  	intel_enable_hdmi(encoder);
> >-	vlv_wait_port_ready(dev_priv, dport);
> >+	vlv_wait_port_ready(dev_priv, dport, 0x0);
> >  }
> >  static void vlv_hdmi_pre_pll_enable(struct intel_encoder *encoder)
> >@@ -1636,7 +1636,7 @@ static void chv_hdmi_pre_enable(struct intel_encoder *encoder)
> >  	intel_enable_hdmi(encoder);
> >-	vlv_wait_port_ready(dev_priv, dport);
> >+	vlv_wait_port_ready(dev_priv, dport, 0x0);
> >  }
> >  static void intel_hdmi_destroy(struct drm_connector *connector)
> 
> Looks good.
> Reviewed-by:  Deepak S<deepak.s@linux.intel.com>

Queued for -next, thanks for the patch.
-Daniel
-- 
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

[PATCH 6/7] drm/i915: Implement PHY lane power gating for CHV

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Powergate the PHY lanes when they're not needed. For HDMI all four lanes
are needed always, but for DP we can enable only the needed lanes. And
when the port is not used all lanes can be power gated. This could
reduce power consumption a bit when only a subset of the lanes in the
PHY are required.

A bit of extra care is needed to reconstruct the initial state of the
DPIO_PHY_CONTROL register since we can't read it. So instead we read the
actual lane status from the DPLL/PHY_STATUS registers and use that to
determine which lanes ought to be powergated initially.

Also sprinkle a few debug prints around so that we can monitor the
DPIO_PHY_STATUS changes without having to read it and risk corrupting
it.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h         |  2 +
 drivers/gpu/drm/i915/intel_dp.c         |  8 ++++
 drivers/gpu/drm/i915/intel_drv.h        |  2 +
 drivers/gpu/drm/i915/intel_hdmi.c       |  5 +++
 drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++++++++--
 5 files changed, 82 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 977bad6..34c366a 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1887,10 +1887,12 @@ enum skl_disp_power_wells {
 #define DPIO_PHY_STATUS			(VLV_DISPLAY_BASE + 0x6240)
 #define   DPLL_PORTD_READY_MASK		(0xf)
 #define DISPLAY_PHY_CONTROL (VLV_DISPLAY_BASE + 0x60100)
+#define   PHY_CH_POWER_DOWN_OVRD_EN(phy, ch)	(1 << (2*(phy)+(ch)+27))
 #define   PHY_LDO_DELAY_0NS			0x0
 #define   PHY_LDO_DELAY_200NS			0x1
 #define   PHY_LDO_DELAY_600NS			0x2
 #define   PHY_LDO_SEQ_DELAY(delay, phy)		((delay) << (2*(phy)+23))
+#define   PHY_CH_POWER_DOWN_OVRD(mask, phy, ch)	((mask) << (8*(phy)+4*(ch)+11))
 #define   PHY_CH_SU_PSR				0x1
 #define   PHY_CH_DEEP_PSR			0x7
 #define   PHY_CH_POWER_MODE(mode, phy, ch)	((mode) << (6*(phy)+3*(ch)+2))
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index ac38fd8..0b43f99 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2336,6 +2336,8 @@ static void chv_post_disable_dp(struct intel_encoder *encoder)
 
 	intel_dp_link_down(intel_dp);
 
+	chv_powergate_phy_lanes(encoder, 0xf);
+
 	mutex_lock(&dev_priv->dpio_lock);
 
 	/* Propagate soft reset to data lane reset */
@@ -2482,6 +2484,12 @@ static void intel_enable_dp(struct intel_encoder *encoder)
 	if (IS_VALLEYVIEW(dev))
 		vlv_init_panel_power_sequencer(intel_dp);
 
+	if (IS_CHERRYVIEW(dev)) {
+		/* FIXME deal with lane reversal */
+		lane_mask = 0xf & ~((1 << intel_dp->lane_count) - 1);
+		chv_powergate_phy_lanes(encoder, lane_mask);
+	}
+
 	intel_dp_enable_port(intel_dp);
 
 	edp_panel_vdd_on(intel_dp);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3ec829a..54bcca8 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1238,6 +1238,8 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
 
 void intel_display_set_init_power(struct drm_i915_private *dev, bool enable);
 
+void chv_powergate_phy_lanes(struct intel_encoder *encoder, unsigned int mask);
+
 /* intel_pm.c */
 void intel_init_clock_gating(struct drm_device *dev);
 void intel_suspend_hw(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 24b0aa1..f5842c3 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -916,6 +916,9 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
 		I915_WRITE(intel_hdmi->hdmi_reg, temp);
 		POSTING_READ(intel_hdmi->hdmi_reg);
 	}
+
+	if (IS_CHERRYVIEW(dev))
+		chv_powergate_phy_lanes(encoder, 0xf);
 }
 
 static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)
@@ -1634,6 +1637,8 @@ static void chv_hdmi_pre_enable(struct intel_encoder *encoder)
 				   intel_crtc->config->has_hdmi_sink,
 				   adjusted_mode);
 
+	chv_powergate_phy_lanes(encoder, 0x0);
+
 	intel_enable_hdmi(encoder);
 
 	vlv_wait_port_ready(dev_priv, dport, 0x0);
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 5cd8a51..d9e00d3 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -668,6 +668,9 @@ static void chv_dpio_cmn_power_well_enable(struct drm_i915_private *dev_priv,
 
 	dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(phy);
 	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
+
+	DRM_DEBUG_KMS("Enabled DPIO PHY%d (DPIO_PHY_CONTROL=0x%08x)\n",
+		      phy, dev_priv->chv_phy_control);
 }
 
 static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
@@ -680,10 +683,15 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
 
 	if (power_well->data == PUNIT_POWER_WELL_DPIO_CMN_BC) {
 		phy = DPIO_PHY0;
+		dev_priv->chv_phy_control |=
+			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH0) |
+			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH1);
 		assert_pll_disabled(dev_priv, PIPE_A);
 		assert_pll_disabled(dev_priv, PIPE_B);
 	} else {
 		phy = DPIO_PHY1;
+		dev_priv->chv_phy_control |=
+			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH0);
 		assert_pll_disabled(dev_priv, PIPE_C);
 	}
 
@@ -691,6 +699,25 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
 	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
 
 	vlv_set_power_well(dev_priv, power_well, false);
+
+	DRM_DEBUG_KMS("Disabled DPIO PHY%d (DPIO_PHY_CONTROL=0x%08x)\n",
+		      phy, dev_priv->chv_phy_control);
+}
+
+void chv_powergate_phy_lanes(struct intel_encoder *encoder, unsigned int mask)
+{
+	struct drm_device *dev = encoder->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
+	enum dpio_phy phy = DPIO_PHY(intel_crtc->pipe);
+	enum dpio_channel ch = vlv_dport_to_channel(enc_to_dig_port(&encoder->base));
+
+	dev_priv->chv_phy_control &= ~PHY_CH_POWER_DOWN_OVRD(0xf, phy, ch);
+	dev_priv->chv_phy_control |= PHY_CH_POWER_DOWN_OVRD(mask, phy, ch);
+	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
+
+	DRM_DEBUG_KMS("Power gating DPIO PHY%d CH%d lanes 0x%x (DPIO_PHY_CONTROL=0x%08x)\n",
+		      phy, ch, mask, dev_priv->chv_phy_control);
 }
 
 static bool chv_pipe_power_well_enabled(struct drm_i915_private *dev_priv,
@@ -1402,19 +1429,53 @@ static void chv_phy_control_init(struct drm_i915_private *dev_priv)
 	 * DISPLAY_PHY_CONTROL can get corrupted if read. As a
 	 * workaround never ever read DISPLAY_PHY_CONTROL, and
 	 * instead maintain a shadow copy ourselves. Use the actual
-	 * power well state to reconstruct the expected initial
-	 * value.
+	 * power well state and lane status to reconstruct the
+	 * expected initial value.
 	 */
 	dev_priv->chv_phy_control =
+		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY0, DPIO_CH0) |
+		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY0, DPIO_CH1) |
+		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY1, DPIO_CH0) |
 		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY0) |
 		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY1) |
 		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH0) |
 		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH1) |
 		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY1, DPIO_CH0);
-	if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc))
+
+	if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc)) {
+		uint32_t status = I915_READ(DPLL(PIPE_A));
+		unsigned int mask;
+
+		mask = status & DPLL_PORTB_READY_MASK;
+		dev_priv->chv_phy_control |=
+			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY0, DPIO_CH0);
+		mask = (status & DPLL_PORTC_READY_MASK) >> 4;
+		dev_priv->chv_phy_control |=
+			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY0, DPIO_CH1);
+
 		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY0);
-	if (cmn_d->ops->is_enabled(dev_priv, cmn_d))
+	} else {
+		dev_priv->chv_phy_control |=
+			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH0) |
+			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH1);
+	}
+
+	if (cmn_d->ops->is_enabled(dev_priv, cmn_d)) {
+		uint32_t status = I915_READ(DPIO_PHY_STATUS);
+		unsigned int mask;
+
+		mask = status & DPLL_PORTD_READY_MASK;
+		dev_priv->chv_phy_control |=
+			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY1, DPIO_CH0);
+
 		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY1);
+	} else {
+		dev_priv->chv_phy_control |=
+			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY1, DPIO_CH0);
+	}
+
+	DRM_DEBUG_KMS("Initial DPIO_PHY_CONTROL=0x%08x\n",
+		      dev_priv->chv_phy_control);
 }
 
 static void vlv_cmnlane_wa(struct drm_i915_private *dev_priv)
-- 
2.0.5

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

Re: [PATCH 6/7] drm/i915: Implement PHY lane power gating for CHV

[Deepak S]

[-- Attachment #1.1: Type: text/plain, Size: 9751 bytes --]



On Friday 10 April 2015 08:51 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Powergate the PHY lanes when they're not needed. For HDMI all four lanes
> are needed always, but for DP we can enable only the needed lanes. And
> when the port is not used all lanes can be power gated. This could
> reduce power consumption a bit when only a subset of the lanes in the
> PHY are required.
>
> A bit of extra care is needed to reconstruct the initial state of the
> DPIO_PHY_CONTROL register since we can't read it. So instead we read the
> actual lane status from the DPLL/PHY_STATUS registers and use that to
> determine which lanes ought to be powergated initially.
>
> Also sprinkle a few debug prints around so that we can monitor the
> DPIO_PHY_STATUS changes without having to read it and risk corrupting
> it.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_reg.h         |  2 +
>   drivers/gpu/drm/i915/intel_dp.c         |  8 ++++
>   drivers/gpu/drm/i915/intel_drv.h        |  2 +
>   drivers/gpu/drm/i915/intel_hdmi.c       |  5 +++
>   drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++++++++--
>   5 files changed, 82 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 977bad6..34c366a 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1887,10 +1887,12 @@ enum skl_disp_power_wells {
>   #define DPIO_PHY_STATUS			(VLV_DISPLAY_BASE + 0x6240)
>   #define   DPLL_PORTD_READY_MASK		(0xf)
>   #define DISPLAY_PHY_CONTROL (VLV_DISPLAY_BASE + 0x60100)
> +#define   PHY_CH_POWER_DOWN_OVRD_EN(phy, ch)	(1 << (2*(phy)+(ch)+27))
>   #define   PHY_LDO_DELAY_0NS			0x0
>   #define   PHY_LDO_DELAY_200NS			0x1
>   #define   PHY_LDO_DELAY_600NS			0x2
>   #define   PHY_LDO_SEQ_DELAY(delay, phy)		((delay) << (2*(phy)+23))
> +#define   PHY_CH_POWER_DOWN_OVRD(mask, phy, ch)	((mask) << (8*(phy)+4*(ch)+11))
>   #define   PHY_CH_SU_PSR				0x1
>   #define   PHY_CH_DEEP_PSR			0x7
>   #define   PHY_CH_POWER_MODE(mode, phy, ch)	((mode) << (6*(phy)+3*(ch)+2))
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index ac38fd8..0b43f99 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2336,6 +2336,8 @@ static void chv_post_disable_dp(struct intel_encoder *encoder)
>   
>   	intel_dp_link_down(intel_dp);
>   
> +	chv_powergate_phy_lanes(encoder, 0xf);
> +
>   	mutex_lock(&dev_priv->dpio_lock);
>   
>   	/* Propagate soft reset to data lane reset */
> @@ -2482,6 +2484,12 @@ static void intel_enable_dp(struct intel_encoder *encoder)
>   	if (IS_VALLEYVIEW(dev))
>   		vlv_init_panel_power_sequencer(intel_dp);
>   
> +	if (IS_CHERRYVIEW(dev)) {
> +		/* FIXME deal with lane reversal */
> +		lane_mask = 0xf & ~((1 << intel_dp->lane_count) - 1);
> +		chv_powergate_phy_lanes(encoder, lane_mask);
> +	}
> +
>   	intel_dp_enable_port(intel_dp);
>   
>   	edp_panel_vdd_on(intel_dp);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3ec829a..54bcca8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1238,6 +1238,8 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
>   
>   void intel_display_set_init_power(struct drm_i915_private *dev, bool enable);
>   
> +void chv_powergate_phy_lanes(struct intel_encoder *encoder, unsigned int mask);
> +
>   /* intel_pm.c */
>   void intel_init_clock_gating(struct drm_device *dev);
>   void intel_suspend_hw(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 24b0aa1..f5842c3 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -916,6 +916,9 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
>   		I915_WRITE(intel_hdmi->hdmi_reg, temp);
>   		POSTING_READ(intel_hdmi->hdmi_reg);
>   	}
> +
> +	if (IS_CHERRYVIEW(dev))
> +		chv_powergate_phy_lanes(encoder, 0xf);
>   }
>   
>   static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)
> @@ -1634,6 +1637,8 @@ static void chv_hdmi_pre_enable(struct intel_encoder *encoder)
>   				   intel_crtc->config->has_hdmi_sink,
>   				   adjusted_mode);
>   
> +	chv_powergate_phy_lanes(encoder, 0x0);
> +
>   	intel_enable_hdmi(encoder);
>   
>   	vlv_wait_port_ready(dev_priv, dport, 0x0);
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 5cd8a51..d9e00d3 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -668,6 +668,9 @@ static void chv_dpio_cmn_power_well_enable(struct drm_i915_private *dev_priv,
>   
>   	dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(phy);
>   	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
> +
> +	DRM_DEBUG_KMS("Enabled DPIO PHY%d (DPIO_PHY_CONTROL=0x%08x)\n",
> +		      phy, dev_priv->chv_phy_control);
>   }
>   
>   static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
> @@ -680,10 +683,15 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
>   
>   	if (power_well->data == PUNIT_POWER_WELL_DPIO_CMN_BC) {
>   		phy = DPIO_PHY0;
> +		dev_priv->chv_phy_control |=
> +			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH0) |
> +			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH1);
>   		assert_pll_disabled(dev_priv, PIPE_A);
>   		assert_pll_disabled(dev_priv, PIPE_B);
>   	} else {
>   		phy = DPIO_PHY1;
> +		dev_priv->chv_phy_control |=
> +			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH0);
>   		assert_pll_disabled(dev_priv, PIPE_C);
>   	}
>   
> @@ -691,6 +699,25 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
>   	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
>   
>   	vlv_set_power_well(dev_priv, power_well, false);
> +
> +	DRM_DEBUG_KMS("Disabled DPIO PHY%d (DPIO_PHY_CONTROL=0x%08x)\n",
> +		      phy, dev_priv->chv_phy_control);
> +}
> +
> +void chv_powergate_phy_lanes(struct intel_encoder *encoder, unsigned int mask)
> +{
> +	struct drm_device *dev = encoder->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> +	enum dpio_phy phy = DPIO_PHY(intel_crtc->pipe);
> +	enum dpio_channel ch = vlv_dport_to_channel(enc_to_dig_port(&encoder->base));
> +
> +	dev_priv->chv_phy_control &= ~PHY_CH_POWER_DOWN_OVRD(0xf, phy, ch);
> +	dev_priv->chv_phy_control |= PHY_CH_POWER_DOWN_OVRD(mask, phy, ch);
> +	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
> +
> +	DRM_DEBUG_KMS("Power gating DPIO PHY%d CH%d lanes 0x%x (DPIO_PHY_CONTROL=0x%08x)\n",
> +		      phy, ch, mask, dev_priv->chv_phy_control);
>   }
>   

Hi Ville,

I think based on the spec, we needto program 0x8170 as 0xxxC3xxxx to affect the DDI0 power down
and this reigster to be programted beforeDPIO_PHY_CONTROL ?

Thanks
Deepak

>   static bool chv_pipe_power_well_enabled(struct drm_i915_private *dev_priv,
> @@ -1402,19 +1429,53 @@ static void chv_phy_control_init(struct drm_i915_private *dev_priv)
>   	 * DISPLAY_PHY_CONTROL can get corrupted if read. As a
>   	 * workaround never ever read DISPLAY_PHY_CONTROL, and
>   	 * instead maintain a shadow copy ourselves. Use the actual
> -	 * power well state to reconstruct the expected initial
> -	 * value.
> +	 * power well state and lane status to reconstruct the
> +	 * expected initial value.
>   	 */
>   	dev_priv->chv_phy_control =
> +		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY0, DPIO_CH0) |
> +		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY0, DPIO_CH1) |
> +		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY1, DPIO_CH0) |
>   		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY0) |
>   		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY1) |
>   		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH0) |
>   		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH1) |
>   		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY1, DPIO_CH0);
> -	if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc))
> +
> +	if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc)) {
> +		uint32_t status = I915_READ(DPLL(PIPE_A));
> +		unsigned int mask;
> +
> +		mask = status & DPLL_PORTB_READY_MASK;
> +		dev_priv->chv_phy_control |=
> +			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY0, DPIO_CH0);
> +		mask = (status & DPLL_PORTC_READY_MASK) >> 4;
> +		dev_priv->chv_phy_control |=
> +			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY0, DPIO_CH1);
> +
>   		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY0);
> -	if (cmn_d->ops->is_enabled(dev_priv, cmn_d))
> +	} else {
> +		dev_priv->chv_phy_control |=
> +			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH0) |
> +			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH1);
> +	}
> +
> +	if (cmn_d->ops->is_enabled(dev_priv, cmn_d)) {
> +		uint32_t status = I915_READ(DPIO_PHY_STATUS);
> +		unsigned int mask;
> +
> +		mask = status & DPLL_PORTD_READY_MASK;
> +		dev_priv->chv_phy_control |=
> +			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY1, DPIO_CH0);
> +
>   		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY1);
> +	} else {
> +		dev_priv->chv_phy_control |=
> +			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY1, DPIO_CH0);
> +	}
> +
> +	DRM_DEBUG_KMS("Initial DPIO_PHY_CONTROL=0x%08x\n",
> +		      dev_priv->chv_phy_control);
>   }
>   
>   static void vlv_cmnlane_wa(struct drm_i915_private *dev_priv)


[-- Attachment #1.2: Type: text/html, Size: 10652 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 6/7] drm/i915: Implement PHY lane power gating for CHV

[Ville Syrjälä]

On Fri, May 08, 2015 at 08:19:12PM +0530, Deepak S wrote:
> 
> 
> On Friday 10 April 2015 08:51 PM, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Powergate the PHY lanes when they're not needed. For HDMI all four lanes
> > are needed always, but for DP we can enable only the needed lanes. And
> > when the port is not used all lanes can be power gated. This could
> > reduce power consumption a bit when only a subset of the lanes in the
> > PHY are required.
> >
> > A bit of extra care is needed to reconstruct the initial state of the
> > DPIO_PHY_CONTROL register since we can't read it. So instead we read the
> > actual lane status from the DPLL/PHY_STATUS registers and use that to
> > determine which lanes ought to be powergated initially.
> >
> > Also sprinkle a few debug prints around so that we can monitor the
> > DPIO_PHY_STATUS changes without having to read it and risk corrupting
> > it.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_reg.h         |  2 +
> >   drivers/gpu/drm/i915/intel_dp.c         |  8 ++++
> >   drivers/gpu/drm/i915/intel_drv.h        |  2 +
> >   drivers/gpu/drm/i915/intel_hdmi.c       |  5 +++
> >   drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++++++++--
> >   5 files changed, 82 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 977bad6..34c366a 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -1887,10 +1887,12 @@ enum skl_disp_power_wells {
> >   #define DPIO_PHY_STATUS			(VLV_DISPLAY_BASE + 0x6240)
> >   #define   DPLL_PORTD_READY_MASK		(0xf)
> >   #define DISPLAY_PHY_CONTROL (VLV_DISPLAY_BASE + 0x60100)
> > +#define   PHY_CH_POWER_DOWN_OVRD_EN(phy, ch)	(1 << (2*(phy)+(ch)+27))
> >   #define   PHY_LDO_DELAY_0NS			0x0
> >   #define   PHY_LDO_DELAY_200NS			0x1
> >   #define   PHY_LDO_DELAY_600NS			0x2
> >   #define   PHY_LDO_SEQ_DELAY(delay, phy)		((delay) << (2*(phy)+23))
> > +#define   PHY_CH_POWER_DOWN_OVRD(mask, phy, ch)	((mask) << (8*(phy)+4*(ch)+11))
> >   #define   PHY_CH_SU_PSR				0x1
> >   #define   PHY_CH_DEEP_PSR			0x7
> >   #define   PHY_CH_POWER_MODE(mode, phy, ch)	((mode) << (6*(phy)+3*(ch)+2))
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index ac38fd8..0b43f99 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -2336,6 +2336,8 @@ static void chv_post_disable_dp(struct intel_encoder *encoder)
> >   
> >   	intel_dp_link_down(intel_dp);
> >   
> > +	chv_powergate_phy_lanes(encoder, 0xf);
> > +
> >   	mutex_lock(&dev_priv->dpio_lock);
> >   
> >   	/* Propagate soft reset to data lane reset */
> > @@ -2482,6 +2484,12 @@ static void intel_enable_dp(struct intel_encoder *encoder)
> >   	if (IS_VALLEYVIEW(dev))
> >   		vlv_init_panel_power_sequencer(intel_dp);
> >   
> > +	if (IS_CHERRYVIEW(dev)) {
> > +		/* FIXME deal with lane reversal */
> > +		lane_mask = 0xf & ~((1 << intel_dp->lane_count) - 1);
> > +		chv_powergate_phy_lanes(encoder, lane_mask);
> > +	}
> > +
> >   	intel_dp_enable_port(intel_dp);
> >   
> >   	edp_panel_vdd_on(intel_dp);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 3ec829a..54bcca8 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1238,6 +1238,8 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
> >   
> >   void intel_display_set_init_power(struct drm_i915_private *dev, bool enable);
> >   
> > +void chv_powergate_phy_lanes(struct intel_encoder *encoder, unsigned int mask);
> > +
> >   /* intel_pm.c */
> >   void intel_init_clock_gating(struct drm_device *dev);
> >   void intel_suspend_hw(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 24b0aa1..f5842c3 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -916,6 +916,9 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
> >   		I915_WRITE(intel_hdmi->hdmi_reg, temp);
> >   		POSTING_READ(intel_hdmi->hdmi_reg);
> >   	}
> > +
> > +	if (IS_CHERRYVIEW(dev))
> > +		chv_powergate_phy_lanes(encoder, 0xf);
> >   }
> >   
> >   static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)
> > @@ -1634,6 +1637,8 @@ static void chv_hdmi_pre_enable(struct intel_encoder *encoder)
> >   				   intel_crtc->config->has_hdmi_sink,
> >   				   adjusted_mode);
> >   
> > +	chv_powergate_phy_lanes(encoder, 0x0);
> > +
> >   	intel_enable_hdmi(encoder);
> >   
> >   	vlv_wait_port_ready(dev_priv, dport, 0x0);
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 5cd8a51..d9e00d3 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -668,6 +668,9 @@ static void chv_dpio_cmn_power_well_enable(struct drm_i915_private *dev_priv,
> >   
> >   	dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(phy);
> >   	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
> > +
> > +	DRM_DEBUG_KMS("Enabled DPIO PHY%d (DPIO_PHY_CONTROL=0x%08x)\n",
> > +		      phy, dev_priv->chv_phy_control);
> >   }
> >   
> >   static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
> > @@ -680,10 +683,15 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
> >   
> >   	if (power_well->data == PUNIT_POWER_WELL_DPIO_CMN_BC) {
> >   		phy = DPIO_PHY0;
> > +		dev_priv->chv_phy_control |=
> > +			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH0) |
> > +			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH1);
> >   		assert_pll_disabled(dev_priv, PIPE_A);
> >   		assert_pll_disabled(dev_priv, PIPE_B);
> >   	} else {
> >   		phy = DPIO_PHY1;
> > +		dev_priv->chv_phy_control |=
> > +			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH0);
> >   		assert_pll_disabled(dev_priv, PIPE_C);
> >   	}
> >   
> > @@ -691,6 +699,25 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
> >   	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
> >   
> >   	vlv_set_power_well(dev_priv, power_well, false);
> > +
> > +	DRM_DEBUG_KMS("Disabled DPIO PHY%d (DPIO_PHY_CONTROL=0x%08x)\n",
> > +		      phy, dev_priv->chv_phy_control);
> > +}
> > +
> > +void chv_powergate_phy_lanes(struct intel_encoder *encoder, unsigned int mask)
> > +{
> > +	struct drm_device *dev = encoder->base.dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> > +	enum dpio_phy phy = DPIO_PHY(intel_crtc->pipe);
> > +	enum dpio_channel ch = vlv_dport_to_channel(enc_to_dig_port(&encoder->base));
> > +
> > +	dev_priv->chv_phy_control &= ~PHY_CH_POWER_DOWN_OVRD(0xf, phy, ch);
> > +	dev_priv->chv_phy_control |= PHY_CH_POWER_DOWN_OVRD(mask, phy, ch);
> > +	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
> > +
> > +	DRM_DEBUG_KMS("Power gating DPIO PHY%d CH%d lanes 0x%x (DPIO_PHY_CONTROL=0x%08x)\n",
> > +		      phy, ch, mask, dev_priv->chv_phy_control);
> >   }
> >   
> 
> Hi Ville,
> 
> I think based on the spec, we needto program 0x8170 as 0xxxC3xxxx to affect the DDI0 power down
> and this reigster to be programted beforeDPIO_PHY_CONTROL ?

Doh. Nice catch. Looks like the dynamic power down bits default to off.
I also found the same bit in CL2 DW6, which isn't mentioned by the spec.
I'll cook up a patch to flip them and run a few tests to see if stuff
still works.

> 
> Thanks
> Deepak
> 
> >   static bool chv_pipe_power_well_enabled(struct drm_i915_private *dev_priv,
> > @@ -1402,19 +1429,53 @@ static void chv_phy_control_init(struct drm_i915_private *dev_priv)
> >   	 * DISPLAY_PHY_CONTROL can get corrupted if read. As a
> >   	 * workaround never ever read DISPLAY_PHY_CONTROL, and
> >   	 * instead maintain a shadow copy ourselves. Use the actual
> > -	 * power well state to reconstruct the expected initial
> > -	 * value.
> > +	 * power well state and lane status to reconstruct the
> > +	 * expected initial value.
> >   	 */
> >   	dev_priv->chv_phy_control =
> > +		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY0, DPIO_CH0) |
> > +		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY0, DPIO_CH1) |
> > +		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY1, DPIO_CH0) |
> >   		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY0) |
> >   		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY1) |
> >   		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH0) |
> >   		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH1) |
> >   		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY1, DPIO_CH0);
> > -	if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc))
> > +
> > +	if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc)) {
> > +		uint32_t status = I915_READ(DPLL(PIPE_A));
> > +		unsigned int mask;
> > +
> > +		mask = status & DPLL_PORTB_READY_MASK;
> > +		dev_priv->chv_phy_control |=
> > +			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY0, DPIO_CH0);
> > +		mask = (status & DPLL_PORTC_READY_MASK) >> 4;
> > +		dev_priv->chv_phy_control |=
> > +			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY0, DPIO_CH1);
> > +
> >   		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY0);
> > -	if (cmn_d->ops->is_enabled(dev_priv, cmn_d))
> > +	} else {
> > +		dev_priv->chv_phy_control |=
> > +			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH0) |
> > +			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH1);
> > +	}
> > +
> > +	if (cmn_d->ops->is_enabled(dev_priv, cmn_d)) {
> > +		uint32_t status = I915_READ(DPIO_PHY_STATUS);
> > +		unsigned int mask;
> > +
> > +		mask = status & DPLL_PORTD_READY_MASK;
> > +		dev_priv->chv_phy_control |=
> > +			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY1, DPIO_CH0);
> > +
> >   		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY1);
> > +	} else {
> > +		dev_priv->chv_phy_control |=
> > +			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY1, DPIO_CH0);
> > +	}
> > +
> > +	DRM_DEBUG_KMS("Initial DPIO_PHY_CONTROL=0x%08x\n",
> > +		      dev_priv->chv_phy_control);
> >   }
> >   
> >   static void vlv_cmnlane_wa(struct drm_i915_private *dev_priv)
> 

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


-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/7] drm/i915: Implement PHY lane power gating for CHV

[Deepak S]

On Friday 08 May 2015 09:35 PM, Ville Syrjälä wrote:
> On Fri, May 08, 2015 at 08:19:12PM +0530, Deepak S wrote:
>>
>> On Friday 10 April 2015 08:51 PM, ville.syrjala@linux.intel.com wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> Powergate the PHY lanes when they're not needed. For HDMI all four lanes
>>> are needed always, but for DP we can enable only the needed lanes. And
>>> when the port is not used all lanes can be power gated. This could
>>> reduce power consumption a bit when only a subset of the lanes in the
>>> PHY are required.
>>>
>>> A bit of extra care is needed to reconstruct the initial state of the
>>> DPIO_PHY_CONTROL register since we can't read it. So instead we read the
>>> actual lane status from the DPLL/PHY_STATUS registers and use that to
>>> determine which lanes ought to be powergated initially.
>>>
>>> Also sprinkle a few debug prints around so that we can monitor the
>>> DPIO_PHY_STATUS changes without having to read it and risk corrupting
>>> it.
>>>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/i915_reg.h         |  2 +
>>>    drivers/gpu/drm/i915/intel_dp.c         |  8 ++++
>>>    drivers/gpu/drm/i915/intel_drv.h        |  2 +
>>>    drivers/gpu/drm/i915/intel_hdmi.c       |  5 +++
>>>    drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++++++++--
>>>    5 files changed, 82 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>> index 977bad6..34c366a 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -1887,10 +1887,12 @@ enum skl_disp_power_wells {
>>>    #define DPIO_PHY_STATUS			(VLV_DISPLAY_BASE + 0x6240)
>>>    #define   DPLL_PORTD_READY_MASK		(0xf)
>>>    #define DISPLAY_PHY_CONTROL (VLV_DISPLAY_BASE + 0x60100)
>>> +#define   PHY_CH_POWER_DOWN_OVRD_EN(phy, ch)	(1 << (2*(phy)+(ch)+27))
>>>    #define   PHY_LDO_DELAY_0NS			0x0
>>>    #define   PHY_LDO_DELAY_200NS			0x1
>>>    #define   PHY_LDO_DELAY_600NS			0x2
>>>    #define   PHY_LDO_SEQ_DELAY(delay, phy)		((delay) << (2*(phy)+23))
>>> +#define   PHY_CH_POWER_DOWN_OVRD(mask, phy, ch)	((mask) << (8*(phy)+4*(ch)+11))
>>>    #define   PHY_CH_SU_PSR				0x1
>>>    #define   PHY_CH_DEEP_PSR			0x7
>>>    #define   PHY_CH_POWER_MODE(mode, phy, ch)	((mode) << (6*(phy)+3*(ch)+2))
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>> index ac38fd8..0b43f99 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -2336,6 +2336,8 @@ static void chv_post_disable_dp(struct intel_encoder *encoder)
>>>    
>>>    	intel_dp_link_down(intel_dp);
>>>    
>>> +	chv_powergate_phy_lanes(encoder, 0xf);
>>> +
>>>    	mutex_lock(&dev_priv->dpio_lock);
>>>    
>>>    	/* Propagate soft reset to data lane reset */
>>> @@ -2482,6 +2484,12 @@ static void intel_enable_dp(struct intel_encoder *encoder)
>>>    	if (IS_VALLEYVIEW(dev))
>>>    		vlv_init_panel_power_sequencer(intel_dp);
>>>    
>>> +	if (IS_CHERRYVIEW(dev)) {
>>> +		/* FIXME deal with lane reversal */
>>> +		lane_mask = 0xf & ~((1 << intel_dp->lane_count) - 1);
>>> +		chv_powergate_phy_lanes(encoder, lane_mask);
>>> +	}
>>> +
>>>    	intel_dp_enable_port(intel_dp);
>>>    
>>>    	edp_panel_vdd_on(intel_dp);
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>> index 3ec829a..54bcca8 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -1238,6 +1238,8 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
>>>    
>>>    void intel_display_set_init_power(struct drm_i915_private *dev, bool enable);
>>>    
>>> +void chv_powergate_phy_lanes(struct intel_encoder *encoder, unsigned int mask);
>>> +
>>>    /* intel_pm.c */
>>>    void intel_init_clock_gating(struct drm_device *dev);
>>>    void intel_suspend_hw(struct drm_device *dev);
>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>>> index 24b0aa1..f5842c3 100644
>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>>> @@ -916,6 +916,9 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
>>>    		I915_WRITE(intel_hdmi->hdmi_reg, temp);
>>>    		POSTING_READ(intel_hdmi->hdmi_reg);
>>>    	}
>>> +
>>> +	if (IS_CHERRYVIEW(dev))
>>> +		chv_powergate_phy_lanes(encoder, 0xf);
>>>    }
>>>    
>>>    static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)
>>> @@ -1634,6 +1637,8 @@ static void chv_hdmi_pre_enable(struct intel_encoder *encoder)
>>>    				   intel_crtc->config->has_hdmi_sink,
>>>    				   adjusted_mode);
>>>    
>>> +	chv_powergate_phy_lanes(encoder, 0x0);
>>> +
>>>    	intel_enable_hdmi(encoder);
>>>    
>>>    	vlv_wait_port_ready(dev_priv, dport, 0x0);
>>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>> index 5cd8a51..d9e00d3 100644
>>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>> @@ -668,6 +668,9 @@ static void chv_dpio_cmn_power_well_enable(struct drm_i915_private *dev_priv,
>>>    
>>>    	dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(phy);
>>>    	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
>>> +
>>> +	DRM_DEBUG_KMS("Enabled DPIO PHY%d (DPIO_PHY_CONTROL=0x%08x)\n",
>>> +		      phy, dev_priv->chv_phy_control);
>>>    }
>>>    
>>>    static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
>>> @@ -680,10 +683,15 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
>>>    
>>>    	if (power_well->data == PUNIT_POWER_WELL_DPIO_CMN_BC) {
>>>    		phy = DPIO_PHY0;
>>> +		dev_priv->chv_phy_control |=
>>> +			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH0) |
>>> +			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH1);
>>>    		assert_pll_disabled(dev_priv, PIPE_A);
>>>    		assert_pll_disabled(dev_priv, PIPE_B);
>>>    	} else {
>>>    		phy = DPIO_PHY1;
>>> +		dev_priv->chv_phy_control |=
>>> +			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH0);
>>>    		assert_pll_disabled(dev_priv, PIPE_C);
>>>    	}
>>>    
>>> @@ -691,6 +699,25 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
>>>    	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
>>>    
>>>    	vlv_set_power_well(dev_priv, power_well, false);
>>> +
>>> +	DRM_DEBUG_KMS("Disabled DPIO PHY%d (DPIO_PHY_CONTROL=0x%08x)\n",
>>> +		      phy, dev_priv->chv_phy_control);
>>> +}
>>> +
>>> +void chv_powergate_phy_lanes(struct intel_encoder *encoder, unsigned int mask)
>>> +{
>>> +	struct drm_device *dev = encoder->base.dev;
>>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>>> +	enum dpio_phy phy = DPIO_PHY(intel_crtc->pipe);
>>> +	enum dpio_channel ch = vlv_dport_to_channel(enc_to_dig_port(&encoder->base));
>>> +
>>> +	dev_priv->chv_phy_control &= ~PHY_CH_POWER_DOWN_OVRD(0xf, phy, ch);
>>> +	dev_priv->chv_phy_control |= PHY_CH_POWER_DOWN_OVRD(mask, phy, ch);
>>> +	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
>>> +
>>> +	DRM_DEBUG_KMS("Power gating DPIO PHY%d CH%d lanes 0x%x (DPIO_PHY_CONTROL=0x%08x)\n",
>>> +		      phy, ch, mask, dev_priv->chv_phy_control);
>>>    }
>>>    
>> Hi Ville,
>>
>> I think based on the spec, we needto program 0x8170 as 0xxxC3xxxx to affect the DDI0 power down
>> and this reigster to be programted beforeDPIO_PHY_CONTROL ?
> Doh. Nice catch. Looks like the dynamic power down bits default to off.
> I also found the same bit in CL2 DW6, which isn't mentioned by the spec.
> I'll cook up a patch to flip them and run a few tests to see if stuff
> still works.

Ok Thanks
One more Q. I think we need to re-program phy lanes power down override /enable values after power gating/ungating PHY0/1?

>
>> Thanks
>> Deepak
>>
>>>    static bool chv_pipe_power_well_enabled(struct drm_i915_private *dev_priv,
>>> @@ -1402,19 +1429,53 @@ static void chv_phy_control_init(struct drm_i915_private *dev_priv)
>>>    	 * DISPLAY_PHY_CONTROL can get corrupted if read. As a
>>>    	 * workaround never ever read DISPLAY_PHY_CONTROL, and
>>>    	 * instead maintain a shadow copy ourselves. Use the actual
>>> -	 * power well state to reconstruct the expected initial
>>> -	 * value.
>>> +	 * power well state and lane status to reconstruct the
>>> +	 * expected initial value.
>>>    	 */
>>>    	dev_priv->chv_phy_control =
>>> +		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY0, DPIO_CH0) |
>>> +		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY0, DPIO_CH1) |
>>> +		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY1, DPIO_CH0) |
>>>    		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY0) |
>>>    		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY1) |
>>>    		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH0) |
>>>    		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH1) |
>>>    		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY1, DPIO_CH0);
>>> -	if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc))
>>> +
>>> +	if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc)) {
>>> +		uint32_t status = I915_READ(DPLL(PIPE_A));
>>> +		unsigned int mask;
>>> +
>>> +		mask = status & DPLL_PORTB_READY_MASK;
>>> +		dev_priv->chv_phy_control |=
>>> +			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY0, DPIO_CH0);
>>> +		mask = (status & DPLL_PORTC_READY_MASK) >> 4;
>>> +		dev_priv->chv_phy_control |=
>>> +			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY0, DPIO_CH1);
>>> +
>>>    		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY0);
>>> -	if (cmn_d->ops->is_enabled(dev_priv, cmn_d))
>>> +	} else {
>>> +		dev_priv->chv_phy_control |=
>>> +			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH0) |
>>> +			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH1);
>>> +	}
>>> +
>>> +	if (cmn_d->ops->is_enabled(dev_priv, cmn_d)) {
>>> +		uint32_t status = I915_READ(DPIO_PHY_STATUS);
>>> +		unsigned int mask;
>>> +
>>> +		mask = status & DPLL_PORTD_READY_MASK;
>>> +		dev_priv->chv_phy_control |=
>>> +			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY1, DPIO_CH0);
>>> +
>>>    		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY1);
>>> +	} else {
>>> +		dev_priv->chv_phy_control |=
>>> +			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY1, DPIO_CH0);
>>> +	}
>>> +
>>> +	DRM_DEBUG_KMS("Initial DPIO_PHY_CONTROL=0x%08x\n",
>>> +		      dev_priv->chv_phy_control);
>>>    }
>>>    
>>>    static void vlv_cmnlane_wa(struct drm_i915_private *dev_priv)
>> _______________________________________________
>> 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 6/7] drm/i915: Implement PHY lane power gating for CHV

[Ville Syrjälä]

On Sat, May 09, 2015 at 11:05:27AM +0530, Deepak S wrote:
> 
> 
> On Friday 08 May 2015 09:35 PM, Ville Syrjälä wrote:
> > On Fri, May 08, 2015 at 08:19:12PM +0530, Deepak S wrote:
> >>
> >> On Friday 10 April 2015 08:51 PM, ville.syrjala@linux.intel.com wrote:
> >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>
> >>> Powergate the PHY lanes when they're not needed. For HDMI all four lanes
> >>> are needed always, but for DP we can enable only the needed lanes. And
> >>> when the port is not used all lanes can be power gated. This could
> >>> reduce power consumption a bit when only a subset of the lanes in the
> >>> PHY are required.
> >>>
> >>> A bit of extra care is needed to reconstruct the initial state of the
> >>> DPIO_PHY_CONTROL register since we can't read it. So instead we read the
> >>> actual lane status from the DPLL/PHY_STATUS registers and use that to
> >>> determine which lanes ought to be powergated initially.
> >>>
> >>> Also sprinkle a few debug prints around so that we can monitor the
> >>> DPIO_PHY_STATUS changes without having to read it and risk corrupting
> >>> it.
> >>>
> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> ---
> >>>    drivers/gpu/drm/i915/i915_reg.h         |  2 +
> >>>    drivers/gpu/drm/i915/intel_dp.c         |  8 ++++
> >>>    drivers/gpu/drm/i915/intel_drv.h        |  2 +
> >>>    drivers/gpu/drm/i915/intel_hdmi.c       |  5 +++
> >>>    drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++++++++--
> >>>    5 files changed, 82 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >>> index 977bad6..34c366a 100644
> >>> --- a/drivers/gpu/drm/i915/i915_reg.h
> >>> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >>> @@ -1887,10 +1887,12 @@ enum skl_disp_power_wells {
> >>>    #define DPIO_PHY_STATUS			(VLV_DISPLAY_BASE + 0x6240)
> >>>    #define   DPLL_PORTD_READY_MASK		(0xf)
> >>>    #define DISPLAY_PHY_CONTROL (VLV_DISPLAY_BASE + 0x60100)
> >>> +#define   PHY_CH_POWER_DOWN_OVRD_EN(phy, ch)	(1 << (2*(phy)+(ch)+27))
> >>>    #define   PHY_LDO_DELAY_0NS			0x0
> >>>    #define   PHY_LDO_DELAY_200NS			0x1
> >>>    #define   PHY_LDO_DELAY_600NS			0x2
> >>>    #define   PHY_LDO_SEQ_DELAY(delay, phy)		((delay) << (2*(phy)+23))
> >>> +#define   PHY_CH_POWER_DOWN_OVRD(mask, phy, ch)	((mask) << (8*(phy)+4*(ch)+11))
> >>>    #define   PHY_CH_SU_PSR				0x1
> >>>    #define   PHY_CH_DEEP_PSR			0x7
> >>>    #define   PHY_CH_POWER_MODE(mode, phy, ch)	((mode) << (6*(phy)+3*(ch)+2))
> >>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >>> index ac38fd8..0b43f99 100644
> >>> --- a/drivers/gpu/drm/i915/intel_dp.c
> >>> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >>> @@ -2336,6 +2336,8 @@ static void chv_post_disable_dp(struct intel_encoder *encoder)
> >>>    
> >>>    	intel_dp_link_down(intel_dp);
> >>>    
> >>> +	chv_powergate_phy_lanes(encoder, 0xf);
> >>> +
> >>>    	mutex_lock(&dev_priv->dpio_lock);
> >>>    
> >>>    	/* Propagate soft reset to data lane reset */
> >>> @@ -2482,6 +2484,12 @@ static void intel_enable_dp(struct intel_encoder *encoder)
> >>>    	if (IS_VALLEYVIEW(dev))
> >>>    		vlv_init_panel_power_sequencer(intel_dp);
> >>>    
> >>> +	if (IS_CHERRYVIEW(dev)) {
> >>> +		/* FIXME deal with lane reversal */
> >>> +		lane_mask = 0xf & ~((1 << intel_dp->lane_count) - 1);
> >>> +		chv_powergate_phy_lanes(encoder, lane_mask);
> >>> +	}
> >>> +
> >>>    	intel_dp_enable_port(intel_dp);
> >>>    
> >>>    	edp_panel_vdd_on(intel_dp);
> >>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >>> index 3ec829a..54bcca8 100644
> >>> --- a/drivers/gpu/drm/i915/intel_drv.h
> >>> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >>> @@ -1238,6 +1238,8 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
> >>>    
> >>>    void intel_display_set_init_power(struct drm_i915_private *dev, bool enable);
> >>>    
> >>> +void chv_powergate_phy_lanes(struct intel_encoder *encoder, unsigned int mask);
> >>> +
> >>>    /* intel_pm.c */
> >>>    void intel_init_clock_gating(struct drm_device *dev);
> >>>    void intel_suspend_hw(struct drm_device *dev);
> >>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> >>> index 24b0aa1..f5842c3 100644
> >>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> >>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> >>> @@ -916,6 +916,9 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
> >>>    		I915_WRITE(intel_hdmi->hdmi_reg, temp);
> >>>    		POSTING_READ(intel_hdmi->hdmi_reg);
> >>>    	}
> >>> +
> >>> +	if (IS_CHERRYVIEW(dev))
> >>> +		chv_powergate_phy_lanes(encoder, 0xf);
> >>>    }
> >>>    
> >>>    static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)
> >>> @@ -1634,6 +1637,8 @@ static void chv_hdmi_pre_enable(struct intel_encoder *encoder)
> >>>    				   intel_crtc->config->has_hdmi_sink,
> >>>    				   adjusted_mode);
> >>>    
> >>> +	chv_powergate_phy_lanes(encoder, 0x0);
> >>> +
> >>>    	intel_enable_hdmi(encoder);
> >>>    
> >>>    	vlv_wait_port_ready(dev_priv, dport, 0x0);
> >>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> >>> index 5cd8a51..d9e00d3 100644
> >>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> >>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> >>> @@ -668,6 +668,9 @@ static void chv_dpio_cmn_power_well_enable(struct drm_i915_private *dev_priv,
> >>>    
> >>>    	dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(phy);
> >>>    	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
> >>> +
> >>> +	DRM_DEBUG_KMS("Enabled DPIO PHY%d (DPIO_PHY_CONTROL=0x%08x)\n",
> >>> +		      phy, dev_priv->chv_phy_control);
> >>>    }
> >>>    
> >>>    static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
> >>> @@ -680,10 +683,15 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
> >>>    
> >>>    	if (power_well->data == PUNIT_POWER_WELL_DPIO_CMN_BC) {
> >>>    		phy = DPIO_PHY0;
> >>> +		dev_priv->chv_phy_control |=
> >>> +			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH0) |
> >>> +			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH1);
> >>>    		assert_pll_disabled(dev_priv, PIPE_A);
> >>>    		assert_pll_disabled(dev_priv, PIPE_B);
> >>>    	} else {
> >>>    		phy = DPIO_PHY1;
> >>> +		dev_priv->chv_phy_control |=
> >>> +			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH0);
> >>>    		assert_pll_disabled(dev_priv, PIPE_C);
> >>>    	}
> >>>    
> >>> @@ -691,6 +699,25 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
> >>>    	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
> >>>    
> >>>    	vlv_set_power_well(dev_priv, power_well, false);
> >>> +
> >>> +	DRM_DEBUG_KMS("Disabled DPIO PHY%d (DPIO_PHY_CONTROL=0x%08x)\n",
> >>> +		      phy, dev_priv->chv_phy_control);
> >>> +}
> >>> +
> >>> +void chv_powergate_phy_lanes(struct intel_encoder *encoder, unsigned int mask)
> >>> +{
> >>> +	struct drm_device *dev = encoder->base.dev;
> >>> +	struct drm_i915_private *dev_priv = dev->dev_private;
> >>> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> >>> +	enum dpio_phy phy = DPIO_PHY(intel_crtc->pipe);
> >>> +	enum dpio_channel ch = vlv_dport_to_channel(enc_to_dig_port(&encoder->base));
> >>> +
> >>> +	dev_priv->chv_phy_control &= ~PHY_CH_POWER_DOWN_OVRD(0xf, phy, ch);
> >>> +	dev_priv->chv_phy_control |= PHY_CH_POWER_DOWN_OVRD(mask, phy, ch);
> >>> +	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
> >>> +
> >>> +	DRM_DEBUG_KMS("Power gating DPIO PHY%d CH%d lanes 0x%x (DPIO_PHY_CONTROL=0x%08x)\n",
> >>> +		      phy, ch, mask, dev_priv->chv_phy_control);
> >>>    }
> >>>    
> >> Hi Ville,
> >>
> >> I think based on the spec, we needto program 0x8170 as 0xxxC3xxxx to affect the DDI0 power down
> >> and this reigster to be programted beforeDPIO_PHY_CONTROL ?
> > Doh. Nice catch. Looks like the dynamic power down bits default to off.
> > I also found the same bit in CL2 DW6, which isn't mentioned by the spec.
> > I'll cook up a patch to flip them and run a few tests to see if stuff
> > still works.
> 
> Ok Thanks
> One more Q. I think we need to re-program phy lanes power down override /enable values after power gating/ungating PHY0/1?

I've taken a stab at this, and sadly its making things quite a bit more
complicated. Whenever the lane is power gated we also lose the relevant
registers, so if we power gate the unused lanes before programming all
the PCS/TX registers, we need to change the code so that it doesn't
try to program the registers in the power gated lanes.

Also I'm not entirely sure how LRC etc. works when some/all lanes power
gated. There are some notes that sort of make me think the hw internally
powers up everything regardless of the input signals to guarantee that
things are properly calibrated, but if we lose the registers does that
mean we lose the calibrationresults as well?

And on the other hand there are a bunch of other notes that make me think
they assumed we would only powergate the unused lanes after link training
has finished, which again suggests that we should train the link with max
lanes initially and drop the unused lanes afterwards. But then it's not
clear what happens if we need to increase the number of lanes afterwards
without doing a full PHY reinit.

I also had the machine hard hang if I enabled the CL1 PSR power down bit
in the single channel PHY, whereas setting it in the dual channel PHY is
apparently OK.

So looks like this topic needs a bit more study.

> 
> >
> >> Thanks
> >> Deepak
> >>
> >>>    static bool chv_pipe_power_well_enabled(struct drm_i915_private *dev_priv,
> >>> @@ -1402,19 +1429,53 @@ static void chv_phy_control_init(struct drm_i915_private *dev_priv)
> >>>    	 * DISPLAY_PHY_CONTROL can get corrupted if read. As a
> >>>    	 * workaround never ever read DISPLAY_PHY_CONTROL, and
> >>>    	 * instead maintain a shadow copy ourselves. Use the actual
> >>> -	 * power well state to reconstruct the expected initial
> >>> -	 * value.
> >>> +	 * power well state and lane status to reconstruct the
> >>> +	 * expected initial value.
> >>>    	 */
> >>>    	dev_priv->chv_phy_control =
> >>> +		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY0, DPIO_CH0) |
> >>> +		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY0, DPIO_CH1) |
> >>> +		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY1, DPIO_CH0) |
> >>>    		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY0) |
> >>>    		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY1) |
> >>>    		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH0) |
> >>>    		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH1) |
> >>>    		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY1, DPIO_CH0);
> >>> -	if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc))
> >>> +
> >>> +	if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc)) {
> >>> +		uint32_t status = I915_READ(DPLL(PIPE_A));
> >>> +		unsigned int mask;
> >>> +
> >>> +		mask = status & DPLL_PORTB_READY_MASK;
> >>> +		dev_priv->chv_phy_control |=
> >>> +			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY0, DPIO_CH0);
> >>> +		mask = (status & DPLL_PORTC_READY_MASK) >> 4;
> >>> +		dev_priv->chv_phy_control |=
> >>> +			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY0, DPIO_CH1);
> >>> +
> >>>    		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY0);
> >>> -	if (cmn_d->ops->is_enabled(dev_priv, cmn_d))
> >>> +	} else {
> >>> +		dev_priv->chv_phy_control |=
> >>> +			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH0) |
> >>> +			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH1);
> >>> +	}
> >>> +
> >>> +	if (cmn_d->ops->is_enabled(dev_priv, cmn_d)) {
> >>> +		uint32_t status = I915_READ(DPIO_PHY_STATUS);
> >>> +		unsigned int mask;
> >>> +
> >>> +		mask = status & DPLL_PORTD_READY_MASK;
> >>> +		dev_priv->chv_phy_control |=
> >>> +			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY1, DPIO_CH0);
> >>> +
> >>>    		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY1);
> >>> +	} else {
> >>> +		dev_priv->chv_phy_control |=
> >>> +			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY1, DPIO_CH0);
> >>> +	}
> >>> +
> >>> +	DRM_DEBUG_KMS("Initial DPIO_PHY_CONTROL=0x%08x\n",
> >>> +		      dev_priv->chv_phy_control);
> >>>    }
> >>>    
> >>>    static void vlv_cmnlane_wa(struct drm_i915_private *dev_priv)
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/7] drm/i915: Implement PHY lane power gating for CHV

[Deepak S]

On Monday 11 May 2015 05:13 PM, Ville Syrjälä wrote:
> On Sat, May 09, 2015 at 11:05:27AM +0530, Deepak S wrote:
>>
>> On Friday 08 May 2015 09:35 PM, Ville Syrjälä wrote:
>>> On Fri, May 08, 2015 at 08:19:12PM +0530, Deepak S wrote:
>>>> On Friday 10 April 2015 08:51 PM, ville.syrjala@linux.intel.com wrote:
>>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>>
>>>>> Powergate the PHY lanes when they're not needed. For HDMI all four lanes
>>>>> are needed always, but for DP we can enable only the needed lanes. And
>>>>> when the port is not used all lanes can be power gated. This could
>>>>> reduce power consumption a bit when only a subset of the lanes in the
>>>>> PHY are required.
>>>>>
>>>>> A bit of extra care is needed to reconstruct the initial state of the
>>>>> DPIO_PHY_CONTROL register since we can't read it. So instead we read the
>>>>> actual lane status from the DPLL/PHY_STATUS registers and use that to
>>>>> determine which lanes ought to be powergated initially.
>>>>>
>>>>> Also sprinkle a few debug prints around so that we can monitor the
>>>>> DPIO_PHY_STATUS changes without having to read it and risk corrupting
>>>>> it.
>>>>>
>>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>> ---
>>>>>     drivers/gpu/drm/i915/i915_reg.h         |  2 +
>>>>>     drivers/gpu/drm/i915/intel_dp.c         |  8 ++++
>>>>>     drivers/gpu/drm/i915/intel_drv.h        |  2 +
>>>>>     drivers/gpu/drm/i915/intel_hdmi.c       |  5 +++
>>>>>     drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++++++++--
>>>>>     5 files changed, 82 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>>>> index 977bad6..34c366a 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>>> @@ -1887,10 +1887,12 @@ enum skl_disp_power_wells {
>>>>>     #define DPIO_PHY_STATUS			(VLV_DISPLAY_BASE + 0x6240)
>>>>>     #define   DPLL_PORTD_READY_MASK		(0xf)
>>>>>     #define DISPLAY_PHY_CONTROL (VLV_DISPLAY_BASE + 0x60100)
>>>>> +#define   PHY_CH_POWER_DOWN_OVRD_EN(phy, ch)	(1 << (2*(phy)+(ch)+27))
>>>>>     #define   PHY_LDO_DELAY_0NS			0x0
>>>>>     #define   PHY_LDO_DELAY_200NS			0x1
>>>>>     #define   PHY_LDO_DELAY_600NS			0x2
>>>>>     #define   PHY_LDO_SEQ_DELAY(delay, phy)		((delay) << (2*(phy)+23))
>>>>> +#define   PHY_CH_POWER_DOWN_OVRD(mask, phy, ch)	((mask) << (8*(phy)+4*(ch)+11))
>>>>>     #define   PHY_CH_SU_PSR				0x1
>>>>>     #define   PHY_CH_DEEP_PSR			0x7
>>>>>     #define   PHY_CH_POWER_MODE(mode, phy, ch)	((mode) << (6*(phy)+3*(ch)+2))
>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>>> index ac38fd8..0b43f99 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>>> @@ -2336,6 +2336,8 @@ static void chv_post_disable_dp(struct intel_encoder *encoder)
>>>>>     
>>>>>     	intel_dp_link_down(intel_dp);
>>>>>     
>>>>> +	chv_powergate_phy_lanes(encoder, 0xf);
>>>>> +
>>>>>     	mutex_lock(&dev_priv->dpio_lock);
>>>>>     
>>>>>     	/* Propagate soft reset to data lane reset */
>>>>> @@ -2482,6 +2484,12 @@ static void intel_enable_dp(struct intel_encoder *encoder)
>>>>>     	if (IS_VALLEYVIEW(dev))
>>>>>     		vlv_init_panel_power_sequencer(intel_dp);
>>>>>     
>>>>> +	if (IS_CHERRYVIEW(dev)) {
>>>>> +		/* FIXME deal with lane reversal */
>>>>> +		lane_mask = 0xf & ~((1 << intel_dp->lane_count) - 1);
>>>>> +		chv_powergate_phy_lanes(encoder, lane_mask);
>>>>> +	}
>>>>> +
>>>>>     	intel_dp_enable_port(intel_dp);
>>>>>     
>>>>>     	edp_panel_vdd_on(intel_dp);
>>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>>>> index 3ec829a..54bcca8 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>>> @@ -1238,6 +1238,8 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
>>>>>     
>>>>>     void intel_display_set_init_power(struct drm_i915_private *dev, bool enable);
>>>>>     
>>>>> +void chv_powergate_phy_lanes(struct intel_encoder *encoder, unsigned int mask);
>>>>> +
>>>>>     /* intel_pm.c */
>>>>>     void intel_init_clock_gating(struct drm_device *dev);
>>>>>     void intel_suspend_hw(struct drm_device *dev);
>>>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>>>>> index 24b0aa1..f5842c3 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>>>>> @@ -916,6 +916,9 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
>>>>>     		I915_WRITE(intel_hdmi->hdmi_reg, temp);
>>>>>     		POSTING_READ(intel_hdmi->hdmi_reg);
>>>>>     	}
>>>>> +
>>>>> +	if (IS_CHERRYVIEW(dev))
>>>>> +		chv_powergate_phy_lanes(encoder, 0xf);
>>>>>     }
>>>>>     
>>>>>     static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)
>>>>> @@ -1634,6 +1637,8 @@ static void chv_hdmi_pre_enable(struct intel_encoder *encoder)
>>>>>     				   intel_crtc->config->has_hdmi_sink,
>>>>>     				   adjusted_mode);
>>>>>     
>>>>> +	chv_powergate_phy_lanes(encoder, 0x0);
>>>>> +
>>>>>     	intel_enable_hdmi(encoder);
>>>>>     
>>>>>     	vlv_wait_port_ready(dev_priv, dport, 0x0);
>>>>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>>>> index 5cd8a51..d9e00d3 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>>>> @@ -668,6 +668,9 @@ static void chv_dpio_cmn_power_well_enable(struct drm_i915_private *dev_priv,
>>>>>     
>>>>>     	dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(phy);
>>>>>     	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
>>>>> +
>>>>> +	DRM_DEBUG_KMS("Enabled DPIO PHY%d (DPIO_PHY_CONTROL=0x%08x)\n",
>>>>> +		      phy, dev_priv->chv_phy_control);
>>>>>     }
>>>>>     
>>>>>     static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
>>>>> @@ -680,10 +683,15 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
>>>>>     
>>>>>     	if (power_well->data == PUNIT_POWER_WELL_DPIO_CMN_BC) {
>>>>>     		phy = DPIO_PHY0;
>>>>> +		dev_priv->chv_phy_control |=
>>>>> +			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH0) |
>>>>> +			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH1);
>>>>>     		assert_pll_disabled(dev_priv, PIPE_A);
>>>>>     		assert_pll_disabled(dev_priv, PIPE_B);
>>>>>     	} else {
>>>>>     		phy = DPIO_PHY1;
>>>>> +		dev_priv->chv_phy_control |=
>>>>> +			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH0);
>>>>>     		assert_pll_disabled(dev_priv, PIPE_C);
>>>>>     	}
>>>>>     
>>>>> @@ -691,6 +699,25 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
>>>>>     	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
>>>>>     
>>>>>     	vlv_set_power_well(dev_priv, power_well, false);
>>>>> +
>>>>> +	DRM_DEBUG_KMS("Disabled DPIO PHY%d (DPIO_PHY_CONTROL=0x%08x)\n",
>>>>> +		      phy, dev_priv->chv_phy_control);
>>>>> +}
>>>>> +
>>>>> +void chv_powergate_phy_lanes(struct intel_encoder *encoder, unsigned int mask)
>>>>> +{
>>>>> +	struct drm_device *dev = encoder->base.dev;
>>>>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>>>> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>>>>> +	enum dpio_phy phy = DPIO_PHY(intel_crtc->pipe);
>>>>> +	enum dpio_channel ch = vlv_dport_to_channel(enc_to_dig_port(&encoder->base));
>>>>> +
>>>>> +	dev_priv->chv_phy_control &= ~PHY_CH_POWER_DOWN_OVRD(0xf, phy, ch);
>>>>> +	dev_priv->chv_phy_control |= PHY_CH_POWER_DOWN_OVRD(mask, phy, ch);
>>>>> +	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
>>>>> +
>>>>> +	DRM_DEBUG_KMS("Power gating DPIO PHY%d CH%d lanes 0x%x (DPIO_PHY_CONTROL=0x%08x)\n",
>>>>> +		      phy, ch, mask, dev_priv->chv_phy_control);
>>>>>     }
>>>>>     
>>>> Hi Ville,
>>>>
>>>> I think based on the spec, we needto program 0x8170 as 0xxxC3xxxx to affect the DDI0 power down
>>>> and this reigster to be programted beforeDPIO_PHY_CONTROL ?
>>> Doh. Nice catch. Looks like the dynamic power down bits default to off.
>>> I also found the same bit in CL2 DW6, which isn't mentioned by the spec.
>>> I'll cook up a patch to flip them and run a few tests to see if stuff
>>> still works.
>> Ok Thanks
>> One more Q. I think we need to re-program phy lanes power down override /enable values after power gating/ungating PHY0/1?
> I've taken a stab at this, and sadly its making things quite a bit more
> complicated. Whenever the lane is power gated we also lose the relevant
> registers, so if we power gate the unused lanes before programming all
> the PCS/TX registers, we need to change the code so that it doesn't
> try to program the registers in the power gated lanes.
>
> Also I'm not entirely sure how LRC etc. works when some/all lanes power
> gated. There are some notes that sort of make me think the hw internally
> powers up everything regardless of the input signals to guarantee that
> things are properly calibrated, but if we lose the registers does that
> mean we lose the calibrationresults as well?
>
> And on the other hand there are a bunch of other notes that make me think
> they assumed we would only powergate the unused lanes after link training
> has finished, which again suggests that we should train the link with max
> lanes initially and drop the unused lanes afterwards. But then it's not
> clear what happens if we need to increase the number of lanes afterwards
> without doing a full PHY reinit.
>
> I also had the machine hard hang if I enabled the CL1 PSR power down bit
> in the single channel PHY, whereas setting it in the dual channel PHY is
> apparently OK.
>
> So looks like this topic needs a bit more study.

Yes ville. I agree we need some more study. I will also do some
experiments to find the HW behavior in different scenario.

>>>> Thanks
>>>> Deepak
>>>>
>>>>>     static bool chv_pipe_power_well_enabled(struct drm_i915_private *dev_priv,
>>>>> @@ -1402,19 +1429,53 @@ static void chv_phy_control_init(struct drm_i915_private *dev_priv)
>>>>>     	 * DISPLAY_PHY_CONTROL can get corrupted if read. As a
>>>>>     	 * workaround never ever read DISPLAY_PHY_CONTROL, and
>>>>>     	 * instead maintain a shadow copy ourselves. Use the actual
>>>>> -	 * power well state to reconstruct the expected initial
>>>>> -	 * value.
>>>>> +	 * power well state and lane status to reconstruct the
>>>>> +	 * expected initial value.
>>>>>     	 */
>>>>>     	dev_priv->chv_phy_control =
>>>>> +		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY0, DPIO_CH0) |
>>>>> +		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY0, DPIO_CH1) |
>>>>> +		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY1, DPIO_CH0) |
>>>>>     		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY0) |
>>>>>     		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY1) |
>>>>>     		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH0) |
>>>>>     		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH1) |
>>>>>     		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY1, DPIO_CH0);
>>>>> -	if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc))
>>>>> +
>>>>> +	if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc)) {
>>>>> +		uint32_t status = I915_READ(DPLL(PIPE_A));
>>>>> +		unsigned int mask;
>>>>> +
>>>>> +		mask = status & DPLL_PORTB_READY_MASK;
>>>>> +		dev_priv->chv_phy_control |=
>>>>> +			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY0, DPIO_CH0);
>>>>> +		mask = (status & DPLL_PORTC_READY_MASK) >> 4;
>>>>> +		dev_priv->chv_phy_control |=
>>>>> +			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY0, DPIO_CH1);
>>>>> +
>>>>>     		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY0);
>>>>> -	if (cmn_d->ops->is_enabled(dev_priv, cmn_d))
>>>>> +	} else {
>>>>> +		dev_priv->chv_phy_control |=
>>>>> +			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH0) |
>>>>> +			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH1);
>>>>> +	}
>>>>> +
>>>>> +	if (cmn_d->ops->is_enabled(dev_priv, cmn_d)) {
>>>>> +		uint32_t status = I915_READ(DPIO_PHY_STATUS);
>>>>> +		unsigned int mask;
>>>>> +
>>>>> +		mask = status & DPLL_PORTD_READY_MASK;
>>>>> +		dev_priv->chv_phy_control |=
>>>>> +			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY1, DPIO_CH0);
>>>>> +
>>>>>     		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY1);
>>>>> +	} else {
>>>>> +		dev_priv->chv_phy_control |=
>>>>> +			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY1, DPIO_CH0);
>>>>> +	}
>>>>> +
>>>>> +	DRM_DEBUG_KMS("Initial DPIO_PHY_CONTROL=0x%08x\n",
>>>>> +		      dev_priv->chv_phy_control);
>>>>>     }
>>>>>     
>>>>>     static void vlv_cmnlane_wa(struct drm_i915_private *dev_priv)
>>>> _______________________________________________
>>>> 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 7/7] drm/i915: Throw out WIP CHV power well definitions

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Expecting CHV power wells to be just an extended versions of the VLV
power wells, a bunch of commented out power wells were added in
anticipation when Punit folks would implement it all. Turns out they
never did, and instead CHV has fewer power wells than VLV. Rip out all
the #if 0'ed junk that's not needed.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h         |  4 --
 drivers/gpu/drm/i915/intel_runtime_pm.c | 97 +--------------------------------
 2 files changed, 3 insertions(+), 98 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 34c366a..f2e0d58 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -595,10 +595,6 @@ enum punit_power_well {
 	PUNIT_POWER_WELL_DPIO_RX0		= 10,
 	PUNIT_POWER_WELL_DPIO_RX1		= 11,
 	PUNIT_POWER_WELL_DPIO_CMN_D		= 12,
-	/* FIXME: guesswork below */
-	PUNIT_POWER_WELL_DPIO_TX_D_LANES_01	= 13,
-	PUNIT_POWER_WELL_DPIO_TX_D_LANES_23	= 14,
-	PUNIT_POWER_WELL_DPIO_RX2		= 15,
 
 	PUNIT_POWER_WELL_NUM,
 };
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index d9e00d3..f208806 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -977,18 +977,6 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
 	BIT(POWER_DOMAIN_AUX_C) |		\
 	BIT(POWER_DOMAIN_INIT))
 
-#define CHV_PIPE_A_POWER_DOMAINS (	\
-	BIT(POWER_DOMAIN_PIPE_A) |	\
-	BIT(POWER_DOMAIN_INIT))
-
-#define CHV_PIPE_B_POWER_DOMAINS (	\
-	BIT(POWER_DOMAIN_PIPE_B) |	\
-	BIT(POWER_DOMAIN_INIT))
-
-#define CHV_PIPE_C_POWER_DOMAINS (	\
-	BIT(POWER_DOMAIN_PIPE_C) |	\
-	BIT(POWER_DOMAIN_INIT))
-
 #define CHV_DPIO_CMN_BC_POWER_DOMAINS (		\
 	BIT(POWER_DOMAIN_PORT_DDI_B_2_LANES) |	\
 	BIT(POWER_DOMAIN_PORT_DDI_B_4_LANES) |	\
@@ -1004,17 +992,6 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
 	BIT(POWER_DOMAIN_AUX_D) |		\
 	BIT(POWER_DOMAIN_INIT))
 
-#define CHV_DPIO_TX_D_LANES_01_POWER_DOMAINS (	\
-	BIT(POWER_DOMAIN_PORT_DDI_D_2_LANES) |	\
-	BIT(POWER_DOMAIN_PORT_DDI_D_4_LANES) |	\
-	BIT(POWER_DOMAIN_AUX_D) |		\
-	BIT(POWER_DOMAIN_INIT))
-
-#define CHV_DPIO_TX_D_LANES_23_POWER_DOMAINS (	\
-	BIT(POWER_DOMAIN_PORT_DDI_D_4_LANES) |	\
-	BIT(POWER_DOMAIN_AUX_D) |		\
-	BIT(POWER_DOMAIN_INIT))
-
 static const struct i915_power_well_ops i9xx_always_on_power_well_ops = {
 	.sync_hw = i9xx_always_on_power_well_noop,
 	.enable = i9xx_always_on_power_well_noop,
@@ -1172,40 +1149,16 @@ static struct i915_power_well chv_power_wells[] = {
 		.domains = VLV_ALWAYS_ON_POWER_DOMAINS,
 		.ops = &i9xx_always_on_power_well_ops,
 	},
-#if 0
-	{
-		.name = "display",
-		.domains = VLV_DISPLAY_POWER_DOMAINS,
-		.data = PUNIT_POWER_WELL_DISP2D,
-		.ops = &vlv_display_power_well_ops,
-	},
-#endif
 	{
 		.name = "pipe-a",
 		/*
-		 * FIXME: pipe A power well seems to be the new disp2d well.
-		 * At least all registers seem to be housed there. Figure
-		 * out if this a a temporary situation in pre-production
-		 * hardware or a permanent state of affairs.
+		 * pipe A power well is the new disp2d well.
+		 * pipe B and C wells don't actually exist.
 		 */
-		.domains = CHV_PIPE_A_POWER_DOMAINS | VLV_DISPLAY_POWER_DOMAINS,
+		.domains = VLV_DISPLAY_POWER_DOMAINS,
 		.data = PIPE_A,
 		.ops = &chv_pipe_power_well_ops,
 	},
-#if 0
-	{
-		.name = "pipe-b",
-		.domains = CHV_PIPE_B_POWER_DOMAINS,
-		.data = PIPE_B,
-		.ops = &chv_pipe_power_well_ops,
-	},
-	{
-		.name = "pipe-c",
-		.domains = CHV_PIPE_C_POWER_DOMAINS,
-		.data = PIPE_C,
-		.ops = &chv_pipe_power_well_ops,
-	},
-#endif
 	{
 		.name = "dpio-common-bc",
 		.domains = CHV_DPIO_CMN_BC_POWER_DOMAINS,
@@ -1218,50 +1171,6 @@ static struct i915_power_well chv_power_wells[] = {
 		.data = PUNIT_POWER_WELL_DPIO_CMN_D,
 		.ops = &chv_dpio_cmn_power_well_ops,
 	},
-#if 0
-	{
-		.name = "dpio-tx-b-01",
-		.domains = VLV_DPIO_TX_B_LANES_01_POWER_DOMAINS |
-			   VLV_DPIO_TX_B_LANES_23_POWER_DOMAINS,
-		.ops = &vlv_dpio_power_well_ops,
-		.data = PUNIT_POWER_WELL_DPIO_TX_B_LANES_01,
-	},
-	{
-		.name = "dpio-tx-b-23",
-		.domains = VLV_DPIO_TX_B_LANES_01_POWER_DOMAINS |
-			   VLV_DPIO_TX_B_LANES_23_POWER_DOMAINS,
-		.ops = &vlv_dpio_power_well_ops,
-		.data = PUNIT_POWER_WELL_DPIO_TX_B_LANES_23,
-	},
-	{
-		.name = "dpio-tx-c-01",
-		.domains = VLV_DPIO_TX_C_LANES_01_POWER_DOMAINS |
-			   VLV_DPIO_TX_C_LANES_23_POWER_DOMAINS,
-		.ops = &vlv_dpio_power_well_ops,
-		.data = PUNIT_POWER_WELL_DPIO_TX_C_LANES_01,
-	},
-	{
-		.name = "dpio-tx-c-23",
-		.domains = VLV_DPIO_TX_C_LANES_01_POWER_DOMAINS |
-			   VLV_DPIO_TX_C_LANES_23_POWER_DOMAINS,
-		.ops = &vlv_dpio_power_well_ops,
-		.data = PUNIT_POWER_WELL_DPIO_TX_C_LANES_23,
-	},
-	{
-		.name = "dpio-tx-d-01",
-		.domains = CHV_DPIO_TX_D_LANES_01_POWER_DOMAINS |
-			   CHV_DPIO_TX_D_LANES_23_POWER_DOMAINS,
-		.ops = &vlv_dpio_power_well_ops,
-		.data = PUNIT_POWER_WELL_DPIO_TX_D_LANES_01,
-	},
-	{
-		.name = "dpio-tx-d-23",
-		.domains = CHV_DPIO_TX_D_LANES_01_POWER_DOMAINS |
-			   CHV_DPIO_TX_D_LANES_23_POWER_DOMAINS,
-		.ops = &vlv_dpio_power_well_ops,
-		.data = PUNIT_POWER_WELL_DPIO_TX_D_LANES_23,
-	},
-#endif
 };
 
 static struct i915_power_well *lookup_power_well(struct drm_i915_private *dev_priv,
-- 
2.0.5

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

Re: [PATCH 7/7] drm/i915: Throw out WIP CHV power well definitions

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6177
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -7              276/276              269/276
ILK                                  301/301              301/301
SNB                 -1              316/316              315/316
IVB                 -1              328/328              327/328
BYT                                  285/285              285/285
HSW                                  394/394              394/394
BDW                                  321/321              321/321
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*PNV  igt@gem_tiled_pread_pwrite      PASS(3)      FAIL(1)PASS(1)
 PNV  igt@gem_userptr_blits@coherency-sync      CRASH(2)PASS(3)      CRASH(1)PASS(1)
 PNV  igt@gem_userptr_blits@coherency-unsync      CRASH(2)PASS(3)      CRASH(2)
 PNV  igt@gen3_render_linear_blits      FAIL(4)PASS(3)      FAIL(2)
 PNV  igt@gen3_render_mixed_blits      FAIL(3)PASS(3)      FAIL(1)PASS(1)
 PNV  igt@gen3_render_tiledx_blits      FAIL(4)PASS(3)      FAIL(2)
 PNV  igt@gen3_render_tiledy_blits      FAIL(4)PASS(3)      FAIL(2)
*SNB  igt@kms_flip@dpms-vs-vblank-race      PASS(5)      DMESG_WARN(2)
(dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up
drm:intel_dp_start_link_train[i915]]*ERROR*failed_to_update_link_training@failed to update link training
drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_get_link_status@failed to get link status
 IVB  igt@gem_pwrite_pread@uncached-copy-performance      DMESG_WARN(1)PASS(3)      DMESG_WARN(1)PASS(1)
(dmesg patch applied)drm:i915_hangcheck_elapsed[i915]]*ERROR*Hangcheck_timer_elapsed...blitter_ring_idle@Hangcheck timer elapsed... blitter ring idle
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/7] drm/i915: Throw out WIP CHV power well definitions

[Deepak S]

On Friday 10 April 2015 08:51 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Expecting CHV power wells to be just an extended versions of the VLV
> power wells, a bunch of commented out power wells were added in
> anticipation when Punit folks would implement it all. Turns out they
> never did, and instead CHV has fewer power wells than VLV. Rip out all
> the #if 0'ed junk that's not needed.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_reg.h         |  4 --
>   drivers/gpu/drm/i915/intel_runtime_pm.c | 97 +--------------------------------
>   2 files changed, 3 insertions(+), 98 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 34c366a..f2e0d58 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -595,10 +595,6 @@ enum punit_power_well {
>   	PUNIT_POWER_WELL_DPIO_RX0		= 10,
>   	PUNIT_POWER_WELL_DPIO_RX1		= 11,
>   	PUNIT_POWER_WELL_DPIO_CMN_D		= 12,
> -	/* FIXME: guesswork below */
> -	PUNIT_POWER_WELL_DPIO_TX_D_LANES_01	= 13,
> -	PUNIT_POWER_WELL_DPIO_TX_D_LANES_23	= 14,
> -	PUNIT_POWER_WELL_DPIO_RX2		= 15,
>   
>   	PUNIT_POWER_WELL_NUM,
>   };
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index d9e00d3..f208806 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -977,18 +977,6 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>   	BIT(POWER_DOMAIN_AUX_C) |		\
>   	BIT(POWER_DOMAIN_INIT))
>   
> -#define CHV_PIPE_A_POWER_DOMAINS (	\
> -	BIT(POWER_DOMAIN_PIPE_A) |	\
> -	BIT(POWER_DOMAIN_INIT))
> -
> -#define CHV_PIPE_B_POWER_DOMAINS (	\
> -	BIT(POWER_DOMAIN_PIPE_B) |	\
> -	BIT(POWER_DOMAIN_INIT))
> -
> -#define CHV_PIPE_C_POWER_DOMAINS (	\
> -	BIT(POWER_DOMAIN_PIPE_C) |	\
> -	BIT(POWER_DOMAIN_INIT))
> -
>   #define CHV_DPIO_CMN_BC_POWER_DOMAINS (		\
>   	BIT(POWER_DOMAIN_PORT_DDI_B_2_LANES) |	\
>   	BIT(POWER_DOMAIN_PORT_DDI_B_4_LANES) |	\
> @@ -1004,17 +992,6 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>   	BIT(POWER_DOMAIN_AUX_D) |		\
>   	BIT(POWER_DOMAIN_INIT))
>   
> -#define CHV_DPIO_TX_D_LANES_01_POWER_DOMAINS (	\
> -	BIT(POWER_DOMAIN_PORT_DDI_D_2_LANES) |	\
> -	BIT(POWER_DOMAIN_PORT_DDI_D_4_LANES) |	\
> -	BIT(POWER_DOMAIN_AUX_D) |		\
> -	BIT(POWER_DOMAIN_INIT))
> -
> -#define CHV_DPIO_TX_D_LANES_23_POWER_DOMAINS (	\
> -	BIT(POWER_DOMAIN_PORT_DDI_D_4_LANES) |	\
> -	BIT(POWER_DOMAIN_AUX_D) |		\
> -	BIT(POWER_DOMAIN_INIT))
> -
>   static const struct i915_power_well_ops i9xx_always_on_power_well_ops = {
>   	.sync_hw = i9xx_always_on_power_well_noop,
>   	.enable = i9xx_always_on_power_well_noop,
> @@ -1172,40 +1149,16 @@ static struct i915_power_well chv_power_wells[] = {
>   		.domains = VLV_ALWAYS_ON_POWER_DOMAINS,
>   		.ops = &i9xx_always_on_power_well_ops,
>   	},
> -#if 0
> -	{
> -		.name = "display",
> -		.domains = VLV_DISPLAY_POWER_DOMAINS,
> -		.data = PUNIT_POWER_WELL_DISP2D,
> -		.ops = &vlv_display_power_well_ops,
> -	},
> -#endif
>   	{
>   		.name = "pipe-a",
>   		/*
> -		 * FIXME: pipe A power well seems to be the new disp2d well.
> -		 * At least all registers seem to be housed there. Figure
> -		 * out if this a a temporary situation in pre-production
> -		 * hardware or a permanent state of affairs.
> +		 * pipe A power well is the new disp2d well.
> +		 * pipe B and C wells don't actually exist.

Can we add a comment saying "enabling pipe A sub system will also enable pipe B & c"
Because it is confusing, We says pipe B and C wells don't actually exist,
then if we use PIPE B to drive. how is it working without powering up the well?

Other than this. patch looks fine
Reviewed-by:  Deepak S<deepak.s@linux.intel.com>

>   		 */
> -		.domains = CHV_PIPE_A_POWER_DOMAINS | VLV_DISPLAY_POWER_DOMAINS,
> +		.domains = VLV_DISPLAY_POWER_DOMAINS,
>   		.data = PIPE_A,
>   		.ops = &chv_pipe_power_well_ops,
>   	},
> -#if 0
> -	{
> -		.name = "pipe-b",
> -		.domains = CHV_PIPE_B_POWER_DOMAINS,
> -		.data = PIPE_B,
> -		.ops = &chv_pipe_power_well_ops,
> -	},
> -	{
> -		.name = "pipe-c",
> -		.domains = CHV_PIPE_C_POWER_DOMAINS,
> -		.data = PIPE_C,
> -		.ops = &chv_pipe_power_well_ops,
> -	},
> -#endif
>   	{
>   		.name = "dpio-common-bc",
>   		.domains = CHV_DPIO_CMN_BC_POWER_DOMAINS,
> @@ -1218,50 +1171,6 @@ static struct i915_power_well chv_power_wells[] = {
>   		.data = PUNIT_POWER_WELL_DPIO_CMN_D,
>   		.ops = &chv_dpio_cmn_power_well_ops,
>   	},
> -#if 0
> -	{
> -		.name = "dpio-tx-b-01",
> -		.domains = VLV_DPIO_TX_B_LANES_01_POWER_DOMAINS |
> -			   VLV_DPIO_TX_B_LANES_23_POWER_DOMAINS,
> -		.ops = &vlv_dpio_power_well_ops,
> -		.data = PUNIT_POWER_WELL_DPIO_TX_B_LANES_01,
> -	},
> -	{
> -		.name = "dpio-tx-b-23",
> -		.domains = VLV_DPIO_TX_B_LANES_01_POWER_DOMAINS |
> -			   VLV_DPIO_TX_B_LANES_23_POWER_DOMAINS,
> -		.ops = &vlv_dpio_power_well_ops,
> -		.data = PUNIT_POWER_WELL_DPIO_TX_B_LANES_23,
> -	},
> -	{
> -		.name = "dpio-tx-c-01",
> -		.domains = VLV_DPIO_TX_C_LANES_01_POWER_DOMAINS |
> -			   VLV_DPIO_TX_C_LANES_23_POWER_DOMAINS,
> -		.ops = &vlv_dpio_power_well_ops,
> -		.data = PUNIT_POWER_WELL_DPIO_TX_C_LANES_01,
> -	},
> -	{
> -		.name = "dpio-tx-c-23",
> -		.domains = VLV_DPIO_TX_C_LANES_01_POWER_DOMAINS |
> -			   VLV_DPIO_TX_C_LANES_23_POWER_DOMAINS,
> -		.ops = &vlv_dpio_power_well_ops,
> -		.data = PUNIT_POWER_WELL_DPIO_TX_C_LANES_23,
> -	},
> -	{
> -		.name = "dpio-tx-d-01",
> -		.domains = CHV_DPIO_TX_D_LANES_01_POWER_DOMAINS |
> -			   CHV_DPIO_TX_D_LANES_23_POWER_DOMAINS,
> -		.ops = &vlv_dpio_power_well_ops,
> -		.data = PUNIT_POWER_WELL_DPIO_TX_D_LANES_01,
> -	},
> -	{
> -		.name = "dpio-tx-d-23",
> -		.domains = CHV_DPIO_TX_D_LANES_01_POWER_DOMAINS |
> -			   CHV_DPIO_TX_D_LANES_23_POWER_DOMAINS,
> -		.ops = &vlv_dpio_power_well_ops,
> -		.data = PUNIT_POWER_WELL_DPIO_TX_D_LANES_23,
> -	},
> -#endif
>   };
>   
>   static struct i915_power_well *lookup_power_well(struct drm_i915_private *dev_priv,

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