diff mbox

[08/12] drm/i915: Fix the IBX transcoder B workarounds

Message ID 1430835458-11187-9-git-send-email-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjälä May 5, 2015, 2:17 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently the IBX transcoder B workarounds are not working correctly.
Well, the HDMI one seems to be working somewhat, but the DP one is
definitely busted.

After a bit of experimentation it looks like the best way to make this
work is first disable the port on transcoder B, and then re-enable it
transcoder A, and immediately disable it again.

We can also clean up the code by noting that we can't be called without
a valid crtc. And also note that port A on ILK does not need the
workaround, so let's check for that one too.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c   | 37 ++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_hdmi.c | 50 ++++++++++++++++++---------------------
 drivers/gpu/drm/i915/intel_sdvo.c | 41 +++++++++++++-------------------
 3 files changed, 60 insertions(+), 68 deletions(-)

Comments

Jesse Barnes May 21, 2015, 7:23 p.m. UTC | #1
On 05/05/2015 07:17 AM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently the IBX transcoder B workarounds are not working correctly.
> Well, the HDMI one seems to be working somewhat, but the DP one is
> definitely busted.
> 
> After a bit of experimentation it looks like the best way to make this
> work is first disable the port on transcoder B, and then re-enable it
> transcoder A, and immediately disable it again.
> 
> We can also clean up the code by noting that we can't be called without
> a valid crtc. And also note that port A on ILK does not need the
> workaround, so let's check for that one too.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c   | 37 ++++++++++++++++-------------
>  drivers/gpu/drm/i915/intel_hdmi.c | 50 ++++++++++++++++++---------------------
>  drivers/gpu/drm/i915/intel_sdvo.c | 41 +++++++++++++-------------------
>  3 files changed, 60 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 17b006c..3401cde 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3821,6 +3821,7 @@ static void
>  intel_dp_link_down(struct intel_dp *intel_dp)
>  {
>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct intel_crtc *crtc = to_intel_crtc(intel_dig_port->base.base.crtc);
>  	enum port port = intel_dig_port->port;
>  	struct drm_device *dev = intel_dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -3837,34 +3838,38 @@ intel_dp_link_down(struct intel_dp *intel_dp)
>  	if ((IS_GEN7(dev) && port == PORT_A) ||
>  	    (HAS_PCH_CPT(dev) && port != PORT_A)) {
>  		DP &= ~DP_LINK_TRAIN_MASK_CPT;
> -		I915_WRITE(intel_dp->output_reg, DP | DP_LINK_TRAIN_PAT_IDLE_CPT);
> +		DP |= DP_LINK_TRAIN_PAT_IDLE_CPT;
>  	} else {
>  		if (IS_CHERRYVIEW(dev))
>  			DP &= ~DP_LINK_TRAIN_MASK_CHV;
>  		else
>  			DP &= ~DP_LINK_TRAIN_MASK;
> -		I915_WRITE(intel_dp->output_reg, DP | DP_LINK_TRAIN_PAT_IDLE);
> +		DP |= DP_LINK_TRAIN_PAT_IDLE;
>  	}
> +	I915_WRITE(intel_dp->output_reg, DP);
>  	POSTING_READ(intel_dp->output_reg);
>  
> -	if (HAS_PCH_IBX(dev) &&
> -	    I915_READ(intel_dp->output_reg) & DP_PIPEB_SELECT) {
> -		/* Hardware workaround: leaving our transcoder select
> -		 * set to transcoder B while it's off will prevent the
> -		 * corresponding HDMI output on transcoder A.
> -		 *
> -		 * Combine this with another hardware workaround:
> -		 * transcoder select bit can only be cleared while the
> -		 * port is enabled.
> -		 */
> -		DP &= ~DP_PIPEB_SELECT;
> +	DP &= ~(DP_PORT_EN | DP_AUDIO_OUTPUT_ENABLE);
> +	I915_WRITE(intel_dp->output_reg, DP);
> +	POSTING_READ(intel_dp->output_reg);
> +
> +	/*
> +	 * HW workaround for IBX, we need to move the port
> +	 * to transcoder A after disabling it to allow the
> +	 * matching HDMI port to be enabled on transcoder A.
> +	 */
> +	if (HAS_PCH_IBX(dev) && crtc->pipe == PIPE_B && port != PORT_A) {
> +		/* always enable with pattern 1 (as per spec) */
> +		DP &= ~(DP_PIPEB_SELECT | DP_LINK_TRAIN_MASK);
> +		DP |= DP_PORT_EN | DP_LINK_TRAIN_PAT_1;
> +		I915_WRITE(intel_dp->output_reg, DP);
> +		POSTING_READ(intel_dp->output_reg);
> +
> +		DP &= ~DP_PORT_EN;
>  		I915_WRITE(intel_dp->output_reg, DP);
>  		POSTING_READ(intel_dp->output_reg);
>  	}
>  
> -	DP &= ~DP_AUDIO_OUTPUT_ENABLE;
> -	I915_WRITE(intel_dp->output_reg, DP & ~DP_PORT_EN);
> -	POSTING_READ(intel_dp->output_reg);
>  	msleep(intel_dp->panel_power_down_delay);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 308015e..9b9a69e 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1096,39 +1096,13 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
>  	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
>  	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
>  	u32 temp;
> -	u32 enable_bits = SDVO_ENABLE | SDVO_AUDIO_ENABLE;
>  
>  	if (crtc->config->has_audio)
>  		intel_audio_codec_disable(encoder);
>  
>  	temp = I915_READ(intel_hdmi->hdmi_reg);
>  
> -	/* HW workaround for IBX, we need to move the port to transcoder A
> -	 * before disabling it. */
> -	if (HAS_PCH_IBX(dev)) {
> -		struct drm_crtc *crtc = encoder->base.crtc;
> -		int pipe = crtc ? to_intel_crtc(crtc)->pipe : -1;
> -
> -		if (temp & SDVO_PIPE_B_SELECT) {
> -			temp &= ~SDVO_PIPE_B_SELECT;
> -			I915_WRITE(intel_hdmi->hdmi_reg, temp);
> -			POSTING_READ(intel_hdmi->hdmi_reg);
> -
> -			/* Again we need to write this twice. */
> -			I915_WRITE(intel_hdmi->hdmi_reg, temp);
> -			POSTING_READ(intel_hdmi->hdmi_reg);
> -
> -			/* Transcoder selection bits only update
> -			 * effectively on vblank. */
> -			if (crtc)
> -				intel_wait_for_vblank(dev, pipe);
> -			else
> -				msleep(50);
> -		}
> -	}
> -
> -	temp &= ~enable_bits;
> -
> +	temp &= ~(SDVO_ENABLE | SDVO_AUDIO_ENABLE);
>  	I915_WRITE(intel_hdmi->hdmi_reg, temp);
>  	POSTING_READ(intel_hdmi->hdmi_reg);
>  
> @@ -1136,6 +1110,28 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
>  		chv_powergate_phy_lanes(encoder, 0xf);
>  
>  	intel_hdmi->set_infoframes(&encoder->base, false, NULL);
> +
> +	/*
> +	 * HW workaround for IBX, we need to move the port
> +	 * to transcoder A after disabling it to allow the
> +	 * matching DP port to be enabled on transcoder A.
> +	 */
> +	if (HAS_PCH_IBX(dev) && crtc->pipe == PIPE_B) {
> +		temp &= ~SDVO_PIPE_B_SELECT;
> +		temp |= SDVO_ENABLE;
> +		/*
> +		 * HW workaround, need to write this twice for issue
> +		 * that may result in first write getting masked.
> +		 */
> +		I915_WRITE(intel_hdmi->hdmi_reg, temp);
> +		POSTING_READ(intel_hdmi->hdmi_reg);
> +		I915_WRITE(intel_hdmi->hdmi_reg, temp);
> +		POSTING_READ(intel_hdmi->hdmi_reg);
> +
> +		temp &= ~SDVO_ENABLE;
> +		I915_WRITE(intel_hdmi->hdmi_reg, temp);
> +		POSTING_READ(intel_hdmi->hdmi_reg);
> +	}
>  }
>  
>  static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index e3e9c98..4a87691 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -1437,6 +1437,7 @@ static void intel_disable_sdvo(struct intel_encoder *encoder)
>  {
>  	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
>  	struct intel_sdvo *intel_sdvo = to_sdvo(encoder);
> +	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
>  	u32 temp;
>  
>  	intel_sdvo_set_active_outputs(intel_sdvo, 0);
> @@ -1445,32 +1446,22 @@ static void intel_disable_sdvo(struct intel_encoder *encoder)
>  						   DRM_MODE_DPMS_OFF);
>  
>  	temp = I915_READ(intel_sdvo->sdvo_reg);
> -	if ((temp & SDVO_ENABLE) != 0) {
> -		/* HW workaround for IBX, we need to move the port to
> -		 * transcoder A before disabling it. */
> -		if (HAS_PCH_IBX(encoder->base.dev)) {
> -			struct drm_crtc *crtc = encoder->base.crtc;
> -			int pipe = crtc ? to_intel_crtc(crtc)->pipe : -1;
> -
> -			if (temp & SDVO_PIPE_B_SELECT) {
> -				temp &= ~SDVO_PIPE_B_SELECT;
> -				I915_WRITE(intel_sdvo->sdvo_reg, temp);
> -				POSTING_READ(intel_sdvo->sdvo_reg);
> -
> -				/* Again we need to write this twice. */
> -				I915_WRITE(intel_sdvo->sdvo_reg, temp);
> -				POSTING_READ(intel_sdvo->sdvo_reg);
> -
> -				/* Transcoder selection bits only update
> -				 * effectively on vblank. */
> -				if (crtc)
> -					intel_wait_for_vblank(encoder->base.dev, pipe);
> -				else
> -					msleep(50);
> -			}
> -		}
>  
> -		intel_sdvo_write_sdvox(intel_sdvo, temp & ~SDVO_ENABLE);
> +	temp &= ~SDVO_ENABLE;
> +	intel_sdvo_write_sdvox(intel_sdvo, temp);
> +
> +	/*
> +	 * HW workaround for IBX, we need to move the port
> +	 * to transcoder A after disabling it to allow the
> +	 * matching DP port to be enabled on transcoder A.
> +	 */
> +	if (HAS_PCH_IBX(dev_priv) && crtc->pipe == PIPE_B) {
> +		temp &= ~SDVO_PIPE_B_SELECT;
> +		temp |= SDVO_ENABLE;
> +		intel_sdvo_write_sdvox(intel_sdvo, temp);
> +
> +		temp &= ~SDVO_ENABLE;
> +		intel_sdvo_write_sdvox(intel_sdvo, temp);
>  	}
>  }
>  
> 

Cool, again, testing wins.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 17b006c..3401cde 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3821,6 +3821,7 @@  static void
 intel_dp_link_down(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct intel_crtc *crtc = to_intel_crtc(intel_dig_port->base.base.crtc);
 	enum port port = intel_dig_port->port;
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3837,34 +3838,38 @@  intel_dp_link_down(struct intel_dp *intel_dp)
 	if ((IS_GEN7(dev) && port == PORT_A) ||
 	    (HAS_PCH_CPT(dev) && port != PORT_A)) {
 		DP &= ~DP_LINK_TRAIN_MASK_CPT;
-		I915_WRITE(intel_dp->output_reg, DP | DP_LINK_TRAIN_PAT_IDLE_CPT);
+		DP |= DP_LINK_TRAIN_PAT_IDLE_CPT;
 	} else {
 		if (IS_CHERRYVIEW(dev))
 			DP &= ~DP_LINK_TRAIN_MASK_CHV;
 		else
 			DP &= ~DP_LINK_TRAIN_MASK;
-		I915_WRITE(intel_dp->output_reg, DP | DP_LINK_TRAIN_PAT_IDLE);
+		DP |= DP_LINK_TRAIN_PAT_IDLE;
 	}
+	I915_WRITE(intel_dp->output_reg, DP);
 	POSTING_READ(intel_dp->output_reg);
 
-	if (HAS_PCH_IBX(dev) &&
-	    I915_READ(intel_dp->output_reg) & DP_PIPEB_SELECT) {
-		/* Hardware workaround: leaving our transcoder select
-		 * set to transcoder B while it's off will prevent the
-		 * corresponding HDMI output on transcoder A.
-		 *
-		 * Combine this with another hardware workaround:
-		 * transcoder select bit can only be cleared while the
-		 * port is enabled.
-		 */
-		DP &= ~DP_PIPEB_SELECT;
+	DP &= ~(DP_PORT_EN | DP_AUDIO_OUTPUT_ENABLE);
+	I915_WRITE(intel_dp->output_reg, DP);
+	POSTING_READ(intel_dp->output_reg);
+
+	/*
+	 * HW workaround for IBX, we need to move the port
+	 * to transcoder A after disabling it to allow the
+	 * matching HDMI port to be enabled on transcoder A.
+	 */
+	if (HAS_PCH_IBX(dev) && crtc->pipe == PIPE_B && port != PORT_A) {
+		/* always enable with pattern 1 (as per spec) */
+		DP &= ~(DP_PIPEB_SELECT | DP_LINK_TRAIN_MASK);
+		DP |= DP_PORT_EN | DP_LINK_TRAIN_PAT_1;
+		I915_WRITE(intel_dp->output_reg, DP);
+		POSTING_READ(intel_dp->output_reg);
+
+		DP &= ~DP_PORT_EN;
 		I915_WRITE(intel_dp->output_reg, DP);
 		POSTING_READ(intel_dp->output_reg);
 	}
 
-	DP &= ~DP_AUDIO_OUTPUT_ENABLE;
-	I915_WRITE(intel_dp->output_reg, DP & ~DP_PORT_EN);
-	POSTING_READ(intel_dp->output_reg);
 	msleep(intel_dp->panel_power_down_delay);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 308015e..9b9a69e 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1096,39 +1096,13 @@  static void intel_disable_hdmi(struct intel_encoder *encoder)
 	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
 	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
 	u32 temp;
-	u32 enable_bits = SDVO_ENABLE | SDVO_AUDIO_ENABLE;
 
 	if (crtc->config->has_audio)
 		intel_audio_codec_disable(encoder);
 
 	temp = I915_READ(intel_hdmi->hdmi_reg);
 
-	/* HW workaround for IBX, we need to move the port to transcoder A
-	 * before disabling it. */
-	if (HAS_PCH_IBX(dev)) {
-		struct drm_crtc *crtc = encoder->base.crtc;
-		int pipe = crtc ? to_intel_crtc(crtc)->pipe : -1;
-
-		if (temp & SDVO_PIPE_B_SELECT) {
-			temp &= ~SDVO_PIPE_B_SELECT;
-			I915_WRITE(intel_hdmi->hdmi_reg, temp);
-			POSTING_READ(intel_hdmi->hdmi_reg);
-
-			/* Again we need to write this twice. */
-			I915_WRITE(intel_hdmi->hdmi_reg, temp);
-			POSTING_READ(intel_hdmi->hdmi_reg);
-
-			/* Transcoder selection bits only update
-			 * effectively on vblank. */
-			if (crtc)
-				intel_wait_for_vblank(dev, pipe);
-			else
-				msleep(50);
-		}
-	}
-
-	temp &= ~enable_bits;
-
+	temp &= ~(SDVO_ENABLE | SDVO_AUDIO_ENABLE);
 	I915_WRITE(intel_hdmi->hdmi_reg, temp);
 	POSTING_READ(intel_hdmi->hdmi_reg);
 
@@ -1136,6 +1110,28 @@  static void intel_disable_hdmi(struct intel_encoder *encoder)
 		chv_powergate_phy_lanes(encoder, 0xf);
 
 	intel_hdmi->set_infoframes(&encoder->base, false, NULL);
+
+	/*
+	 * HW workaround for IBX, we need to move the port
+	 * to transcoder A after disabling it to allow the
+	 * matching DP port to be enabled on transcoder A.
+	 */
+	if (HAS_PCH_IBX(dev) && crtc->pipe == PIPE_B) {
+		temp &= ~SDVO_PIPE_B_SELECT;
+		temp |= SDVO_ENABLE;
+		/*
+		 * HW workaround, need to write this twice for issue
+		 * that may result in first write getting masked.
+		 */
+		I915_WRITE(intel_hdmi->hdmi_reg, temp);
+		POSTING_READ(intel_hdmi->hdmi_reg);
+		I915_WRITE(intel_hdmi->hdmi_reg, temp);
+		POSTING_READ(intel_hdmi->hdmi_reg);
+
+		temp &= ~SDVO_ENABLE;
+		I915_WRITE(intel_hdmi->hdmi_reg, temp);
+		POSTING_READ(intel_hdmi->hdmi_reg);
+	}
 }
 
 static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index e3e9c98..4a87691 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1437,6 +1437,7 @@  static void intel_disable_sdvo(struct intel_encoder *encoder)
 {
 	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
 	struct intel_sdvo *intel_sdvo = to_sdvo(encoder);
+	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
 	u32 temp;
 
 	intel_sdvo_set_active_outputs(intel_sdvo, 0);
@@ -1445,32 +1446,22 @@  static void intel_disable_sdvo(struct intel_encoder *encoder)
 						   DRM_MODE_DPMS_OFF);
 
 	temp = I915_READ(intel_sdvo->sdvo_reg);
-	if ((temp & SDVO_ENABLE) != 0) {
-		/* HW workaround for IBX, we need to move the port to
-		 * transcoder A before disabling it. */
-		if (HAS_PCH_IBX(encoder->base.dev)) {
-			struct drm_crtc *crtc = encoder->base.crtc;
-			int pipe = crtc ? to_intel_crtc(crtc)->pipe : -1;
-
-			if (temp & SDVO_PIPE_B_SELECT) {
-				temp &= ~SDVO_PIPE_B_SELECT;
-				I915_WRITE(intel_sdvo->sdvo_reg, temp);
-				POSTING_READ(intel_sdvo->sdvo_reg);
-
-				/* Again we need to write this twice. */
-				I915_WRITE(intel_sdvo->sdvo_reg, temp);
-				POSTING_READ(intel_sdvo->sdvo_reg);
-
-				/* Transcoder selection bits only update
-				 * effectively on vblank. */
-				if (crtc)
-					intel_wait_for_vblank(encoder->base.dev, pipe);
-				else
-					msleep(50);
-			}
-		}
 
-		intel_sdvo_write_sdvox(intel_sdvo, temp & ~SDVO_ENABLE);
+	temp &= ~SDVO_ENABLE;
+	intel_sdvo_write_sdvox(intel_sdvo, temp);
+
+	/*
+	 * HW workaround for IBX, we need to move the port
+	 * to transcoder A after disabling it to allow the
+	 * matching DP port to be enabled on transcoder A.
+	 */
+	if (HAS_PCH_IBX(dev_priv) && crtc->pipe == PIPE_B) {
+		temp &= ~SDVO_PIPE_B_SELECT;
+		temp |= SDVO_ENABLE;
+		intel_sdvo_write_sdvox(intel_sdvo, temp);
+
+		temp &= ~SDVO_ENABLE;
+		intel_sdvo_write_sdvox(intel_sdvo, temp);
 	}
 }