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

[PATCH 1/2] 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.

v2: Implement suggestions from Chris Wilson:
- add pipe variable to cut down on code noise
- write the reg value twice to w/a hw issues (Bspec is unclear on
  which bit actually require the write twice stuff, but better be
  paranoid about it)
- untangle the if logic

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

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index b507d38..69637db 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -569,7 +569,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);
@@ -591,6 +591,36 @@ 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;
+		int pipe = crtc ? to_intel_crtc(crtc)->pipe : -1;
+
+		if (mode != DRM_MODE_DPMS_ON) {
+			if (temp & SDVO_PIPE_B_SELECT) {
+				temp &= ~SDVO_PIPE_B_SELECT;
+				I915_WRITE(intel_hdmi->sdvox_reg, temp);
+				POSTING_READ(intel_hdmi->sdvox_reg);
+
+				/* Again we need to write this twice. */
+				I915_WRITE(intel_hdmi->sdvox_reg, temp);
+				POSTING_READ(intel_hdmi->sdvox_reg);
+
+				/* Transcoder selection bits only update
+				 * effectively on vblank. */
+				if (crtc)
+					intel_wait_for_vblank(dev, pipe);
+				else
+					msleep(50);
+			}
+		} else {
+			/* Restore the transcoder select bit. */
+			if (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

[PATCH 2/2] 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 0161d94..5e76031 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1232,6 +1232,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,
@@ -1241,6 +1244,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 2/2] drm/i915: assert that the IBX port transcoder select w/a is implemented

[Chris Wilson]

On Tue,  5 Jun 2012 11:03:40 +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>

Happy with both these patches after our earlier discussions,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Daniel Vetter]

On Tue, Jun 05, 2012 at 10:47:58AM +0100, Chris Wilson wrote:
> On Tue,  5 Jun 2012 11:03:40 +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>
> 
> Happy with both these patches after our earlier discussions,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Both queued for -next, thanks for the review.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48