Intel HDMI ELD fixes

PATCH Superseded Wu Fengguang 1 participant 9 messages Nov 28, 2011 – Dec 9, 2011

Subsystems (2)

DRM drivers Intel i915

lore

Share & tools

lore

Wu Fengguang

author

2011-11-28 07:33 UTC #0 lore raw

cover Intel HDMI ELD fixes

Keith,

The 4 patches are well tested and integrates the review comments.

The only missing part is hot plug notification for DP -- most DP monitors in the market don't support DP audio well. So I cannot test this for now.

Thanks,
Fengguang

Wu Fengguang

author

2011-11-28 07:41 UTC #1 lore raw

PATCH 1/4 drm/i915: fix ELD writing for SandyBridge

SandyBridge should be using the same register addresses as IvyBridge.

Signed-off-by Wu Fengguang <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_display.c +1 -1

@@ -5857,7 +5857,7 @@ static void ironlake_write_eld(struct dr
 	int aud_cntl_st;
 	int aud_cntrl_st2;
 
-	if (IS_IVYBRIDGE(connector->dev)) {
+	if (HAS_PCH_IBX(connector->dev)) {
 		hdmiw_hdmiedid = GEN7_HDMIW_HDMIEDID_A;
 		aud_cntl_st = GEN7_AUD_CNTRL_ST_A;
 		aud_cntrl_st2 = GEN7_AUD_CNTRL_ST2;

Wu Fengguang

author

2011-11-29 11:53 UTC #2 lore raw

PATCH v2 1/4 drm/i915: fix ELD writing for SandyBridge

SandyBridge should be using the same register addresses as IvyBridge.

Signed-off-by Wu Fengguang <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Sorry, this moves some necessary changes from patch 2/4 to here.

drivers/gpu/drm/i915/intel_display.c +5 -5

@@ -5857,14 +5857,14 @@ static void ironlake_write_eld(struct dr
 	int aud_cntl_st;
 	int aud_cntrl_st2;
 
-	if (IS_IVYBRIDGE(connector->dev)) {
-		hdmiw_hdmiedid = GEN7_HDMIW_HDMIEDID_A;
-		aud_cntl_st = GEN7_AUD_CNTRL_ST_A;
-		aud_cntrl_st2 = GEN7_AUD_CNTRL_ST2;
-	} else {
+	if (HAS_PCH_IBX(connector->dev)) {
 		hdmiw_hdmiedid = GEN5_HDMIW_HDMIEDID_A;
 		aud_cntl_st = GEN5_AUD_CNTL_ST_A;
 		aud_cntrl_st2 = GEN5_AUD_CNTL_ST2;
+	} else {
+		hdmiw_hdmiedid = GEN7_HDMIW_HDMIEDID_A;
+		aud_cntl_st = GEN7_AUD_CNTRL_ST_A;
+		aud_cntrl_st2 = GEN7_AUD_CNTRL_ST2;
 	}
 
 	i = to_intel_crtc(crtc)->pipe;

Wu Fengguang

author

2011-11-28 07:33 UTC #3 lore raw

PATCH 2/4 drm/i915: rename audio ELD registers

In reply to Wu Fengguang

Change the definitions from GEN5 to IBX as they aren't in the CPU and some SNB systems actually shipped with IBX chipsets (or, at least that's a supported configuration).

The GEN7_* register addresses actually take effect since GEN6 and should be prefixed by CPT, the PCH code name.

Signed-off-by Wu Fengguang <[email protected]>

---
 drivers/gpu/drm/i915/i915_reg.h      |   22 +++++++++++-----------
 drivers/gpu/drm/i915/intel_display.c |   22 +++++++++++-----------
 2 files changed, 22 insertions(+), 22 deletions(-)

drivers/gpu/drm/i915/i915_reg.h +11 ~~-11~~

@@ -3534,17 +3534,17 @@
 #define G4X_ELD_ACK			(1 << 4)
 #define G4X_HDMIW_HDMIEDID		0x6210C
 
-#define GEN5_HDMIW_HDMIEDID_A		0xE2050
-#define GEN5_AUD_CNTL_ST_A		0xE20B4
-#define GEN5_ELD_BUFFER_SIZE		(0x1f << 10)
-#define GEN5_ELD_ADDRESS		(0x1f << 5)
-#define GEN5_ELD_ACK			(1 << 4)
-#define GEN5_AUD_CNTL_ST2		0xE20C0
-#define GEN5_ELD_VALIDB			(1 << 0)
-#define GEN5_CP_READYB			(1 << 1)
+#define IBX_HDMIW_HDMIEDID_A		0xE2050
+#define IBX_AUD_CNTL_ST_A		0xE20B4
+#define IBX_ELD_BUFFER_SIZE		(0x1f << 10)
+#define IBX_ELD_ADDRESS			(0x1f << 5)
+#define IBX_ELD_ACK			(1 << 4)
+#define IBX_AUD_CNTL_ST2		0xE20C0
+#define IBX_ELD_VALIDB			(1 << 0)
+#define IBX_CP_READYB			(1 << 1)
 
-#define GEN7_HDMIW_HDMIEDID_A		0xE5050
-#define GEN7_AUD_CNTRL_ST_A		0xE50B4
-#define GEN7_AUD_CNTRL_ST2		0xE50C0
+#define CPT_HDMIW_HDMIEDID_A		0xE5050
+#define CPT_AUD_CNTL_ST_A		0xE50B4
+#define CPT_AUD_CNTRL_ST2		0xE50C0
 
 #endif /* _I915_REG_H_ */

drivers/gpu/drm/i915/intel_display.c +11 ~~-11~~

@@ -5858,13 +5858,13 @@ static void ironlake_write_eld(struct dr
 	int aud_cntrl_st2;
 
 	if (HAS_PCH_IBX(connector->dev)) {
-		hdmiw_hdmiedid = GEN7_HDMIW_HDMIEDID_A;
-		aud_cntl_st = GEN7_AUD_CNTRL_ST_A;
-		aud_cntrl_st2 = GEN7_AUD_CNTRL_ST2;
+		hdmiw_hdmiedid = IBX_HDMIW_HDMIEDID_A;
+		aud_cntl_st = IBX_AUD_CNTL_ST_A;
+		aud_cntrl_st2 = IBX_AUD_CNTL_ST2;
 	} else {
-		hdmiw_hdmiedid = GEN5_HDMIW_HDMIEDID_A;
-		aud_cntl_st = GEN5_AUD_CNTL_ST_A;
-		aud_cntrl_st2 = GEN5_AUD_CNTL_ST2;
+		hdmiw_hdmiedid = CPT_HDMIW_HDMIEDID_A;
+		aud_cntl_st = CPT_AUD_CNTL_ST_A;
+		aud_cntrl_st2 = CPT_AUD_CNTRL_ST2;
 	}
 
 	i = to_intel_crtc(crtc)->pipe;

@@ -5878,12 +5878,12 @@ static void ironlake_write_eld(struct dr
 	if (!i) {
 		DRM_DEBUG_DRIVER("Audio directed to unknown port\n");
 		/* operate blindly on all ports */
-		eldv = GEN5_ELD_VALIDB;
-		eldv |= GEN5_ELD_VALIDB << 4;
-		eldv |= GEN5_ELD_VALIDB << 8;
+		eldv = IBX_ELD_VALIDB;
+		eldv |= IBX_ELD_VALIDB << 4;
+		eldv |= IBX_ELD_VALIDB << 8;
 	} else {
 		DRM_DEBUG_DRIVER("ELD on port %c\n", 'A' + i);
-		eldv = GEN5_ELD_VALIDB << ((i - 1) * 4);
+		eldv = IBX_ELD_VALIDB << ((i - 1) * 4);
 	}
 
 	i = I915_READ(aud_cntrl_st2);

@@ -5899,7 +5899,7 @@ static void ironlake_write_eld(struct dr
 	}
 
 	i = I915_READ(aud_cntl_st);
-	i &= ~GEN5_ELD_ADDRESS;
+	i &= ~IBX_ELD_ADDRESS;
 	I915_WRITE(aud_cntl_st, i);
 
 	len = min_t(uint8_t, eld[2], 21);	/* 84 bytes of hw ELD buffer */

Wu Fengguang

author

2011-11-29 11:54 UTC #4 lore raw

PATCH v2 2/4 drm/i915: rename audio ELD registers

Change the definitions from GEN5 to IBX as they aren't in the CPU and some SNB systems actually shipped with IBX chipsets (or, at least that's a supported configuration).

The GEN7_* register addresses actually take effect since GEN6 and should be prefixed by CPT, the PCH code name.

Signed-off-by Wu Fengguang <[email protected]>

---
 drivers/gpu/drm/i915/i915_reg.h      |   22 +++++++++++-----------
 drivers/gpu/drm/i915/intel_display.c |   22 +++++++++++-----------
 2 files changed, 22 insertions(+), 22 deletions(-)

drivers/gpu/drm/i915/i915_reg.h +11 ~~-11~~

@@ -3534,17 +3534,17 @@
 #define G4X_ELD_ACK			(1 << 4)
 #define G4X_HDMIW_HDMIEDID		0x6210C
 
-#define GEN5_HDMIW_HDMIEDID_A		0xE2050
-#define GEN5_AUD_CNTL_ST_A		0xE20B4
-#define GEN5_ELD_BUFFER_SIZE		(0x1f << 10)
-#define GEN5_ELD_ADDRESS		(0x1f << 5)
-#define GEN5_ELD_ACK			(1 << 4)
-#define GEN5_AUD_CNTL_ST2		0xE20C0
-#define GEN5_ELD_VALIDB			(1 << 0)
-#define GEN5_CP_READYB			(1 << 1)
+#define IBX_HDMIW_HDMIEDID_A		0xE2050
+#define IBX_AUD_CNTL_ST_A		0xE20B4
+#define IBX_ELD_BUFFER_SIZE		(0x1f << 10)
+#define IBX_ELD_ADDRESS			(0x1f << 5)
+#define IBX_ELD_ACK			(1 << 4)
+#define IBX_AUD_CNTL_ST2		0xE20C0
+#define IBX_ELD_VALIDB			(1 << 0)
+#define IBX_CP_READYB			(1 << 1)
 
-#define GEN7_HDMIW_HDMIEDID_A		0xE5050
-#define GEN7_AUD_CNTRL_ST_A		0xE50B4
-#define GEN7_AUD_CNTRL_ST2		0xE50C0
+#define CPT_HDMIW_HDMIEDID_A		0xE5050
+#define CPT_AUD_CNTL_ST_A		0xE50B4
+#define CPT_AUD_CNTRL_ST2		0xE50C0
 
 #endif /* _I915_REG_H_ */

drivers/gpu/drm/i915/intel_display.c +11 ~~-11~~

@@ -5858,13 +5858,13 @@ static void ironlake_write_eld(struct dr
 	int aud_cntrl_st2;
 
 	if (HAS_PCH_IBX(connector->dev)) {
-		hdmiw_hdmiedid = GEN5_HDMIW_HDMIEDID_A;
-		aud_cntl_st = GEN5_AUD_CNTL_ST_A;
-		aud_cntrl_st2 = GEN5_AUD_CNTL_ST2;
+		hdmiw_hdmiedid = IBX_HDMIW_HDMIEDID_A;
+		aud_cntl_st = IBX_AUD_CNTL_ST_A;
+		aud_cntrl_st2 = IBX_AUD_CNTL_ST2;
 	} else {
-		hdmiw_hdmiedid = GEN7_HDMIW_HDMIEDID_A;
-		aud_cntl_st = GEN7_AUD_CNTRL_ST_A;
-		aud_cntrl_st2 = GEN7_AUD_CNTRL_ST2;
+		hdmiw_hdmiedid = CPT_HDMIW_HDMIEDID_A;
+		aud_cntl_st = CPT_AUD_CNTL_ST_A;
+		aud_cntrl_st2 = CPT_AUD_CNTRL_ST2;
 	}
 
 	i = to_intel_crtc(crtc)->pipe;

@@ -5878,12 +5878,12 @@ static void ironlake_write_eld(struct dr
 	if (!i) {
 		DRM_DEBUG_DRIVER("Audio directed to unknown port\n");
 		/* operate blindly on all ports */
-		eldv = GEN5_ELD_VALIDB;
-		eldv |= GEN5_ELD_VALIDB << 4;
-		eldv |= GEN5_ELD_VALIDB << 8;
+		eldv = IBX_ELD_VALIDB;
+		eldv |= IBX_ELD_VALIDB << 4;
+		eldv |= IBX_ELD_VALIDB << 8;
 	} else {
 		DRM_DEBUG_DRIVER("ELD on port %c\n", 'A' + i);
-		eldv = GEN5_ELD_VALIDB << ((i - 1) * 4);
+		eldv = IBX_ELD_VALIDB << ((i - 1) * 4);
 	}
 
 	i = I915_READ(aud_cntrl_st2);

@@ -5899,7 +5899,7 @@ static void ironlake_write_eld(struct dr
 	}
 
 	i = I915_READ(aud_cntl_st);
-	i &= ~GEN5_ELD_ADDRESS;
+	i &= ~IBX_ELD_ADDRESS;
 	I915_WRITE(aud_cntl_st, i);
 
 	len = min_t(uint8_t, eld[2], 21);	/* 84 bytes of hw ELD buffer */

Wu Fengguang

author

2011-11-28 07:33 UTC #5 lore raw

PATCH 3/4 drm/i915: dont trigger hotplug events on unchanged ELD

In reply to Wu Fengguang

The ELD may or may not change when switching the video mode. If unchanged, don't trigger hot plug events to HDMI audio driver.

This avoids disturbing the user with repeated printks.

Reported-by Nick Bowler <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c |   51 ++++++++++++++++++++++---
 1 file changed, 46 insertions(+), 5 deletions(-)

drivers/gpu/drm/i915/intel_display.c +46 -5

@@ -5811,6 +5811,35 @@ static int intel_crtc_mode_set(struct dr
 	return ret;
 }
 
+static bool intel_eld_uptodate(struct drm_connector *connector,
+			       int reg_eldv, uint32_t bits_eldv,
+			       int reg_elda, uint32_t bits_elda,
+			       int reg_edid)
+{
+	struct drm_i915_private *dev_priv = connector->dev->dev_private;
+	uint8_t *eld = connector->eld;
+	uint32_t i;
+
+	i = I915_READ(reg_eldv);
+	i &= bits_eldv;
+
+	if (!eld[0])
+		return !i;
+
+	if (!i)
+		return false;
+
+	i = I915_READ(reg_elda);
+	i &= ~bits_elda;
+	I915_WRITE(reg_elda, i);
+
+	for (i = 0; i < eld[2]; i++)
+		if (I915_READ(reg_edid) != *((uint32_t *)eld + i))
+			return false;
+
+	return true;
+}
+
 static void g4x_write_eld(struct drm_connector *connector,
 			  struct drm_crtc *crtc)
 {

@@ -5827,6 +5856,12 @@ static void g4x_write_eld(struct drm_con
 	else
 		eldv = G4X_ELDV_DEVCTG;
 
+	if (intel_eld_uptodate(connector,
+			       G4X_AUD_CNTL_ST, eldv,
+			       G4X_AUD_CNTL_ST, G4X_ELD_ADDR,
+			       G4X_HDMIW_HDMIEDID))
+		return;
+
 	i = I915_READ(G4X_AUD_CNTL_ST);
 	i &= ~(eldv | G4X_ELD_ADDR);
 	len = (i >> 9) & 0x1f;		/* ELD buffer size */

@@ -5886,6 +5921,17 @@ static void ironlake_write_eld(struct dr
 		eldv = IBX_ELD_VALIDB << ((i - 1) * 4);
 	}
 
+	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT)) {
+		DRM_DEBUG_DRIVER("ELD: DisplayPort detected\n");
+		eld[5] |= (1 << 2);	/* Conn_Type, 0x1 = DisplayPort */
+	}
+
+	if (intel_eld_uptodate(connector,
+			       aud_cntrl_st2, eldv,
+			       aud_cntl_st, IBX_ELD_ADDRESS,
+			       hdmiw_hdmiedid))
+		return;
+
 	i = I915_READ(aud_cntrl_st2);
 	i &= ~eldv;
 	I915_WRITE(aud_cntrl_st2, i);

@@ -5893,11 +5939,6 @@ static void ironlake_write_eld(struct dr
 	if (!eld[0])
 		return;
 
-	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT)) {
-		DRM_DEBUG_DRIVER("ELD: DisplayPort detected\n");
-		eld[5] |= (1 << 2);	/* Conn_Type, 0x1 = DisplayPort */
-	}
-
 	i = I915_READ(aud_cntl_st);
 	i &= ~IBX_ELD_ADDRESS;
 	I915_WRITE(aud_cntl_st, i);

Wu Fengguang

author

2011-11-28 07:33 UTC #6 lore raw

PATCH 4/4 drm/i915: HDMI hot remove notification to audio driver

In reply to Wu Fengguang

On HDMI monitor hot remove, clear SDVO_AUDIO_ENABLE accordingly, so that the audio driver will receive hot plug events and take action to refresh its device state and ELD contents.

The cleared SDVO_AUDIO_ENABLE bit needs to be restored to prevent losing HDMI audio after DPMS on.

CC Wang Zhenyu <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c   |    4 +++-
 drivers/gpu/drm/i915/intel_hdmi.c |    8 ++++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/intel_hdmi.c +6 -2

@@ -269,6 +269,10 @@ static void intel_hdmi_dpms(struct drm_e
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
 	u32 temp;
+	u32 enable_bits = SDVO_ENABLE;
+
+	if (intel_hdmi->has_audio)
+		enable_bits |= SDVO_AUDIO_ENABLE;
 
 	temp = I915_READ(intel_hdmi->sdvox_reg);

@@ -281,9 +285,9 @@ static void intel_hdmi_dpms(struct drm_e
 	}
 
 	if (mode != DRM_MODE_DPMS_ON) {
-		temp &= ~SDVO_ENABLE;
+		temp &= ~enable_bits;
 	} else {
-		temp |= SDVO_ENABLE;
+		temp |= enable_bits;
 	}
 
 	I915_WRITE(intel_hdmi->sdvox_reg, temp);

Wu Fengguang

author

2011-11-28 08:06 UTC #7 lore raw

In reply to Wu Fengguang

Quoted 2 lines

The only missing part is hot plug notification for DP -- most DP monitors in the market don't support DP audio well. So I cannot test this for now.

btw, we'd like to buy a Philips 221P3LPYEB that _seems_ to have DP audio support. I'd appreciate if any one can help confirm.

Thanks,
Fengguang

Wu Fengguang

author

2011-12-09 13:12 UTC #8 lore raw

Philips 221P3LPYEB has working DP audio

Quoted 5 lines · Wu Fengguang

Quoted 2 lines
The only missing part is hot plug notification for DP -- most DP monitors in the market don't support DP audio well. So I cannot test this for now.
btw, we'd like to buy a Philips 221P3LPYEB that _seems_ to have DP audio support. I'd appreciate if any one can help confirm.

I'm glad to say that its DP audio works great! And the built in speakers make the testing really convenient :-)

For the record, here is the ELD:

monitor_present         1
eld_valid               1
monitor_name            221P3LPY
connection_type         DisplayPort
eld_version             [0x2] CEA-861D or below
edid_version            [0x3] CEA-861-B, C or D
manufacture_id          0xc41
product_id              0x8a3
port_id                 0x0
support_hdcp            0
support_ai              0
audio_sync_delay        0
speakers                [0x1] FL/FR
sad_count               1
sad0_coding_type        [0x1] LPCM
sad0_channels           2
sad0_rates              [0x6e0] 32000 44100 48000 96000 176400
sad0_bits               [0xe0000] 16 20 24

Note that we didn't buy the 24" 241P3LYEB because it's temporarily not available.

Thanks,
Fengguang