On Wed, Mar 11, 2015 at 09:28:20PM +0200, Ville Syrjälä wrote: > On Wed, Feb 11, 2015 at 02:05:21PM +0200, Ville Syrjälä wrote: > > 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. > > Did this fall through the cracks, or are we waiting for some resolution > to that one bug? Yeah it sounds a lot like wrong kernel boot from reading bugzilla. I merged this to drm-misc, we can easily back it out if it causes real trouble. And the speedup itself is nice already imo. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
