[PATCH] DRM: ignore invalid EDID extensions

[PATCH] DRM: ignore invalid EDID extensions

[sam tygier]

Without this patch i get no display (X or VT) on myEIZO S2242W monitor when using KMS

This was discussed back in september, this is the second version of the patch based on ajax's comments. i have opened a bug for the issue:
https://bugs.freedesktop.org/show_bug.cgi?id=31154

Thanks

Sam

---

 From 33d4041a583c417c00f71a5453fed6cff5278de5 Mon Sep 17 00:00:00 2001
From: Sam Tygier <samtygier@yahoo.co.uk>
Date: Thu, 23 Sep 2010 10:11:01 +0100
Subject: [PATCH] DRM: ignore invalid EDID extensions

Currently an invalid EDID extension will cause the whole EDID to be considered invalid. Instead just drop the invalid extensions, and return the valid ones. The base block is modified to claim to have the number valid extensions, and the check sum is updated.

For my EIZO S2242W the base block is fine, but the extension block is all zeros. Without this patch I get no X and no VTs.

  Signed-off-by: Sam Tygier <samtygier@yahoo.co.uk>

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

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 96e9631..2e208fa 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -241,7 +241,7 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf,
              .addr    = DDC_ADDR,
              .flags    = I2C_M_RD,
              .len    = len,
-            .buf    = buf + start,
+            .buf    = buf,
          }
      };
  
@@ -254,7 +254,7 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf,
  static u8 *
  drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
  {
-    int i, j = 0;
+    int i, j = 0, valid_extensions = 0;
      u8 *block, *new;
  
      if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
@@ -281,14 +281,28 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
  
      for (j = 1; j <= block[0x7e]; j++) {
          for (i = 0; i < 4; i++) {
-            if (drm_do_probe_ddc_edid(adapter, block, j,
-                          EDID_LENGTH))
+            if (drm_do_probe_ddc_edid(adapter,
+                  block + (valid_extensions + 1) * EDID_LENGTH,
+                  j, EDID_LENGTH))
                  goto out;
-            if (drm_edid_block_valid(block + j * EDID_LENGTH))
+            if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH)) {
+                valid_extensions++;
                  break;
+            }
          }
          if (i == 4)
-            goto carp;
+            dev_warn(connector->dev->dev,
+             "%s: Ignoring invalid EDID block %d.\n",
+             drm_get_connector_name(connector), j);
+    }
+
+    if (valid_extensions != block[0x7e]) {
+        block[EDID_LENGTH-1] += block[0x7e] - valid_extensions;
+        block[0x7e] = valid_extensions;
+        new = krealloc(block, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
+        if (!new)
+            goto out;
+        block = new;
      }
  
      return block;
-- 
1.7.1

Re: [PATCH] DRM: ignore invalid EDID extensions

[Marius Gröger]

On 28.10.2010 11:12, wrote sam tygier:
> Without this patch i get no display (X or VT) on myEIZO S2242W monitor
> when using KMS
>
> This was discussed back in september, this is the second version of the
> patch based on ajax's comments. i have opened a bug for the issue:
> https://bugs.freedesktop.org/show_bug.cgi?id=31154

Might this even be a cure for the bug in recent drm that ignores all 
50Hz modes? The patch looks like that--without it--there isn't even an 
error displayed upon hitting a bad EDID record. I'm sorry I can't try 
this out at the moment though.

Regards,
Marius

[PATCH] DRM: ignore invalid EDID extensions

[Sam Tygier]

Currently an invalid EDID extension will cause the whole EDID to be considered invalid. Instead just drop the extension, and return the valid base block. The base block is modified to not claim to have extensions, and update the checksum.

For my EIZO S2242W the base block is fine, but the extension block is all zeros. Without this patch I get no X and no VTs.

Signed-off-by: Sam Tygier <samtygier@yahoo.co.uk>
---
  drivers/gpu/drm/drm_edid.c |    9 +++++++++
  1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index f87bf10..5ade343 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -285,6 +285,15 @@ carp:
  	dev_warn(connector->dev->dev, "%s: EDID block %d invalid.\n",
  		 drm_get_connector_name(connector), j);
  
+	/* Invalid extension, so set block[0x7e] to zero, and return
+	 * the base block */
+	block[EDID_LENGTH-1] += block[0x7e];
+	block[0x7e] = 0;
+	new = krealloc(block, EDID_LENGTH, GFP_KERNEL);
+	if (!new)
+		goto out;
+	block = new;
+	return block;
  out:
  	kfree(block);
  	return NULL;
-- 
1.7.1

Re: [PATCH] DRM: ignore invalid EDID extensions

[Adam Jackson]

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

On Tue, 2010-09-21 at 23:02 +0100, Sam Tygier wrote:
> Currently an invalid EDID extension will cause the whole EDID to be
> considered invalid. Instead just drop the extension, and return the
> valid base block. The base block is modified to not claim to have
> extensions, and update the checksum.

This does not appear to be what your patch does.  I mean, yes, if
there's only one extension block, that's what it does, but if there's
more than one...

> For my EIZO S2242W the base block is fine, but the extension block is
> all zeros. Without this patch I get no X and no VTs.

I suspect what's actually happening there is that we're failing to get
the extension block, not that it's being returned as zeros.

Also, we shouldn't be converting "checksum failure" to "display
disconnected".  That's the real bug here.

- ajax

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] DRM: ignore invalid EDID extensions

