Re: [PATCH] DRM: ignore invalid EDID extensions

[Sam Tygier <samtygier@yahoo.co.uk>]

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