[PATCH 1/3] drm/i915: clarify IBX dp workaround

[PATCH 1/3] drm/i915: clarify IBX dp workaround

[Daniel Vetter]

Instead of checking for !CPT, check for IBX to make it clearer that
this is a IBX-specific workaround. No functional change because we
smash the PPT PCH into the HAS_PCH_CPT check and atm DP isn't enabled
on the haswell LPT PCH yet.

See Bspec Vol 3, Part 3, Section 4.[3-5].1 about DP[BCD], bit 30 for
details of this workaround.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_dp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 9b2effc..df3b027 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1910,7 +1910,7 @@ intel_dp_link_down(struct intel_dp *intel_dp)
 			DP |= DP_LINK_TRAIN_OFF;
 	}
 
-	if (!HAS_PCH_CPT(dev) &&
+	if (HAS_PCH_IBX(dev) &&
 	    I915_READ(intel_dp->output_reg) & DP_PIPEB_SELECT) {
 		struct drm_crtc *crtc = intel_dp->base.base.crtc;
 
-- 
1.7.7.5

Re: [PATCH 1/3] drm/i915: clarify IBX dp workaround

[Chris Wilson]

On Wed, 30 May 2012 12:31:56 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Instead of checking for !CPT, check for IBX to make it clearer that
> this is a IBX-specific workaround. No functional change because we
> smash the PPT PCH into the HAS_PCH_CPT check and atm DP isn't enabled
> on the haswell LPT PCH yet.
> 
> See Bspec Vol 3, Part 3, Section 4.[3-5].1 about DP[BCD], bit 30 for
> details of this workaround.
> 
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/3] drm/i915: clarify IBX dp workaround

[Daniel Vetter]

On Wed, May 30, 2012 at 11:28:13AM +0100, Chris Wilson wrote:
> On Wed, 30 May 2012 12:31:56 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > Instead of checking for !CPT, check for IBX to make it clearer that
> > this is a IBX-specific workaround. No functional change because we
> > smash the PPT PCH into the HAS_PCH_CPT check and atm DP isn't enabled
> > on the haswell LPT PCH yet.
> > 
> > See Bspec Vol 3, Part 3, Section 4.[3-5].1 about DP[BCD], bit 30 for
> > details of this workaround.
> > 
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Queued for -next, thanks for the review. Can you also please look at the
other two patches again whether my replies are satisfactory?
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 1/3] drm/i915: clarify IBX dp workaround

[Chris Wilson]

On Fri, 1 Jun 2012 21:40:18 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, May 30, 2012 at 11:28:13AM +0100, Chris Wilson wrote:
> > On Wed, 30 May 2012 12:31:56 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > > Instead of checking for !CPT, check for IBX to make it clearer that
> > > this is a IBX-specific workaround. No functional change because we
> > > smash the PPT PCH into the HAS_PCH_CPT check and atm DP isn't enabled
> > > on the haswell LPT PCH yet.
> > > 
> > > See Bspec Vol 3, Part 3, Section 4.[3-5].1 about DP[BCD], bit 30 for
> > > details of this workaround.
> > > 
> > > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Queued for -next, thanks for the review. Can you also please look at the
> other two patches again whether my replies are satisfactory?

Sure, the replies were explanation enough to point out my mistake. As
always a reminder closer to the code would be useful...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 2/3] drm/i915: implement IBX hdmi transcoder select workaround

[Daniel Vetter]

Bspec Vol 3, Part 3, Section 3.8.1.1, bit 30:

"[DevIBX] Writing to this bit only takes effect when port is enabled.
Due to hardware issue it is required that this bit be cleared when port
is disabled. To clear this bit software must temporarily enable this
port on transcoder A."

Unfortunately the public Bspec misses totally out on the same language
for HDMIB. Internal Bspec also mentions that one of the bad
side-effects is that DPx can fail to light up on transcoder A if HDMIx
is disabled but using transcoder B.

I've found this while reviewing Bsepc. We already implement the same
workaround for the DP ports.