[Sam Tygier]

On 22/09/10 21:59, Adam Jackson wrote:
> On Tue, 2010-09-21 at 23:02 +0100, Sam Tygier wrote:
>> Currently an invalid EDID extension will cause the whole EDID to be
>> considered invalid. Instead just drop the extension, and return the
>> valid base block. The base block is modified to not claim to have
>> extensions, and update the checksum.
>
> This does not appear to be what your patch does.  I mean, yes, if
> there's only one extension block, that's what it does, but if there's
> more than one...

I could modify it to skip the only invalid extension blocks. As my monitor only claims to have 1 extension block, i could not test this properly. I also spotted that I should make sure that my fix is not run when the base block fails the checksum 4 times. May it should be put within the for loop, replacing "goto carp;".

>> For my EIZO S2242W the base block is fine, but the extension block is
>> all zeros. Without this patch I get no X and no VTs.
>
> I suspect what's actually happening there is that we're failing to get
> the extension block, not that it's being returned as zeros.

Could this be fixed? Anything I could try?

> Also, we shouldn't be converting "checksum failure" to "display
> disconnected".  That's the real bug here.

In my case it would still need to return base block, as that correctly contains all the modes and stuff.

Thanks

Sam

Re: [PATCH] DRM: ignore invalid EDID extensions

[Adam Jackson]

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

On Wed, 2010-09-22 at 22:42 +0100, Sam Tygier wrote:
> On 22/09/10 21:59, Adam Jackson wrote:
> > On Tue, 2010-09-21 at 23:02 +0100, Sam Tygier wrote:
> >> Currently an invalid EDID extension will cause the whole EDID to be
> >> considered invalid. Instead just drop the extension, and return the
> >> valid base block. The base block is modified to not claim to have
> >> extensions, and update the checksum.
> >
> > This does not appear to be what your patch does.  I mean, yes, if
> > there's only one extension block, that's what it does, but if there's
> > more than one...
> 
> I could modify it to skip the only invalid extension blocks. As my
> monitor only claims to have 1 extension block, i could not test this
> properly. I also spotted that I should make sure that my fix is not
> run when the base block fails the checksum 4 times. May it should be
> put within the for loop, replacing "goto carp;".

Yeah, I hate to just drop extension blocks, but it's better than the
alternative.  They're optional for a reason I suppose.

> >> For my EIZO S2242W the base block is fine, but the extension block is
> >> all zeros. Without this patch I get no X and no VTs.
> >
> > I suspect what's actually happening there is that we're failing to get
> > the extension block, not that it's being returned as zeros.
> 
> Could this be fixed? Anything I could try?

Depends on what your driver is, I suspect.  Did it ever work?

- ajax

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] DRM: ignore invalid EDID extensions

[Sam Tygier]

On 22/09/10 22:55, Adam Jackson wrote:
> On Wed, 2010-09-22 at 22:42 +0100, Sam Tygier wrote:
>> On 22/09/10 21:59, Adam Jackson wrote:
>>> On Tue, 2010-09-21 at 23:02 +0100, Sam Tygier wrote:
>>>> Currently an invalid EDID extension will cause the whole EDID to be
>>>> considered invalid. Instead just drop the extension, and return the
>>>> valid base block. The base block is modified to not claim to have
>>>> extensions, and update the checksum.
>>>
>>> This does not appear to be what your patch does.  I mean, yes, if
>>> there's only one extension block, that's what it does, but if there's
>>> more than one...
>>
>> I could modify it to skip the only invalid extension blocks. As my
>> monitor only claims to have 1 extension block, i could not test this
>> properly. I also spotted that I should make sure that my fix is not
>> run when the base block fails the checksum 4 times. May it should be
>> put within the for loop, replacing "goto carp;".
>
> Yeah, I hate to just drop extension blocks, but it's better than the
> alternative.  They're optional for a reason I suppose.
>
>>>> For my EIZO S2242W the base block is fine, but the extension block is
>>>> all zeros. Without this patch I get no X and no VTs.
>>>
>>> I suspect what's actually happening there is that we're failing to get
>>> the extension block, not that it's being returned as zeros.
>>
>> Could this be fixed? Anything I could try?
>
> Depends on what your driver is, I suspect.  Did it ever work?

