drm_get_edid i2c failure

drm_get_edid i2c failure

[Michael Buesch]

Getting the EDID succeeds most of the time on my system. However,
every now and then, especially when the machine is highly loaded,
i2c communication and thus drm_get_edid() fails.
My guess is that some kind of electrical noise and/or misdesign in
the used components is the root cause of this.

The display with the issues is an Apple Cinema connected via DVI.
Another LG display is connected through VGA to the other connector
of the ATI Radeon HD 5450. (xrandr is used)

This is what kernel log shows on errors:

[ 9509.250829] i2c i2c-3: sendbytes: NAK bailout.
[ 9519.655190] i2c i2c-3: readbytes: ack/nak timeout
[ 9519.655258] [drm:radeon_dvi_detect] *ERROR* DVI-I-1: probed a monitor but no|invalid EDID
[ 9550.716078] [drm:radeon_dvi_detect] *ERROR* DVI-I-1: probed a monitor but no|invalid EDID
[ 9560.738835] i2c i2c-3: sendbytes: NAK bailout.
[ 9612.778073] [drm:radeon_dvi_detect] *ERROR* DVI-I-1: probed a monitor but no|invalid EDID

The display flickers for a few seconds and then continues to
work correctly.

So I applied the following patch to the kernel to workaround the issue:

Index: linux-2.6.37/drivers/gpu/drm/drm_edid.c
===================================================================
--- linux-2.6.37.orig/drivers/gpu/drm/drm_edid.c	2011-03-05 13:09:17.001960834 +0100
+++ linux-2.6.37/drivers/gpu/drm/drm_edid.c	2011-03-05 13:11:34.974194354 +0100
@@ -343,9 +343,16 @@ struct edid *drm_get_edid(struct drm_con
 			  struct i2c_adapter *adapter)
 {
 	struct edid *edid = NULL;
+	unsigned int tries;
 
-	if (drm_probe_ddc(adapter))
-		edid = (struct edid *)drm_do_get_edid(connector, adapter);
+	for (tries = 0; tries < 20; tries++) {
+		if (drm_probe_ddc(adapter))
+			edid = (struct edid *)drm_do_get_edid(connector, adapter);
+		if (edid)
+			break;
+	}
+	if (tries > 0 && tries < 20)
+		printk(KERN_INFO "drm_get_edid(): Succeed after %u tries\n", tries);
 
 	connector->display_info.raw_edid = (char *)edid;
 

This seems to help. I get messages like:

[ 6506.029922] i2c i2c-3: sendbytes: NAK bailout.
[ 6506.030828] i2c i2c-3: sendbytes: NAK bailout.
[ 6506.137459] drm_get_edid(): Succeed after 3 tries
[ 8647.073894] i2c i2c-3: sendbytes: NAK bailout.
[ 8647.196343] drm_get_edid(): Succeed after 2 tries
[ 8749.381956] drm_get_edid(): Succeed after 1 tries
[ 8759.523868] i2c i2c-3: readbytes: ack/nak timeout
[ 8759.668134] drm_get_edid(): Succeed after 1 tries

I was wondering if a real fix or workaround could be found for mainline inclusion.
Would it be acceptable to have a retry loop similar to the above patch?
Some suggestions?

-- 
Greetings, Michael.

[PATCH] drm: Retry i2c transfer of EDID block after failure

[Chris Wilson]

Usually EDID retrieval is fine. However, sometimes, especially when the
machine is loaded, it fails, but succeeds after a few retries.

Based on a patch by Michael Buesch.

Reported-by: Michael Buesch <mb@bu3sch.de>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---

I was going to suggest tuning the i2c_adapter.retries of the affected
components, but that only covers EGAIN and not EREMOTEIO/ETIMEDOUT that
afflicts us. With the alteration to only retry the transfer of the
affected block, this looks to be a reasonable idea and complements the
logic to retry corrupted transfers.
-Chris

---
 drivers/gpu/drm/drm_edid.c |   44 ++++++++++++++++++++++++++------------------
 1 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index a245d17..ddc3da9 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -230,24 +230,32 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf,
 		      int block, int len)
 {
 	unsigned char start = block * EDID_LENGTH;
-	struct i2c_msg msgs[] = {
-		{
-			.addr	= DDC_ADDR,
-			.flags	= 0,
-			.len	= 1,
-			.buf	= &start,
-		}, {
-			.addr	= DDC_ADDR,
-			.flags	= I2C_M_RD,
-			.len	= len,
-			.buf	= buf,
-		}
-	};
+	int ret, retries = 5;
 
-	if (i2c_transfer(adapter, msgs, 2) == 2)
-		return 0;
+	/* The core i2c driver will automatically retry the transfer if the
+	 * adapter reports EAGAIN. However, we find that bit-banging transfers
+	 * are susceptible to errors under a heavily loaded machine and
+	 * generate spurious NAKs and timeouts. Retrying the transfer
+	 * of the individual block a few times seems to overcome this.
+	 */
+	do {
+		struct i2c_msg msgs[] = {
+			{
+				.addr	= DDC_ADDR,
+				.flags	= 0,
+				.len	= 1,
+				.buf	= &start,
+			}, {
+				.addr	= DDC_ADDR,
+				.flags	= I2C_M_RD,
+				.len	= len,
+				.buf	= buf,
+			}
+		};
+		ret = i2c_transfer(adapter, msgs, 2);
+	} while (ret != 2 && --retries);
 
-	return -1;
+	return ret == 2 ? 0 : -1;
 }
 
 static u8 *
