From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH v3] DRM/KMS/EDID: Consolidate EDID Error Handling (v3) Date: Thu, 22 Nov 2012 20:54:05 +0200 Message-ID: <20121122185405.GI3296@intel.com> References: <1353579788-30637-15-git-send-email-eich@suse.com> <1353595482-23157-1-git-send-email-eich@suse.com> <20121122160947.GH3296@intel.com> <20654.28380.153117.164559@linux-qknr.site> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0887320410==" Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 3EFD2E5EB1 for ; Thu, 22 Nov 2012 10:54:33 -0800 (PST) In-Reply-To: <20654.28380.153117.164559@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 --===============0887320410== Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Nov 22, 2012 at 07:28:44PM +0100, Egbert Eich wrote: > Ville Syrj=E4l=E4 writes: > > >=20 > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid= .c > > > index dd0df60..aa9b34d 100644 > > > --- a/drivers/gpu/drm/drm_edid.c > > > +++ b/drivers/gpu/drm/drm_edid.c > > > @@ -157,6 +157,17 @@ int drm_edid_header_is_valid(const u8 *raw_ed= id) > > > } > > > EXPORT_SYMBOL(drm_edid_header_is_valid); > > > =20 > > > +static bool drm_edid_is_zero(u8 *in_edid, int length) > > > +{ > > > + int i; > > > + u32 *raw_edid =3D (u32 *)in_edid; > > > + > > > + for (i =3D 0; i < length / 4; i++) > > > + if (*(raw_edid + i) !=3D 0) > > > + return false; > > > + return true; >=20 > [...] > >=20 > > You could use memchr_inv() here. But the compiler can't optimize it > > since it's not inline, so I suppose it might make it slower. >=20 >=20 > > > + > > > +bool > > > +drm_edid_block_valid(u8 *raw_edid, int block, unsigned last_error= _flags) > > > +{ > > > + if (!drm_edid_block_check_error(raw_edid, block, last_error_flag= s)) > > > + return true; > > > + return false; > >=20 > > return !drm_edid_block_check_error(); >=20 > Right.=20 > It's stupid anyway. See below. >=20 > >=20 > > > } > > > EXPORT_SYMBOL(drm_edid_block_valid); > > > =20 > > > @@ -241,7 +268,7 @@ bool drm_edid_is_valid(struct edid *edid) > > > return false; > > > =20 > > > for (i =3D 0; i <=3D edid->extensions; i++) > > > - if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true)) > > > + if (drm_edid_block_check_error(raw + i * EDID_LENGTH, i, true)) > > = ^^^^ > >=20 > > That looks wrong. Also the 's/!drm_edid_block_valid/drm_edid_block_c= heck_error' >=20 > Oops, right. Leftover from old code. >=20 > > change seems superfluous since you're not using the more detailed re= turn > > value from drm_edid_block_check_error(). >=20 > This should probably be changed.=20 > For the drm_edid_block_valid() I'm already using the previous error sta= te > as argument - but I don't tell the result of this read.=20 > Doesn't make much sense :( >=20 > Something similar should be done for drm_edid_is_valid() - even if the=20 > driver doesn't bother (for instance because this function is only calle= d > once when the device structures are initialized). >=20 > The current code ignores the error state for extension blocks i guess i= t > should not if we want to avoid having repreated logging of errors in th= e > extension blocks. I'm not sure. The current code dump all failed extension block, doesn't it? Maybe we actually do want that to happen, at least w/ debugs enabled. Then there are various retry loops in the code, which may or may not be necessary. I have a feeling some of them may have been attempts at papering over a bug that I fixed [1] in the i2c bitbanging code. But if they are necessary, I'm not sure we really appreciate repeated dumps of the same block. [1] https://git.kernel.org/?p=3Dlinux/kernel/git/torvalds/linux.git;a=3Dc= ommit;h=3D8ee161ce5e0cfc689eb677f227a6248191165fac --=20 Ville Syrj=C3=A4l=C3=A4 Intel OTC --===============0887320410== 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 --===============0887320410==--