All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jamie Lokier <jamie@shareable.org>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: [PATCH] Revert "drm/i915/crt: Use a DDC probe on 0xA0 before load-detect"
Date: Sat, 30 Oct 2010 20:09:06 +0100	[thread overview]
Message-ID: <20101030190906.GA26020@shareable.org> (raw)

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
-- 
1.7.3.rc2.dirty

                 reply	other threads:[~2010-10-30 19:09 UTC|newest]

Thread overview: [no followups] expand[flat|nested]  mbox.gz  Atom feed

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20101030190906.GA26020@shareable.org \
    --to=jamie@shareable.org \
    --cc=chris@chris-wilson.co.uk \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=intel-gfx@lists.freedesktop.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.