* [PATCH] drm/i915: check whether we actually received an edid in detect_ddc @ 2012-07-11 9:47 Daniel Vetter 2012-07-11 9:58 ` Chris Wilson 0 siblings, 1 reply; 7+ messages in thread From: Daniel Vetter @ 2012-07-11 9:47 UTC (permalink / raw) To: Intel Graphics Development; +Cc: Daniel Vetter 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. 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 | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c index 61d55d3..540561c 100644 --- a/drivers/gpu/drm/i915/intel_crt.c +++ b/drivers/gpu/drm/i915/intel_crt.c @@ -353,6 +353,9 @@ static bool intel_crt_detect_ddc(struct drm_connector *connector) is_digital = edid->input & DRM_EDID_INPUT_DIGITAL; connector->display_info.raw_edid = NULL; kfree(edid); + } else { + DRM_DEBUG_KMS("CRT not detected via DDC:0x50 [no valid EDID found]\n"); + return false; } if (!is_digital) { -- 1.7.10 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: check whether we actually received an edid in detect_ddc 2012-07-11 9:47 [PATCH] drm/i915: check whether we actually received an edid in detect_ddc Daniel Vetter @ 2012-07-11 9:58 ` Chris Wilson 2012-07-11 10:31 ` [PATCH 1/2] " Daniel Vetter 0 siblings, 1 reply; 7+ messages in thread From: Chris Wilson @ 2012-07-11 9:58 UTC (permalink / raw) To: Intel Graphics Development; +Cc: Daniel Vetter On Wed, 11 Jul 2012 11:47:51 +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. I'd prefer if we flatten the control flow in that function, state that the intention is to only return a definite positive result and if in any doubt we return false. Note that the ddc probe is implicit in drm_get_edid() and that we then have a stale comment about handling a broken EDID!. So intel_crtc_detect_ddc() { BUG_ON(crt->type != ANALOG); ret = false; if ((edid = drm_get_edid()) { ret = edid_is_analog(edid); kfree(edid); } return ret; } -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] drm/i915: check whether we actually received an edid in detect_ddc 2012-07-11 9:58 ` Chris Wilson @ 2012-07-11 10:31 ` Daniel Vetter 2012-07-11 10:31 ` [PATCH 2/2] drm/i915: kill intel_ddc_probe Daniel Vetter 2012-07-11 10:44 ` [PATCH 1/2] drm/i915: check whether we actually received an edid in detect_ddc Chris Wilson 0 siblings, 2 replies; 7+ messages in thread From: Daniel Vetter @ 2012-07-11 10:31 UTC (permalink / raw) To: Intel Graphics Development; +Cc: Daniel Vetter 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(-) 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; } -- 1.7.10 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] drm/i915: kill intel_ddc_probe 2012-07-11 10:31 ` [PATCH 1/2] " Daniel Vetter @ 2012-07-11 10:31 ` Daniel Vetter 2012-07-11 10:43 ` Chris Wilson 2012-07-11 10:44 ` [PATCH 1/2] drm/i915: check whether we actually received an edid in detect_ddc Chris Wilson 1 sibling, 1 reply; 7+ messages in thread From: Daniel Vetter @ 2012-07-11 10:31 UTC (permalink / raw) To: Intel Graphics Development; +Cc: Daniel Vetter We have way too much lying hardware to rely on a simple "does someone answer on the ddc i2c address?" check. And now it's unused, so just kill it. Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_drv.h | 1 - drivers/gpu/drm/i915/intel_modes.c | 28 ---------------------------- 2 files changed, 29 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 6f3bf22..8435355 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -342,7 +342,6 @@ struct intel_fbc_work { }; int intel_ddc_get_modes(struct drm_connector *c, struct i2c_adapter *adapter); -extern bool intel_ddc_probe(struct intel_encoder *intel_encoder, int ddc_bus); extern void intel_attach_force_audio_property(struct drm_connector *connector); extern void intel_attach_broadcast_rgb_property(struct drm_connector *connector); diff --git a/drivers/gpu/drm/i915/intel_modes.c b/drivers/gpu/drm/i915/intel_modes.c index d67ec3a..45848b9 100644 --- a/drivers/gpu/drm/i915/intel_modes.c +++ b/drivers/gpu/drm/i915/intel_modes.c @@ -33,34 +33,6 @@ #include "i915_drv.h" /** - * intel_ddc_probe - * - */ -bool intel_ddc_probe(struct intel_encoder *intel_encoder, int ddc_bus) -{ - struct drm_i915_private *dev_priv = intel_encoder->base.dev->dev_private; - u8 out_buf[] = { 0x0, 0x0}; - u8 buf[2]; - struct i2c_msg msgs[] = { - { - .addr = DDC_ADDR, - .flags = 0, - .len = 1, - .buf = out_buf, - }, - { - .addr = DDC_ADDR, - .flags = I2C_M_RD, - .len = 1, - .buf = buf, - } - }; - - return i2c_transfer(intel_gmbus_get_adapter(dev_priv, ddc_bus), - msgs, 2) == 2; -} - -/** * intel_ddc_get_modes - get modelist from monitor * @connector: DRM connector device to use * @adapter: i2c adapter -- 1.7.10 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] drm/i915: kill intel_ddc_probe 2012-07-11 10:31 ` [PATCH 2/2] drm/i915: kill intel_ddc_probe Daniel Vetter @ 2012-07-11 10:43 ` Chris Wilson 0 siblings, 0 replies; 7+ messages in thread From: Chris Wilson @ 2012-07-11 10:43 UTC (permalink / raw) To: Intel Graphics Development; +Cc: Daniel Vetter On Wed, 11 Jul 2012 12:31:53 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > We have way too much lying hardware to rely on a simple "does someone > answer on the ddc i2c address?" check. And now it's unused, so just > kill it. > > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] drm/i915: check whether we actually received an edid in detect_ddc 2012-07-11 10:31 ` [PATCH 1/2] " Daniel Vetter 2012-07-11 10:31 ` [PATCH 2/2] drm/i915: kill intel_ddc_probe Daniel Vetter @ 2012-07-11 10:44 ` Chris Wilson 2012-07-11 20:02 ` Daniel Vetter 1 sibling, 1 reply; 7+ messages in thread From: Chris Wilson @ 2012-07-11 10:44 UTC (permalink / raw) To: Intel Graphics Development; +Cc: Daniel Vetter 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 -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] drm/i915: check whether we actually received an edid in detect_ddc 2012-07-11 10:44 ` [PATCH 1/2] drm/i915: check whether we actually received an edid in detect_ddc Chris Wilson @ 2012-07-11 20:02 ` Daniel Vetter 0 siblings, 0 replies; 7+ messages in thread From: Daniel Vetter @ 2012-07-11 20:02 UTC (permalink / raw) To: Chris Wilson; +Cc: Daniel Vetter, Intel Graphics Development 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 -- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-07-11 20:02 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-07-11 9:47 [PATCH] drm/i915: check whether we actually received an edid in detect_ddc Daniel Vetter 2012-07-11 9:58 ` Chris Wilson 2012-07-11 10:31 ` [PATCH 1/2] " Daniel Vetter 2012-07-11 10:31 ` [PATCH 2/2] drm/i915: kill intel_ddc_probe Daniel Vetter 2012-07-11 10:43 ` Chris Wilson 2012-07-11 10:44 ` [PATCH 1/2] drm/i915: check whether we actually received an edid in detect_ddc Chris Wilson 2012-07-11 20:02 ` Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).