From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Farnsworth Subject: Re: [PATCH] drm/dp: Use large transactions for I2C over AUX Date: Tue, 10 Feb 2015 18:42:25 +0000 Message-ID: <83761778.5Lnlq5SByB@f19simon> References: <1423593488-14470-1-git-send-email-simon.farnsworth@onelan.co.uk> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1413316348==" Return-path: Received: from claranet-outbound-smtp00.uk.clara.net (claranet-outbound-smtp00.uk.clara.net [195.8.89.33]) by gabe.freedesktop.org (Postfix) with ESMTP id C66346E1D8 for ; Tue, 10 Feb 2015 10:42:36 -0800 (PST) In-Reply-To: <1423593488-14470-1-git-send-email-simon.farnsworth@onelan.co.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: dri-devel@lists.freedesktop.org Cc: Thierry Reding List-Id: dri-devel@lists.freedesktop.org --===============1413316348== Content-Type: multipart/signed; boundary="nextPart6027694.83LA829bLn"; micalg="pgp-sha1"; protocol="application/pgp-signature" --nextPart6027694.83LA829bLn Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="iso-8859-1" A note: This is *not* enough to bring us to parity with Windows with these adapters. There's also something wrong with our HPD handling that trips= them up - I suspect, based on AUX traces from one that's happy, that it's ou= r handling of short HPDs that's imperfect, as the device does a short HPD= when it sees the DVI-D HPD sense change. Simon On Tuesday 10 February 2015 18:38:08 Simon Farnsworth wrote: > Older DisplayPort to DVI-D Dual Link adapters designed by Bizlink hav= e bugs > in their I2C over AUX implementation (fixed in newer revisions). They= work > fine with Windows, but fail with Linux. >=20 > It turns out that they cannot keep an I2C transaction open unless the= > previous read was 16 bytes; shorter reads can only be followed by a z= ero > byte transfer ending the I2C transaction. >=20 > Copy Windows's behaviour, and read 16 bytes at a time. If we get a sh= ort > reply, assume that there's a hardware bottleneck, and shrink our read= size > to match. For this purpose, use the algorithm in the DisplayPort 1.2 = spec, > in the hopes that it'll be closest to what Windows does. >=20 > Also provide an unsafe module parameter for testing smaller transfer = sizes, > in case there are sinks out there that cannot work with Windows. >=20 > Note also that despite the previous comment in drm_dp_i2c_xfer, this = speeds > up native DP EDID reads; Ville Syrj=E4l=E4 found > the following changes in his testing: >=20 > Device under test: old -> with this patch > DP->DVI (OUI 001cf8): 40ms -> 35ms > DP->VGA (OUI 0022b9): 45ms -> 38ms > Zotac DP->2xHDMI: 25ms -> 4ms > Asus PB278 monitor: 22ms -> 3ms >=20 > A back of the envelope calculation shows that peak theoretical transf= er rate > for 1 byte reads is around 60 kbit/s; with 16 byte reads, this increa= ses to > around 500 kbit/s, which explains the increase in speed. >=20 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=3D55228 > Tested-by: Aidan Marks > Signed-off-by: Simon Farnsworth > --- >=20 > v4 changes: >=20 > * Change short reply algorithm after suggestions from Ville. >=20 > * Expanded commit message. >=20 > * Mark the module parameter unsafe. >=20 > * Use clamp() to bring the module parameter into range when used. >=20 > v3 changes, after feedback from Ville and more testing of Windows: >=20 > * Change the short reply algorithm to match Ville's description of t= he > DisplayPort 1.2 spec wording. >=20 > * Add a module parameter to set the default transfer size for > experiments. Requested over IRC by Ville. >=20 > No-one's been able to find a device that does short replies, but expe= riments > show that bigger reads are faster on most devices. Ville got: >=20 > DP->DVI (OUI 001cf8): 40ms -> 35ms > DP->VGA (OUI 0022b9): 45ms -> 38ms > Zotac DP->2xHDMI: 25ms -> 4ms >=20 > v2 changes, after feedback from Thierry and Ville: >=20 > * Handle short replies. I've decided (arbitrarily) that a short repl= y > results in us dropping back to the newly chosen size for the rest = of this > I2C transaction. Thus, given an attempt to read the first 16 bytes= of > EDID, and a sink that only does 4 bytes of buffering, we will see = the > following AUX transfers for the EDID read (after address is set): >=20 > > Read 16 bytes from I2C over AUX. > Reply with 4 bytes > Read 4 bytes > Reply with 4 bytes > Read 4 bytes > Reply with 4 bytes > Read 4 bytes > Reply with 4 bytes > >=20 > Note that I've not looked at MST support - I have neither the DP 1.2 = spec > nor any MST branch devices, so I can't test anything I write or check= it > against a spec. It looks from the code, however, as if MST has the br= anch > device do the split from a big request into small transactions. >=20 > drivers/gpu/drm/drm_dp_helper.c | 76 +++++++++++++++++++++++++++++++= =2D--------- > include/drm/drm_dp_helper.h | 5 +++ > 2 files changed, 63 insertions(+), 18 deletions(-) >=20 > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp= _helper.c > index 79968e3..105fd66 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 i2c_= adapter *adapter) > * retrying the transaction as appropriate. It is assumed that the > * aux->transfer function does not modify anything in the msg other = 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_a= ux_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 de= vice > @@ -409,14 +411,14 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux = *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 *a= ux, 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 > =09=09case DP_AUX_I2C_REPLY_NACK: > =09=09=09DRM_DEBUG_KMS("I2C nack\n"); > @@ -482,14 +482,55 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux = *aux, struct drm_dp_aux_msg *msg) > =09return -EREMOTEIO; > } > =20 > +/* > + * Keep retrying drm_dp_i2c_do_msg until all data has been transferr= ed. > + * > + * Returns an error code on failure, or a recommended transfer size = on success. > + */ > +static int drm_dp_i2c_drain_msg(struct drm_dp_aux *aux, struct drm_d= p_aux_msg *orig_msg) > +{ > +=09int err, ret =3D orig_msg->size; > +=09struct drm_dp_aux_msg msg =3D *orig_msg; > + > +=09while (msg.size > 0) { > +=09=09err =3D drm_dp_i2c_do_msg(aux, &msg); > +=09=09if (err <=3D 0) > +=09=09=09return err =3D=3D 0 ? -EPROTO : err; > + > +=09=09if (err < msg.size && err < ret) { > +=09=09=09DRM_DEBUG_KMS("Partial I2C reply: requested %zu bytes got %= d bytes\n", > +=09=09=09=09 msg.size, err); > +=09=09=09ret =3D err; > +=09=09} > + > +=09=09msg.size -=3D err; > +=09=09msg.buffer +=3D err; > +=09} > + > +=09return ret; > +} > + > +/* > + * Bizlink designed DP->DVI-D Dual Link adapters require the I2C ove= r AUX > + * packets to be as large as possible. If not, the I2C transactions = never > + * succeed. Hence the default is maximum. > + */ > +static int dp_aux_i2c_transfer_size __read_mostly =3D DP_AUX_MAX_PAY= LOAD_BYTES; > +module_param_unsafe(dp_aux_i2c_transfer_size, int, 0644); > +MODULE_PARM_DESC(dp_aux_i2c_transfer_size, > +=09=09 "Number of bytes to transfer in a single I2C over DP AUX CH m= essage, (1-16, default 16)"); > + > static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_m= sg *msgs, > =09=09=09 int num) > { > =09struct drm_dp_aux *aux =3D adapter->algo_data; > =09unsigned int i, j; > +=09unsigned transfer_size; > =09struct drm_dp_aux_msg msg; > =09int err =3D 0; > =20 > +=09dp_aux_i2c_transfer_size =3D clamp(dp_aux_i2c_transfer_size, 1, D= P_AUX_MAX_PAYLOAD_BYTES); > + > =09memset(&msg, 0, sizeof(msg)); > =20 > =09for (i =3D 0; i < num; i++) { > @@ -507,20 +548,19 @@ 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(transfer_size, msgs[i].len - j); > =20 > -=09=09=09err =3D drm_dp_i2c_do_msg(aux, &msg); > +=09=09=09err =3D drm_dp_i2c_drain_msg(aux, &msg); > =09=09=09if (err < 0) > =09=09=09=09break; > +=09=09=09transfer_size =3D err; > =09=09} > =09=09if (err < 0) > =09=09=09break; > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.= h > index 11f8c84..444d51b 100644 > --- a/include/drm/drm_dp_helper.h > +++ b/include/drm/drm_dp_helper.h > @@ -42,6 +42,8 @@ > * 1.2 formally includes both eDP and DPI definitions. > */ > =20 > +#define DP_AUX_MAX_PAYLOAD_BYTES=0916 > + > #define DP_AUX_I2C_WRITE=09=090x0 > #define DP_AUX_I2C_READ=09=09=090x1 > #define DP_AUX_I2C_STATUS=09=090x2 > @@ -519,6 +521,9 @@ struct drm_dp_aux_msg { > * transactions. The drm_dp_aux_register_i2c_bus() function register= s an > * I2C adapter that can be passed to drm_probe_ddc(). Upon removal, = drivers > * should call drm_dp_aux_unregister_i2c_bus() to remove the I2C ada= pter. > + * The I2C adapter uses long transfers by default; if a partial resp= onse is > + * received, the adapter will drop down to the size given by the par= tial > + * response for this transaction only. > * > * Note that the aux helper code assumes that the .transfer() functi= on > * only modifies the reply field of the drm_dp_aux_msg structure. T= he >=20 =2D-=20 Simon Farnsworth Software Engineer ONELAN Ltd http://www.onelan.com --nextPart6027694.83LA829bLn 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 iQEcBAABAgAGBQJU2lEVAAoJEOsKZy3xM+c7+TEH/05Irg3KACRSApqrVDQj3VmF gFmwAhvOEcHBGSI8GwNFxQqc6C5W4oZbzqI8ocDisJuCUNWoCDzaDnal3EKA6CPT MaHBOUJweNxr3Wxu24t1PJH1FwQBWxeXO4yG5PnGcpZD3XZOrAIGCzEGLOaI6L9f 1yNM0OhRRJNE0AVazI8e2VHLgrDuwAQVHOkA0b/6es5pNe5iUMBg51lnTGA7Y8+A YcjVLy0zZE9EGVIpFN9Kqi1IYpeTVQ8kEH/u07wGdnoJwNpkDB9BWpsYbKelWDmU BFnWSgUiFaEqhWjQ1y0RoLt7QyziMpGOw4j4ov+OqChZkTh694Fxlk1xjUl/WJc= =E3q/ -----END PGP SIGNATURE----- --nextPart6027694.83LA829bLn-- --===============1413316348== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK --===============1413316348==--