[PATCH 1/2] drm/i915/crt: Remove 0xa0 probe for CRT

[PATCH 1/2] drm/i915/crt: Remove 0xa0 probe for CRT

[Chris Wilson]

Following the fix to reset the GMBUS controller after a NAK, we finally
utilize the 0xa0 probe for a CRT connection. And discover that it is
useless as it simply detects the presence of the controller and not the
actual monitor. Given that we already probe 0x50 prior to performing the
EDID retrieval, we can simply remove the redundant probe to 0xa0.

Reported-and-tested-by: Sitsofe Wheeler <sitsofe@yahoo.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=35904
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_crt.c |   20 --------------------
 1 files changed, 0 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 8342259..d03fc05 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -269,21 +269,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_connector *connector)
 {
 	struct intel_crt *crt = intel_attached_crt(connector);
@@ -293,11 +278,6 @@ static bool intel_crt_detect_ddc(struct drm_connector *connector)
 	if (crt->base.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(&crt->base, dev_priv->crt_ddc_pin)) {
 		struct edid *edid;
 		bool is_digital = false;
-- 
1.7.4.1

[PATCH 2/2] drm/i915/crt: Explicitly return false if connected to a digital monitor

[Chris Wilson]

Rather than proceed on to the next test, return the result from
detecting a connection but to a digital panel and include some debug
output as to why.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_crt.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index d03fc05..5adc5b2 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -298,6 +298,9 @@ static bool intel_crt_detect_ddc(struct drm_connector *connector)
 		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");
+			return false;
 		}
 	}
 
-- 
1.7.4.1

Re: [PATCH 2/2] drm/i915/crt: Explicitly return false if connected to a digital monitor

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 368 bytes --]

On Mon,  4 Apr 2011 07:26:45 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Rather than proceed on to the next test, return the result from
> detecting a connection but to a digital panel and include some debug
> output as to why.

I'm worried that this will falsely reject some monitors with broken EDID
information.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/crt: Explicitly return false if connected to a digital monitor

[Chris Wilson]

On Mon, 04 Apr 2011 08:09:42 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Mon,  4 Apr 2011 07:26:45 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > Rather than proceed on to the next test, return the result from
> > detecting a connection but to a digital panel and include some debug
> > output as to why.
> 
> I'm worried that this will falsely reject some monitors with broken EDID
> information.

This doesn't change the nature of the test, just adds a debug
message so that we explain why it was rejected. Vital information should
anyone report a bug with a connected VGA monitor.

This is another case where we are damned if we do and damned if we don't.
Some hardware requires us to differentiate between VGA/DVI connections on
the same i2c wire and the rejection here was originally added to enable us
to function correctly on such a machine.

As far as broken EDIDs go, there are plenty of bugs to go around.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915/crt: Explicitly return false if connected to a digital monitor

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 448 bytes --]

On Mon, 04 Apr 2011 16:25:27 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> This doesn't change the nature of the test, just adds a debug
> message so that we explain why it was rejected. Vital information should
> anyone report a bug with a connected VGA monitor.

Sorry, I didn't look at the context of the patch closely enough. This
looks good.

Reviewed-by: Keith Packard <keithp@keithp.com>

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/crt: Remove 0xa0 probe for CRT

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 954 bytes --]

On Mon,  4 Apr 2011 07:26:44 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Following the fix to reset the GMBUS controller after a NAK, we finally
> utilize the 0xa0 probe for a CRT connection. And discover that it is
> useless as it simply detects the presence of the controller and not the
> actual monitor. Given that we already probe 0x50 prior to performing the
> EDID retrieval, we can simply remove the redundant probe to 0xa0.

I don't understand -- are you saying that there is additional hardware
somewhere in the machine responding to I2C transactions on the monitor's
DDC bus?

