Hi Ville,

On Monday 13 February 2012 07:24:23 pm Ville Syrjälä wrote:

On Wed, Dec 07, 2011 at 03:11:07PM +0100, Jean Delvare wrote:

...

When 2 or more EDID extension blocks are present, segment must be selected prior to reading the extended EDID block over the DDC channel. Add support for this.

Signed-off-by: Jean Delvare jdelvare@suse.de Cc: Adam Jackson ajax@redhat.com

---

This needs testing by someone with access to such a display.

drivers/gpu/drm/drm_edid.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)

--- linux-3.2-rc3.orig/drivers/gpu/drm/drm_edid.c 2011-11-09 15:53:31.000000000 +0100 +++ linux-3.2-rc3/drivers/gpu/drm/drm_edid.c 2011-12-03 10:12:47.000000000 +0100 @@ -242,7 +242,8 @@ static int drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, int block, int len) {

• unsigned char start = block * EDID_LENGTH;

• unsigned char segment = block >> 1;

• unsigned char start = (block & 0x01) * EDID_LENGTH; int ret, retries = 5;

  /* The core i2c driver will automatically retry the transfer if

the @@ -254,6 +255,11 @@ drm_do_probe_ddc_edid(struct i2c_adapter do { struct i2c_msg msgs[] = { {

• 		.addr	= DDC_SEGMENT_ADDR,
  

• 		.flags	= 0,
  

• 		.len	= 1,
  

• 		.buf	= &segment,
  

• 	}, {
  	.addr	= DDC_ADDR,
  	.flags	= 0,
  	.len	= 1,
  

@@ -265,7 +271,18 @@ drm_do_probe_ddc_edid(struct i2c_adapter .buf = buf, } };

• ret = i2c_transfer(adapter, msgs, 2);
  

• /* Don't write segment if it is 0, for compatibility */
  

• if (segment) {
  

• 	ret = i2c_transfer(adapter, msgs, 3);
  

• 	/* The E-DDC specification says that the first ack is
  

• 	 * optional, so retry in ignore-nak mode if we get no
  

• 	 * ack at first.
  

• 	 */
  

• 	if (ret == -ENXIO)
  

• 		msgs[0].flags |= I2C_M_IGNORE_NAK;
  

This seems a bit wrong to me. The spec says that the ack for the segment address is "don't care", but for the segment pointer the ack is required (when segment != 0).

Correct.

With I2C_M_IGNORE_NAK we would in fact end up reading segment 0 from a non E-DDC display, if we try to read segment != 0 from it. Of course we shouldn't do that unless the display lied to us about what extension blocks it provides.

Still correct.

So I'm not sure if it would be better to trust that the display never lies about the extension blocks, or if we should just assume all E-DDC displays ack both segment addr and pointer.

I went for the former, as should be obvious from my proposed implementation. Whether this is the best decision is impossible to tell until the code is tested in the fields.

The no-ack feature seems to there for backwards compatibility, for cases where the host always sends the segment addr/pointer even when reading segment 0 (which your code doesn't do).

I agree.

To handle it exactly as the spec says, I2C_M_IGNORE_NAK should be split into two flags (one for addr, other for data).

This is correct, but this seemed a little overkill. I would only implement this in i2c-core if it turns out to be absolutely necessary to properly handle one real-world display.

I would suggest that my patch gets applied as is for now, and it can be adjusted later if needed. It is certainly better than the current code anyway.

Thanks for the review,