[PATCH] drm/dp: Use large transactions for I2C over AUX
Ville Syrjälä
ville.syrjala at linux.intel.com
Wed Feb 11 04:05:21 PST 2015
On Tue, Feb 10, 2015 at 06:38:08PM +0000, Simon Farnsworth wrote:
> Older DisplayPort to DVI-D Dual Link adapters designed by Bizlink have bugs
> in their I2C over AUX implementation (fixed in newer revisions). 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.
>
> Also provide an unsafe module parameter for testing smaller transfer sizes,
> in case there are sinks out there that cannot work with Windows.
>
> Note also that despite the previous comment in drm_dp_i2c_xfer, this speeds
> up native DP EDID reads; Ville Syrjälä <ville.syrjala at linux.intel.com> found
> the following changes in his testing:
>
> 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
>
> A back of the envelope calculation shows that peak theoretical transfer rate
> for 1 byte reads is around 60 kbit/s; with 16 byte reads, this increases to
> around 500 kbit/s, which explains the increase in speed.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=55228
> Tested-by: Aidan Marks <aidanamarks at gmail.com>
> Signed-off-by: Simon Farnsworth <simon.farnsworth at onelan.co.uk>
> ---
>
> v4 changes:
>
> * Change short reply algorithm after suggestions from Ville.
>
> * Expanded commit message.
>
> * Mark the module parameter unsafe.
>
> * Use clamp() to bring the module parameter into range when used.
>
> 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):
>
> <set address, block etc>
> 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
> <end I2C transaction>
>
> 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 | 76 +++++++++++++++++++++++++++++++----------
> include/drm/drm_dp_helper.h | 5 +++
> 2 files changed, 63 insertions(+), 18 deletions(-)
>
> 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_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,55 @@ 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 *orig_msg)
orig_msg should be const to make sure someone doesn't change it by
accident since that would break drm_dp_i2c_xfer().
Otherwise looks good to me, so
Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
I also tested this version with a few dongles and everything seems to
work as intended.
> +{
> + int err, ret = orig_msg->size;
> + struct drm_dp_aux_msg msg = *orig_msg;
> +
> + while (msg.size > 0) {
> + err = drm_dp_i2c_do_msg(aux, &msg);
> + if (err <= 0)
> + return err == 0 ? -EPROTO : err;
> +
> + if (err < msg.size && err < ret) {
> + DRM_DEBUG_KMS("Partial I2C reply: requested %zu bytes got %d bytes\n",
> + msg.size, err);
> + ret = err;
> + }
> +
> + msg.size -= err;
> + msg.buffer += err;
> + }
> +
> + 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_unsafe(dp_aux_i2c_transfer_size, int, 0644);
> +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;
> + unsigned transfer_size;
> struct drm_dp_aux_msg msg;
> int err = 0;
>
> + dp_aux_i2c_transfer_size = clamp(dp_aux_i2c_transfer_size, 1, DP_AUX_MAX_PAYLOAD_BYTES);
> +
> memset(&msg, 0, sizeof(msg));
>
> for (i = 0; i < num; i++) {
> @@ -507,20 +548,19 @@ 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(transfer_size, msgs[i].len - j);
>
> - err = drm_dp_i2c_do_msg(aux, &msg);
> + err = drm_dp_i2c_drain_msg(aux, &msg);
> if (err < 0)
> break;
> + transfer_size = err;
> }
> 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
> --
> 2.1.0
--
Ville Syrjälä
Intel OTC
More information about the dri-devel
mailing list