From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH v2 05/18] DRM/KMS/EDID: Test EDDC if EDID announces more than one Extension Block (v2) Date: Thu, 22 Nov 2012 14:29:45 +0200 Message-ID: <20121122122945.GF3296@intel.com> References: <1353356598-10634-1-git-send-email-eich@suse.de> <1353579788-30637-1-git-send-email-eich@suse.com> <1353579788-30637-6-git-send-email-eich@suse.com> <20121122112009.GE3296@intel.com> <20654.5504.513136.392670@linux-qknr.site> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2019204899==" Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id C2D81E5E0E for ; Thu, 22 Nov 2012 04:29:49 -0800 (PST) In-Reply-To: <20654.5504.513136.392670@linux-qknr.site> 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: Egbert Eich Cc: tiwai@suse.com, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============2019204899== Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Nov 22, 2012 at 01:07:28PM +0100, Egbert Eich wrote: > Ville Syrj=E4l=E4 writes: > > On Thu, Nov 22, 2012 at 05:22:55AM -0500, Egbert Eich wrote: > > > There are displays which announce EDID extension blocks in the > > > Extension Flag of the EDID base block although they are not EDDC > > > capable (ie. take a segment address at I2C slave address 0x30). > > > We test this by looking for an EDID header which is only possible > > > in the base block. > > > If the segment address is not taken into account, this block will > > > be identical to the base block in which case we stop reading furth= er > > > EEDID blocks, correct the extension flag and just return the base > > > block. > > >=20 > > > v2: Split up EDID fixup code into separate commit. > > >=20 > > > Signed-off-by: Egbert Eich > > > --- > > > drivers/gpu/drm/drm_edid.c | 13 +++++++++++++ > > > 1 files changed, 13 insertions(+), 0 deletions(-) > > >=20 > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid= .c > > > index a952cfe..5a0e331 100644 > > > --- a/drivers/gpu/drm/drm_edid.c > > > +++ b/drivers/gpu/drm/drm_edid.c > > > @@ -364,6 +364,19 @@ drm_do_get_edid(struct drm_connector *connect= or, struct i2c_adapter *adapter) > > > } > > > if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID= _LENGTH, j, print_bad_edid)) { > > > valid_extensions++; > > > + /* Test if base block announced extension blocks although > > > + * display is not EDDC capable. > > > + */ > > > + if (j =3D=3D 2) { > > > + int k; > > > + for (k =3D 0; k < sizeof(edid_header); k++) > > > + if (block[(EDID_LENGTH * 2) + k] !=3D edid_header[k]) > > > + break; > > > + if (k =3D=3D sizeof(edid_header)) { > > > + valid_extensions =3D 0; > > > + goto done_fix_extension_count; > > > + } > >=20 > > memcmp()? Also couldn't we just memcmp() the whole block against the= base > > block, instead of just the header part? >=20 > I don't see an advantage of comparing the entire block with the base bl= ock: > the signature should already be unique. However I don't insist ;) Me neither. I just figured it might reduce the chance of false positives. But if you say that can't happen, I'll take your word for it. > Regarding memcmp() you are definitely right, I will change the code. >=20 > >=20 > > Also the comment is somehow misleading. It talks about the base bloc= k > > even though we're looking at the extension block. >=20 >=20 > Reason for this patch: > I had a bug report for a monitor announcing extension blocks in the ext= ension > block flag of the base block (over 200!) although it wasn't EDDC capabl= e.=20 > For some reason it got past the ACK check when the segment number was w= ritten > to address 0x30 and happily transferred the base block for any odd numb= ered=20 > block and some garbage for even ones. That's nasty. When I saw your patch I was immediately thinking that this is caused by the IGNORE_NAK flag, but then I read the current code, and realized that we're not using that flag. It was used in the original EDDC patch, and I was worried that this kind of bad behaviour would be possible if use the flag. But it seems I underestimated how crappy the monitor hardwar can be. > The only reliable way we found to catch this condition early was to che= ck if=20 > block 2 had the header of a base block which will happen when the displ= ay > cannot deal with the segment number. >=20 > This was what I tried to summarize in very few words - maybe i should r= eword > it a bit. Right. The idea seems reasonable, I just found the comment somehow a bit confusing when I was reading the code following it. So maybe something like 'Test if base block ..., by checking whether the extension block is a duplicate the base block.' Although the use of memcmp() will already make things much clearer. --=20 Ville Syrj=C3=A4l=C3=A4 Intel OTC --===============2019204899== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel --===============2019204899==--