[PATCH] drm/edid: Add support for extension blocks beyond the first

[PATCH] drm/edid: Add support for extension blocks beyond the first

[Jean Delvare]

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;
+		} else
+			ret = i2c_transfer(adapter, msgs + 1, 2);
 	} while (ret != 2 && --retries);
 
 	return ret == 2 ? 0 : -1;

-- 
Jean Delvare
Suse L3

Re: [PATCH] drm/edid: Add support for extension blocks beyond the first

[Ville Syrjälä]

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).

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.

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. 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).

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

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/edid: Add support for extension blocks beyond the first

[Jean Delvare]

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,
-- 
Jean Delvare
Suse L3