All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 00/14] More HDMI fixes V2
@ 2012-05-28 19:42 Paulo Zanoni
  2012-05-28 19:42 ` [PATCH 01/14] drm/i915: add set_infoframes to struct intel_hdmi Paulo Zanoni
                   ` (13 more replies)
  0 siblings, 14 replies; 19+ messages in thread
From: Paulo Zanoni @ 2012-05-28 19:42 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Following the comments I received and the discussions I had on IRC with Chris
and Daniel, here is the updated version. Even patch 0001 had changes, so I had
to do small adjustments on basically every single patch of the series so they
apply again.

Here is a list of changes:

0001: Add 'static' keywords (suggested by Chris)
0002: Small code change to be more defensive
0004: Add WARN to prevent future mistakes (suggested by Daniel)
0007: Small code change to be more defensive

New patches:
0012: drm/i915: clarify confusion between HDMI and SDVO registers
0013: drm/i915: add some barriers when changing DIPs
0014: drm/i915: make sure HDMI port is disabled inside set_infoframes

Patch 12 is my attempt to fix confusing names and prevent regressions like the
one that happened recently. Patch 13 was suggested by Chris and patch 14 was
suggested by Daniel.

So now the code seems pretty defensive :)

After this series is applied, I think we should go through bugzilla and start
suggesting people to test their HDMI bugs against the new tree.

Cheers,
Paulo

Paulo Zanoni (14):
  drm/i915: add set_infoframes to struct intel_hdmi
  drm/i915: properly alternate between DVI and HDMI
  drm/i915: only set the HDMI port on the DIP once
  drm/i915: enable DIP before enabling each InfoFrame
  drm/i915: don't wait for vblank while writing InfoFrames
  drm/i915: explicitly disable the DIPs we're not using
  drm/i915: disable DIP while changing the port
  drm/i915: don't write 0 to DIP control at HDMI init
  drm/i915: don't set SDVO_BORDER_ENABLE when we're HDMI
  drm/i915: rename sdvox_reg to hdmi_reg on HDMI context
  drm/i915: remove comment about HSW HDMI DIPs
  drm/i915: clarify confusion between HDMI and SDVO registers
  drm/i915: add some barriers when changing DIPs
  drm/i915: make sure HDMI port is disabled inside set_infoframes

 drivers/gpu/drm/i915/i915_reg.h      |   26 ++-
 drivers/gpu/drm/i915/intel_ddi.c     |    3 +-
 drivers/gpu/drm/i915/intel_display.c |   38 ++--
 drivers/gpu/drm/i915/intel_drv.h     |    9 +-
 drivers/gpu/drm/i915/intel_hdmi.c    |  377 +++++++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_sdvo.c    |   18 +-
 6 files changed, 334 insertions(+), 137 deletions(-)

-- 
1.7.10

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH 01/14] drm/i915: add set_infoframes to struct intel_hdmi
  2012-05-28 19:42 [PATCH 00/14] More HDMI fixes V2 Paulo Zanoni
@ 2012-05-28 19:42 ` Paulo Zanoni
  2012-05-28 19:42 ` [PATCH 02/14] drm/i915: properly alternate between DVI and HDMI Paulo Zanoni
                   ` (12 subsequent siblings)
  13 siblings, 0 replies; 19+ messages in thread
From: Paulo Zanoni @ 2012-05-28 19:42 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

We need a function that is able to fully 'set' the state of the DIP
registers to a known state.

Currently, we have the write_infoframe function that is called twice:
once for AVI and once for SPD. The problem is that write_infoframe
tries to keep the state of the DIP register as it is, changing only
the minimum necessary bits. The second problem is that
write_infoframe does twice (once for each time it is called) some
work that should be done only once (like waiting for vblank and
setting the port). If we add even more DIPs, it will do even more
repeated work.

This patch only adds the infrastructure keeping the code behavior the
same as before.

v2: add static keywords

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c  |    3 +--
 drivers/gpu/drm/i915/intel_drv.h  |    5 ++--
 drivers/gpu/drm/i915/intel_hdmi.c |   47 +++++++++++++++++++++++++++++++++----
 3 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 46d1e88..f33fe1a 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -726,8 +726,7 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
 
 	I915_WRITE(DDI_FUNC_CTL(pipe), temp);
 
-	intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
-	intel_hdmi_set_spd_infoframe(encoder);
+	intel_hdmi->set_infoframes(encoder, adjusted_mode);
 }
 
 void intel_ddi_dpms(struct drm_encoder *encoder, int mode)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3e09188..39d7b07 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -301,6 +301,8 @@ struct intel_hdmi {
 	enum hdmi_force_audio force_audio;
 	void (*write_infoframe)(struct drm_encoder *encoder,
 				struct dip_infoframe *frame);
+	void (*set_infoframes)(struct drm_encoder *encoder,
+			       struct drm_display_mode *adjusted_mode);
 };
 
 static inline struct drm_crtc *
@@ -343,9 +345,6 @@ extern void intel_attach_broadcast_rgb_property(struct drm_connector *connector)
 extern void intel_crt_init(struct drm_device *dev);
 extern void intel_hdmi_init(struct drm_device *dev, int sdvox_reg);
 extern struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder);
-extern void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
-			    struct drm_display_mode *adjusted_mode);
-extern void intel_hdmi_set_spd_infoframe(struct drm_encoder *encoder);
 extern void intel_dip_infoframe_csum(struct dip_infoframe *avi_if);
 extern bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg,
 			    bool is_sdvob);
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 4c6f141..8d892b0 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -315,7 +315,7 @@ static void intel_set_infoframe(struct drm_encoder *encoder,
 	intel_hdmi->write_infoframe(encoder, frame);
 }
 
-void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
+static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
 					 struct drm_display_mode *adjusted_mode)
 {
 	struct dip_infoframe avi_if = {
@@ -330,7 +330,7 @@ void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
 	intel_set_infoframe(encoder, &avi_if);
 }
 
-void intel_hdmi_set_spd_infoframe(struct drm_encoder *encoder)
+static void intel_hdmi_set_spd_infoframe(struct drm_encoder *encoder)
 {
 	struct dip_infoframe spd_if;
 
@@ -345,6 +345,41 @@ void intel_hdmi_set_spd_infoframe(struct drm_encoder *encoder)
 	intel_set_infoframe(encoder, &spd_if);
 }
 
+static void g4x_set_infoframes(struct drm_encoder *encoder,
+			       struct drm_display_mode *adjusted_mode)
+{
+	intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
+	intel_hdmi_set_spd_infoframe(encoder);
+}
+
+static void ibx_set_infoframes(struct drm_encoder *encoder,
+			       struct drm_display_mode *adjusted_mode)
+{
+	intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
+	intel_hdmi_set_spd_infoframe(encoder);
+}
+
+static void cpt_set_infoframes(struct drm_encoder *encoder,
+			       struct drm_display_mode *adjusted_mode)
+{
+	intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
+	intel_hdmi_set_spd_infoframe(encoder);
+}
+
+static void vlv_set_infoframes(struct drm_encoder *encoder,
+			       struct drm_display_mode *adjusted_mode)
+{
+	intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
+	intel_hdmi_set_spd_infoframe(encoder);
+}
+
+static void hsw_set_infoframes(struct drm_encoder *encoder,
+			       struct drm_display_mode *adjusted_mode)
+{
+	intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
+	intel_hdmi_set_spd_infoframe(encoder);
+}
+
 static void intel_hdmi_mode_set(struct drm_encoder *encoder,
 				struct drm_display_mode *mode,
 				struct drm_display_mode *adjusted_mode)
@@ -388,8 +423,7 @@ static void intel_hdmi_mode_set(struct drm_encoder *encoder,
 	I915_WRITE(intel_hdmi->sdvox_reg, sdvox);
 	POSTING_READ(intel_hdmi->sdvox_reg);
 
-	intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
-	intel_hdmi_set_spd_infoframe(encoder);
+	intel_hdmi->set_infoframes(encoder, adjusted_mode);
 }
 
 static void intel_hdmi_dpms(struct drm_encoder *encoder, int mode)
@@ -734,9 +768,11 @@ void intel_hdmi_init(struct drm_device *dev, int sdvox_reg)
 
 	if (!HAS_PCH_SPLIT(dev)) {
 		intel_hdmi->write_infoframe = g4x_write_infoframe;
+		intel_hdmi->set_infoframes = g4x_set_infoframes;
 		I915_WRITE(VIDEO_DIP_CTL, 0);
 	} else if (IS_VALLEYVIEW(dev)) {
 		intel_hdmi->write_infoframe = vlv_write_infoframe;
+		intel_hdmi->set_infoframes = vlv_set_infoframes;
 		for_each_pipe(i)
 			I915_WRITE(VLV_TVIDEO_DIP_CTL(i), 0);
 	} else if (IS_HASWELL(dev)) {
@@ -744,14 +780,17 @@ void intel_hdmi_init(struct drm_device *dev, int sdvox_reg)
 		 * just doing the minimal required for HDMI to work at this stage.
 		 */
 		intel_hdmi->write_infoframe = hsw_write_infoframe;
+		intel_hdmi->set_infoframes = hsw_set_infoframes;
 		for_each_pipe(i)
 			I915_WRITE(HSW_TVIDEO_DIP_CTL(i), 0);
 	} else if (HAS_PCH_IBX(dev)) {
 		intel_hdmi->write_infoframe = ibx_write_infoframe;
+		intel_hdmi->set_infoframes = ibx_set_infoframes;
 		for_each_pipe(i)
 			I915_WRITE(TVIDEO_DIP_CTL(i), 0);
 	} else {
 		intel_hdmi->write_infoframe = cpt_write_infoframe;
+		intel_hdmi->set_infoframes = cpt_set_infoframes;
 		for_each_pipe(i)
 			I915_WRITE(TVIDEO_DIP_CTL(i), 0);
 	}
-- 
1.7.10

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 02/14] drm/i915: properly alternate between DVI and HDMI
  2012-05-28 19:42 [PATCH 00/14] More HDMI fixes V2 Paulo Zanoni
  2012-05-28 19:42 ` [PATCH 01/14] drm/i915: add set_infoframes to struct intel_hdmi Paulo Zanoni
@ 2012-05-28 19:42 ` Paulo Zanoni
  2012-05-28 19:53   ` Chris Wilson
  2012-05-28 19:42 ` [PATCH 03/14] drm/i915: only set the HDMI port on the DIP once Paulo Zanoni
                   ` (11 subsequent siblings)
  13 siblings, 1 reply; 19+ messages in thread
From: Paulo Zanoni @ 2012-05-28 19:42 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

This solves problems that happen when you alternate between HDMI and
DVI on the same port. I can reproduce these problems using DP->HDMI
and DP->DVI adapters on a DP port.

When you first plug HDMI and then plug DVI, you need to stop sending
DIPs, even if the port is in DVI mode (see the HDMI register spec). If
you don't stop sending DIPs, you'll see a pink vertical line on the
left side of the screen, some modes will give you a black screen, some
modes won't work correctly.

When you first plug DVI and then plug HDMI, you need to properly
enable the DIPs, otherwise the HW won't send them. After spending a
lot of time investigating this, I concluded that if the DIPs are
disabled, we should not write to the DIP register again because when
we do this, we also set the AVI InfoFrame frequency to "once", and
this seems to really confuse our hardware. Since this problem was not
exactly easy to debug, I'm adopting the defensive behavior and not
just avoing the "disable twice" sequence, but also explicitly
selecting the AVI InfoFrame and setting its frequency to a correct
one.

