On Tue, 27 Jan 2015, Simon Farnsworth <simon.farnsworth@xxxxxxxxxxxx> wrote: > 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 <simon.farnsworth@xxxxxxxxxxxx> > --- > > 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. IMO module parameters are ABI we shouldn't add just because we might need them. Also see my reply in the other thread http://mid.gmane.org/877fw7s2lh.fsf@xxxxxxxxx BR, Jani. > > 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 | 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 > -- > 2.1.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