My driver is radeon (HD 3650). With older kernels the monitor worked fine. I suspect because the extension was not read at all.

Here is a second version of the patch.

Now only the invalid extensions are dropped. Good ones should be kept (I don't have any hardware with multiple extensions so I can't test this).

It was necessary to to change the meaning of the buf parameter to drm_do_get_edid() to be where the EDID should be written, not the start of block. This allows you to read the Nth extension into the Mth position where N!=M. All the other calls to drm_do_get_edid() have block set to zero, so are unchanged. int valid_extensions is needed to keep track of how many valid extensions have been retrieved.

The skipping, and fixing code was moved out from the carp block.

---

 From 33d4041a583c417c00f71a5453fed6cff5278de5 Mon Sep 17 00:00:00 2001
From: Sam Tygier <samtygier@yahoo.co.uk>
Date: Thu, 23 Sep 2010 10:11:01 +0100
Subject: [PATCH] DRM: ignore invalid EDID extensions

Currently an invalid EDID extension will cause the whole EDID to be considered invalid. Instead just drop the invalid extensions, and return the valid ones. The base block is modified to claim to have the number valid extensions, and the check sum is updated.

For my EIZO S2242W the base block is fine, but the extension block is all zeros. Without this patch I get no X and no VTs.

  Signed-off-by: Sam Tygier <samtygier@yahoo.co.uk>

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

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 96e9631..2e208fa 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -241,7 +241,7 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf,
  			.addr	= DDC_ADDR,
  			.flags	= I2C_M_RD,
  			.len	= len,
-			.buf	= buf + start,
+			.buf	= buf,
  		}
  	};
  
@@ -254,7 +254,7 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf,
  static u8 *
  drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
  {
-	int i, j = 0;
+	int i, j = 0, valid_extensions = 0;
  	u8 *block, *new;
  
  	if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
@@ -281,14 +281,28 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
  
  	for (j = 1; j <= block[0x7e]; j++) {
  		for (i = 0; i < 4; i++) {
-			if (drm_do_probe_ddc_edid(adapter, block, j,
-						  EDID_LENGTH))
+			if (drm_do_probe_ddc_edid(adapter,
+				  block + (valid_extensions + 1) * EDID_LENGTH,
+				  j, EDID_LENGTH))
  				goto out;
-			if (drm_edid_block_valid(block + j * EDID_LENGTH))
+			if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH)) {
+				valid_extensions++;
  				break;
+			}
  		}
  		if (i == 4)