Also, move the "is_dvi" check from intel_set_infoframe to the
set_infoframes functions since now they're going to be the first ones
to deal with the DIP registers.

This patch adds the code to fix the problem, but it depends on the
removal of some code that can't be removed right now and will come
later in the patch series. The patch that we need is:
  - drm/i915: don't write 0 to DIP control at HDMI init

V2: Be even more defensive by selecting AVI and setting its frequency
outside the "is_dvi" check.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c |   88 +++++++++++++++++++++++++++++++++++--
 1 file changed, 85 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 8d892b0..2f2adc4 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -308,9 +308,6 @@ static void intel_set_infoframe(struct drm_encoder *encoder,
 {
 	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
 
-	if (!intel_hdmi->has_hdmi_sink)
-		return;
-
 	intel_dip_infoframe_csum(frame);
 	intel_hdmi->write_infoframe(encoder, frame);
 }
@@ -348,6 +345,30 @@ static void intel_hdmi_set_spd_infoframe(struct drm_encoder *encoder)
 static void g4x_set_infoframes(struct drm_encoder *encoder,
 			       struct drm_display_mode *adjusted_mode)
 {
+	struct drm_i915_private *dev_priv = encoder->dev->dev_private;
+	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
+	u32 reg = VIDEO_DIP_CTL;
+	u32 val = I915_READ(reg);
+
+	/* If the registers were not initialized yet, they might be zeroes,
+	 * which means we're selecting the AVI DIP and we're setting its
+	 * frequency to once. This seems to really confuse the HW and make
+	 * things stop working (the register spec says the AVI always needs to
+	 * be sent every VSync). So here we avoid writing to the register more
+	 * than we need and also explicitly select the AVI DIP and explicitly
+	 * set its frequency to every VSync. Avoiding to write it twice seems to
+	 * be enough to solve the problem, but being defensive shouldn't hurt us
+	 * either. */
+	val |= VIDEO_DIP_SELECT_AVI | VIDEO_DIP_FREQ_VSYNC;
+
+	if (!intel_hdmi->has_hdmi_sink) {
+		if (!(val & VIDEO_DIP_ENABLE))
+			return;
+		val &= ~VIDEO_DIP_ENABLE;
+		I915_WRITE(reg, val);
+		return;
+	}
+
 	intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
 	intel_hdmi_set_spd_infoframe(encoder);
 }
@@ -355,6 +376,23 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
 static void ibx_set_infoframes(struct drm_encoder *encoder,
 			       struct drm_display_mode *adjusted_mode)
 {
+	struct drm_i915_private *dev_priv = encoder->dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
+	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
+	u32 reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
+	u32 val = I915_READ(reg);
+
+	/* See the big comment in g4x_set_infoframes() */
+	val |= VIDEO_DIP_SELECT_AVI | VIDEO_DIP_FREQ_VSYNC;
+
+	if (!intel_hdmi->has_hdmi_sink) {
+		if (!(val & VIDEO_DIP_ENABLE))
+			return;
+		val &= ~VIDEO_DIP_ENABLE;
+		I915_WRITE(reg, val);
+		return;
+	}
+
 	intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
 	intel_hdmi_set_spd_infoframe(encoder);
 }
@@ -362,6 +400,23 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
 static void cpt_set_infoframes(struct drm_encoder *encoder,
 			       struct drm_display_mode *adjusted_mode)
 {
+	struct drm_i915_private *dev_priv = encoder->dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
+	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
+	u32 reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
+	u32 val = I915_READ(reg);
+
+	/* See the big comment in g4x_set_infoframes() */
+	val |= VIDEO_DIP_SELECT_AVI | VIDEO_DIP_FREQ_VSYNC;
+
+	if (!intel_hdmi->has_hdmi_sink) {
+		if (!(val & VIDEO_DIP_ENABLE))
+			return;
+		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI);
+		I915_WRITE(reg, val);
+		return;
+	}
+
 	intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
 	intel_hdmi_set_spd_infoframe(encoder);
 }
