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 14:24:09 +0000 Message-ID: <5088334.4MFZWJ3IA6@f19simon> References: <1422373429-10137-1-git-send-email-simon.farnsworth@onelan.co.uk> <20150129133036.GV19354@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1348570569==" Return-path: Received: from claranet-outbound-smtp04.uk.clara.net (claranet-outbound-smtp04.uk.clara.net [195.8.89.37]) by gabe.freedesktop.org (Postfix) with ESMTP id 5AED9720AD for ; Thu, 29 Jan 2015 06:24:21 -0800 (PST) In-Reply-To: <20150129133036.GV19354@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 --===============1348570569== Content-Type: multipart/signed; boundary="nextPart2084579.hjZZg4KRmS"; micalg="pgp-sha1"; protocol="application/pgp-signature" --nextPart2084579.hjZZg4KRmS Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="us-ascii" 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-- > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_= dp_helper.c > > index 79968e3..3db116c 100644 > > --- a/drivers/gpu/drm/drm_dp_helper.c > > +++ b/drivers/gpu/drm/drm_dp_helper.c > > @@ -396,11 +396,13 @@ static u32 drm_dp_i2c_functionality(struct i2= c_adapter *adapter) > > * retrying the transaction as appropriate. It is assumed that th= e > > * aux->transfer function does not modify anything in the msg othe= r than the > > * reply field. > > + * > > + * Returns bytes transferred on success, or a negative error code = on failure. > > */ > > static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp= _aux_msg *msg) > > { > > =09unsigned int retry; > > -=09int err; > > +=09int ret; > > =20 > > =09/* > > =09 * DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source = device > > @@ -409,14 +411,14 @@ static int drm_dp_i2c_do_msg(struct drm_dp_au= x *aux, struct drm_dp_aux_msg *msg) > > =09 */ > > =09for (retry =3D 0; retry < 7; retry++) { > > =09=09mutex_lock(&aux->hw_mutex); > > -=09=09err =3D aux->transfer(aux, msg); > > +=09=09ret =3D aux->transfer(aux, msg); > > =09=09mutex_unlock(&aux->hw_mutex); > > -=09=09if (err < 0) { > > -=09=09=09if (err =3D=3D -EBUSY) > > +=09=09if (ret < 0) { > > +=09=09=09if (ret =3D=3D -EBUSY) > > =09=09=09=09continue; > > =20 > > -=09=09=09DRM_DEBUG_KMS("transaction failed: %d\n", err); > > -=09=09=09return err; > > +=09=09=09DRM_DEBUG_KMS("transaction failed: %d\n", ret); > > +=09=09=09return ret; > > =09=09} > > =20 > > =20 > > @@ -457,9 +459,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux = *aux, struct drm_dp_aux_msg *msg) > > =09=09=09 * Both native ACK and I2C ACK replies received. We > > =09=09=09 * can assume the transfer was successful. > > =09=09=09 */ > > -=09=09=09if (err < msg->size) > > -=09=09=09=09return -EPROTO; > > -=09=09=09return 0; > > +=09=09=09return ret; >=20 > The s/err/ret/ seems a bit superfluous, but OTOH it does make sense > since it's no longer just an error value. At first glance it just > confused me a bit since I wasn't expecting that many changes to this > function. > I'm going to hang onto that change - it makes things clearer when you r= ead the function outside the context of this patch. > > =20 > > =09=09case DP_AUX_I2C_REPLY_NACK: > > =09=09=09DRM_DEBUG_KMS("I2C nack\n"); > > @@ -482,14 +482,67 @@ static int drm_dp_i2c_do_msg(struct drm_dp_au= x *aux, struct drm_dp_aux_msg *msg) > > =09return -EREMOTEIO; > > } > > =20 > > +/* > > + * Keep retrying drm_dp_i2c_do_msg until all data has been transfe= rred. > > + * > > + * Returns an error code on failure, or a recommended transfer siz= e on success. > > + */ > > +static int drm_dp_i2c_drain_msg(struct drm_dp_aux *aux, struct drm= _dp_aux_msg *msg) > > +{ > > +=09int ret; > > +=09struct drm_dp_aux_msg drain_msg; > > +=09int drain_bytes; > > + > > +=09ret =3D drm_dp_i2c_do_msg(aux, msg); > > + > > +=09if (ret =3D=3D msg->size || ret <=3D 0) > > +=09=09return ret =3D=3D 0 ? -EPROTO : ret; > > + > > +=09/* > > +=09 * We had a partial reply. Drain out the rest of the bytes we > > +=09 * originally requested, then return the size of the partial > > +=09 * reply. In theory, this should match DP 1.2's desired behavio= ur > > +=09 * for I2C over AUX. > > +=09 */ >=20 > The spec is a bit self contradictory, but there is a section which se= ems > to suggest this. It's only mentioned for reads mind you, but I don't = see > it causing harm for writes either. Not that we actually handle > partial/deferred writes correctly at the moment. >=20 > > +=09DRM_DEBUG_KMS("Partial I2C reply: requested %zu bytes got %d by= tes\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 draining.= > 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, causing = a partial response that's 3 bytes long. We then drain out the remaining 1= 3 bytes of the initial request (in case it's set up a 16 byte I2C transfe= r), and the last of the reads is guaranteed to be 1 byte long. We then shrink to 1 byte transfers, when the device would be capable of= 3 byte transfers, and could possibly perform better with 3 byte transfers= rather than 1. Having said that, this is all hypothetical until we find devices that d= o partial replies - no-one's been able to find such a device so far. I'll have a think and see if I can come up with a way to get the behavi= our I want with less code duplication - I might be able to do something by us= ing a sentinel value to spot first time round the loop. >=20 > > +=09return ret; > > +} > > + > > +/* > > + * Bizlink designed DP->DVI-D Dual Link adapters require the I2C o= ver AUX > > + * packets to be as large as possible. If not, the I2C transaction= s never > > + * succeed. Hence the default is maximum. > > + */ > > +static int dp_aux_i2c_transfer_size __read_mostly =3D DP_AUX_MAX_P= AYLOAD_BYTES; > > +module_param_named(dp_aux_i2c_transfer_size, dp_aux_i2c_transfer_s= ize, int, 0600); > > +MODULE_PARM_DESC(dp_aux_i2c_transfer_size, > > +=09=09 "Number of bytes to transfer in a single I2C over DP AUX CH= message, (1-16, default 16)"); > > + > > static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c= _msg *msgs, > > =09=09=09 int num) > > { > > =09struct drm_dp_aux *aux =3D adapter->algo_data; > > =09unsigned int i, j; > > +=09int transfer_size; > > =09struct drm_dp_aux_msg msg; > > =09int err =3D 0; > > =20 > > +=09if (dp_aux_i2c_transfer_size < 1 || dp_aux_i2c_transfer_size > = DP_AUX_MAX_PAYLOAD_BYTES) { > > +=09=09DRM_ERROR("dp_aux_i2c_transfer_size invalid - changing from = %d to %d\n", > > +=09=09=09 dp_aux_i2c_transfer_size, DP_AUX_MAX_PAYLOAD_BYTES); > > +=09=09dp_aux_i2c_transfer_size =3D DP_AUX_MAX_PAYLOAD_BYTES; > > +=09} >=20 > The invalid values could be rejected with custom struct > kernel_param_ops, but maybe that's a bit overkill. If not that, then > I'm not sure the error message really has that much value. So I'm thi= nking > we could just 'clamp(..., 1, DP_AUX_MAX_PAYLOAD_BYTES)' here. > I'll remove the message for v4, as well as marking the parameter unsafe= with module_param_named_unsafe, and use clamp instead of if(). > > + > > =09memset(&msg, 0, sizeof(msg)); > > =20 > > =09for (i =3D 0; i < num; i++) { > > @@ -507,20 +560,20 @@ static int drm_dp_i2c_xfer(struct i2c_adapter= *adapter, struct i2c_msg *msgs, > > =09=09err =3D drm_dp_i2c_do_msg(aux, &msg); > > =09=09if (err < 0) > > =09=09=09break; > > -=09=09/* > > -=09=09 * Many hardware implementations support FIFOs larger than a= > > -=09=09 * single byte, but it has been empirically determined that > > -=09=09 * transferring data in larger chunks can actually lead to > > -=09=09 * decreased performance. Therefore each message is simply > > -=09=09 * transferred byte-by-byte. > > +=09=09/* We want each transaction to be as large as possible, but > > +=09=09 * we'll go to smaller sizes if the hardware gives us a > > +=09=09 * short reply. > > =09=09 */ > > -=09=09for (j =3D 0; j < msgs[i].len; j++) { > > +=09=09transfer_size =3D dp_aux_i2c_transfer_size; > > +=09=09for (j =3D 0; j < msgs[i].len; j +=3D msg.size) { > > =09=09=09msg.buffer =3D msgs[i].buf + j; > > -=09=09=09msg.size =3D 1; > > +=09=09=09msg.size =3D min((unsigned)transfer_size, msgs[i].len - j= ); >=20 > Could make transfer_size unsigned in the first place. >=20 > > =20 > > -=09=09=09err =3D drm_dp_i2c_do_msg(aux, &msg); > > -=09=09=09if (err < 0) > > +=09=09=09transfer_size =3D drm_dp_i2c_drain_msg(aux, &msg); > > +=09=09=09if (transfer_size < 0) { > > +=09=09=09=09err =3D transfer_size; > > =09=09=09=09break; > > +=09=09=09} >=20 > Maybe this is a bit more straight forward? >=20 > err =3D drm_dp_i2c_drain_msg(aux, &msg); > if (err < 0) > =09break; > transfer_size =3D err; > That and making transfer_size unsigned seems like a good combination. W= ill adopt for v4. =2D-=20 Simon Farnsworth Software Engineer ONELAN Ltd http://www.onelan.com --nextPart2084579.hjZZg4KRmS 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 iQEcBAABAgAGBQJUykKNAAoJEOsKZy3xM+c7iTEH+gK6SNJDwLgc8GZEPCRitU9J ouDgCaSOuCSTyKe18XPcrKqB50fOz04HxRiNSvvDH2zaAl0vscJw2VZBJoEN2caU Y3/hLpKiRmkxlN9LxeJY52zYESJWhiiJD7NVUo6TXHR0GyIRW4lCGPM2aK7QJAWc TNNN7fRuZ22kf8EpvyOGbSeEC3Zc9WPfSVxjCh4BuLV/Q0mOFlSvns7jPtv2Eyrc /cwJR4Wn8B2JYyVgBVk4ut8A4mHrjAj/ejxnW9HBRSndgbDJbJwSCXV8DU4zcCEY PN7FWkrBJ8RvoWoIGEsEnxFd6nvQD7/RyTlKVE6byMWMPechAp6HgzkHQYtDrmM= =gyPu -----END PGP SIGNATURE----- --nextPart2084579.hjZZg4KRmS-- --===============1348570569== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK --===============1348570569==--