Re: [PATCH] drm/dp: Use large transactions for I2C over AUX

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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