And if so, how was this extra transaction breaking things? The GMBUS
reset patch should only have an effect when there are failed
transactions, not successful ones, and then it should only cause future
transactions to fail, not succeed. Besides, the 0xA0 transaction looks
like the first one on the bus.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/crt: Remove 0xa0 probe for CRT

[Chris Wilson]

On Mon, 04 Apr 2011 08:08:40 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Mon,  4 Apr 2011 07:26:44 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > Following the fix to reset the GMBUS controller after a NAK, we finally
> > utilize the 0xa0 probe for a CRT connection. And discover that it is
> > useless as it simply detects the presence of the controller and not the
> > actual monitor. Given that we already probe 0x50 prior to performing the
> > EDID retrieval, we can simply remove the redundant probe to 0xa0.
> 
> I don't understand -- are you saying that there is additional hardware
> somewhere in the machine responding to I2C transactions on the monitor's
> DDC bus?

Yes. I'm saying that that the controller accepts a write to port 0xa0.
 
> And if so, how was this extra transaction breaking things? The GMBUS
> reset patch should only have an effect when there are failed
> transactions, not successful ones, and then it should only cause future
> transactions to fail, not succeed. Besides, the 0xA0 transaction looks
> like the first one on the bus.

We do several i2c xfers before CRT.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915/crt: Remove 0xa0 probe for CRT

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 1353 bytes --]

On Mon, 04 Apr 2011 16:29:55 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Yes. I'm saying that that the controller accepts a write to port 0xa0.

So it's the GMBUS controller itself then, I guess. Weird.

Let me see if I understand how it used to work and why fixing the GMBUS
reset causes it to break in this case.

In the distant past (pre-GMBUS)

 1) Some previous DDC transaction would fail, but without GMBUS
    this would not break the bus
 2) The 0xA0 transaction would fail as there wasn't anyone
    listening on the DDC bus.
 3) The 0x50 transaction would also fail, again because no-one
    was listening
 4) The monitor would be reported as disconnected.

In the recent past (post-GMBUS):

 1) Some previous DDC transaction would fail, wedging the GMBUS
 2) The 0xA0 transaction would then fail due to the GMBUS breakage
 3) The 0x50 transaction would also fail as the GMBUS was wedged
 4) The VGA port would be reported as disconnected

With the GMBUS reset:

 1) Some previous DDC transaction would fail, but the GMBUS would get
    reset
 2) The 0xA0 transaction would now succeed.
 3) The VGA port would be reported as connected.

Do we have any idea what ports the GMBUS controller is listening
internally for? And, whether this differs from chip to chip?

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/crt: Remove 0xa0 probe for CRT

[Dave Airlie]

On Tue, Apr 5, 2011 at 2:26 AM, Keith Packard <keithp@keithp.com> wrote:
> On Mon, 04 Apr 2011 16:29:55 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
>> Yes. I'm saying that that the controller accepts a write to port 0xa0.
>
> So it's the GMBUS controller itself then, I guess. Weird.
>
> Let me see if I understand how it used to work and why fixing the GMBUS
> reset causes it to break in this case.
>

I could be missing something here, but aren't i2c addresses 8-bit in this case?

7-bit addr + direction bit, which means 0xa0 isn't valid i2c address,
since in Linux
the read/write bits are specified separately.

I could be wrong though.

Dave.

Re: [PATCH 1/2] drm/i915/crt: Remove 0xa0 probe for CRT

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 1093 bytes --]

On Tue, 5 Apr 2011 10:19:16 +1000, Dave Airlie <airlied@gmail.com> wrote:
> On Tue, Apr 5, 2011 at 2:26 AM, Keith Packard <keithp@keithp.com> wrote:
> > On Mon, 04 Apr 2011 16:29:55 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >
> >> Yes. I'm saying that that the controller accepts a write to port 0xa0.
> >
> > So it's the GMBUS controller itself then, I guess. Weird.
> >
> > Let me see if I understand how it used to work and why fixing the GMBUS
> > reset causes it to break in this case.
> >
> 
> I could be missing something here, but aren't i2c addresses 8-bit in this case?
> 
> 7-bit addr + direction bit, which means 0xa0 isn't valid i2c address,
> since in Linux
> the read/write bits are specified separately.