Also replace a magic 1 with PIPE_B I've found while looking through the
code.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_hdmi.c |   29 ++++++++++++++++++++++++++++-
 1 files changed, 28 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 4c6f141..4ebdcf1 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -382,7 +382,7 @@ static void intel_hdmi_mode_set(struct drm_encoder *encoder,
 
 	if (HAS_PCH_CPT(dev))
 		sdvox |= PORT_TRANS_SEL_CPT(intel_crtc->pipe);
-	else if (intel_crtc->pipe == 1)
+	else if (intel_crtc->pipe == PIPE_B)
 		sdvox |= SDVO_PIPE_B_SELECT;
 
 	I915_WRITE(intel_hdmi->sdvox_reg, sdvox);
@@ -405,6 +405,33 @@ static void intel_hdmi_dpms(struct drm_encoder *encoder, int mode)
 
 	temp = I915_READ(intel_hdmi->sdvox_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->crtc;
+		struct intel_crtc *intel_crtc;
+
+		if (crtc)
+			intel_crtc = to_intel_crtc(crtc);
+		else
+			intel_crtc = NULL;
+
+		if (mode != DRM_MODE_DPMS_ON && (temp & SDVO_PIPE_B_SELECT)) {
+			temp &= ~SDVO_PIPE_B_SELECT;
+			I915_WRITE(intel_hdmi->sdvox_reg, temp);
+			POSTING_READ(intel_hdmi->sdvox_reg);
+
+			if (intel_crtc)
+				intel_wait_for_vblank(dev, intel_crtc->pipe);
+			else
+				msleep(50);
+		} else if (mode == DRM_MODE_DPMS_ON){
+			/* Restore the transcoder select bit. */
+			if (intel_crtc->pipe == PIPE_B)
+				enable_bits |= SDVO_PIPE_B_SELECT;
+		}
+	}
+
 	/* HW workaround, need to toggle enable bit off and on for 12bpc, but
 	 * we do this anyway which shows more stable in testing.
 	 */
-- 
1.7.7.5

Re: [PATCH 2/3] drm/i915: implement IBX hdmi transcoder select workaround

[Chris Wilson]

On Wed, 30 May 2012 12:31:57 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Bspec Vol 3, Part 3, Section 3.8.1.1, bit 30:
> 
> "[DevIBX] Writing to this bit only takes effect when port is enabled.
> Due to hardware issue it is required that this bit be cleared when port
> is disabled. To clear this bit software must temporarily enable this
> port on transcoder A."
> 
> Unfortunately the public Bspec misses totally out on the same language
> for HDMIB. Internal Bspec also mentions that one of the bad
> side-effects is that DPx can fail to light up on transcoder A if HDMIx
> is disabled but using transcoder B.
> 
> I've found this while reviewing Bsepc. We already implement the same
> workaround for the DP ports.
> 
> Also replace a magic 1 with PIPE_B I've found while looking through the
> code.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_hdmi.c |   29 ++++++++++++++++++++++++++++-
>  1 files changed, 28 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 4c6f141..4ebdcf1 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -382,7 +382,7 @@ static void intel_hdmi_mode_set(struct drm_encoder *encoder,
>  
>  	if (HAS_PCH_CPT(dev))
>  		sdvox |= PORT_TRANS_SEL_CPT(intel_crtc->pipe);
> -	else if (intel_crtc->pipe == 1)
> +	else if (intel_crtc->pipe == PIPE_B)
>  		sdvox |= SDVO_PIPE_B_SELECT;
>  
>  	I915_WRITE(intel_hdmi->sdvox_reg, sdvox);
> @@ -405,6 +405,33 @@ static void intel_hdmi_dpms(struct drm_encoder *encoder, int mode)
>  
>  	temp = I915_READ(intel_hdmi->sdvox_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->crtc;
> +		struct intel_crtc *intel_crtc;
> +
> +		if (crtc)
> +			intel_crtc = to_intel_crtc(crtc);
> +		else
> +			intel_crtc = NULL;
If you extracted pipe = crtc ? to_intel_crtc(crtc)->pipe : -1; then that
would make the following code less ugly.
> +
> +		if (mode != DRM_MODE_DPMS_ON && (temp & SDVO_PIPE_B_SELECT)) {

Split this into a nested if:

 if (mode != DPMS_MODE_DPMS_ON) {
   if (temp & SDVO_PIPE_B_SELECT) {
    // w/a
   } 
 } else {
   // restore
 }

> +			temp &= ~SDVO_PIPE_B_SELECT;
> +			I915_WRITE(intel_hdmi->sdvox_reg, temp);
> +			POSTING_READ(intel_hdmi->sdvox_reg);

Should we not worry about the HW w/a requiring this to be written twice?

static void intel_hdmi_write_sdvox(intel_hdmi, u32 val)
 {
   struct drm_i915_private *dev_priv = intel_hdmi->base.dev->dev_private;

   I915_WRITE(intel_hdmi->sdvox_reg, val);
   POSTING_READ(intel_hdmi->sdvo_reg);

   /* HW workaround, need to write this twice for issues that may result
    * in first write getting masked.
    */
   if (dev_priv->info.has_pch_split) {
     I915_WRITE(intel_hdmi->sdvox_reg, val);
     POSTING_READ(intel_hdmi->sdvox_reg);
   }
 }
> +
> +			if (intel_crtc)
> +				intel_wait_for_vblank(dev, intel_crtc->pipe);
> +			else
> +				msleep(50);
> +		} else if (mode == DRM_MODE_DPMS_ON){
> +			/* Restore the transcoder select bit. */
> +			if (intel_crtc->pipe == PIPE_B)
> +				enable_bits |= SDVO_PIPE_B_SELECT;
> +		}
> +	}
> +
>  	/* HW workaround, need to toggle enable bit off and on for 12bpc, but
>  	 * we do this anyway which shows more stable in testing.
>  	 */

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 3/3] drm/i915: assert that the IBX port transcoder select w/a is implemented

[Daniel Vetter]

Let's be a bit more paranoid here.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3c71850..a9bc673 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1224,6 +1224,9 @@ static void assert_pch_dp_disabled(struct drm_i915_private *dev_priv,
 	WARN(dp_pipe_enabled(dev_priv, pipe, port_sel, val),
 	     "PCH DP (0x%08x) enabled on transcoder %c, should be disabled\n",
 	     reg, pipe_name(pipe));
+
+	WARN(HAS_PCH_IBX(dev_priv->dev) && (val & SDVO_PIPE_B_SELECT),
+	     "IBX PCH dp port still using transcoder B\n");
 }
 
 static void assert_pch_hdmi_disabled(struct drm_i915_private *dev_priv,
@@ -1233,6 +1236,9 @@ static void assert_pch_hdmi_disabled(struct drm_i915_private *dev_priv,
 	WARN(hdmi_pipe_enabled(dev_priv, val, pipe),
 	     "PCH HDMI (0x%08x) enabled on transcoder %c, should be disabled\n",
 	     reg, pipe_name(pipe));
+
+	WARN(HAS_PCH_IBX(dev_priv->dev) && (val & SDVO_PIPE_B_SELECT),
+	     "IBX PCH hdmi port still using transcoder B\n");
 }
 
 static void assert_pch_ports_disabled(struct drm_i915_private *dev_priv,
-- 
1.7.7.5

Re: [PATCH 3/3] drm/i915: assert that the IBX port transcoder select w/a is implemented

[Chris Wilson]

On Wed, 30 May 2012 12:31:58 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Let's be a bit more paranoid here.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3c71850..a9bc673 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1224,6 +1224,9 @@ static void assert_pch_dp_disabled(struct drm_i915_private *dev_priv,
>  	WARN(dp_pipe_enabled(dev_priv, pipe, port_sel, val),
>  	     "PCH DP (0x%08x) enabled on transcoder %c, should be disabled\n",
>  	     reg, pipe_name(pipe));
> +
> +	WARN(HAS_PCH_IBX(dev_priv->dev) && (val & SDVO_PIPE_B_SELECT),
> +	     "IBX PCH dp port still using transcoder B\n");

Only an issue if both pipe B selected and enabled, right?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/3] drm/i915: assert that the IBX port transcoder select w/a is implemented

[Daniel Vetter]

On Wed, May 30, 2012 at 11:27:46AM +0100, Chris Wilson wrote:
> On Wed, 30 May 2012 12:31:58 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > Let's be a bit more paranoid here.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |    6 ++++++
> >  1 files changed, 6 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 3c71850..a9bc673 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1224,6 +1224,9 @@ static void assert_pch_dp_disabled(struct drm_i915_private *dev_priv,
> >  	WARN(dp_pipe_enabled(dev_priv, pipe, port_sel, val),
> >  	     "PCH DP (0x%08x) enabled on transcoder %c, should be disabled\n",
> >  	     reg, pipe_name(pipe));
> > +
> > +	WARN(HAS_PCH_IBX(dev_priv->dev) && (val & SDVO_PIPE_B_SELECT),
> > +	     "IBX PCH dp port still using transcoder B\n");
> 
> Only an issue if both pipe B selected and enabled, right?

The problem is actually when it's disabled, but still selects transcoder
B. The complicating issue is that we can only change the transcoder while
the port is enabled.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48