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@xxxxxxxxxxxxxxx> 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@xxxxxxxxx> > Signed-off-by: Simon Farnsworth <simon.farnsworth@xxxxxxxxxxxx> > --- > > 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@xxxxxxxxxxxxxxx> 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