@@ -369,6 +424,23 @@ static void cpt_set_infoframes(struct drm_encoder *encoder,
 static void vlv_set_infoframes(struct drm_encoder *encoder,
 			       struct drm_display_mode *adjusted_mode)
 {
+	struct drm_i915_private *dev_priv = encoder->dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
+	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
+	u32 reg = VLV_TVIDEO_DIP_CTL(intel_crtc->pipe);
+	u32 val = I915_READ(reg);
+
+	/* See the big comment in g4x_set_infoframes() */
+	val |= VIDEO_DIP_SELECT_AVI | VIDEO_DIP_FREQ_VSYNC;
+
+	if (!intel_hdmi->has_hdmi_sink) {
+		if (!(val & VIDEO_DIP_ENABLE))
+			return;
+		val &= ~VIDEO_DIP_ENABLE;
+		I915_WRITE(reg, val);
+		return;
+	}
+
 	intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
 	intel_hdmi_set_spd_infoframe(encoder);
 }
@@ -376,6 +448,16 @@ static void vlv_set_infoframes(struct drm_encoder *encoder,
 static void hsw_set_infoframes(struct drm_encoder *encoder,
 			       struct drm_display_mode *adjusted_mode)
 {
+	struct drm_i915_private *dev_priv = encoder->dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
+	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
+	u32 reg = HSW_TVIDEO_DIP_CTL(intel_crtc->pipe);
+
+	if (!intel_hdmi->has_hdmi_sink) {
+		I915_WRITE(reg, 0);
+		return;
+	}
+
 	intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
 	intel_hdmi_set_spd_infoframe(encoder);
 }
-- 
1.7.10

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 03/14] drm/i915: only set the HDMI port on the DIP once
  2012-05-28 19:42 [PATCH 00/14] More HDMI fixes V2 Paulo Zanoni
  2012-05-28 19:42 ` [PATCH 01/14] drm/i915: add set_infoframes to struct intel_hdmi Paulo Zanoni
  2012-05-28 19:42 ` [PATCH 02/14] drm/i915: properly alternate between DVI and HDMI Paulo Zanoni
@ 2012-05-28 19:42 ` Paulo Zanoni
  2012-05-28 19:42 ` [PATCH 04/14] drm/i915: enable DIP before enabling each InfoFrame Paulo Zanoni
                   ` (10 subsequent siblings)
  13 siblings, 0 replies; 19+ messages in thread
From: Paulo Zanoni @ 2012-05-28 19:42 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Not once for each InfoFrame. Now we have a function that allows us to
do this.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c |   56 ++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 2f2adc4..1df1ec7 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -121,18 +121,9 @@ static void g4x_write_infoframe(struct drm_encoder *encoder,
 	uint32_t *data = (uint32_t *)frame;
 	struct drm_device *dev = encoder->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
 	u32 val = I915_READ(VIDEO_DIP_CTL);
 	unsigned i, len = DIP_HEADER_SIZE + frame->len;
 
-	val &= ~VIDEO_DIP_PORT_MASK;
-	if (intel_hdmi->sdvox_reg == SDVOB)
-		val |= VIDEO_DIP_PORT_B;
-	else if (intel_hdmi->sdvox_reg == SDVOC)
-		val |= VIDEO_DIP_PORT_C;
-	else
-		return;
-
 	val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
 	val |= g4x_infoframe_index(frame);
 
@@ -160,26 +151,10 @@ static void ibx_write_infoframe(struct drm_encoder *encoder,
 	struct drm_device *dev = encoder->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
-	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
 	int reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
 	unsigned i, len = DIP_HEADER_SIZE + frame->len;
 	u32 val = I915_READ(reg);
 
-	val &= ~VIDEO_DIP_PORT_MASK;
-	switch (intel_hdmi->sdvox_reg) {
-	case HDMIB:
-		val |= VIDEO_DIP_PORT_B;
-		break;
-	case HDMIC:
-		val |= VIDEO_DIP_PORT_C;
-		break;
-	case HDMID:
-		val |= VIDEO_DIP_PORT_D;
-		break;
-	default:
-		return;
-	}
-
 	intel_wait_for_vblank(dev, intel_crtc->pipe);
 
 	val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
@@ -369,6 +344,20 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
 		return;
 	}
 
+	val &= ~VIDEO_DIP_PORT_MASK;
+	switch (intel_hdmi->sdvox_reg) {
+	case SDVOB:
+		val |= VIDEO_DIP_PORT_B;
+		break;
+	case SDVOC:
+		val |= VIDEO_DIP_PORT_C;
+		break;
+	default:
+		return;
+	}
+
+	I915_WRITE(reg, val);
+
 	intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
 	intel_hdmi_set_spd_infoframe(encoder);
 }
@@ -393,6 +382,23 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
 		return;
 	}
 
+	val &= ~VIDEO_DIP_PORT_MASK;
+	switch (intel_hdmi->sdvox_reg) {
+	case HDMIB:
+		val |= VIDEO_DIP_PORT_B;
+		break;
+	case HDMIC:
+		val |= VIDEO_DIP_PORT_C;
+		break;
+	case HDMID:
+		val |= VIDEO_DIP_PORT_D;
+		break;
+	default:
+		return;
+	}
+
+	I915_WRITE(reg, val);
+
 	intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
 	intel_hdmi_set_spd_infoframe(encoder);
 }
-- 
1.7.10

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 04/14] drm/i915: enable DIP before enabling each InfoFrame
  2012-05-28 19:42 [PATCH 00/14] More HDMI fixes V2 Paulo Zanoni
                   ` (2 preceding siblings ...)
  2012-05-28 19:42 ` [PATCH 03/14] drm/i915: only set the HDMI port on the DIP once Paulo Zanoni
@ 2012-05-28 19:42 ` Paulo Zanoni
  2012-05-28 19:42 ` [PATCH 05/14] drm/i915: don't wait for vblank while writing InfoFrames Paulo Zanoni
                   ` (9 subsequent siblings)
  13 siblings, 0 replies; 19+ messages in thread
From: Paulo Zanoni @ 2012-05-28 19:42 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

So the write_infoframe function can assume the DIP is on.

V2: Be more defensive and add WARN().

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c |   30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 1df1ec7..de6f4c2 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -124,11 +124,12 @@ static void g4x_write_infoframe(struct drm_encoder *encoder,
 	u32 val = I915_READ(VIDEO_DIP_CTL);
 	unsigned i, len = DIP_HEADER_SIZE + frame->len;
 
+	WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n");
+
 	val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
 	val |= g4x_infoframe_index(frame);
 
 	val &= ~g4x_infoframe_enable(frame);
-	val |= VIDEO_DIP_ENABLE;
 
 	I915_WRITE(VIDEO_DIP_CTL, val);
 
@@ -155,13 +156,14 @@ static void ibx_write_infoframe(struct drm_encoder *encoder,
 	unsigned i, len = DIP_HEADER_SIZE + frame->len;
 	u32 val = I915_READ(reg);
 
+	WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n");
+
 	intel_wait_for_vblank(dev, intel_crtc->pipe);
 
 	val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
 	val |= g4x_infoframe_index(frame);
 
 	val &= ~g4x_infoframe_enable(frame);
-	val |= VIDEO_DIP_ENABLE;
 
 	I915_WRITE(reg, val);
 
@@ -188,6 +190,8 @@ static void cpt_write_infoframe(struct drm_encoder *encoder,
 	unsigned i, len = DIP_HEADER_SIZE + frame->len;
 	u32 val = I915_READ(reg);
 
+	WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n");
+
 	intel_wait_for_vblank(dev, intel_crtc->pipe);
 
 	val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
@@ -195,13 +199,9 @@ static void cpt_write_infoframe(struct drm_encoder *encoder,
 
 	/* The DIP control register spec says that we need to update the AVI
 	 * infoframe without clearing its enable bit */
-	if (frame->type == DIP_TYPE_AVI)
-		val |= VIDEO_DIP_ENABLE_AVI;
-	else
+	if (frame->type != DIP_TYPE_AVI)
 		val &= ~g4x_infoframe_enable(frame);
 
-	val |= VIDEO_DIP_ENABLE;
-
 	I915_WRITE(reg, val);
 
 	for (i = 0; i < len; i += 4) {
@@ -227,13 +227,14 @@ static void vlv_write_infoframe(struct drm_encoder *encoder,
 	unsigned i, len = DIP_HEADER_SIZE + frame->len;
 	u32 val = I915_READ(reg);
 
+	WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n");
+
 	intel_wait_for_vblank(dev, intel_crtc->pipe);
 
 	val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
 	val |= g4x_infoframe_index(frame);
 
 	val &= ~g4x_infoframe_enable(frame);
-	val |= VIDEO_DIP_ENABLE;
 
 	I915_WRITE(reg, val);
 
@@ -356,6 +357,8 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
 		return;
 	}
 
+	val |= VIDEO_DIP_ENABLE;
+
 	I915_WRITE(reg, val);
 
 	intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
@@ -397,6 +400,8 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
 		return;
 	}
 
+	val |= VIDEO_DIP_ENABLE;
+
 	I915_WRITE(reg, val);
 
 	intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
@@ -423,6 +428,11 @@ static void cpt_set_infoframes(struct drm_encoder *encoder,
 		return;
 	}
 
+	/* Set both together, unset both together: see the spec. */
+	val |= VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI;
+
+	I915_WRITE(reg, val);
+
 	intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
 	intel_hdmi_set_spd_infoframe(encoder);
 }
@@ -447,6 +457,10 @@ static void vlv_set_infoframes(struct drm_encoder *encoder,
 		return;
 	}
 
+	val |= VIDEO_DIP_ENABLE;
+
+	I915_WRITE(reg, val);
+
 	intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
 	intel_hdmi_set_spd_infoframe(encoder);
 }
-- 
1.7.10

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 05/14] drm/i915: don't wait for vblank while writing InfoFrames
  2012-05-28 19:42 [PATCH 00/14] More HDMI fixes V2 Paulo Zanoni
                   ` (3 preceding siblings ...)
  2012-05-28 19:42 ` [PATCH 04/14] drm/i915: enable DIP before enabling each InfoFrame Paulo Zanoni
@ 2012-05-28 19:42 ` Paulo Zanoni
  2012-05-28 19:42 ` [PATCH 06/14] drm/i915: explicitly disable the DIPs we're not using Paulo Zanoni
                   ` (8 subsequent siblings)
  13 siblings, 0 replies; 19+ messages in thread
From: Paulo Zanoni @ 2012-05-28 19:42 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

This function is called when the pipe is disabled, so it always gets
the 50ms timeout.

This function is called once for each InfoFrame, so we actually get a
100ms timeout. Will be more if we add more InfoFrames.

Also, the spec says we need to "wait for a VSync to ensure completion
of any pending DIP transmissions", not for a VBlank. OTOH, the
register documentation suggests that the DIPs are sent *during* the
VSync, so shouldn't we be waiting until *after* the VSync to ensure
all DIPs are sent?

So this wait_for_vblank seems, besides useless, totally wrong.

If we ever want to change some specific InfoFrame on-the-fly (outside
of the modeset code), the code that changes the InfoFrame will have to
do the waiting itself, and properly.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c |    8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index de6f4c2..614d83f 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -158,8 +158,6 @@ static void ibx_write_infoframe(struct drm_encoder *encoder,
 
 	WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n");
 
-	intel_wait_for_vblank(dev, intel_crtc->pipe);
-
 	val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
 	val |= g4x_infoframe_index(frame);
 
@@ -192,8 +190,6 @@ static void cpt_write_infoframe(struct drm_encoder *encoder,
 
 	WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n");
 
-	intel_wait_for_vblank(dev, intel_crtc->pipe);
-
 	val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
 	val |= g4x_infoframe_index(frame);
 
@@ -229,8 +225,6 @@ static void vlv_write_infoframe(struct drm_encoder *encoder,
 
 	WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n");
 
-	intel_wait_for_vblank(dev, intel_crtc->pipe);
-
 	val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
 	val |= g4x_infoframe_index(frame);
 
@@ -265,8 +259,6 @@ static void hsw_write_infoframe(struct drm_encoder *encoder,
 	if (data_reg == 0)
 		return;
 
-	intel_wait_for_vblank(dev, intel_crtc->pipe);
-
 	val &= ~hsw_infoframe_enable(frame);
 	I915_WRITE(ctl_reg, val);
 
-- 
1.7.10

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 06/14] drm/i915: explicitly disable the DIPs we're not using
  2012-05-28 19:42 [PATCH 00/14] More HDMI fixes V2 Paulo Zanoni
                   ` (4 preceding siblings ...)
  2012-05-28 19:42 ` [PATCH 05/14] drm/i915: don't wait for vblank while writing InfoFrames Paulo Zanoni
@ 2012-05-28 19:42 ` Paulo Zanoni
  2012-05-28 19:42 ` [PATCH 07/14] drm/i915: disable DIP while changing the port Paulo Zanoni
                   ` (7 subsequent siblings)
  13 siblings, 0 replies; 19+ messages in thread
From: Paulo Zanoni @ 2012-05-28 19:42 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

>From this point on, the 'set_infoframe' functions always set the DIP
registers to a known state, so anything done will always be undone at
the modeset.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h   |    6 ++++++
 drivers/gpu/drm/i915/intel_hdmi.c |   13 +++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0f45a18..1855ac7 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1717,8 +1717,10 @@
 #define   VIDEO_DIP_PORT_C		(2 << 29)
 #define   VIDEO_DIP_PORT_D		(3 << 29)
 #define   VIDEO_DIP_PORT_MASK		(3 << 29)
+#define   VIDEO_DIP_ENABLE_GCP		(1 << 25)
 #define   VIDEO_DIP_ENABLE_AVI		(1 << 21)
 #define   VIDEO_DIP_ENABLE_VENDOR	(2 << 21)
+#define   VIDEO_DIP_ENABLE_GAMUT	(4 << 21)
 #define   VIDEO_DIP_ENABLE_SPD		(8 << 21)
 #define   VIDEO_DIP_SELECT_AVI		(0 << 19)
 #define   VIDEO_DIP_SELECT_VENDOR	(1 << 19)
@@ -1729,7 +1731,11 @@
 #define   VIDEO_DIP_FREQ_2VSYNC		(2 << 16)
 #define   VIDEO_DIP_FREQ_MASK		(3 << 16)
 /* HSW and later: */
+#define   VIDEO_DIP_ENABLE_VSC_HSW	(1 << 20)
+#define   VIDEO_DIP_ENABLE_GCP_HSW	(1 << 16)
 #define   VIDEO_DIP_ENABLE_AVI_HSW	(1 << 12)
+#define   VIDEO_DIP_ENABLE_VS_HSW	(1 << 8)
+#define   VIDEO_DIP_ENABLE_GMP_HSW	(1 << 4)
 #define   VIDEO_DIP_ENABLE_SPD_HSW	(1 << 0)
 
 /* Panel power sequencing */
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 614d83f..620b0db 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -350,6 +350,7 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
 	}
 
 	val |= VIDEO_DIP_ENABLE;
+	val &= ~VIDEO_DIP_ENABLE_VENDOR;
 
 	I915_WRITE(reg, val);
 
@@ -393,6 +394,8 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
 	}
 
 	val |= VIDEO_DIP_ENABLE;
+	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
+		 VIDEO_DIP_ENABLE_GCP);
 
 	I915_WRITE(reg, val);
 
@@ -422,6 +425,8 @@ static void cpt_set_infoframes(struct drm_encoder *encoder,
 
 	/* Set both together, unset both together: see the spec. */
 	val |= VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI;
+	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
+		 VIDEO_DIP_ENABLE_GCP);
 
 	I915_WRITE(reg, val);
 
@@ -450,6 +455,8 @@ static void vlv_set_infoframes(struct drm_encoder *encoder,
 	}
 
 	val |= VIDEO_DIP_ENABLE;
+	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
+		 VIDEO_DIP_ENABLE_GCP);
 
 	I915_WRITE(reg, val);
 
@@ -464,12 +471,18 @@ static void hsw_set_infoframes(struct drm_encoder *encoder,
 	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
 	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
 	u32 reg = HSW_TVIDEO_DIP_CTL(intel_crtc->pipe);
+	u32 val = I915_READ(reg);
 
 	if (!intel_hdmi->has_hdmi_sink) {
 		I915_WRITE(reg, 0);
 		return;
 	}
 
+	val &= ~(VIDEO_DIP_ENABLE_VSC_HSW | VIDEO_DIP_ENABLE_GCP_HSW |
+		 VIDEO_DIP_ENABLE_VS_HSW | VIDEO_DIP_ENABLE_GMP_HSW);
+
+	I915_WRITE(reg, val);
+
 	intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
 	intel_hdmi_set_spd_infoframe(encoder);
 }
-- 
1.7.10

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 07/14] drm/i915: disable DIP while changing the port
  2012-05-28 19:42 [PATCH 00/14] More HDMI fixes V2 Paulo Zanoni
                   ` (5 preceding siblings ...)
  2012-05-28 19:42 ` [PATCH 06/14] drm/i915: explicitly disable the DIPs we're not using Paulo Zanoni
