This reverts commit 6ec3d0c0e9c0c605696e91048eebaca7b0c36695.

When DDC documentation refers to "address 0xA0", it means what the Linux I2C subsystem calls address 0x50. Both refer to the address used for reading EDID over DDC.

I2C documentation often suffers from confusion over whether the address includes the low-order direction bit or not, and DDC documentation (using I2C) is no exception.

The 'addr' field of struct i2c_msg is 7 bits wide (except when I2C_M_TEN is used). The upper bit of 0xA0 is discarded. For example drivers/i2c/algos/i2c-algo-bit.c::bit_doAddress:

unsigned char addr; /* ... */ } else { /* normal 7bit address */ addr = msg->addr << 1;

The test added by the reverted commit is bogus; it really probes address 0x40/0x20, which has no VESA meaning.

Another bogosity writing a random byte from the stack if the address probe finds something. Some attached devices may use address 0x40/0x20 for something proprietary or accidentally exposed, so this is quite bad. I have seen monitors respond to a range of non-standard addresses, although not this one.

What the BIOS writer's guide surely intends is (almost) performed by intel_ddc_probe(), although that does a byte read after the address probe and byte write, so perhaps intel_ddc_probe() needs a bit of slimming too. I'm inclined to think there is no need to even write a byte; just probe the address, with zero message bytes.

Signed-off-by: Jamie Lokier jamie@shareable.org --- drivers/gpu/drm/i915/intel_crt.c | 39 +++---------------------------------- 1 files changed, 4 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c index c55c770..4e8ad21 100644 --- a/drivers/gpu/drm/i915/intel_crt.c +++ b/drivers/gpu/drm/i915/intel_crt.c @@ -262,21 +262,6 @@ static bool intel_crt_detect_hotplug(struct drm_connector *connector) return ret; }

-static bool intel_crt_ddc_probe(struct drm_i915_private *dev_priv, int ddc_bus) -{ - u8 buf; - struct i2c_msg msgs[] = { - { - .addr = 0xA0, - .flags = 0, - .len = 1, - .buf = &buf, - }, - }; - /* DDC monitor detect: Does it ACK a write to 0xA0? */ - return i2c_transfer(&dev_priv->gmbus[ddc_bus].adapter, msgs, 1) == 1; -} - static bool intel_crt_detect_ddc(struct drm_encoder *encoder) { struct intel_encoder *intel_encoder = to_intel_encoder(encoder); @@ -286,17 +271,7 @@ static bool intel_crt_detect_ddc(struct drm_encoder *encoder) if (intel_encoder->type != INTEL_OUTPUT_ANALOG) return false;

- if (intel_crt_ddc_probe(dev_priv, dev_priv->crt_ddc_pin)) { - DRM_DEBUG_KMS("CRT detected via DDC:0xa0\n"); - return true; - } - - if (intel_ddc_probe(intel_encoder, dev_priv->crt_ddc_pin)) { - DRM_DEBUG_KMS("CRT detected via DDC:0x50 [EDID]\n"); - return true; - } - - return false; + return intel_ddc_probe(intel_encoder, dev_priv->crt_ddc_pin); }

static enum drm_connector_status @@ -322,8 +297,6 @@ intel_crt_load_detect(struct drm_crtc *crtc, struct intel_encoder *intel_encoder uint8_t st00; enum drm_connector_status status;

- DRM_DEBUG_KMS("starting load-detect on CRT\n"); - if (pipe == 0) { bclrpat_reg = BCLRPAT_A; vtotal_reg = VTOTAL_A; @@ -440,10 +413,9 @@ intel_crt_detect(struct drm_connector *connector, bool force) enum drm_connector_status status;

if (I915_HAS_HOTPLUG(dev)) { - if (intel_crt_detect_hotplug(connector)) { - DRM_DEBUG_KMS("CRT detected via hotplug\n"); + if (intel_crt_detect_hotplug(connector)) return connector_status_connected; - } else + else return connector_status_disconnected; }

@@ -460,10 +432,7 @@ intel_crt_detect(struct drm_connector *connector, bool force) crtc = intel_get_load_detect_pipe(encoder, connector, NULL, &dpms_mode); if (crtc) { - if (intel_crt_detect_ddc(&encoder->base)) - status = connector_status_connected; - else - status = intel_crt_load_detect(crtc, encoder); + status = intel_crt_load_detect(crtc, encoder); intel_release_load_detect_pipe(encoder, connector, dpms_mode); } else