From patchwork Tue Jan 27 15:43:49 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Farnsworth X-Patchwork-Id: 5718511 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 6C8999F2ED for ; Tue, 27 Jan 2015 15:44:07 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id D1CF02018E for ; Tue, 27 Jan 2015 15:44:05 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id E5E032010F for ; Tue, 27 Jan 2015 15:44:03 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 530806E351; Tue, 27 Jan 2015 07:44:02 -0800 (PST) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org 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 10E1F6E351 for ; Tue, 27 Jan 2015 07:44:01 -0800 (PST) Received: from 110.100.155.90.in-addr.arpa ([90.155.100.110]:57575 helo=f19simon.office.onelan.co.uk) by relay16.mail.eu.clara.net (relay.clara.net [81.171.239.36]:1025) with esmtpa (authdaemon_plain:simon.farnsworth@onelan.co.uk) id 1YG8Iz-0004LK-JU (return-path ); Tue, 27 Jan 2015 15:43:53 +0000 From: Simon Farnsworth To: dri-devel@lists.freedesktop.org Subject: [PATCH v3] drm/dp: Use large transactions for I2C over AUX Date: Tue, 27 Jan 2015 15:43:49 +0000 Message-Id: <1422373429-10137-1-git-send-email-simon.farnsworth@onelan.co.uk> X-Mailer: git-send-email 2.1.0 Cc: Thierry Reding X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP DisplayPort to DVI-D Dual Link adapters designed by Bizlink have bugs in their I2C over AUX implementation. They work fine with Windows, but fail with Linux. 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 zero byte transfer ending the I2C transaction. Copy Windows's behaviour, and read 16 bytes at a time. If we get a short 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, as no sink I've found actually gives short replies. Also provide a module parameter for testing smaller transfer sizes, in case there are sinks out there that cannot work with Windows. Signed-off-by: Simon Farnsworth Tested-by: Aidan Marks --- v3 changes, after feedback from Ville and more testing of Windows: * Change the short reply algorithm to match Ville's description of the DisplayPort 1.2 spec wording. * Add a module parameter to set the default transfer size for experiments. Requested over IRC by Ville. No-one's been able to find a device that does short replies, but experiments show that bigger reads are faster on most devices. Ville got: DP->DVI (OUI 001cf8): 40ms -> 35ms DP->VGA (OUI 0022b9): 45ms -> 38ms Zotac DP->2xHDMI: 25ms -> 4ms v2 changes, after feedback from Thierry and Ville: * Handle short replies. I've decided (arbitrarily) that a short reply 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): 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 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 branch device do the split from a big request into small transactions. drivers/gpu/drm/drm_dp_helper.c | 91 ++++++++++++++++++++++++++++++++--------- include/drm/drm_dp_helper.h | 5 +++ 2 files changed, 77 insertions(+), 19 deletions(-) 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 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_aux_msg *msg) { unsigned int retry; - int err; + int ret; /* * 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_aux *aux, struct drm_dp_aux_msg *msg) */ for (retry = 0; retry < 7; retry++) { mutex_lock(&aux->hw_mutex); - err = aux->transfer(aux, msg); + ret = aux->transfer(aux, msg); mutex_unlock(&aux->hw_mutex); - if (err < 0) { - if (err == -EBUSY) + if (ret < 0) { + if (ret == -EBUSY) continue; - DRM_DEBUG_KMS("transaction failed: %d\n", err); - return err; + DRM_DEBUG_KMS("transaction failed: %d\n", ret); + return ret; } @@ -457,9 +459,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) * Both native ACK and I2C ACK replies received. We * can assume the transfer was successful. */ - if (err < msg->size) - return -EPROTO; - return 0; + return ret; case DP_AUX_I2C_REPLY_NACK: DRM_DEBUG_KMS("I2C nack\n"); @@ -482,14 +482,67 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) return -EREMOTEIO; } +/* + * Keep retrying drm_dp_i2c_do_msg until all data has been transferred. + * + * 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_dp_aux_msg *msg) +{ + int ret; + struct drm_dp_aux_msg drain_msg; + int drain_bytes; + + ret = drm_dp_i2c_do_msg(aux, msg); + + if (ret == msg->size || ret <= 0) + return ret == 0 ? -EPROTO : ret; + + /* + * We had a partial reply. Drain out the rest of the bytes we + * originally requested, then return the size of the partial + * reply. In theory, this should match DP 1.2's desired behaviour + * for I2C over AUX. + */ + DRM_DEBUG_KMS("Partial I2C reply: requested %zu bytes got %d bytes\n", msg->size, ret); + drain_msg = *msg; + drain_msg.size -= ret; + drain_msg.buffer += ret; + while (drain_msg.size != 0) { + drain_bytes = drm_dp_i2c_do_msg(aux, &drain_msg); + if (drain_bytes <= 0) + return drain_bytes == 0 ? -EPROTO : drain_bytes; + drain_msg.size -= drain_bytes; + drain_msg.buffer += drain_bytes; + } + return ret; +} + +/* + * Bizlink designed DP->DVI-D Dual Link adapters require the I2C over 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 = DP_AUX_MAX_PAYLOAD_BYTES; +module_param_named(dp_aux_i2c_transfer_size, dp_aux_i2c_transfer_size, int, 0600); +MODULE_PARM_DESC(dp_aux_i2c_transfer_size, + "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, int num) { struct drm_dp_aux *aux = adapter->algo_data; unsigned int i, j; + int transfer_size; struct drm_dp_aux_msg msg; int err = 0; + if (dp_aux_i2c_transfer_size < 1 || dp_aux_i2c_transfer_size > DP_AUX_MAX_PAYLOAD_BYTES) { + DRM_ERROR("dp_aux_i2c_transfer_size invalid - changing from %d to %d\n", + dp_aux_i2c_transfer_size, DP_AUX_MAX_PAYLOAD_BYTES); + dp_aux_i2c_transfer_size = DP_AUX_MAX_PAYLOAD_BYTES; + } + memset(&msg, 0, sizeof(msg)); for (i = 0; i < num; i++) { @@ -507,20 +560,20 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, err = drm_dp_i2c_do_msg(aux, &msg); if (err < 0) break; - /* - * Many hardware implementations support FIFOs larger than a - * single byte, but it has been empirically determined that - * transferring data in larger chunks can actually lead to - * decreased performance. Therefore each message is simply - * transferred byte-by-byte. + /* We want each transaction to be as large as possible, but + * we'll go to smaller sizes if the hardware gives us a + * short reply. */ - for (j = 0; j < msgs[i].len; j++) { + transfer_size = dp_aux_i2c_transfer_size; + for (j = 0; j < msgs[i].len; j += msg.size) { msg.buffer = msgs[i].buf + j; - msg.size = 1; + msg.size = min((unsigned)transfer_size, msgs[i].len - j); - err = drm_dp_i2c_do_msg(aux, &msg); - if (err < 0) + transfer_size = drm_dp_i2c_drain_msg(aux, &msg); + if (transfer_size < 0) { + err = transfer_size; break; + } } if (err < 0) break; 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. */ +#define DP_AUX_MAX_PAYLOAD_BYTES 16 + #define DP_AUX_I2C_WRITE 0x0 #define DP_AUX_I2C_READ 0x1 #define DP_AUX_I2C_STATUS 0x2 @@ -519,6 +521,9 @@ struct drm_dp_aux_msg { * transactions. The drm_dp_aux_register_i2c_bus() function registers 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 adapter. + * The I2C adapter uses long transfers by default; if a partial response is + * received, the adapter will drop down to the size given by the partial + * response for this transaction only. * * Note that the aux helper code assumes that the .transfer() function * only modifies the reply field of the drm_dp_aux_msg structure. The