On Thu, Nov 22, 2012 at 01:07:28PM +0100, Egbert Eich wrote:

Ville Syrj�l� writes:

...

On Thu, Nov 22, 2012 at 05:22:55AM -0500, Egbert Eich wrote:

...

There are displays which announce EDID extension blocks in the Extension Flag of the EDID base block although they are not EDDC capable (ie. take a segment address at I2C slave address 0x30). We test this by looking for an EDID header which is only possible in the base block. If the segment address is not taken into account, this block will be identical to the base block in which case we stop reading further EEDID blocks, correct the extension flag and just return the base block.

v2: Split up EDID fixup code into separate commit.

Signed-off-by: Egbert Eich eich@suse.com

drivers/gpu/drm/drm_edid.c | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index a952cfe..5a0e331 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -364,6 +364,19 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) } if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) { valid_extensions++;

• 		/* Test if base block announced extension blocks although
  

• 		 * display is not EDDC capable.
  

• 		 */
  

• 		if (j == 2) {
  

• 			int k;
  

• 			for (k = 0; k < sizeof(edid_header); k++)
  

• 				if (block[(EDID_LENGTH * 2) + k] != edid_header[k])
  

• 					break;
  

• 			if (k == sizeof(edid_header)) {
  

• 				valid_extensions = 0;
  

• 				goto done_fix_extension_count;
  

• 			}
  

memcmp()? Also couldn't we just memcmp() the whole block against the base block, instead of just the header part?

I don't see an advantage of comparing the entire block with the base block: the signature should already be unique. However I don't insist ;)

Me neither. I just figured it might reduce the chance of false positives. But if you say that can't happen, I'll take your word for it.

Regarding memcmp() you are definitely right, I will change the code.

...

Also the comment is somehow misleading. It talks about the base block even though we're looking at the extension block.

Reason for this patch: I had a bug report for a monitor announcing extension blocks in the extension block flag of the base block (over 200!) although it wasn't EDDC capable. For some reason it got past the ACK check when the segment number was written to address 0x30 and happily transferred the base block for any odd numbered block and some garbage for even ones.

That's nasty. When I saw your patch I was immediately thinking that this is caused by the IGNORE_NAK flag, but then I read the current code, and realized that we're not using that flag. It was used in the original EDDC patch, and I was worried that this kind of bad behaviour would be possible if use the flag. But it seems I underestimated how crappy the monitor hardwar can be.

The only reliable way we found to catch this condition early was to check if block 2 had the header of a base block which will happen when the display cannot deal with the segment number.

This was what I tried to summarize in very few words - maybe i should reword it a bit.

Right. The idea seems reasonable, I just found the comment somehow a bit confusing when I was reading the code following it.

So maybe something like 'Test if base block ..., by checking whether the extension block is a duplicate the base block.' Although the use of memcmp() will already make things much clearer.