-			goto carp;
+			dev_warn(connector->dev->dev,
+			 "%s: Ignoring invalid EDID block %d.\n",
+			 drm_get_connector_name(connector), j);
+	}
+
+	if (valid_extensions != block[0x7e]) {
+		block[EDID_LENGTH-1] += block[0x7e] - valid_extensions;
+		block[0x7e] = valid_extensions;
+		new = krealloc(block, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
+		if (!new)
+			goto out;
+		block = new;
  	}
  
  	return block;
-- 
1.7.1

Re: [PATCH] DRM: ignore invalid EDID extensions

[Sam Tygier]

On 22/09/10 22:55, Adam Jackson wrote:
> Yeah, I hate to just drop extension blocks, but it's better than the
> alternative.  They're optional for a reason I suppose.
>
>>>> For my EIZO S2242W the base block is fine, but the extension block is
>>>> all zeros. Without this patch I get no X and no VTs.
>>>
>>> I suspect what's actually happening there is that we're failing to get
>>> the extension block, not that it's being returned as zeros.
>>
>> Could this be fixed? Anything I could try?
>
> Depends on what your driver is, I suspect.  Did it ever work?
>
> - ajax

I have git bisected, which takes me to

61e57a8d72f2336faf39b5d940215cf085e01e6e is the first bad commit
commit 61e57a8d72f2336faf39b5d940215cf085e01e6e
Author: Adam Jackson <ajax@redhat.com>
Date:   Mon Mar 29 21:43:18 2010 +0000

     drm/edid: Fix secondary block fetch.
     
     This makes fetching the second EDID block on HDMI monitors actually
     work.  DDC can't transfer more than 128 bytes at a time.  Also,
     rearrange the code so the pure DDC bits are separate from block parse.
     
     Signed-off-by: Adam Jackson <ajax@redhat.com>
     Signed-off-by: Dave Airlie <airlied@redhat.com>

:040000 040000 a0b022ba678bd92c46e2ecba63d5f2c78f69b0c3 77e672eec31b83465f6fe8ee3ca6c0153a0af230 M	drivers
:040000 040000 99e72b1f7f6b4016606ed3388b902a5f39b9c13b 3b1e49f0fb674e4974a77021bc90dfce7098ecc6 M	include

full log

git bisect start
# good: [e40152ee1e1c7a63f4777791863215e3faa37a86] Linus 2.6.34
git bisect good e40152ee1e1c7a63f4777791863215e3faa37a86
# bad: [815c4163b6c8ebf8152f42b0a5fd015cfdcedc78] Linux 2.6.35-rc4
git bisect bad 815c4163b6c8ebf8152f42b0a5fd015cfdcedc78
# good: [c316ba3b518bc35ce5aef5421135220389f4eb98] Merge branch 'linux-next' of git://git.infradead.org/ubi-2.6
git bisect good c316ba3b518bc35ce5aef5421135220389f4eb98
# bad: [6d94d4081048756df78444a07201156f4930fe48] lis3: interrupt handlers for 8bit wakeup and click events
git bisect bad 6d94d4081048756df78444a07201156f4930fe48
# bad: [6e80e8ed5eb92d0112674aabe82951266a6a1051] Merge branch 'for-2.6.35' of git://git.kernel.dk/linux-2.6-block
git bisect bad 6e80e8ed5eb92d0112674aabe82951266a6a1051
# good: [ac3ee84c604502240122c47b52f0542ec8774f15] Merge branch 'dbg-early-merge' of git://git.kernel.org/pub/scm/linux/kernel/git/jwessel/linux-2.6-kgdb
git bisect good ac3ee84c604502240122c47b52f0542ec8774f15
# bad: [59534f7298c5e28aaa64e6ed550e247f64ee72ae] Merge branch 'drm-for-2.6.35' of git://git.kernel.org/pub/scm/linux/kernel/git/airlied/drm-2.6
git bisect bad 59534f7298c5e28aaa64e6ed550e247f64ee72ae
# bad: [7547a917fa5f3b2406f52c7dcf7ec9ad3c8532eb] Merge branch 'drm-ttm-unmappable' into drm-core-next
git bisect bad 7547a917fa5f3b2406f52c7dcf7ec9ad3c8532eb
# good: [c2b41276da65481d36311a13d69020d150861c43] Merge branch 'drm-ttm-pool' into drm-core-next
git bisect good c2b41276da65481d36311a13d69020d150861c43
# bad: [7fff400be6fbf64f10abca9939718aaf1d61c255] Merge branch 'drm-fbdev-cleanup' into drm-core-next
git bisect bad 7fff400be6fbf64f10abca9939718aaf1d61c255
# bad: [171fdd892979081e8a9b1a67ce86c7008b7abbdf] drm/modes: Fix interlaced mode names
git bisect bad 171fdd892979081e8a9b1a67ce86c7008b7abbdf
# bad: [59d8aff6e4fc2705053e7ce2948b51f7fe507536] drm/edid: Remove some silly comments
git bisect bad 59d8aff6e4fc2705053e7ce2948b51f7fe507536
# bad: [2b470ab075b30aaeeab29d67b8f1f111096a5fbe] drm/edid: Remove a redundant check
git bisect bad 2b470ab075b30aaeeab29d67b8f1f111096a5fbe
# bad: [fbcc06b6439024b967cf6927b95d303f4c3c1a4f] drm/edid: Remove some misleading comments
git bisect bad fbcc06b6439024b967cf6927b95d303f4c3c1a4f
# bad: [61e57a8d72f2336faf39b5d940215cf085e01e6e] drm/edid: Fix secondary block fetch.
git bisect bad 61e57a8d72f2336faf39b5d940215cf085e01e6e

before 61e57a8d72f2336faf39b5d940215cf085e01e6e, the monitor is detected and gives the correct resolution and everything boots normally with kms.

after i get the
radeon 0000:08:00.0: DVI-I-1: EDID block 1 invalid.
and no X or VTs, unless i disable KMS (in which case it boots fine)

Is there anything else you want me to test? would it be helpful for me to start a bug on fdo?

Thanks

Sam