[v2,5/9] drm/i915: Fix 12bpc HDMI enable for IBX
diff mbox

Message ID 1430834787-10255-6-git-send-email-ville.syrjala@linux.intel.com
State New
Headers show

Commit Message

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

Follow the procedure listed in Bspec to toggle the port enable bit off
and on when enabling HDMI with 12bpc and pixel repeat on IBX. The old
code didn't actually enable the port before "toggling" the bit back off,
so the whole workaround was essentially a nop.

Also take the opportunity to clarify the code by splitting the gmch
platforms to a separate (much more straightforward) function.

v2: Rebased due to crtc->config changes

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

Comments

Chandra Konduru June 3, 2015, 8:52 p.m. UTC | #1
> -----Original Message-----

> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of

> ville.syrjala@linux.intel.com

> Sent: Tuesday, May 05, 2015 7:06 AM

> To: intel-gfx@lists.freedesktop.org

> Subject: [Intel-gfx] [PATCH v2 5/9] drm/i915: Fix 12bpc HDMI enable for IBX

> 

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

> 

> Follow the procedure listed in Bspec to toggle the port enable bit off

> and on when enabling HDMI with 12bpc and pixel repeat on IBX. The old

> code didn't actually enable the port before "toggling" the bit back off,

> so the whole workaround was essentially a nop.

> 

> Also take the opportunity to clarify the code by splitting the gmch

> platforms to a separate (much more straightforward) function.


Per prior reviews seen before, it would be better if there are
separate patches for fixing issue and refactoring code.

I think it is fine either way, but is bit easy for reviewing code
having separate patches.

With that,
Reviewed-by: Chandra Konduru <Chandra.konduru@intel.com>


> 

> v2: Rebased due to crtc->config changes

> 

> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---

>  drivers/gpu/drm/i915/intel_hdmi.c | 78 ++++++++++++++++++++++++++---------

> ----

>  1 file changed, 53 insertions(+), 25 deletions(-)

> 

> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c

> b/drivers/gpu/drm/i915/intel_hdmi.c

> index 2e98e33..766bdb1 100644

> --- a/drivers/gpu/drm/i915/intel_hdmi.c

> +++ b/drivers/gpu/drm/i915/intel_hdmi.c

> @@ -944,47 +944,73 @@ static void intel_enable_hdmi_audio(struct

> intel_encoder *encoder)

>  	intel_audio_codec_enable(encoder);

>  }

> 

> -static void intel_enable_hdmi(struct intel_encoder *encoder)

> +static void g4x_enable_hdmi(struct intel_encoder *encoder)

>  {

>  	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);

> +	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);


Unrelated change

>  	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);

>  	u32 temp;

> -	u32 enable_bits = SDVO_ENABLE;

> -

> -	if (intel_crtc->config->has_audio)

> -		enable_bits |= SDVO_AUDIO_ENABLE;

> 

>  	temp = I915_READ(intel_hdmi->hdmi_reg);

> 

> -	/* HW workaround for IBX, we need to move the port to transcoder A

> -	 * before disabling it, so restore the transcoder select bit here. */

> -	if (HAS_PCH_IBX(dev))

> -		enable_bits |= SDVO_PIPE_SEL(intel_crtc->pipe);

> +	temp |= SDVO_ENABLE;

> +	if (crtc->config->has_audio)

> +		temp |= SDVO_AUDIO_ENABLE;

> 

> -	/* HW workaround, need to toggle enable bit off and on for 12bpc, but

> -	 * we do this anyway which shows more stable in testing.

> -	 */

> -	if (HAS_PCH_SPLIT(dev)) {

> -		I915_WRITE(intel_hdmi->hdmi_reg, temp & ~SDVO_ENABLE);

> -		POSTING_READ(intel_hdmi->hdmi_reg);

> -	}

> +	I915_WRITE(intel_hdmi->hdmi_reg, temp);

