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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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