From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Farnsworth Subject: Re: [PATCH v3] drm/dp: Use large transactions for I2C over AUX Date: Thu, 29 Jan 2015 15:14:05 +0000 Message-ID: <2400774.U9Nnvh8Xyy@f19simon> References: <1422373429-10137-1-git-send-email-simon.farnsworth@onelan.co.uk> <5088334.4MFZWJ3IA6@f19simon> <20150129143655.GW19354@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1589451991==" Return-path: Received: from claranet-outbound-smtp06.uk.clara.net (claranet-outbound-smtp06.uk.clara.net [195.8.89.39]) by gabe.freedesktop.org (Postfix) with ESMTP id F3DA76E7A1 for ; Thu, 29 Jan 2015 07:14:12 -0800 (PST) In-Reply-To: <20150129143655.GW19354@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Ville =?ISO-8859-1?Q?Syrj=E4l=E4?= Cc: Thierry Reding , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============1589451991== Content-Type: multipart/signed; boundary="nextPart3720829.LHRGpsiqnU"; micalg="pgp-sha1"; protocol="application/pgp-signature" --nextPart3720829.LHRGpsiqnU Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="iso-8859-1" On Thursday 29 January 2015 16:36:55 Ville Syrj=E4l=E4 wrote: > On Thu, Jan 29, 2015 at 02:24:09PM +0000, Simon Farnsworth wrote: > > On Thursday 29 January 2015 15:30:36 you wrote: > > > On Tue, Jan 27, 2015 at 03:43:49PM +0000, Simon Farnsworth wrote:= =2D-snip-- > > > > +=09DRM_DEBUG_KMS("Partial I2C reply: requested %zu bytes got %= d bytes\n", msg->size, ret); > > > > +=09drain_msg =3D *msg; > > > > +=09drain_msg.size -=3D ret; > > > > +=09drain_msg.buffer +=3D ret; > > > > +=09while (drain_msg.size !=3D 0) { > > > > +=09=09drain_bytes =3D drm_dp_i2c_do_msg(aux, &drain_msg); > > > > +=09=09if (drain_bytes <=3D 0) > > > > +=09=09=09return drain_bytes =3D=3D 0 ? -EPROTO : drain_bytes; > > > > +=09=09drain_msg.size -=3D drain_bytes; > > > > +=09=09drain_msg.buffer +=3D drain_bytes; > > > > +=09} > > >=20 > > > Somehow I don't like the duplicated code end up having here. So > > > putting it all in a single loop would seem nicer to me. Maybe > > > something along these lines? > > >=20 > > > struct drm_dp_aux_msg msg =3D *orig_msg; > > > int ret =3D msg.size; > > > while (msg.size > 0) { > > > =09int err =3D drm_dp_i2c_do_msg(aux, &msg); > > > =09if (err <=3D 0) > > > =09=09return err =3D=3D 0 ? -EPROTO : err; > > >=20 > > > =09if (err < msg.size && err < ret) { > > > =09=09DRM_DEBUG_KMS("Partial I2C reply: requested %zu > > > =09=09=09 bytes got %d bytes\n", msg.size, err); > > > =09=09ret =3D err; > > > =09} > > >=20 > > > =09msg.size -=3D err; > > > =09msg.buffer +=3D err; > > > } > > >=20 > > > It would also reduce the returned preferred transfer size further= if we > > > (for whatever reason) got an even shorter reply while we're drain= ing. > > > > > I'm not sure that that's the right behaviour, though. If we assume = a 3 byte > > FIFO in a device that does partial reads, we ask for 16 bytes, caus= ing a > > partial response that's 3 bytes long. We then drain out the remaini= ng 13 > > bytes of the initial request (in case it's set up a 16 byte I2C tra= nsfer), > > and the last of the reads is guaranteed to be 1 byte long. >=20 > My proposed code wouldn't reduce the transfer size in that case due t= o > the err when the reply came back with less data than was requested. > I see, yes. So the only time it'd reduce further is if the hypothetical= device gave a 3 byte response to the 16 byte transfer, then 2 byte resp= onses to the remaining requests that try to drain the FIFO. Given that this is now in the realms of "if hardware designers took ple= asure in making their hardware horrible", rather than just "HW designer takes= a short cut and lets software handle the pain", I'll take your code for v= 4. > >=20 > > We then shrink to 1 byte transfers, when the device would be capabl= e of 3 > > byte transfers, and could possibly perform better with 3 byte trans= fers > > rather than 1. > >=20 > > Having said that, this is all hypothetical until we find devices th= at do > > partial replies - no-one's been able to find such a device so far. > >=20 =2D-=20 Simon Farnsworth Software Engineer ONELAN Ltd http://www.onelan.com --nextPart3720829.LHRGpsiqnU Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part. Content-Transfer-Encoding: 7Bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAABAgAGBQJUyk49AAoJEOsKZy3xM+c7qfQH/istAfFDnjAm9mAguo+yhfak f9EcOJoiDJEKEJmvQB4UEyO30VdZkOASzb7neGZPVbKEVdfHdjU5SB2qxhwlWuzT wT08gq9xepuCVtWIY0duG1uVGxi0GX5uBG+ocHeHbqwIls0MB8xD+b6oNBv6ZmBK gnXc8a4xZ1pX3U9zhaayshIRcdDqvqGNqqRG6hXStUIs6A3XwQblyJ6i/zwF7woH gmy3T8yV9h+LQa2Zu4wzJlOxpwMqarFCqwUwKnKjcLGuLVyqKTT5yeCTbjPzonAN QvtxBYu8Og4EXPXvT/nBqNgiT9GYzGnM0Lai77xz6rXX7i0tPtblZ/z4y35LmfM= =iQaf -----END PGP SIGNATURE----- --nextPart3720829.LHRGpsiqnU-- --===============1589451991== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK --===============1589451991==--