diff mbox

[1/2] drm/i915: check whether we actually received an edid in detect_ddc

Message ID 1342002713-12628-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Vetter July 11, 2012, 10:31 a.m. UTC
Somehow detect_ddc manages to fall through all checks when we think
that something responds on the ddc i2c address, but the edid read
failed. Fix this up by explicitly checking for this case.

This fixes a regression on newer chips because since

commit aaa377302b2994fcc2c66741b47da33feb489dca
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Sat Jun 16 15:30:32 2012 +0200

    drm/i915/crt: Do not rely upon the HPD presence pin

we use ddc detection also on hotplug capable platforms. And one of
these reads all 0s for any i2c transaction if nothing is connected to
the vga port.

v2: Implement Chris Wilson's review:
- simplify logic, default to "nothing detected"
- kill stale comment
- BUG_ON(!crt->type != ANALOG)

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=51900
Tested-by: Yang Guang <guang.a.yang@intel.com>
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_crt.c |   33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

Comments

Chris Wilson July 11, 2012, 10:44 a.m. UTC | #1
On Wed, 11 Jul 2012 12:31:52 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Somehow detect_ddc manages to fall through all checks when we think
> that something responds on the ddc i2c address, but the edid read
> failed. Fix this up by explicitly checking for this case.
> 
> This fixes a regression on newer chips because since
> 
> commit aaa377302b2994fcc2c66741b47da33feb489dca
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Sat Jun 16 15:30:32 2012 +0200
> 
>     drm/i915/crt: Do not rely upon the HPD presence pin
> 
> we use ddc detection also on hotplug capable platforms. And one of
> these reads all 0s for any i2c transaction if nothing is connected to
> the vga port.
> 
> v2: Implement Chris Wilson's review:
> - simplify logic, default to "nothing detected"
> - kill stale comment
> - BUG_ON(!crt->type != ANALOG)
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=51900
> Tested-by: Yang Guang <guang.a.yang@intel.com>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I'm happy with that, and thanks for the extra clarification in the debug
messages.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
Daniel Vetter July 11, 2012, 8:02 p.m. UTC | #2
On Wed, Jul 11, 2012 at 11:44:36AM +0100, Chris Wilson wrote:
> On Wed, 11 Jul 2012 12:31:52 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > Somehow detect_ddc manages to fall through all checks when we think
> > that something responds on the ddc i2c address, but the edid read
> > failed. Fix this up by explicitly checking for this case.
> > 
> > This fixes a regression on newer chips because since
> > 
> > commit aaa377302b2994fcc2c66741b47da33feb489dca
> > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Date:   Sat Jun 16 15:30:32 2012 +0200
> > 
> >     drm/i915/crt: Do not rely upon the HPD presence pin
> > 
> > we use ddc detection also on hotplug capable platforms. And one of
> > these reads all 0s for any i2c transaction if nothing is connected to
> > the vga port.
> > 
> > v2: Implement Chris Wilson's review:
> > - simplify logic, default to "nothing detected"
> > - kill stale comment
> > - BUG_ON(!crt->type != ANALOG)
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=51900
> > Tested-by: Yang Guang <guang.a.yang@intel.com>
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> I'm happy with that, and thanks for the extra clarification in the debug
> messages.
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Both patches merged to dinq, thanks a lot for the review.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 61d55d3..3c6028c 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -330,39 +330,34 @@  static bool intel_crt_detect_ddc(struct drm_connector *connector)
 {
 	struct intel_crt *crt = intel_attached_crt(connector);
 	struct drm_i915_private *dev_priv = crt->base.base.dev->dev_private;
+	struct edid *edid;
+	struct i2c_adapter *i2c;
+
+	BUG_ON(crt->base.type != INTEL_OUTPUT_ANALOG);
 
-	/* CRT should always be at 0, but check anyway */
-	if (crt->base.type != INTEL_OUTPUT_ANALOG)
-		return false;
+	i2c = intel_gmbus_get_adapter(dev_priv, dev_priv->crt_ddc_pin);
+	edid = drm_get_edid(connector, i2c);
 
-	if (intel_ddc_probe(&crt->base, dev_priv->crt_ddc_pin)) {
-		struct edid *edid;
-		bool is_digital = false;
-		struct i2c_adapter *i2c;
+	if (edid) {
+		bool is_digital = edid->input & DRM_EDID_INPUT_DIGITAL;
 
-		i2c = intel_gmbus_get_adapter(dev_priv, dev_priv->crt_ddc_pin);
-		edid = drm_get_edid(connector, i2c);
 		/*
 		 * This may be a DVI-I connector with a shared DDC
 		 * link between analog and digital outputs, so we
 		 * have to check the EDID input spec of the attached device.
-		 *
-		 * On the other hand, what should we do if it is a broken EDID?
 		 */
-		if (edid != NULL) {
-			is_digital = edid->input & DRM_EDID_INPUT_DIGITAL;
-			connector->display_info.raw_edid = NULL;
-			kfree(edid);
-		}
-
 		if (!is_digital) {
 			DRM_DEBUG_KMS("CRT detected via DDC:0x50 [EDID]\n");
 			return true;
-		} else {
-			DRM_DEBUG_KMS("CRT not detected via DDC:0x50 [EDID reports a digital panel]\n");
 		}
+
+		DRM_DEBUG_KMS("CRT not detected via DDC:0x50 [EDID reports a digital panel]\n");
+	} else {
+		DRM_DEBUG_KMS("CRT not detected via DDC:0x50 [no valid EDID found]\n");
 	}
 
+	kfree(edid);
+
 	return false;
 }