-- 
1.7.2.3

Re: [PATCH] drm: Retry i2c transfer of EDID block after failure

[Alex Deucher]

On Tue, Mar 15, 2011 at 7:04 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Usually EDID retrieval is fine. However, sometimes, especially when the
> machine is loaded, it fails, but succeeds after a few retries.
>
> Based on a patch by Michael Buesch.
>
> Reported-by: Michael Buesch <mb@bu3sch.de>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Looks good.

Reviewed-by: Alex Deucher <alexdeucher@gmail.com>

> ---
>
> I was going to suggest tuning the i2c_adapter.retries of the affected
> components, but that only covers EGAIN and not EREMOTEIO/ETIMEDOUT that
> afflicts us. With the alteration to only retry the transfer of the
> affected block, this looks to be a reasonable idea and complements the
> logic to retry corrupted transfers.
> -Chris
>
> ---
>  drivers/gpu/drm/drm_edid.c |   44 ++++++++++++++++++++++++++------------------
>  1 files changed, 26 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index a245d17..ddc3da9 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -230,24 +230,32 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf,
>                      int block, int len)
>  {
>        unsigned char start = block * EDID_LENGTH;
> -       struct i2c_msg msgs[] = {
> -               {
> -                       .addr   = DDC_ADDR,
> -                       .flags  = 0,
> -                       .len    = 1,
> -                       .buf    = &start,
> -               }, {
> -                       .addr   = DDC_ADDR,
> -                       .flags  = I2C_M_RD,
> -                       .len    = len,
> -                       .buf    = buf,
> -               }
> -       };
> +       int ret, retries = 5;
>
> -       if (i2c_transfer(adapter, msgs, 2) == 2)
> -               return 0;
> +       /* The core i2c driver will automatically retry the transfer if the
> +        * adapter reports EAGAIN. However, we find that bit-banging transfers
> +        * are susceptible to errors under a heavily loaded machine and
> +        * generate spurious NAKs and timeouts. Retrying the transfer
> +        * of the individual block a few times seems to overcome this.
> +        */
> +       do {
> +               struct i2c_msg msgs[] = {
> +                       {
> +                               .addr   = DDC_ADDR,
> +                               .flags  = 0,
> +                               .len    = 1,
> +                               .buf    = &start,
> +                       }, {
> +                               .addr   = DDC_ADDR,
> +                               .flags  = I2C_M_RD,
> +                               .len    = len,
> +                               .buf    = buf,
> +                       }
> +               };
> +               ret = i2c_transfer(adapter, msgs, 2);
> +       } while (ret != 2 && --retries);
>
> -       return -1;
> +       return ret == 2 ? 0 : -1;
>  }
>
>  static u8 *
> --
> 1.7.2.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Re: [PATCH] drm: Retry i2c transfer of EDID block after failure

[Michael Buesch]

On Tue, 2011-03-15 at 11:04 +0000, Chris Wilson wrote:
> Usually EDID retrieval is fine. However, sometimes, especially when the
> machine is loaded, it fails, but succeeds after a few retries.
> 
> Based on a patch by Michael Buesch.
> 
> Reported-by: Michael Buesch <mb@bu3sch.de>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---

I'm currently testing this. It boots, but to see if
it fixes the issue, I'll have to run some more tests.

Note that this patch is subject to .37-table
and probably earlier releases, too.

-- 
Greetings, Michael.