@ 2012-05-28 19:42 ` Paulo Zanoni
  2012-05-28 19:42 ` [PATCH 08/14] drm/i915: don't write 0 to DIP control at HDMI init Paulo Zanoni
                   ` (6 subsequent siblings)
  13 siblings, 0 replies; 19+ messages in thread
From: Paulo Zanoni @ 2012-05-28 19:42 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

The register specification says we need to do this.

V2: Only write the register if the port is enabled.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c |   32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 620b0db..c858da9 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -317,6 +317,7 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
 	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
 	u32 reg = VIDEO_DIP_CTL;
 	u32 val = I915_READ(reg);
+	u32 port;
 
 	/* If the registers were not initialized yet, they might be zeroes,
 	 * which means we're selecting the AVI DIP and we're setting its
@@ -337,18 +338,26 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
 		return;
 	}
 
-	val &= ~VIDEO_DIP_PORT_MASK;
 	switch (intel_hdmi->sdvox_reg) {
 	case SDVOB:
-		val |= VIDEO_DIP_PORT_B;
+		port = VIDEO_DIP_PORT_B;
 		break;
 	case SDVOC:
-		val |= VIDEO_DIP_PORT_C;
+		port = VIDEO_DIP_PORT_C;
 		break;
 	default:
 		return;
 	}
 
+	if (port != (val & VIDEO_DIP_PORT_MASK)) {
+		if (val & VIDEO_DIP_ENABLE) {
+			val &= ~VIDEO_DIP_ENABLE;
+			I915_WRITE(reg, val);
+		}
+		val &= ~VIDEO_DIP_PORT_MASK;
+		val |= port;
+	}
+
 	val |= VIDEO_DIP_ENABLE;
 	val &= ~VIDEO_DIP_ENABLE_VENDOR;
 
@@ -366,6 +375,7 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
 	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
 	u32 reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
 	u32 val = I915_READ(reg);
+	u32 port;
 
 	/* See the big comment in g4x_set_infoframes() */
 	val |= VIDEO_DIP_SELECT_AVI | VIDEO_DIP_FREQ_VSYNC;
@@ -378,21 +388,29 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
 		return;
 	}
 
-	val &= ~VIDEO_DIP_PORT_MASK;
 	switch (intel_hdmi->sdvox_reg) {
 	case HDMIB:
-		val |= VIDEO_DIP_PORT_B;
+		port = VIDEO_DIP_PORT_B;
 		break;
 	case HDMIC:
-		val |= VIDEO_DIP_PORT_C;
+		port = VIDEO_DIP_PORT_C;
 		break;
 	case HDMID:
-		val |= VIDEO_DIP_PORT_D;
+		port = VIDEO_DIP_PORT_D;
 		break;
 	default:
 		return;
 	}
 
+	if (port != (val & VIDEO_DIP_PORT_MASK)) {
+		if (val & VIDEO_DIP_ENABLE) {
+			val &= ~VIDEO_DIP_ENABLE;
+			I915_WRITE(reg, val);
+		}
+		val &= ~VIDEO_DIP_PORT_MASK;
+		val |= port;
+	}
+
 	val |= VIDEO_DIP_ENABLE;
 	val &= ~(VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
 		 VIDEO_DIP_ENABLE_GCP);
-- 
1.7.10

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 08/14] drm/i915: don't write 0 to DIP control at HDMI init
  2012-05-28 19:42 [PATCH 00/14] More HDMI fixes V2 Paulo Zanoni
                   ` (6 preceding siblings ...)
  2012-05-28 19:42 ` [PATCH 07/14] drm/i915: disable DIP while changing the port Paulo Zanoni
@ 2012-05-28 19:42 ` Paulo Zanoni
  2012-05-28 19:42 ` [PATCH 09/14] drm/i915: don't set SDVO_BORDER_ENABLE when we're HDMI Paulo Zanoni
                   ` (5 subsequent siblings)
  13 siblings, 0 replies; 19+ messages in thread
From: Paulo Zanoni @ 2012-05-28 19:42 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

At this time, the HDMI port is enabled, and the DIP control register
specification says we need to disable the port *before* disabling the
DIPs. Also, while doing this we risk telling the HW to send the AVI
DIPs once (not every VSync), which really seems to confuse the HW and
trigger bugs where the DIPs are not sent.

This code was here just to set the DIP register to a 'known state'
before using it, but since now the set_infoframes functions already
set the control registers to a known state, this code can go away.

Also, the previous code disables *all* the DIP registers for *each*
HDMI port, so we end disabling each DIP register more than once.

This patch solves a problem I can reproduce on my IVB machine. When I
boot it with just a single HDMI monitor, the AVI InfoFrames are not
sent. With this patch, the InfoFrames are sent. Previously, I wrote a
patch to 'touch the DIP registers after we enable the HDMI port' to
solve this same problem, but that patch doesn't seem to be needed
anymore after this patch.

All this patch does is revert a chunk of the following commit:

    commit 64a8fc0145a1d0fdc25fc9367c2e6c621955fb3b
    Author: Jesse Barnes <jbarnes@virtuousgeek.org>
    Date:   Thu Sep 22 11:16:00 2011 +0530

        drm/i915: fix ILK+ infoframe support

So bugs that can be bisected to that commit may be fixed now.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=43256
Acked-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c |   10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index c858da9..65af12e 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -816,7 +816,6 @@ void intel_hdmi_init(struct drm_device *dev, int sdvox_reg)
 	struct intel_encoder *intel_encoder;
 	struct intel_connector *intel_connector;
 	struct intel_hdmi *intel_hdmi;
-	int i;
 
 	intel_hdmi = kzalloc(sizeof(struct intel_hdmi), GFP_KERNEL);
 	if (!intel_hdmi)
@@ -894,30 +893,21 @@ void intel_hdmi_init(struct drm_device *dev, int sdvox_reg)
 	if (!HAS_PCH_SPLIT(dev)) {
 		intel_hdmi->write_infoframe = g4x_write_infoframe;
 		intel_hdmi->set_infoframes = g4x_set_infoframes;
-		I915_WRITE(VIDEO_DIP_CTL, 0);
 	} else if (IS_VALLEYVIEW(dev)) {
 		intel_hdmi->write_infoframe = vlv_write_infoframe;
 		intel_hdmi->set_infoframes = vlv_set_infoframes;
-		for_each_pipe(i)
-			I915_WRITE(VLV_TVIDEO_DIP_CTL(i), 0);
 	} else if (IS_HASWELL(dev)) {
 		/* FIXME: Haswell has a new set of DIP frame registers, but we are
 		 * just doing the minimal required for HDMI to work at this stage.
 		 */
 		intel_hdmi->write_infoframe = hsw_write_infoframe;
 		intel_hdmi->set_infoframes = hsw_set_infoframes;
-		for_each_pipe(i)
-			I915_WRITE(HSW_TVIDEO_DIP_CTL(i), 0);
 	} else if (HAS_PCH_IBX(dev)) {
 		intel_hdmi->write_infoframe = ibx_write_infoframe;
 		intel_hdmi->set_infoframes = ibx_set_infoframes;
-		for_each_pipe(i)
-			I915_WRITE(TVIDEO_DIP_CTL(i), 0);
 	} else {
 		intel_hdmi->write_infoframe = cpt_write_infoframe;
 		intel_hdmi->set_infoframes = cpt_set_infoframes;
-		for_each_pipe(i)
-			I915_WRITE(TVIDEO_DIP_CTL(i), 0);
 	}
 
 	if (IS_HASWELL(dev))
-- 
1.7.10

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 09/14] drm/i915: don't set SDVO_BORDER_ENABLE when we're HDMI
  2012-05-28 19:42 [PATCH 00/14] More HDMI fixes V2 Paulo Zanoni
                   ` (7 preceding siblings ...)
  2012-05-28 19:42 ` [PATCH 08/14] drm/i915: don't write 0 to DIP control at HDMI init Paulo Zanoni
@ 2012-05-28 19:42 ` Paulo Zanoni
  2012-05-28 19:42 ` [PATCH 10/14] drm/i915: rename sdvox_reg to hdmi_reg on HDMI context Paulo Zanoni
                   ` (4 subsequent siblings)
  13 siblings, 0 replies; 19+ messages in thread
From: Paulo Zanoni @ 2012-05-28 19:42 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

The register that controls the HDMI port can be used to control the
sDVO port. Some bits are defined only for sDVO, and SDVO_BORDER_ENABLE
is one of those: HDMI ports that can't be used in sDVO mode don't even
have this bit defined in their specifications.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 65af12e..194b453 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -515,7 +515,7 @@ static void intel_hdmi_mode_set(struct drm_encoder *encoder,
 	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
 	u32 sdvox;
 
-	sdvox = SDVO_ENCODING_HDMI | SDVO_BORDER_ENABLE;
+	sdvox = SDVO_ENCODING_HDMI;
 	if (!HAS_PCH_SPLIT(dev))
 		sdvox |= intel_hdmi->color_range;
 	if (adjusted_mode->flags & DRM_MODE_FLAG_PVSYNC)
-- 
1.7.10

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 10/14] drm/i915: rename sdvox_reg to hdmi_reg on HDMI context
  2012-05-28 19:42 [PATCH 00/14] More HDMI fixes V2 Paulo Zanoni
                   ` (8 preceding siblings ...)
  2012-05-28 19:42 ` [PATCH 09/14] drm/i915: don't set SDVO_BORDER_ENABLE when we're HDMI Paulo Zanoni
@ 2012-05-28 19:42 ` Paulo Zanoni
  2012-05-28 19:42 ` [PATCH 11/14] drm/i915: remove comment about HSW HDMI DIPs Paulo Zanoni
                   ` (3 subsequent siblings)
  13 siblings, 0 replies; 19+ messages in thread
From: Paulo Zanoni @ 2012-05-28 19:42 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Some (but not all) of the HDMI registers can be used to control sDVO,
so these registers have two names. When we're talking about HDMI, we
really should call the HDMI control register "hdmi_reg" instead of
"sdvox_reg". So now "struct intel_hdmi" has a member called "hdmi_reg"
instead of "sdvox_reg".

And don't worry: "struct intel_sdvo" still has a member called
"sdvo_reg".

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h  |    4 +--
 drivers/gpu/drm/i915/intel_hdmi.c |   70 ++++++++++++++++++-------------------
 2 files changed, 37 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 39d7b07..fb5939d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -292,7 +292,7 @@ struct dip_infoframe {
 
 struct intel_hdmi {
 	struct intel_encoder base;
-	u32 sdvox_reg;
+	u32 hdmi_reg;
 	int ddc_bus;
 	int ddi_port;
 	uint32_t color_range;
@@ -343,7 +343,7 @@ extern void intel_attach_force_audio_property(struct drm_connector *connector);
 extern void intel_attach_broadcast_rgb_property(struct drm_connector *connector);
 
 extern void intel_crt_init(struct drm_device *dev);
-extern void intel_hdmi_init(struct drm_device *dev, int sdvox_reg);
+extern void intel_hdmi_init(struct drm_device *dev, int hdmi_reg);
 extern struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder);
 extern void intel_dip_infoframe_csum(struct dip_infoframe *avi_if);
 extern bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg,
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 194b453..e348f31 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -338,7 +338,7 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
 		return;
 	}
 