Could this be is a mis-translation of some X server code? The X
server stuck the direction bit in the LSB of the I2C address and
required that the drivers shift the register up and add the direction
bit manually (yes, a terrible API).

This is starting to make a lot more sense now. 0xa0 == 0x50 << 1.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/crt: Remove 0xa0 probe for CRT

[Chris Wilson]

On Mon, 04 Apr 2011 09:26:20 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Mon, 04 Apr 2011 16:29:55 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > Yes. I'm saying that that the controller accepts a write to port 0xa0.
> 
> So it's the GMBUS controller itself then, I guess. Weird.
> 
> Let me see if I understand how it used to work and why fixing the GMBUS
> reset causes it to break in this case.

It also requires just the right combination of hardware to reproduce:
in my case a 915GM (pre-CRT hotplug detect I think is the critical factor).

> In the distant past (pre-GMBUS)
> 
>  1) Some previous DDC transaction would fail, but without GMBUS
>     this would not break the bus
>  2) The 0xA0 transaction would fail as there wasn't anyone
>     listening on the DDC bus.

Not quite. In this case, it fails because the core i2c bitbanging algo
doesn't seem to handle a solitary write message and reports EREMOTEIO.
Whereas using GMBUS to drive the i2c xfer, the hardware completes the
message without reporting an error.

>  3) The 0x50 transaction would also fail, again because no-one
>     was listening
>  4) The monitor would be reported as disconnected.
> 
> In the recent past (post-GMBUS):
> 
>  1) Some previous DDC transaction would fail, wedging the GMBUS
>  2) The 0xA0 transaction would then fail due to the GMBUS breakage

On my test hardware, there happens to be no previous NAK and so it reports
"CRT detected via DDC:0xa0" anyway. But a NAK here can only explain how
the regressions were only reported after 7f58aabc.

>  3) The 0x50 transaction would also fail as the GMBUS was wedged
>  4) The VGA port would be reported as disconnected
> 
> With the GMBUS reset:
> 
>  1) Some previous DDC transaction would fail, but the GMBUS would get
>     reset
>  2) The 0xA0 transaction would now succeed.
>  3) The VGA port would be reported as connected.

Technically as unknown, since although we decided that there was a
connection, we could not retrieve an EDID.

> Do we have any idea what ports the GMBUS controller is listening
> internally for? And, whether this differs from chip to chip?

As Dave suggested, using 0xa0 was fubar. And in this case the controller
was just presumably accepting a write to 0x50, when I expected it to be
NAKed due to no attached slave listening on that address.

Of course, in testing, I choose a combination of hardware (915GM and an
old VGA panel) that proved impossible to retrieve the EDID for, whether
using bitbanging i2c or GMBUS. (I'm seeing the same invalid checksum on a
SugarBar, so it is probably not timing related - but I think this is a
regression in itself.) That did prevent testing a few of the other cases
since even when connected, we could do no better than unknown.

So what I am less clear on is how it actually worked if the GMBUS was
NAKed, and so proceeded past the 0xa0 probe to the real EDID probe and
yet appear to work.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915/crt: Remove 0xa0 probe for CRT

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 604 bytes --]

On Mon,  4 Apr 2011 07:26:44 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Following the fix to reset the GMBUS controller after a NAK, we finally
> utilize the 0xa0 probe for a CRT connection. And discover that it is
> useless as it simply detects the presence of the controller and not the
> actual monitor. Given that we already probe 0x50 prior to performing the
> EDID retrieval, we can simply remove the redundant probe to 0xa0.

This looks like a revert of
6ec3d0c0e9c0c605696e91048eebaca7b0c36695. I'd rather see it as that than
a new patch.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx