From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jean Delvare Subject: Re: [PATCH] drm/edid: Add support for extension blocks beyond the first Date: Wed, 15 Feb 2012 11:02:46 +0100 Message-ID: <201202151102.46414.jdelvare@suse.de> References: <201112071511.07990.jdelvare@suse.de> <20120213182423.GG16897@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mx2.suse.de (cantor2.suse.de [195.135.220.15]) by gabe.freedesktop.org (Postfix) with ESMTP id B6081A09C0 for ; Wed, 15 Feb 2012 02:02:49 -0800 (PST) In-Reply-To: <20120213182423.GG16897@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Ville =?iso-8859-1?q?Syrj=E4l=E4?= Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org Hi Ville, On Monday 13 February 2012 07:24:23 pm Ville Syrj=E4l=E4 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 > > Cc: Adam Jackson > > --- > > 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 =3D block * EDID_LENGTH; > > + unsigned char segment =3D block >> 1; > > + unsigned char start =3D (block & 0x01) * EDID_LENGTH; > > int ret, retries =3D 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[] =3D { > > { > > + .addr =3D DDC_SEGMENT_ADDR, > > + .flags =3D 0, > > + .len =3D 1, > > + .buf =3D &segment, > > + }, { > > .addr =3D DDC_ADDR, > > .flags =3D 0, > > .len =3D 1, > > @@ -265,7 +271,18 @@ drm_do_probe_ddc_edid(struct i2c_adapter > > .buf =3D buf, > > } > > }; > > - ret =3D i2c_transfer(adapter, msgs, 2); > > + > > + /* Don't write segment if it is 0, for compatibility */ > > + if (segment) { > > + ret =3D 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 =3D=3D -ENXIO) > > + msgs[0].flags |=3D 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 !=3D 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 !=3D 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