> +	POSTING_READ(intel_hdmi->hdmi_reg);

> 

> -	temp |= enable_bits;

> +	if (crtc->config->has_audio)

> +		intel_enable_hdmi_audio(encoder);

> +}

> 

> +static void ibx_enable_hdmi(struct intel_encoder *encoder)

> +{

> +	struct drm_device *dev = encoder->base.dev;

> +	struct drm_i915_private *dev_priv = dev->dev_private;

> +	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);

> +	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);

> +	u32 temp;

> +

> +	temp = I915_READ(intel_hdmi->hdmi_reg);

> +

> +	temp |= SDVO_ENABLE;

> +	if (crtc->config->has_audio)

> +		temp |= SDVO_AUDIO_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);

> 

> -	/* HW workaround, need to write this twice for issue that may result

> -	 * in first write getting masked.

> +	/*

> +	 * HW workaround, need to toggle enable bit off and on

> +	 * for 12bpc with pixel repeat.

> +	 *

> +	 * FIXME: BSpec says this should be done at the end of

> +	 * of the modeset sequence, so not sure if this isn't too soon.

>  	 */

> -	if (HAS_PCH_SPLIT(dev)) {

> +	if (crtc->config->pipe_bpp > 24 &&

> +	    crtc->config->pixel_multiplier > 1) {

> +		I915_WRITE(intel_hdmi->hdmi_reg, temp & ~SDVO_ENABLE);

> +		POSTING_READ(intel_hdmi->hdmi_reg);

> +

> +		/*

> +		 * 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);

>  	}

> 

> -	if (intel_crtc->config->has_audio)

> +	if (crtc->config->has_audio)

>  		intel_enable_hdmi_audio(encoder);

>  }

> 

> @@ -1509,7 +1535,7 @@ static void vlv_hdmi_pre_enable(struct intel_encoder

> *encoder)

>  				   intel_crtc->config->has_hdmi_sink,

>  				   adjusted_mode);

> 

> -	intel_enable_hdmi(encoder);

> +	g4x_enable_hdmi(encoder);

> 

>  	vlv_wait_port_ready(dev_priv, dport, 0x0);

>  }

> @@ -1828,7 +1854,7 @@ static void chv_hdmi_pre_enable(struct intel_encoder

> *encoder)

> 

>  	chv_powergate_phy_lanes(encoder, 0x0);

> 

> -	intel_enable_hdmi(encoder);

> +	g4x_enable_hdmi(encoder);

> 

>  	vlv_wait_port_ready(dev_priv, dport, 0x0);

>  }

> @@ -2012,8 +2038,10 @@ void intel_hdmi_init(struct drm_device *dev, int

> hdmi_reg, enum port port)

>  		intel_encoder->pre_enable = intel_hdmi_pre_enable;

>  		if (HAS_PCH_CPT(dev))

>  			intel_encoder->enable = cpt_enable_hdmi;

> +		else if (HAS_PCH_IBX(dev))

> +			intel_encoder->enable = ibx_enable_hdmi;

>  		else

> -			intel_encoder->enable = intel_enable_hdmi;

> +			intel_encoder->enable = g4x_enable_hdmi;

>  	}

> 

>  	intel_encoder->type = INTEL_OUTPUT_HDMI;

> --

> 2.0.5

> 

> _______________________________________________

> Intel-gfx mailing list

> Intel-gfx@lists.freedesktop.org

> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 2e98e33..766bdb1 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -944,47 +944,73 @@  static void intel_enable_hdmi_audio(struct intel_encoder *encoder)
 	intel_audio_codec_enable(encoder);
 }
 
-static void intel_enable_hdmi(struct intel_encoder *encoder)
+static void g4x_enable_hdmi(struct intel_encoder *encoder)
 {
 	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);
+	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
 	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
 	u32 temp;
-	u32 enable_bits = SDVO_ENABLE;
-
-	if (intel_crtc->config->has_audio)
-		enable_bits |= SDVO_AUDIO_ENABLE;
 
 	temp = I915_READ(intel_hdmi->hdmi_reg);
 
-	/* HW workaround for IBX, we need to move the port to transcoder A
-	 * before disabling it, so restore the transcoder select bit here. */
-	if (HAS_PCH_IBX(dev))
-		enable_bits |= SDVO_PIPE_SEL(intel_crtc->pipe);
+	temp |= SDVO_ENABLE;
+	if (crtc->config->has_audio)
+		temp |= SDVO_AUDIO_ENABLE;
 
-	/* HW workaround, need to toggle enable bit off and on for 12bpc, but
-	 * we do this anyway which shows more stable in testing.
-	 */
-	if (HAS_PCH_SPLIT(dev)) {
-		I915_WRITE(intel_hdmi->hdmi_reg, temp & ~SDVO_ENABLE);
-		POSTING_READ(intel_hdmi->hdmi_reg);
-	}
+	I915_WRITE(intel_hdmi->hdmi_reg, temp);
+	POSTING_READ(intel_hdmi->hdmi_reg);
 
-	temp |= enable_bits;
+	if (crtc->config->has_audio)
+		intel_enable_hdmi_audio(encoder);
+}
 
+static void ibx_enable_hdmi(struct intel_encoder *encoder)
+{
+	struct drm_device *dev = encoder->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
+	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
+	u32 temp;
+
+	temp = I915_READ(intel_hdmi->hdmi_reg);
+
+	temp |= SDVO_ENABLE;
+	if (crtc->config->has_audio)
+		temp |= SDVO_AUDIO_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);
 
-	/* HW workaround, need to write this twice for issue that may result
-	 * in first write getting masked.
+	/*
+	 * HW workaround, need to toggle enable bit off and on
+	 * for 12bpc with pixel repeat.
+	 *
+	 * FIXME: BSpec says this should be done at the end of
+	 * of the modeset sequence, so not sure if this isn't too soon.
 	 */
-	if (HAS_PCH_SPLIT(dev)) {
+	if (crtc->config->pipe_bpp > 24 &&
+	    crtc->config->pixel_multiplier > 1) {
+		I915_WRITE(intel_hdmi->hdmi_reg, temp & ~SDVO_ENABLE);
+		POSTING_READ(intel_hdmi->hdmi_reg);
+
+		/*
+		 * 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);
 	}
 
-	if (intel_crtc->config->has_audio)
+	if (crtc->config->has_audio)
 		intel_enable_hdmi_audio(encoder);
 }
 
@@ -1509,7 +1535,7 @@  static void vlv_hdmi_pre_enable(struct intel_encoder *encoder)
 				   intel_crtc->config->has_hdmi_sink,
 				   adjusted_mode);
 
-	intel_enable_hdmi(encoder);
+	g4x_enable_hdmi(encoder);
 
 	vlv_wait_port_ready(dev_priv, dport, 0x0);
 }
@@ -1828,7 +1854,7 @@  static void chv_hdmi_pre_enable(struct intel_encoder *encoder)
 
 	chv_powergate_phy_lanes(encoder, 0x0);
 
-	intel_enable_hdmi(encoder);
+	g4x_enable_hdmi(encoder);
 
 	vlv_wait_port_ready(dev_priv, dport, 0x0);
 }
@@ -2012,8 +2038,10 @@  void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
 		intel_encoder->pre_enable = intel_hdmi_pre_enable;
 		if (HAS_PCH_CPT(dev))
 			intel_encoder->enable = cpt_enable_hdmi;
+		else if (HAS_PCH_IBX(dev))
+			intel_encoder->enable = ibx_enable_hdmi;
 		else
-			intel_encoder->enable = intel_enable_hdmi;
+			intel_encoder->enable = g4x_enable_hdmi;
 	}
 
 	intel_encoder->type = INTEL_OUTPUT_HDMI;