[PATCH] drm/dp: Use large transactions for I2C over AUX
Ville Syrjälä
ville.syrjala at linux.intel.com
Wed Mar 11 12:28:20 PDT 2015
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 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.
Did this fall through the cracks, or are we waiting for some resolution
to that one bug?
--
Ville Syrjälä
Intel OTC
More information about the dri-devel
mailing list