Patchwork drm/i915: Fix PCH port pipe select in CPT disable paths

login
register
mail settings
Submitter Keng-Yu Lin
Date Dec. 20, 2011, 8:15 a.m.
Message ID <1324368909-9593-2-git-send-email-kengyu@canonical.com>
Download mbox | patch
Permalink /patch/132632/
State New
Headers show

Comments

Keng-Yu Lin - Dec. 20, 2011, 8:15 a.m.
From: Keith Packard <keithp@keithp.com>

CPT pipe select is different from previous generations (using two bits
instead of one). All of the paths from intel_disable_pch_ports were
not making this distinction.

Mode setting with pipe A turned off would then also force all outputs
on pipe B to get turned off as the disable code would mistakenly
decide that all of these outputs were on pipe A and turn them off.

This is an extension of the CPT DP disable fix (why didn't I fix this then?)

Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
(cherry picked from commit 1519b9956eb4b4180fa3f47c73341463cdcfaa37)

BugLink: http://bugs.launchpad.net/bugs/906756

Signed-off-by: Keng-Yu Lin <kengyu@canonical.com>
---
 drivers/gpu/drm/i915/i915_reg.h      |   13 ++----
 drivers/gpu/drm/i915/intel_display.c |   81 +++++++++++++++++++++++++++++++---
 2 files changed, 79 insertions(+), 15 deletions(-)
Seth Forshee - Jan. 3, 2012, 2:49 p.m.
On Tue, Dec 20, 2011 at 04:15:09PM +0800, Keng-Yu Lin wrote:
> From: Keith Packard <keithp@keithp.com>
> 
> CPT pipe select is different from previous generations (using two bits
> instead of one). All of the paths from intel_disable_pch_ports were
> not making this distinction.
> 
> Mode setting with pipe A turned off would then also force all outputs
> on pipe B to get turned off as the disable code would mistakenly
> decide that all of these outputs were on pipe A and turn them off.
> 
> This is an extension of the CPT DP disable fix (why didn't I fix this then?)
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> (cherry picked from commit 1519b9956eb4b4180fa3f47c73341463cdcfaa37)
> 
> BugLink: http://bugs.launchpad.net/bugs/906756
> 
> Signed-off-by: Keng-Yu Lin <kengyu@canonical.com>

I'd echo Sefan's comments, but otherwise:

Acked-by: Seth Forshee <seth.forshee@canonical.com>

> ---
>  drivers/gpu/drm/i915/i915_reg.h      |   13 ++----
>  drivers/gpu/drm/i915/intel_display.c |   81 +++++++++++++++++++++++++++++++---
>  2 files changed, 79 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 63162e6..9e69326 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1313,6 +1313,7 @@
>  #define   ADPA_PIPE_SELECT_MASK	(1<<30)
>  #define   ADPA_PIPE_A_SELECT	0
>  #define   ADPA_PIPE_B_SELECT	(1<<30)
> +#define   ADPA_PIPE_SELECT(pipe) ((pipe) << 30)
>  #define   ADPA_USE_VGA_HVPOLARITY (1<<15)
>  #define   ADPA_SETS_HVPOLARITY	0
>  #define   ADPA_VSYNC_CNTL_DISABLE (1<<11)
> @@ -1455,6 +1456,7 @@
>  /* Selects pipe B for LVDS data.  Must be set on pre-965. */
>  #define   LVDS_PIPEB_SELECT		(1 << 30)
>  #define   LVDS_PIPE_MASK		(1 << 30)
> +#define   LVDS_PIPE(pipe)		((pipe) << 30)
>  /* LVDS dithering flag on 965/g4x platform */
>  #define   LVDS_ENABLE_DITHER		(1 << 25)
>  /* LVDS sync polarity flags. Set to invert (i.e. negative) */
> @@ -1494,9 +1496,6 @@
>  #define   LVDS_B0B3_POWER_DOWN		(0 << 2)
>  #define   LVDS_B0B3_POWER_UP		(3 << 2)
>  
> -#define LVDS_PIPE_ENABLED(V, P) \
> -	(((V) & (LVDS_PIPE_MASK | LVDS_PORT_EN)) == ((P) << 30 | LVDS_PORT_EN))
> -
>  /* Video Data Island Packet control */
>  #define VIDEO_DIP_DATA		0x61178
>  #define VIDEO_DIP_CTL		0x61170
> @@ -3238,14 +3237,12 @@
>  #define  ADPA_CRT_HOTPLUG_VOLREF_475MV  (1<<17)
>  #define  ADPA_CRT_HOTPLUG_FORCE_TRIGGER (1<<16)
>  
> -#define ADPA_PIPE_ENABLED(V, P) \
> -	(((V) & (ADPA_TRANS_SELECT_MASK | ADPA_DAC_ENABLE)) == ((P) << 30 | ADPA_DAC_ENABLE))
> -
>  /* or SDVOB */
>  #define HDMIB   0xe1140
>  #define  PORT_ENABLE    (1 << 31)
>  #define  TRANSCODER_A   (0)
>  #define  TRANSCODER_B   (1 << 30)
> +#define  TRANSCODER(pipe)	((pipe) << 30)
>  #define  TRANSCODER_MASK   (1 << 30)
>  #define  COLOR_FORMAT_8bpc      (0)
>  #define  COLOR_FORMAT_12bpc     (3 << 26)
> @@ -3262,9 +3259,6 @@
>  #define  HSYNC_ACTIVE_HIGH      (1 << 3)
>  #define  PORT_DETECTED          (1 << 2)
>  
> -#define HDMI_PIPE_ENABLED(V, P) \
> -	(((V) & (TRANSCODER_MASK | PORT_ENABLE)) == ((P) << 30 | PORT_ENABLE))
> -
>  /* PCH SDVOB multiplex with HDMIB */
>  #define PCH_SDVOB	HDMIB
>  
> @@ -3331,6 +3325,7 @@
>  #define  PORT_TRANS_B_SEL_CPT	(1<<29)
>  #define  PORT_TRANS_C_SEL_CPT	(2<<29)
>  #define  PORT_TRANS_SEL_MASK	(3<<29)
> +#define  PORT_TRANS_SEL_CPT(pipe)	((pipe) << 29)
>  
>  #define TRANS_DP_CTL_A		0xe0300
>  #define TRANS_DP_CTL_B		0xe1300
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 50e4dd3..80ec58e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -979,6 +979,71 @@ static void assert_transcoder_disabled(struct drm_i915_private *dev_priv,
>  	     pipe_name(pipe));
>  }
>  
> +static bool dp_pipe_enabled(struct drm_i915_private *dev_priv, enum pipe pipe,
> +			    int reg, u32 port_sel, u32 val)
> +{
> +	if ((val & DP_PORT_EN) == 0)
> +		return false;
> +
> +	if (HAS_PCH_CPT(dev_priv->dev)) {
> +		u32	trans_dp_ctl_reg = TRANS_DP_CTL(pipe);
> +		u32	trans_dp_ctl = I915_READ(trans_dp_ctl_reg);
> +		if ((trans_dp_ctl & TRANS_DP_PORT_SEL_MASK) != port_sel)
> +			return false;
> +	} else {
> +		if ((val & DP_PIPE_MASK) != (pipe << 30))
> +			return false;
> +	}
> +	return true;
> +}
> +
> +static bool hdmi_pipe_enabled(struct drm_i915_private *dev_priv,
> +			      enum pipe pipe, u32 val)
> +{
> +	if ((val & PORT_ENABLE) == 0)
> +		return false;
> +
> +	if (HAS_PCH_CPT(dev_priv->dev)) {
> +		if ((val & PORT_TRANS_SEL_MASK) != PORT_TRANS_SEL_CPT(pipe))
> +			return false;
> +	} else {
> +		if ((val & TRANSCODER_MASK) != TRANSCODER(pipe))
> +			return false;
> +	}
> +	return true;
> +}
> +
> +static bool lvds_pipe_enabled(struct drm_i915_private *dev_priv,
> +			      enum pipe pipe, u32 val)
> +{
> +	if ((val & LVDS_PORT_EN) == 0)
> +		return false;
> +
> +	if (HAS_PCH_CPT(dev_priv->dev)) {
> +		if ((val & PORT_TRANS_SEL_MASK) != PORT_TRANS_SEL_CPT(pipe))
> +			return false;
> +	} else {
> +		if ((val & LVDS_PIPE_MASK) != LVDS_PIPE(pipe))
> +			return false;
> +	}
> +	return true;
> +}
> +
> +static bool adpa_pipe_enabled(struct drm_i915_private *dev_priv,
> +			      enum pipe pipe, u32 val)
> +{
> +	if ((val & ADPA_DAC_ENABLE) == 0)
> +		return false;
> +	if (HAS_PCH_CPT(dev_priv->dev)) {
> +		if ((val & PORT_TRANS_SEL_MASK) != PORT_TRANS_SEL_CPT(pipe))
> +			return false;
> +	} else {
> +		if ((val & ADPA_PIPE_SELECT_MASK) != ADPA_PIPE_SELECT(pipe))
> +			return false;
> +	}
> +	return true;
> +}
> +
>  static void assert_pch_dp_disabled(struct drm_i915_private *dev_priv,
>  				   enum pipe pipe, int reg)
>  {
> @@ -992,7 +1057,7 @@ static void assert_pch_hdmi_disabled(struct drm_i915_private *dev_priv,
>  				     enum pipe pipe, int reg)
>  {
>  	u32 val = I915_READ(reg);
> -	WARN(HDMI_PIPE_ENABLED(val, pipe),
> +	WARN(hdmi_pipe_enabled(dev_priv, val, pipe),
>  	     "PCH DP (0x%08x) enabled on transcoder %c, should be disabled\n",
>  	     reg, pipe_name(pipe));
>  }
> @@ -1009,13 +1074,13 @@ static void assert_pch_ports_disabled(struct drm_i915_private *dev_priv,
>  
>  	reg = PCH_ADPA;
>  	val = I915_READ(reg);
> -	WARN(ADPA_PIPE_ENABLED(val, pipe),
> +	WARN(adpa_pipe_enabled(dev_priv, val, pipe),
>  	     "PCH VGA enabled on transcoder %c, should be disabled\n",
>  	     pipe_name(pipe));
>  
>  	reg = PCH_LVDS;
>  	val = I915_READ(reg);
> -	WARN(LVDS_PIPE_ENABLED(val, pipe),
> +	WARN(lvds_pipe_enabled(dev_priv, val, pipe),
>  	     "PCH LVDS enabled on transcoder %c, should be disabled\n",
>  	     pipe_name(pipe));
>  
> @@ -1345,8 +1410,11 @@ static void disable_pch_hdmi(struct drm_i915_private *dev_priv,
>  			     enum pipe pipe, int reg)
>  {
>  	u32 val = I915_READ(reg);
> -	if (HDMI_PIPE_ENABLED(val, pipe))
> +	if (hdmi_pipe_enabled(dev_priv, val, pipe)) {
> +		DRM_DEBUG_KMS("Disabling pch HDMI %x on pipe %d\n",
> +			      reg, pipe);
>  		I915_WRITE(reg, val & ~PORT_ENABLE);
> +	}
>  }
>  
>  /* Disable any ports connected to this transcoder */
> @@ -1364,12 +1432,13 @@ static void intel_disable_pch_ports(struct drm_i915_private *dev_priv,
>  
>  	reg = PCH_ADPA;
>  	val = I915_READ(reg);
> -	if (ADPA_PIPE_ENABLED(val, pipe))
> +	if (adpa_pipe_enabled(dev_priv, val, pipe))
>  		I915_WRITE(reg, val & ~ADPA_DAC_ENABLE);
>  
>  	reg = PCH_LVDS;
>  	val = I915_READ(reg);
> -	if (LVDS_PIPE_ENABLED(val, pipe)) {
> +	if (lvds_pipe_enabled(dev_priv, val, pipe)) {
> +		DRM_DEBUG_KMS("disable lvds on pipe %d val 0x%08x\n", pipe, val);
>  		I915_WRITE(reg, val & ~LVDS_PORT_EN);
>  		POSTING_READ(reg);
>  		udelay(100);
> -- 
> 1.7.5.4
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

Patch

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 63162e6..9e69326 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1313,6 +1313,7 @@ 
 #define   ADPA_PIPE_SELECT_MASK	(1<<30)
 #define   ADPA_PIPE_A_SELECT	0
 #define   ADPA_PIPE_B_SELECT	(1<<30)
+#define   ADPA_PIPE_SELECT(pipe) ((pipe) << 30)
 #define   ADPA_USE_VGA_HVPOLARITY (1<<15)
 #define   ADPA_SETS_HVPOLARITY	0
 #define   ADPA_VSYNC_CNTL_DISABLE (1<<11)
@@ -1455,6 +1456,7 @@ 
 /* Selects pipe B for LVDS data.  Must be set on pre-965. */
 #define   LVDS_PIPEB_SELECT		(1 << 30)
 #define   LVDS_PIPE_MASK		(1 << 30)
+#define   LVDS_PIPE(pipe)		((pipe) << 30)
 /* LVDS dithering flag on 965/g4x platform */
 #define   LVDS_ENABLE_DITHER		(1 << 25)
 /* LVDS sync polarity flags. Set to invert (i.e. negative) */
@@ -1494,9 +1496,6 @@ 
 #define   LVDS_B0B3_POWER_DOWN		(0 << 2)
 #define   LVDS_B0B3_POWER_UP		(3 << 2)
 
-#define LVDS_PIPE_ENABLED(V, P) \
-	(((V) & (LVDS_PIPE_MASK | LVDS_PORT_EN)) == ((P) << 30 | LVDS_PORT_EN))
-
 /* Video Data Island Packet control */
 #define VIDEO_DIP_DATA		0x61178
 #define VIDEO_DIP_CTL		0x61170
@@ -3238,14 +3237,12 @@ 
 #define  ADPA_CRT_HOTPLUG_VOLREF_475MV  (1<<17)
 #define  ADPA_CRT_HOTPLUG_FORCE_TRIGGER (1<<16)
 
-#define ADPA_PIPE_ENABLED(V, P) \
-	(((V) & (ADPA_TRANS_SELECT_MASK | ADPA_DAC_ENABLE)) == ((P) << 30 | ADPA_DAC_ENABLE))
-
 /* or SDVOB */
 #define HDMIB   0xe1140
 #define  PORT_ENABLE    (1 << 31)
 #define  TRANSCODER_A   (0)
 #define  TRANSCODER_B   (1 << 30)
+#define  TRANSCODER(pipe)	((pipe) << 30)
 #define  TRANSCODER_MASK   (1 << 30)
 #define  COLOR_FORMAT_8bpc      (0)
 #define  COLOR_FORMAT_12bpc     (3 << 26)
@@ -3262,9 +3259,6 @@ 
 #define  HSYNC_ACTIVE_HIGH      (1 << 3)
 #define  PORT_DETECTED          (1 << 2)
 
-#define HDMI_PIPE_ENABLED(V, P) \
-	(((V) & (TRANSCODER_MASK | PORT_ENABLE)) == ((P) << 30 | PORT_ENABLE))
-
 /* PCH SDVOB multiplex with HDMIB */
 #define PCH_SDVOB	HDMIB
 
@@ -3331,6 +3325,7 @@ 
 #define  PORT_TRANS_B_SEL_CPT	(1<<29)
 #define  PORT_TRANS_C_SEL_CPT	(2<<29)
 #define  PORT_TRANS_SEL_MASK	(3<<29)
+#define  PORT_TRANS_SEL_CPT(pipe)	((pipe) << 29)
 
 #define TRANS_DP_CTL_A		0xe0300
 #define TRANS_DP_CTL_B		0xe1300
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 50e4dd3..80ec58e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -979,6 +979,71 @@  static void assert_transcoder_disabled(struct drm_i915_private *dev_priv,
 	     pipe_name(pipe));
 }
 
+static bool dp_pipe_enabled(struct drm_i915_private *dev_priv, enum pipe pipe,
+			    int reg, u32 port_sel, u32 val)
+{
+	if ((val & DP_PORT_EN) == 0)
+		return false;
+
+	if (HAS_PCH_CPT(dev_priv->dev)) {
+		u32	trans_dp_ctl_reg = TRANS_DP_CTL(pipe);
+		u32	trans_dp_ctl = I915_READ(trans_dp_ctl_reg);
+		if ((trans_dp_ctl & TRANS_DP_PORT_SEL_MASK) != port_sel)
+			return false;
+	} else {
+		if ((val & DP_PIPE_MASK) != (pipe << 30))
+			return false;
+	}
+	return true;
+}
+
+static bool hdmi_pipe_enabled(struct drm_i915_private *dev_priv,
+			      enum pipe pipe, u32 val)
+{
+	if ((val & PORT_ENABLE) == 0)
+		return false;
+
+	if (HAS_PCH_CPT(dev_priv->dev)) {
+		if ((val & PORT_TRANS_SEL_MASK) != PORT_TRANS_SEL_CPT(pipe))
+			return false;
+	} else {
+		if ((val & TRANSCODER_MASK) != TRANSCODER(pipe))
+			return false;
+	}
+	return true;
+}
+
+static bool lvds_pipe_enabled(struct drm_i915_private *dev_priv,
+			      enum pipe pipe, u32 val)
+{
+	if ((val & LVDS_PORT_EN) == 0)
+		return false;
+
+	if (HAS_PCH_CPT(dev_priv->dev)) {
+		if ((val & PORT_TRANS_SEL_MASK) != PORT_TRANS_SEL_CPT(pipe))
+			return false;
+	} else {
+		if ((val & LVDS_PIPE_MASK) != LVDS_PIPE(pipe))
+			return false;
+	}
+	return true;
+}
+
+static bool adpa_pipe_enabled(struct drm_i915_private *dev_priv,
+			      enum pipe pipe, u32 val)
+{
+	if ((val & ADPA_DAC_ENABLE) == 0)
+		return false;
+	if (HAS_PCH_CPT(dev_priv->dev)) {
+		if ((val & PORT_TRANS_SEL_MASK) != PORT_TRANS_SEL_CPT(pipe))
+			return false;
+	} else {
+		if ((val & ADPA_PIPE_SELECT_MASK) != ADPA_PIPE_SELECT(pipe))
+			return false;
+	}
+	return true;
+}
+
 static void assert_pch_dp_disabled(struct drm_i915_private *dev_priv,
 				   enum pipe pipe, int reg)
 {
@@ -992,7 +1057,7 @@  static void assert_pch_hdmi_disabled(struct drm_i915_private *dev_priv,
 				     enum pipe pipe, int reg)
 {
 	u32 val = I915_READ(reg);
-	WARN(HDMI_PIPE_ENABLED(val, pipe),
+	WARN(hdmi_pipe_enabled(dev_priv, val, pipe),
 	     "PCH DP (0x%08x) enabled on transcoder %c, should be disabled\n",
 	     reg, pipe_name(pipe));
 }
@@ -1009,13 +1074,13 @@  static void assert_pch_ports_disabled(struct drm_i915_private *dev_priv,
 
 	reg = PCH_ADPA;
 	val = I915_READ(reg);
-	WARN(ADPA_PIPE_ENABLED(val, pipe),
+	WARN(adpa_pipe_enabled(dev_priv, val, pipe),
 	     "PCH VGA enabled on transcoder %c, should be disabled\n",
 	     pipe_name(pipe));
 
 	reg = PCH_LVDS;
 	val = I915_READ(reg);
-	WARN(LVDS_PIPE_ENABLED(val, pipe),
+	WARN(lvds_pipe_enabled(dev_priv, val, pipe),
 	     "PCH LVDS enabled on transcoder %c, should be disabled\n",
 	     pipe_name(pipe));
 
@@ -1345,8 +1410,11 @@  static void disable_pch_hdmi(struct drm_i915_private *dev_priv,
 			     enum pipe pipe, int reg)
 {
 	u32 val = I915_READ(reg);
-	if (HDMI_PIPE_ENABLED(val, pipe))
+	if (hdmi_pipe_enabled(dev_priv, val, pipe)) {
+		DRM_DEBUG_KMS("Disabling pch HDMI %x on pipe %d\n",
+			      reg, pipe);
 		I915_WRITE(reg, val & ~PORT_ENABLE);
+	}
 }
 
 /* Disable any ports connected to this transcoder */
@@ -1364,12 +1432,13 @@  static void intel_disable_pch_ports(struct drm_i915_private *dev_priv,
 
 	reg = PCH_ADPA;
 	val = I915_READ(reg);
-	if (ADPA_PIPE_ENABLED(val, pipe))
+	if (adpa_pipe_enabled(dev_priv, val, pipe))
 		I915_WRITE(reg, val & ~ADPA_DAC_ENABLE);
 
 	reg = PCH_LVDS;
 	val = I915_READ(reg);
-	if (LVDS_PIPE_ENABLED(val, pipe)) {
+	if (lvds_pipe_enabled(dev_priv, val, pipe)) {
+		DRM_DEBUG_KMS("disable lvds on pipe %d val 0x%08x\n", pipe, val);
 		I915_WRITE(reg, val & ~LVDS_PORT_EN);
 		POSTING_READ(reg);
 		udelay(100);