-	switch (intel_hdmi->sdvox_reg) {
+	switch (intel_hdmi->hdmi_reg) {
 	case SDVOB:
 		port = VIDEO_DIP_PORT_B;
 		break;
@@ -388,7 +388,7 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
 		return;
 	}
 
-	switch (intel_hdmi->sdvox_reg) {
+	switch (intel_hdmi->hdmi_reg) {
 	case HDMIB:
 		port = VIDEO_DIP_PORT_B;
 		break;
@@ -513,40 +513,40 @@ static void intel_hdmi_mode_set(struct drm_encoder *encoder,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
 	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
-	u32 sdvox;
+	u32 hdmi_val;
 
-	sdvox = SDVO_ENCODING_HDMI;
+	hdmi_val = SDVO_ENCODING_HDMI;
 	if (!HAS_PCH_SPLIT(dev))
-		sdvox |= intel_hdmi->color_range;
+		hdmi_val |= intel_hdmi->color_range;
 	if (adjusted_mode->flags & DRM_MODE_FLAG_PVSYNC)
-		sdvox |= SDVO_VSYNC_ACTIVE_HIGH;
+		hdmi_val |= SDVO_VSYNC_ACTIVE_HIGH;
 	if (adjusted_mode->flags & DRM_MODE_FLAG_PHSYNC)
-		sdvox |= SDVO_HSYNC_ACTIVE_HIGH;
+		hdmi_val |= SDVO_HSYNC_ACTIVE_HIGH;
 
 	if (intel_crtc->bpp > 24)
-		sdvox |= COLOR_FORMAT_12bpc;
+		hdmi_val |= COLOR_FORMAT_12bpc;
 	else
-		sdvox |= COLOR_FORMAT_8bpc;
+		hdmi_val |= COLOR_FORMAT_8bpc;
 
 	/* Required on CPT */
 	if (intel_hdmi->has_hdmi_sink && HAS_PCH_CPT(dev))
-		sdvox |= HDMI_MODE_SELECT;
+		hdmi_val |= HDMI_MODE_SELECT;
 
 	if (intel_hdmi->has_audio) {
 		DRM_DEBUG_DRIVER("Enabling HDMI audio on pipe %c\n",
 				 pipe_name(intel_crtc->pipe));
-		sdvox |= SDVO_AUDIO_ENABLE;
-		sdvox |= SDVO_NULL_PACKETS_DURING_VSYNC;
+		hdmi_val |= SDVO_AUDIO_ENABLE;
+		hdmi_val |= SDVO_NULL_PACKETS_DURING_VSYNC;
 		intel_write_eld(encoder, adjusted_mode);
 	}
 
 	if (HAS_PCH_CPT(dev))
-		sdvox |= PORT_TRANS_SEL_CPT(intel_crtc->pipe);
+		hdmi_val |= PORT_TRANS_SEL_CPT(intel_crtc->pipe);
 	else if (intel_crtc->pipe == 1)
-		sdvox |= SDVO_PIPE_B_SELECT;
+		hdmi_val |= SDVO_PIPE_B_SELECT;
 
-	I915_WRITE(intel_hdmi->sdvox_reg, sdvox);
-	POSTING_READ(intel_hdmi->sdvox_reg);
+	I915_WRITE(intel_hdmi->hdmi_reg, hdmi_val);
+	POSTING_READ(intel_hdmi->hdmi_reg);
 
 	intel_hdmi->set_infoframes(encoder, adjusted_mode);
 }
@@ -562,14 +562,14 @@ static void intel_hdmi_dpms(struct drm_encoder *encoder, int mode)
 	if (intel_hdmi->has_audio)
 		enable_bits |= SDVO_AUDIO_ENABLE;
 
-	temp = I915_READ(intel_hdmi->sdvox_reg);
+	temp = I915_READ(intel_hdmi->hdmi_reg);
 
 	/* 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->sdvox_reg, temp & ~SDVO_ENABLE);
-		POSTING_READ(intel_hdmi->sdvox_reg);
+		I915_WRITE(intel_hdmi->hdmi_reg, temp & ~SDVO_ENABLE);
+		POSTING_READ(intel_hdmi->hdmi_reg);
 	}
 
 	if (mode != DRM_MODE_DPMS_ON) {
@@ -578,15 +578,15 @@ static void intel_hdmi_dpms(struct drm_encoder *encoder, int mode)
 		temp |= enable_bits;
 	}
 
-	I915_WRITE(intel_hdmi->sdvox_reg, temp);
-	POSTING_READ(intel_hdmi->sdvox_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.
 	 */
 	if (HAS_PCH_SPLIT(dev)) {
-		I915_WRITE(intel_hdmi->sdvox_reg, temp);
-		POSTING_READ(intel_hdmi->sdvox_reg);
+		I915_WRITE(intel_hdmi->hdmi_reg, temp);
+		POSTING_READ(intel_hdmi->hdmi_reg);
 	}
 }
 
@@ -617,7 +617,7 @@ static bool g4x_hdmi_connected(struct intel_hdmi *intel_hdmi)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t bit;
 
-	switch (intel_hdmi->sdvox_reg) {
+	switch (intel_hdmi->hdmi_reg) {
 	case SDVOB:
 		bit = HDMIB_HOTPLUG_LIVE_STATUS;
 		break;
@@ -809,7 +809,7 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c
 	intel_attach_broadcast_rgb_property(connector);
 }
 
-void intel_hdmi_init(struct drm_device *dev, int sdvox_reg)
+void intel_hdmi_init(struct drm_device *dev, int hdmi_reg)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_connector *connector;
@@ -844,51 +844,51 @@ void intel_hdmi_init(struct drm_device *dev, int sdvox_reg)
 	intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
 
 	/* Set up the DDC bus. */
-	if (sdvox_reg == SDVOB) {
+	if (hdmi_reg == SDVOB) {
 		intel_encoder->clone_mask = (1 << INTEL_HDMIB_CLONE_BIT);
 		intel_hdmi->ddc_bus = GMBUS_PORT_DPB;
 		dev_priv->hotplug_supported_mask |= HDMIB_HOTPLUG_INT_STATUS;
-	} else if (sdvox_reg == SDVOC) {
+	} else if (hdmi_reg == SDVOC) {
 		intel_encoder->clone_mask = (1 << INTEL_HDMIC_CLONE_BIT);
 		intel_hdmi->ddc_bus = GMBUS_PORT_DPC;
 		dev_priv->hotplug_supported_mask |= HDMIC_HOTPLUG_INT_STATUS;
-	} else if (sdvox_reg == HDMIB) {
+	} else if (hdmi_reg == HDMIB) {
 		intel_encoder->clone_mask = (1 << INTEL_HDMID_CLONE_BIT);
 		intel_hdmi->ddc_bus = GMBUS_PORT_DPB;
 		dev_priv->hotplug_supported_mask |= HDMIB_HOTPLUG_INT_STATUS;
-	} else if (sdvox_reg == HDMIC) {
+	} else if (hdmi_reg == HDMIC) {
 		intel_encoder->clone_mask = (1 << INTEL_HDMIE_CLONE_BIT);
 		intel_hdmi->ddc_bus = GMBUS_PORT_DPC;
 		dev_priv->hotplug_supported_mask |= HDMIC_HOTPLUG_INT_STATUS;
-	} else if (sdvox_reg == HDMID) {
+	} else if (hdmi_reg == HDMID) {
 		intel_encoder->clone_mask = (1 << INTEL_HDMIF_CLONE_BIT);
 		intel_hdmi->ddc_bus = GMBUS_PORT_DPD;
 		dev_priv->hotplug_supported_mask |= HDMID_HOTPLUG_INT_STATUS;
-	} else if (sdvox_reg == DDI_BUF_CTL(PORT_B)) {
+	} else if (hdmi_reg == DDI_BUF_CTL(PORT_B)) {
 		DRM_DEBUG_DRIVER("LPT: detected output on DDI B\n");
 		intel_encoder->clone_mask = (1 << INTEL_HDMIB_CLONE_BIT);
 		intel_hdmi->ddc_bus = GMBUS_PORT_DPB;
 		intel_hdmi->ddi_port = PORT_B;
 		dev_priv->hotplug_supported_mask |= HDMIB_HOTPLUG_INT_STATUS;
-	} else if (sdvox_reg == DDI_BUF_CTL(PORT_C)) {
+	} else if (hdmi_reg == DDI_BUF_CTL(PORT_C)) {
 		DRM_DEBUG_DRIVER("LPT: detected output on DDI C\n");
 		intel_encoder->clone_mask = (1 << INTEL_HDMIC_CLONE_BIT);
 		intel_hdmi->ddc_bus = GMBUS_PORT_DPC;
 		intel_hdmi->ddi_port = PORT_C;
 		dev_priv->hotplug_supported_mask |= HDMIC_HOTPLUG_INT_STATUS;
-	} else if (sdvox_reg == DDI_BUF_CTL(PORT_D)) {
+	} else if (hdmi_reg == DDI_BUF_CTL(PORT_D)) {
 		DRM_DEBUG_DRIVER("LPT: detected output on DDI D\n");
 		intel_encoder->clone_mask = (1 << INTEL_HDMID_CLONE_BIT);
 		intel_hdmi->ddc_bus = GMBUS_PORT_DPD;
 		intel_hdmi->ddi_port = PORT_D;
 		dev_priv->hotplug_supported_mask |= HDMID_HOTPLUG_INT_STATUS;
 	} else {
-		/* If we got an unknown sdvox_reg, things are pretty much broken
+		/* If we got an unknown hdmi_reg, things are pretty much broken
 		 * in a way that we should let the kernel know about it */
 		BUG();
 	}
 
-	intel_hdmi->sdvox_reg = sdvox_reg;
+	intel_hdmi->hdmi_reg = hdmi_reg;
 
 	if (!HAS_PCH_SPLIT(dev)) {
 		intel_hdmi->write_infoframe = g4x_write_infoframe;
-- 
1.7.10

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 11/14] drm/i915: remove comment about HSW HDMI DIPs
  2012-05-28 19:42 [PATCH 00/14] More HDMI fixes V2 Paulo Zanoni
                   ` (9 preceding siblings ...)
  2012-05-28 19:42 ` [PATCH 10/14] drm/i915: rename sdvox_reg to hdmi_reg on HDMI context Paulo Zanoni
@ 2012-05-28 19:42 ` Paulo Zanoni
  2012-05-28 19:42 ` [PATCH 12/14] drm/i915: clarify confusion between HDMI and SDVO registers Paulo Zanoni
                   ` (2 subsequent siblings)
  13 siblings, 0 replies; 19+ messages in thread
From: Paulo Zanoni @ 2012-05-28 19:42 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

HSW support is now just like all the other generations: we send AVI
and SPD InfoFrames. There are other DIPs for HSW, but there are other
DIPs for the previous generations too. For each gen, we can see which
DIPs are missing by looking at the 'set_infoframes' function: we
explicitly disable the DIPs we're not using.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index e348f31..fc09d04 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -897,9 +897,6 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg)
 		intel_hdmi->write_infoframe = vlv_write_infoframe;
 		intel_hdmi->set_infoframes = vlv_set_infoframes;
 	} else if (IS_HASWELL(dev)) {
-		/* FIXME: Haswell has a new set of DIP frame registers, but we are
-		 * just doing the minimal required for HDMI to work at this stage.
-		 */
 		intel_hdmi->write_infoframe = hsw_write_infoframe;
 		intel_hdmi->set_infoframes = hsw_set_infoframes;
 	} else if (HAS_PCH_IBX(dev)) {
-- 
1.7.10

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 12/14] drm/i915: clarify confusion between HDMI and SDVO registers
  2012-05-28 19:42 [PATCH 00/14] More HDMI fixes V2 Paulo Zanoni
                   ` (10 preceding siblings ...)
  2012-05-28 19:42 ` [PATCH 11/14] drm/i915: remove comment about HSW HDMI DIPs Paulo Zanoni
@ 2012-05-28 19:42 ` Paulo Zanoni
  2012-05-28 19:43 ` [PATCH 13/14] drm/i915: add some barriers when changing DIPs Paulo Zanoni
  2012-05-28 19:43 ` [PATCH 14/14] drm/i915: make sure HDMI port is disabled inside set_infoframes Paulo Zanoni
  13 siblings, 0 replies; 19+ messages in thread
From: Paulo Zanoni @ 2012-05-28 19:42 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Some HDMI registers can be used for SDVO, so saying "HDMIB" should be
the same as saying "SDVOB" for a given HW generation. This was not
true and led to confusions and even a regression.

Previously we had:
 - SDVO{B,C} defined as the Gen3+ resgisters
 - HDMI{B,C,D} and PCH_SDVOB defined as the PCH registers

Now we have:
  - GEN3_SDVO{B,C} defined as the Gen3+ registers
  - GEN4_HDMI{B,C} defined as the Gen4+ registers
  - PCH_SDVOB defined as the PCH register
  - PCH_HDMI{B,C,D} defined as the PCH registers

So now PCH_SDVOB is the same as PCH_HDMIB and GEN3_SDVOB is the same
as GEN4_HDMIB.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      |   20 +++++++++---------
 drivers/gpu/drm/i915/intel_display.c |   38 +++++++++++++++++-----------------
 drivers/gpu/drm/i915/intel_hdmi.c    |   24 ++++++++++-----------
 drivers/gpu/drm/i915/intel_sdvo.c    |   18 ++++++++--------
 4 files changed, 50 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 1855ac7..e1f44059 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1588,8 +1588,9 @@
 #define   SDVOB_HOTPLUG_INT_STATUS_I915		(1 << 6)
 
 /* SDVO port control */
-#define SDVOB			0x61140
-#define SDVOC			0x61160
+#define GEN3_SDVOB		0x61140
+#define GEN3_SDVOC		0x61160
+#define PCH_SDVOB		0xe1140
 #define   SDVO_ENABLE		(1 << 31)
 #define   SDVO_PIPE_B_SELECT	(1 << 30)
 #define   SDVO_STALL_SELECT	(1 << 29)
@@ -3806,9 +3807,14 @@
 #define  ADPA_CRT_HOTPLUG_VOLREF_475MV  (1<<17)
 #define  ADPA_CRT_HOTPLUG_FORCE_TRIGGER (1<<16)
 
-/* or SDVOB */
+/* The same register may be used for SDVO or HDMI */
+#define GEN4_HDMIB GEN3_SDVOB
+#define GEN4_HDMIC GEN3_SDVOC
 #define VLV_HDMIB 0x61140
-#define HDMIB   0xe1140
+#define PCH_HDMIB   PCH_SDVOB
+#define PCH_HDMIC   0xe1150
+#define PCH_HDMID   0xe1160
+
 #define  PORT_ENABLE    (1 << 31)
 #define  TRANSCODER(pipe)       ((pipe) << 30)
 #define  TRANSCODER_CPT(pipe)   ((pipe) << 29)
@@ -3829,12 +3835,6 @@
 #define  HSYNC_ACTIVE_HIGH      (1 << 3)
 #define  PORT_DETECTED          (1 << 2)
 
-/* PCH SDVOB multiplex with HDMIB */
-#define PCH_SDVOB	HDMIB
-
-#define HDMIC   0xe1150
-#define HDMID   0xe1160
-
 #define PCH_LVDS	0xe1180
 #define  LVDS_DETECTED	(1 << 1)
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3c71850..d1013b7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1257,9 +1257,9 @@ static void assert_pch_ports_disabled(struct drm_i915_private *dev_priv,
 	     "PCH LVDS enabled on transcoder %c, should be disabled\n",
 	     pipe_name(pipe));
 
-	assert_pch_hdmi_disabled(dev_priv, pipe, HDMIB);
-	assert_pch_hdmi_disabled(dev_priv, pipe, HDMIC);
-	assert_pch_hdmi_disabled(dev_priv, pipe, HDMID);
+	assert_pch_hdmi_disabled(dev_priv, pipe, PCH_HDMIB);
+	assert_pch_hdmi_disabled(dev_priv, pipe, PCH_HDMIC);
+	assert_pch_hdmi_disabled(dev_priv, pipe, PCH_HDMID);
 }
 
 /**
@@ -1747,9 +1747,9 @@ static void intel_disable_pch_ports(struct drm_i915_private *dev_priv,
 		udelay(100);
 	}
 
-	disable_pch_hdmi(dev_priv, pipe, HDMIB);
-	disable_pch_hdmi(dev_priv, pipe, HDMIC);
-	disable_pch_hdmi(dev_priv, pipe, HDMID);
+	disable_pch_hdmi(dev_priv, pipe, PCH_HDMIB);
+	disable_pch_hdmi(dev_priv, pipe, PCH_HDMIC);
+	disable_pch_hdmi(dev_priv, pipe, PCH_HDMID);
 }
 
 int
@@ -6519,20 +6519,20 @@ static void intel_setup_outputs(struct drm_device *dev)
 	} else if (HAS_PCH_SPLIT(dev)) {
 		int found;
 
-		if (I915_READ(HDMIB) & PORT_DETECTED) {
+		if (I915_READ(PCH_HDMIB) & PORT_DETECTED) {
 			/* PCH SDVOB multiplex with HDMIB */
 			found = intel_sdvo_init(dev, PCH_SDVOB, true);
 			if (!found)
-				intel_hdmi_init(dev, HDMIB);
+				intel_hdmi_init(dev, PCH_HDMIB);
 			if (!found && (I915_READ(PCH_DP_B) & DP_DETECTED))
 				intel_dp_init(dev, PCH_DP_B);
 		}
 
-		if (I915_READ(HDMIC) & PORT_DETECTED)
-			intel_hdmi_init(dev, HDMIC);
+		if (I915_READ(PCH_HDMIC) & PORT_DETECTED)
+			intel_hdmi_init(dev, PCH_HDMIC);
 
-		if (I915_READ(HDMID) & PORT_DETECTED)
-			intel_hdmi_init(dev, HDMID);
+		if (I915_READ(PCH_HDMID) & PORT_DETECTED)
+			intel_hdmi_init(dev, PCH_HDMID);
 
 		if (I915_READ(PCH_DP_C) & DP_DETECTED)
 			intel_dp_init(dev, PCH_DP_C);
@@ -6543,12 +6543,12 @@ static void intel_setup_outputs(struct drm_device *dev)
 	} else if (SUPPORTS_DIGITAL_OUTPUTS(dev)) {
 		bool found = false;
 
-		if (I915_READ(SDVOB) & SDVO_DETECTED) {
+		if (I915_READ(GEN3_SDVOB) & SDVO_DETECTED) {
 			DRM_DEBUG_KMS("probing SDVOB\n");
-			found = intel_sdvo_init(dev, SDVOB, true);
+			found = intel_sdvo_init(dev, GEN3_SDVOB, true);
 			if (!found && SUPPORTS_INTEGRATED_HDMI(dev)) {
 				DRM_DEBUG_KMS("probing HDMI on SDVOB\n");
-				intel_hdmi_init(dev, SDVOB);
+				intel_hdmi_init(dev, GEN4_HDMIB);
 			}
 
 			if (!found && SUPPORTS_INTEGRATED_DP(dev)) {
@@ -6559,16 +6559,16 @@ static void intel_setup_outputs(struct drm_device *dev)
 
 		/* Before G4X SDVOC doesn't have its own detect register */
 
-		if (I915_READ(SDVOB) & SDVO_DETECTED) {
+		if (I915_READ(GEN3_SDVOB) & SDVO_DETECTED) {
 			DRM_DEBUG_KMS("probing SDVOC\n");
-			found = intel_sdvo_init(dev, SDVOC, false);
+			found = intel_sdvo_init(dev, GEN3_SDVOC, false);
 		}
 
-		if (!found && (I915_READ(SDVOC) & SDVO_DETECTED)) {
+		if (!found && (I915_READ(GEN3_SDVOC) & SDVO_DETECTED)) {
 
 			if (SUPPORTS_INTEGRATED_HDMI(dev)) {
 				DRM_DEBUG_KMS("probing HDMI on SDVOC\n");
-				intel_hdmi_init(dev, SDVOC);
+				intel_hdmi_init(dev, GEN3_SDVOC);
 			}
 			if (SUPPORTS_INTEGRATED_DP(dev)) {
 				DRM_DEBUG_KMS("probing DP_C\n");
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index fc09d04..d0a466a 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -339,10 +339,10 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
 	}
 
 	switch (intel_hdmi->hdmi_reg) {
-	case SDVOB:
+	case GEN4_HDMIB:
 		port = VIDEO_DIP_PORT_B;
 		break;
-	case SDVOC:
+	case GEN4_HDMIC:
 		port = VIDEO_DIP_PORT_C;
 		break;
 	default:
@@ -389,13 +389,13 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
 	}
 
 	switch (intel_hdmi->hdmi_reg) {
-	case HDMIB:
+	case PCH_HDMIB:
 		port = VIDEO_DIP_PORT_B;
 		break;
-	case HDMIC:
+	case PCH_HDMIC:
 		port = VIDEO_DIP_PORT_C;
 		break;
-	case HDMID:
+	case PCH_HDMID:
 		port = VIDEO_DIP_PORT_D;
 		break;
 	default:
@@ -618,10 +618,10 @@ static bool g4x_hdmi_connected(struct intel_hdmi *intel_hdmi)
 	uint32_t bit;
 
 	switch (intel_hdmi->hdmi_reg) {
-	case SDVOB:
+	case GEN4_HDMIB:
 		bit = HDMIB_HOTPLUG_LIVE_STATUS;
 		break;
-	case SDVOC:
+	case GEN4_HDMIC:
 		bit = HDMIC_HOTPLUG_LIVE_STATUS;
 		break;
 	default:
@@ -844,23 +844,23 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg)
 	intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
 
 	/* Set up the DDC bus. */
-	if (hdmi_reg == SDVOB) {
+	if (hdmi_reg == GEN4_HDMIB) {
 		intel_encoder->clone_mask = (1 << INTEL_HDMIB_CLONE_BIT);
 		intel_hdmi->ddc_bus = GMBUS_PORT_DPB;
 		dev_priv->hotplug_supported_mask |= HDMIB_HOTPLUG_INT_STATUS;
-	} else if (hdmi_reg == SDVOC) {
+	} else if (hdmi_reg == GEN4_HDMIC) {
 		intel_encoder->clone_mask = (1 << INTEL_HDMIC_CLONE_BIT);
 		intel_hdmi->ddc_bus = GMBUS_PORT_DPC;
 		dev_priv->hotplug_supported_mask |= HDMIC_HOTPLUG_INT_STATUS;
-	} else if (hdmi_reg == HDMIB) {
+	} else if (hdmi_reg == PCH_HDMIB) {
 		intel_encoder->clone_mask = (1 << INTEL_HDMID_CLONE_BIT);
 		intel_hdmi->ddc_bus = GMBUS_PORT_DPB;
 		dev_priv->hotplug_supported_mask |= HDMIB_HOTPLUG_INT_STATUS;
-	} else if (hdmi_reg == HDMIC) {
+	} else if (hdmi_reg == PCH_HDMIC) {
 		intel_encoder->clone_mask = (1 << INTEL_HDMIE_CLONE_BIT);
 		intel_hdmi->ddc_bus = GMBUS_PORT_DPC;
 		dev_priv->hotplug_supported_mask |= HDMIC_HOTPLUG_INT_STATUS;
-	} else if (hdmi_reg == HDMID) {
+	} else if (hdmi_reg == PCH_HDMID) {
 		intel_encoder->clone_mask = (1 << INTEL_HDMIF_CLONE_BIT);
 		intel_hdmi->ddc_bus = GMBUS_PORT_DPD;
 		dev_priv->hotplug_supported_mask |= HDMID_HOTPLUG_INT_STATUS;
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index a1840f4..2b2919c 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -240,10 +240,10 @@ static void intel_sdvo_write_sdvox(struct intel_sdvo *intel_sdvo, u32 val)
 		return;
 	}
 
-	if (intel_sdvo->sdvo_reg == SDVOB) {
-		cval = I915_READ(SDVOC);
+	if (intel_sdvo->sdvo_reg == GEN3_SDVOB) {
+		cval = I915_READ(GEN3_SDVOC);
 	} else {
-		bval = I915_READ(SDVOB);
+		bval = I915_READ(GEN3_SDVOB);
 	}
 	/*
 	 * Write the registers twice for luck. Sometimes,
@@ -252,10 +252,10 @@ static void intel_sdvo_write_sdvox(struct intel_sdvo *intel_sdvo, u32 val)
 	 */
 	for (i = 0; i < 2; i++)
 	{
-		I915_WRITE(SDVOB, bval);
-		I915_READ(SDVOB);
-		I915_WRITE(SDVOC, cval);
-		I915_READ(SDVOC);
+		I915_WRITE(GEN3_SDVOB, bval);
+		I915_READ(GEN3_SDVOB);
+		I915_WRITE(GEN3_SDVOC, cval);
+		I915_READ(GEN3_SDVOC);
 	}
 }
 
@@ -1103,10 +1103,10 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
 	} else {
 		sdvox = I915_READ(intel_sdvo->sdvo_reg);
 		switch (intel_sdvo->sdvo_reg) {
-		case SDVOB:
+		case GEN3_SDVOB:
 			sdvox &= SDVOB_PRESERVE_MASK;
 			break;
-		case SDVOC:
+		case GEN3_SDVOC:
 			sdvox &= SDVOC_PRESERVE_MASK;
 			break;
 		}
-- 
1.7.10

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 13/14] drm/i915: add some barriers when changing DIPs
  2012-05-28 19:42 [PATCH 00/14] More HDMI fixes V2 Paulo Zanoni
                   ` (11 preceding siblings ...)
  2012-05-28 19:42 ` [PATCH 12/14] drm/i915: clarify confusion between HDMI and SDVO registers Paulo Zanoni
@ 2012-05-28 19:43 ` Paulo Zanoni
  2012-05-30 21:08   ` Daniel Vetter
  2012-05-28 19:43 ` [PATCH 14/14] drm/i915: make sure HDMI port is disabled inside set_infoframes Paulo Zanoni
  13 siblings, 1 reply; 19+ messages in thread
From: Paulo Zanoni @ 2012-05-28 19:43 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

On IVB and older, we basically have two registers: the control and the
data register. We write a few consecutitve times to the control
register, and we need these writes to arrive exactly in the specified
order.

Also, when we're changing the data register, we need to guarantee that
anything written to the control register already arrived (since
changing the control register can change where the data register
points to). Also, we need to make sure all the writes to the data
register happen exactly in the specified order, and we also *can't*
read the data register during this process, since reading and/or
writing it will change the place it points to.

So invoke the "better safe than sorry" rule and just be careful and
put barriers everywhere :)

On HSW we still have a control register that we write many times, but
we have many data registers.

Demanded-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index d0a466a..f61af21 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -133,16 +133,19 @@ static void g4x_write_infoframe(struct drm_encoder *encoder,
 
 	I915_WRITE(VIDEO_DIP_CTL, val);
 
+	mmiowb();
 	for (i = 0; i < len; i += 4) {
 		I915_WRITE(VIDEO_DIP_DATA, *data);
 		data++;
 	}
+	mmiowb();
 
 	val |= g4x_infoframe_enable(frame);
 	val &= ~VIDEO_DIP_FREQ_MASK;
 	val |= VIDEO_DIP_FREQ_VSYNC;
 
 	I915_WRITE(VIDEO_DIP_CTL, val);
+	POSTING_READ(VIDEO_DIP_CTL);
 }
 
 static void ibx_write_infoframe(struct drm_encoder *encoder,
@@ -165,16 +168,19 @@ static void ibx_write_infoframe(struct drm_encoder *encoder,
 
 	I915_WRITE(reg, val);
 
+	mmiowb();
 	for (i = 0; i < len; i += 4) {
 		I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), *data);
 		data++;
 	}
+	mmiowb();
 
 	val |= g4x_infoframe_enable(frame);
 	val &= ~VIDEO_DIP_FREQ_MASK;
 	val |= VIDEO_DIP_FREQ_VSYNC;
 
 	I915_WRITE(reg, val);
+	POSTING_READ(reg);
 }
 
 static void cpt_write_infoframe(struct drm_encoder *encoder,
@@ -200,16 +206,19 @@ static void cpt_write_infoframe(struct drm_encoder *encoder,
 
 	I915_WRITE(reg, val);
 
+	mmiowb();
 	for (i = 0; i < len; i += 4) {
 		I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), *data);
 		data++;
 	}
+	mmiowb();
 
 	val |= g4x_infoframe_enable(frame);
 	val &= ~VIDEO_DIP_FREQ_MASK;
 	val |= VIDEO_DIP_FREQ_VSYNC;
 
 	I915_WRITE(reg, val);
+	POSTING_READ(reg);
 }
 
 static void vlv_write_infoframe(struct drm_encoder *encoder,
@@ -232,16 +241,19 @@ static void vlv_write_infoframe(struct drm_encoder *encoder,
 
 	I915_WRITE(reg, val);
 
+	mmiowb();
 	for (i = 0; i < len; i += 4) {
 		I915_WRITE(VLV_TVIDEO_DIP_DATA(intel_crtc->pipe), *data);
 		data++;
 	}
+	mmiowb();
 
 	val |= g4x_infoframe_enable(frame);
 	val &= ~VIDEO_DIP_FREQ_MASK;
 	val |= VIDEO_DIP_FREQ_VSYNC;
 
 	I915_WRITE(reg, val);
+	POSTING_READ(reg);
 }
 
 static void hsw_write_infoframe(struct drm_encoder *encoder,
@@ -262,13 +274,16 @@ static void hsw_write_infoframe(struct drm_encoder *encoder,
 	val &= ~hsw_infoframe_enable(frame);
 	I915_WRITE(ctl_reg, val);
 
+	mmiowb();
 	for (i = 0; i < len; i += 4) {
 		I915_WRITE(data_reg + i, *data);
 		data++;
 	}
+	mmiowb();
 
 	val |= hsw_infoframe_enable(frame);
 	I915_WRITE(ctl_reg, val);
+	POSTING_READ(ctl_reg);
 }
 
 static void intel_set_infoframe(struct drm_encoder *encoder,
@@ -335,6 +350,7 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
 			return;
 		val &= ~VIDEO_DIP_ENABLE;
 		I915_WRITE(reg, val);
+		POSTING_READ(reg);
 		return;
 	}
 
@@ -353,6 +369,7 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
 		if (val & VIDEO_DIP_ENABLE) {
 			val &= ~VIDEO_DIP_ENABLE;
 			I915_WRITE(reg, val);
+			POSTING_READ(reg);
 		}
 		val &= ~VIDEO_DIP_PORT_MASK;
 		val |= port;
@@ -362,6 +379,7 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
 	val &= ~VIDEO_DIP_ENABLE_VENDOR;
 
 	I915_WRITE(reg, val);
+	POSTING_READ(reg);
 
 	intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
 	intel_hdmi_set_spd_infoframe(encoder);
@@ -385,6 +403,7 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
 			return;
 		val &= ~VIDEO_DIP_ENABLE;
 		I915_WRITE(reg, val);
+		POSTING_READ(reg);
 		return;
 	}
 
@@ -406,6 +425,7 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
 		if (val & VIDEO_DIP_ENABLE) {
 			val &= ~VIDEO_DIP_ENABLE;
 			I915_WRITE(reg, val);
+			POSTING_READ(reg);
 		}
 		val &= ~VIDEO_DIP_PORT_MASK;
 		val |= port;
@@ -416,6 +436,7 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
 		 VIDEO_DIP_ENABLE_GCP);
 
 	I915_WRITE(reg, val);
+	POSTING_READ(reg);
 
 	intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
 	intel_hdmi_set_spd_infoframe(encoder);
@@ -438,6 +459,7 @@ static void cpt_set_infoframes(struct drm_encoder *encoder,
 			return;
 		val &= ~(VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI);
 		I915_WRITE(reg, val);
+		POSTING_READ(reg);
 		return;
 	}
 
@@ -447,6 +469,7 @@ static void cpt_set_infoframes(struct drm_encoder *encoder,
 		 VIDEO_DIP_ENABLE_GCP);
 
 	I915_WRITE(reg, val);
+	POSTING_READ(reg);
 
 	intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
 	intel_hdmi_set_spd_infoframe(encoder);
@@ -469,6 +492,7 @@ static void vlv_set_infoframes(struct drm_encoder *encoder,
 			return;
 		val &= ~VIDEO_DIP_ENABLE;
 		I915_WRITE(reg, val);
+		POSTING_READ(reg);
 		return;
 	}
 
@@ -477,6 +501,7 @@ static void vlv_set_infoframes(struct drm_encoder *encoder,
 		 VIDEO_DIP_ENABLE_GCP);
 
 	I915_WRITE(reg, val);
+	POSTING_READ(reg);
 
 	intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
 	intel_hdmi_set_spd_infoframe(encoder);
@@ -493,6 +518,7 @@ static void hsw_set_infoframes(struct drm_encoder *encoder,
 
 	if (!intel_hdmi->has_hdmi_sink) {
 		I915_WRITE(reg, 0);
+		POSTING_READ(reg);
 		return;
 	}
 
@@ -500,6 +526,7 @@ static void hsw_set_infoframes(struct drm_encoder *encoder,
 		 VIDEO_DIP_ENABLE_VS_HSW | VIDEO_DIP_ENABLE_GMP_HSW);
 
 	I915_WRITE(reg, val);
+	POSTING_READ(reg);
 
 	intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
 	intel_hdmi_set_spd_infoframe(encoder);
-- 
1.7.10

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 14/14] drm/i915: make sure HDMI port is disabled inside set_infoframes
  2012-05-28 19:42 [PATCH 00/14] More HDMI fixes V2 Paulo Zanoni
                   ` (12 preceding siblings ...)
  2012-05-28 19:43 ` [PATCH 13/14] drm/i915: add some barriers when changing DIPs Paulo Zanoni
@ 2012-05-28 19:43 ` Paulo Zanoni
  2012-05-30 12:15   ` Daniel Vetter
  13 siblings, 1 reply; 19+ messages in thread
From: Paulo Zanoni @ 2012-05-28 19:43 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

This function is supposed to be used at mode set time, so prevent
against future mistakes by adding a WARN().

Problems when calling it when the HDMI port is enabled:
  - It may disable sending DIPs (on IVB and older), and the register
    specification says we shouldn't disable DIPs with the HDMI port
    enabled.
  - It does not wait for the VSync because the pipe is disabled at
    mode set.

The second condition can be fixed with some additional code, but the
first one is not so easy... Ideally, code changing the infoframes
outside of the mode set should wait for the VSync and then directly
call the write_infoframe functions.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index f61af21..6fda359 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -334,6 +334,9 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
 	u32 val = I915_READ(reg);
 	u32 port;
 
+	WARN(I915_READ(intel_hdmi->hdmi_reg) & SDVO_ENABLE,
+	     "Calling set_infoframes with HDMI port enabled");
+
 	/* If the registers were not initialized yet, they might be zeroes,
 	 * which means we're selecting the AVI DIP and we're setting its
 	 * frequency to once. This seems to really confuse the HW and make
@@ -395,6 +398,9 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
 	u32 val = I915_READ(reg);
 	u32 port;
 
+	WARN(I915_READ(intel_hdmi->hdmi_reg) & SDVO_ENABLE,
+	     "Calling set_infoframes with HDMI port enabled");
+
 	/* See the big comment in g4x_set_infoframes() */
 	val |= VIDEO_DIP_SELECT_AVI | VIDEO_DIP_FREQ_VSYNC;
 
@@ -451,6 +457,9 @@ static void cpt_set_infoframes(struct drm_encoder *encoder,
 	u32 reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
 	u32 val = I915_READ(reg);
 
+	WARN(I915_READ(intel_hdmi->hdmi_reg) & SDVO_ENABLE,
+	     "Calling set_infoframes with HDMI port enabled");
+
 	/* See the big comment in g4x_set_infoframes() */
 	val |= VIDEO_DIP_SELECT_AVI | VIDEO_DIP_FREQ_VSYNC;
 
@@ -484,6 +493,9 @@ static void vlv_set_infoframes(struct drm_encoder *encoder,
 	u32 reg = VLV_TVIDEO_DIP_CTL(intel_crtc->pipe);
 	u32 val = I915_READ(reg);
 
+	WARN(I915_READ(intel_hdmi->hdmi_reg) & SDVO_ENABLE,
+	     "Calling set_infoframes with HDMI port enabled");
+
 	/* See the big comment in g4x_set_infoframes() */
 	val |= VIDEO_DIP_SELECT_AVI | VIDEO_DIP_FREQ_VSYNC;
 
@@ -516,6 +528,9 @@ static void hsw_set_infoframes(struct drm_encoder *encoder,
 	u32 reg = HSW_TVIDEO_DIP_CTL(intel_crtc->pipe);
 	u32 val = I915_READ(reg);
 
+	WARN(I915_READ(intel_hdmi->hdmi_reg) & DDI_BUF_CTL_ENABLE,
+	     "Calling set_infoframes with HDMI port enabled");
+
 	if (!intel_hdmi->has_hdmi_sink) {
 		I915_WRITE(reg, 0);
 		POSTING_READ(reg);
-- 
1.7.10

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH 02/14] drm/i915: properly alternate between DVI and HDMI
  2012-05-28 19:42 ` [PATCH 02/14] drm/i915: properly alternate between DVI and HDMI Paulo Zanoni
@ 2012-05-28 19:53   ` Chris Wilson
  2012-05-28 20:04     ` Paulo Zanoni
  0 siblings, 1 reply; 19+ messages in thread
From: Chris Wilson @ 2012-05-28 19:53 UTC (permalink / raw)
  To: Paulo Zanoni, intel-gfx; +Cc: Paulo Zanoni

On Mon, 28 May 2012 16:42:49 -0300, Paulo Zanoni <przanoni@gmail.com> wrote:
> This patch adds the code to fix the problem, but it depends on the
> removal of some code that can't be removed right now and will come
> later in the patch series. The patch that we need is:
>   - drm/i915: don't write 0 to DIP control at HDMI init

I was going to grumble a bit more and ask if these could be split into
generational patches so that a bisect doesn't land on a commit affecting
them all. However, it sounds like bisecting through this series is going
to be problematic anyway... Just to be sure, we are not introducing
issues to be resolved in later patches?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 02/14] drm/i915: properly alternate between DVI and HDMI
  2012-05-28 19:53   ` Chris Wilson
@ 2012-05-28 20:04     ` Paulo Zanoni
  0 siblings, 0 replies; 19+ messages in thread
From: Paulo Zanoni @ 2012-05-28 20:04 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

2012/5/28 Chris Wilson <chris@chris-wilson.co.uk>:
> On Mon, 28 May 2012 16:42:49 -0300, Paulo Zanoni <przanoni@gmail.com> wrote:
>> This patch adds the code to fix the problem, but it depends on the
>> removal of some code that can't be removed right now and will come
>> later in the patch series. The patch that we need is:
>>   - drm/i915: don't write 0 to DIP control at HDMI init
>
> I was going to grumble a bit more and ask if these could be split into
> generational patches so that a bisect doesn't land on a commit affecting
> them all. However, it sounds like bisecting through this series is going
> to be problematic anyway... Just to be sure, we are not introducing
> issues to be resolved in later patches?

No. The idea of depending on a patch that comes later in the series is
because I tried to avoid any regressions in the middle of the series
and also tried to avoid moving code to the "set_infoframes" function
only to remove it later. The "don't write 0 to DIP" patch needs the
set_infoframes function to completely set the state of the control
register, so it basically needed all the previous patches. Any
regression introduced in the middle of the series and then removed
later is an accident.

I could split patches into generational patches, but then we'd have
like 60 patches, most of them one-liners...


> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 14/14] drm/i915: make sure HDMI port is disabled inside set_infoframes
  2012-05-28 19:43 ` [PATCH 14/14] drm/i915: make sure HDMI port is disabled inside set_infoframes Paulo Zanoni
@ 2012-05-30 12:15   ` Daniel Vetter
  0 siblings, 0 replies; 19+ messages in thread
From: Daniel Vetter @ 2012-05-30 12:15 UTC (permalink / raw)
  To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni

On Mon, May 28, 2012 at 04:43:01PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> This function is supposed to be used at mode set time, so prevent
> against future mistakes by adding a WARN().
> 
> Problems when calling it when the HDMI port is enabled:
>   - It may disable sending DIPs (on IVB and older), and the register
>     specification says we shouldn't disable DIPs with the HDMI port
>     enabled.
>   - It does not wait for the VSync because the pipe is disabled at
>     mode set.
> 
> The second condition can be fixed with some additional code, but the
> first one is not so easy... Ideally, code changing the infoframes
> outside of the mode set should wait for the VSync and then directly
> call the write_infoframe functions.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Can't we extract this into a little assert_hdmi_port_disabled(dev_priv,
reg) like we have with e.g. assert_pch_hdmi_disabled? Difference would be
that we don't check for the pipe here ... Imo that would make the code
slight less verbose and the intention behind the check a bit clearer.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_hdmi.c |   15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index f61af21..6fda359 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -334,6 +334,9 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
>  	u32 val = I915_READ(reg);
>  	u32 port;
>  
> +	WARN(I915_READ(intel_hdmi->hdmi_reg) & SDVO_ENABLE,
> +	     "Calling set_infoframes with HDMI port enabled");
> +
>  	/* If the registers were not initialized yet, they might be zeroes,
>  	 * which means we're selecting the AVI DIP and we're setting its
>  	 * frequency to once. This seems to really confuse the HW and make
> @@ -395,6 +398,9 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
>  	u32 val = I915_READ(reg);
>  	u32 port;
>  
> +	WARN(I915_READ(intel_hdmi->hdmi_reg) & SDVO_ENABLE,
> +	     "Calling set_infoframes with HDMI port enabled");
> +
>  	/* See the big comment in g4x_set_infoframes() */
>  	val |= VIDEO_DIP_SELECT_AVI | VIDEO_DIP_FREQ_VSYNC;
>  
> @@ -451,6 +457,9 @@ static void cpt_set_infoframes(struct drm_encoder *encoder,
>  	u32 reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
>  	u32 val = I915_READ(reg);
>  
> +	WARN(I915_READ(intel_hdmi->hdmi_reg) & SDVO_ENABLE,
> +	     "Calling set_infoframes with HDMI port enabled");
> +
>  	/* See the big comment in g4x_set_infoframes() */
>  	val |= VIDEO_DIP_SELECT_AVI | VIDEO_DIP_FREQ_VSYNC;
>  
> @@ -484,6 +493,9 @@ static void vlv_set_infoframes(struct drm_encoder *encoder,
>  	u32 reg = VLV_TVIDEO_DIP_CTL(intel_crtc->pipe);
>  	u32 val = I915_READ(reg);
>  
> +	WARN(I915_READ(intel_hdmi->hdmi_reg) & SDVO_ENABLE,
> +	     "Calling set_infoframes with HDMI port enabled");
> +
>  	/* See the big comment in g4x_set_infoframes() */
>  	val |= VIDEO_DIP_SELECT_AVI | VIDEO_DIP_FREQ_VSYNC;
>  
> @@ -516,6 +528,9 @@ static void hsw_set_infoframes(struct drm_encoder *encoder,
>  	u32 reg = HSW_TVIDEO_DIP_CTL(intel_crtc->pipe);
>  	u32 val = I915_READ(reg);
>  
> +	WARN(I915_READ(intel_hdmi->hdmi_reg) & DDI_BUF_CTL_ENABLE,
> +	     "Calling set_infoframes with HDMI port enabled");
> +
>  	if (!intel_hdmi->has_hdmi_sink) {
>  		I915_WRITE(reg, 0);
>  		POSTING_READ(reg);
> -- 
> 1.7.10
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 13/14] drm/i915: add some barriers when changing DIPs
  2012-05-28 19:43 ` [PATCH 13/14] drm/i915: add some barriers when changing DIPs Paulo Zanoni
@ 2012-05-30 21:08   ` Daniel Vetter
  0 siblings, 0 replies; 19+ messages in thread
From: Daniel Vetter @ 2012-05-30 21:08 UTC (permalink / raw)
  To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni

On Mon, May 28, 2012 at 04:43:00PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> On IVB and older, we basically have two registers: the control and the
> data register. We write a few consecutitve times to the control
> register, and we need these writes to arrive exactly in the specified
> order.
> 
> Also, when we're changing the data register, we need to guarantee that
> anything written to the control register already arrived (since
> changing the control register can change where the data register
> points to). Also, we need to make sure all the writes to the data
> register happen exactly in the specified order, and we also *can't*
> read the data register during this process, since reading and/or
> writing it will change the place it points to.
> 
> So invoke the "better safe than sorry" rule and just be careful and
> put barriers everywhere :)
> 
> On HSW we still have a control register that we write many times, but
> we have many data registers.
> 
> Demanded-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Ok, I've all patches from this series for -next safe for the patches 10,
12 and 14:
- for 14 I've already dumped a bikeshed
- 10 and 12 I like, but I fear we'll get too many merge conflicts against
  -fixes if I merge them this early. I'd still like to include them for
  3.6, so can you please resend these two later in the 3.5 cycle, when
  things have settled a bit for -fixes?

Thanks a lot for digging into this infoframe maze.

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2012-05-30 21:06 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-05-28 19:42 [PATCH 00/14] More HDMI fixes V2 Paulo Zanoni
2012-05-28 19:42 ` [PATCH 01/14] drm/i915: add set_infoframes to struct intel_hdmi Paulo Zanoni
2012-05-28 19:42 ` [PATCH 02/14] drm/i915: properly alternate between DVI and HDMI Paulo Zanoni
2012-05-28 19:53   ` Chris Wilson
2012-05-28 20:04     ` Paulo Zanoni
2012-05-28 19:42 ` [PATCH 03/14] drm/i915: only set the HDMI port on the DIP once Paulo Zanoni
2012-05-28 19:42 ` [PATCH 04/14] drm/i915: enable DIP before enabling each InfoFrame Paulo Zanoni
2012-05-28 19:42 ` [PATCH 05/14] drm/i915: don't wait for vblank while writing InfoFrames Paulo Zanoni
2012-05-28 19:42 ` [PATCH 06/14] drm/i915: explicitly disable the DIPs we're not using Paulo Zanoni
2012-05-28 19:42 ` [PATCH 07/14] drm/i915: disable DIP while changing the port Paulo Zanoni
2012-05-28 19:42 ` [PATCH 08/14] drm/i915: don't write 0 to DIP control at HDMI init Paulo Zanoni
2012-05-28 19:42 ` [PATCH 09/14] drm/i915: don't set SDVO_BORDER_ENABLE when we're HDMI Paulo Zanoni
2012-05-28 19:42 ` [PATCH 10/14] drm/i915: rename sdvox_reg to hdmi_reg on HDMI context Paulo Zanoni
2012-05-28 19:42 ` [PATCH 11/14] drm/i915: remove comment about HSW HDMI DIPs Paulo Zanoni
2012-05-28 19:42 ` [PATCH 12/14] drm/i915: clarify confusion between HDMI and SDVO registers Paulo Zanoni
2012-05-28 19:43 ` [PATCH 13/14] drm/i915: add some barriers when changing DIPs Paulo Zanoni
2012-05-30 21:08   ` Daniel Vetter
2012-05-28 19:43 ` [PATCH 14/14] drm/i915: make sure HDMI port is disabled inside set_infoframes Paulo Zanoni
2012-05-30 12:15   ` Daniel Vetter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.