drm/i915: properly alternate between DVI and HDMI

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 [danvet: Paulo clarified that this additional patch is only required to make the fix complete, this patch here alone doesn't introduce a regression but only partially solves the problem of randomly clearing the dip registers.] 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> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

drm/i915: properly alternate between DVI and HDMI
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 [danvet: Paulo clarified that this additional patch is only required to make the fix complete, this patch here alone doesn't introduce a regression but only partially solves the problem of randomly clearing the dip registers.] 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> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Paulo Zanoni · Daniel Vetter
1 parent 687f4d06db
Showing 1 changed file with 85 additions and 3 deletions Side-by-side Diff
drivers/gpu/drm/i915/intel_hdmi.c
@@ -308,9 +308,6 @@
 {
 	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 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 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 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 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 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);
 }
...	...	@@ -308,9 +308,6 @@
308	308	{
309	309	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
310	310
311		- if (!intel_hdmi->has_hdmi_sink)
312		- return;
313		-
314	311	intel_dip_infoframe_csum(frame);
315	312	intel_hdmi->write_infoframe(encoder, frame);
316	313	}
...	...	@@ -348,6 +345,30 @@
348	345	static void g4x_set_infoframes(struct drm_encoder *encoder,
349	346	struct drm_display_mode *adjusted_mode)
350	347	{
	348	+ struct drm_i915_private *dev_priv = encoder->dev->dev_private;
	349	+ struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
	350	+ u32 reg = VIDEO_DIP_CTL;
	351	+ u32 val = I915_READ(reg);
	352	+
	353	+ /* If the registers were not initialized yet, they might be zeroes,
	354	+ * which means we're selecting the AVI DIP and we're setting its
	355	+ * frequency to once. This seems to really confuse the HW and make
	356	+ * things stop working (the register spec says the AVI always needs to
	357	+ * be sent every VSync). So here we avoid writing to the register more
	358	+ * than we need and also explicitly select the AVI DIP and explicitly
	359	+ * set its frequency to every VSync. Avoiding to write it twice seems to
	360	+ * be enough to solve the problem, but being defensive shouldn't hurt us
	361	+ * either. */
	362	+ val \|= VIDEO_DIP_SELECT_AVI \| VIDEO_DIP_FREQ_VSYNC;
	363	+
	364	+ if (!intel_hdmi->has_hdmi_sink) {
	365	+ if (!(val & VIDEO_DIP_ENABLE))
	366	+ return;
	367	+ val &= ~VIDEO_DIP_ENABLE;
	368	+ I915_WRITE(reg, val);
	369	+ return;
	370	+ }
	371	+
351	372	intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
352	373	intel_hdmi_set_spd_infoframe(encoder);
353	374	}
...	...	@@ -355,6 +376,23 @@
355	376	static void ibx_set_infoframes(struct drm_encoder *encoder,
356	377	struct drm_display_mode *adjusted_mode)
357	378	{
	379	+ struct drm_i915_private *dev_priv = encoder->dev->dev_private;
	380	+ struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
	381	+ struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
	382	+ u32 reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
	383	+ u32 val = I915_READ(reg);
	384	+
	385	+ /* See the big comment in g4x_set_infoframes() */
	386	+ val \|= VIDEO_DIP_SELECT_AVI \| VIDEO_DIP_FREQ_VSYNC;
	387	+
	388	+ if (!intel_hdmi->has_hdmi_sink) {
	389	+ if (!(val & VIDEO_DIP_ENABLE))
	390	+ return;
	391	+ val &= ~VIDEO_DIP_ENABLE;
	392	+ I915_WRITE(reg, val);
	393	+ return;
	394	+ }
	395	+
358	396	intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
359	397	intel_hdmi_set_spd_infoframe(encoder);
360	398	}
...	...	@@ -362,6 +400,23 @@
362	400	static void cpt_set_infoframes(struct drm_encoder *encoder,
363	401	struct drm_display_mode *adjusted_mode)
364	402	{
	403	+ struct drm_i915_private *dev_priv = encoder->dev->dev_private;
	404	+ struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
	405	+ struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
	406	+ u32 reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
	407	+ u32 val = I915_READ(reg);
	408	+
	409	+ /* See the big comment in g4x_set_infoframes() */
	410	+ val \|= VIDEO_DIP_SELECT_AVI \| VIDEO_DIP_FREQ_VSYNC;
	411	+
	412	+ if (!intel_hdmi->has_hdmi_sink) {
	413	+ if (!(val & VIDEO_DIP_ENABLE))
	414	+ return;
	415	+ val &= ~(VIDEO_DIP_ENABLE \| VIDEO_DIP_ENABLE_AVI);
	416	+ I915_WRITE(reg, val);
	417	+ return;
	418	+ }
	419	+
365	420	intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
366	421	intel_hdmi_set_spd_infoframe(encoder);
367	422	}
...	...	@@ -369,6 +424,23 @@
369	424	static void vlv_set_infoframes(struct drm_encoder *encoder,
370	425	struct drm_display_mode *adjusted_mode)
371	426	{
	427	+ struct drm_i915_private *dev_priv = encoder->dev->dev_private;
	428	+ struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
	429	+ struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
	430	+ u32 reg = VLV_TVIDEO_DIP_CTL(intel_crtc->pipe);
	431	+ u32 val = I915_READ(reg);
	432	+
	433	+ /* See the big comment in g4x_set_infoframes() */
	434	+ val \|= VIDEO_DIP_SELECT_AVI \| VIDEO_DIP_FREQ_VSYNC;
	435	+
	436	+ if (!intel_hdmi->has_hdmi_sink) {
	437	+ if (!(val & VIDEO_DIP_ENABLE))
	438	+ return;
	439	+ val &= ~VIDEO_DIP_ENABLE;
	440	+ I915_WRITE(reg, val);
	441	+ return;
	442	+ }
	443	+
372	444	intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
373	445	intel_hdmi_set_spd_infoframe(encoder);
374	446	}
...	...	@@ -376,6 +448,16 @@
376	448	static void hsw_set_infoframes(struct drm_encoder *encoder,
377	449	struct drm_display_mode *adjusted_mode)
378	450	{
	451	+ struct drm_i915_private *dev_priv = encoder->dev->dev_private;
	452	+ struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
	453	+ struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
	454	+ u32 reg = HSW_TVIDEO_DIP_CTL(intel_crtc->pipe);
	455	+
	456	+ if (!intel_hdmi->has_hdmi_sink) {
	457	+ I915_WRITE(reg, 0);
	458	+ return;
	459	+ }
	460	+
379	461	intel_hdmi_set_avi_infoframe(encoder, adjusted_mode);
380	462	intel_hdmi_set_spd_infoframe(encoder);
381	463	}