[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 at suse.de>
> > Cc: Adam Jackson <ajax at 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