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

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

[Simon Farnsworth]

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.

Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
---

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 | 42 ++++++++++++++++++++++-------------------
 include/drm/drm_dp_helper.h     |  5 +++++
 2 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 79968e3..701b201 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");
@@ -487,6 +487,7 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
 {
 	struct drm_dp_aux *aux = adapter->algo_data;
 	unsigned int i, j;
+	int transfer_size = DP_AUX_MAX_PAYLOAD_BYTES;
 	struct drm_dp_aux_msg msg;
 	int err = 0;
 
@@ -507,20 +508,23 @@ 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.
+		/* 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.
+		 *
+		 * We therefore start by requesting 16 byte transfers. If
+		 * the hardware gives us a partial ACK, we stick to the new
+		 * smaller size from the partial ACK.
 		 */
-		for (j = 0; j < msgs[i].len; j++) {
+		for (j = 0; j < msgs[i].len; j += transfer_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);
-			if (err < 0)
+			transfer_size = drm_dp_i2c_do_msg(aux, &msg);
+			if (transfer_size <= 0) {
+				err = transfer_size == 0 ? -EPROTO : 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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

On Mon, Jan 26, 2015 at 03:22:48PM +0000, Simon Farnsworth 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.
> 
> Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
> ---
> 
> 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>

I think that's agaisnt the spec. IIRC you have to keep repeating the
same transaction (meaning address/len are unchanged) until all the data
was transferred.

> 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 | 42 ++++++++++++++++++++++-------------------
>  include/drm/drm_dp_helper.h     |  5 +++++
>  2 files changed, 28 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 79968e3..701b201 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");
> @@ -487,6 +487,7 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
>  {
>  	struct drm_dp_aux *aux = adapter->algo_data;
>  	unsigned int i, j;
> +	int transfer_size = DP_AUX_MAX_PAYLOAD_BYTES;
>  	struct drm_dp_aux_msg msg;
>  	int err = 0;
>  
> @@ -507,20 +508,23 @@ 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.
> +		/* 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.
> +		 *
> +		 * We therefore start by requesting 16 byte transfers. If
> +		 * the hardware gives us a partial ACK, we stick to the new
> +		 * smaller size from the partial ACK.
>  		 */
> -		for (j = 0; j < msgs[i].len; j++) {
> +		for (j = 0; j < msgs[i].len; j += transfer_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);
> -			if (err < 0)
> +			transfer_size = drm_dp_i2c_do_msg(aux, &msg);
> +			if (transfer_size <= 0) {
> +				err = transfer_size == 0 ? -EPROTO : 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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Simon Farnsworth]

[-- Attachment #1.1: Type: text/plain, Size: 2604 bytes --]

On Monday 26 January 2015 17:33:35 Ville Syrjälä wrote:
> On Mon, Jan 26, 2015 at 03:22:48PM +0000, Simon Farnsworth 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.
> > 
> > Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
> > ---
> > 
> > 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>
> 
> I think that's agaisnt the spec. IIRC you have to keep repeating the
> same transaction (meaning address/len are unchanged) until all the data
> was transferred.
>
Do you have a spec reference against the DisplayPort 1.1a (last public
version) spec? My chosen behaviour matches Table 2-50 in the 1.1a spec.

I can't see anything in section 2.4.5 (I2C over AUX) that prohibits me from
changing the length or address mid-transaction, and there is text that says
that when the address changes without the source clearing the MOT bit, the
sink should do an I2C Repeated Start to change address. The length is never
supplied to the I2C bus - it's purely an artifact of DisplayPort.

> > 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.
> >
--snip code--
-- 
Simon Farnsworth
Software Engineer
ONELAN Ltd
http://www.onelan.com

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

On Mon, Jan 26, 2015 at 05:33:35PM +0200, Ville Syrjälä wrote:
> On Mon, Jan 26, 2015 at 03:22:48PM +0000, Simon Farnsworth 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.
> > 
> > Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
> > ---
> > 
> > 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>
> 
> I think that's agaisnt the spec. IIRC you have to keep repeating the
> same transaction (meaning address/len are unchanged) until all the data
> was transferred.

Actually for an i2c read it seems you can specify the same len or reduce
the len by the amount you already received. Reducing seems more
approriate as the spec goes to state that when repeating the the message
with the original length the sink is likely to read more i2c data than
will be needed. Althoguh maybe it can buffer it so that if the original
amount of data was more than 16 bytes we might get the answer to the
next message quicker. So maybe we should just keep updating the length
with 'min(16, total_bytes - total_bytes_received)' ?

For writes we don't seem to do anything correctly atm. When getting a
defer or ack w/ M value we should issue a I2C_WRITE_STATUS_UPDATE command
to prompt the sink to report back with an update M value. Currently we
seem to just retry with the same write message.

> 
> > 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 | 42 ++++++++++++++++++++++-------------------
> >  include/drm/drm_dp_helper.h     |  5 +++++
> >  2 files changed, 28 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > index 79968e3..701b201 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");
> > @@ -487,6 +487,7 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
> >  {
> >  	struct drm_dp_aux *aux = adapter->algo_data;
> >  	unsigned int i, j;
> > +	int transfer_size = DP_AUX_MAX_PAYLOAD_BYTES;
> >  	struct drm_dp_aux_msg msg;
> >  	int err = 0;
> >  
> > @@ -507,20 +508,23 @@ 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.
> > +		/* 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.
> > +		 *
> > +		 * We therefore start by requesting 16 byte transfers. If
> > +		 * the hardware gives us a partial ACK, we stick to the new
> > +		 * smaller size from the partial ACK.
> >  		 */
> > -		for (j = 0; j < msgs[i].len; j++) {
> > +		for (j = 0; j < msgs[i].len; j += transfer_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);
> > -			if (err < 0)
> > +			transfer_size = drm_dp_i2c_do_msg(aux, &msg);
> > +			if (transfer_size <= 0) {
> > +				err = transfer_size == 0 ? -EPROTO : 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
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

On Mon, Jan 26, 2015 at 03:47:13PM +0000, Simon Farnsworth wrote:
> On Monday 26 January 2015 17:33:35 Ville Syrjälä wrote:
> > On Mon, Jan 26, 2015 at 03:22:48PM +0000, Simon Farnsworth 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.
> > > 
> > > Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
> > > ---
> > > 
> > > 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>
> > 
> > I think that's agaisnt the spec. IIRC you have to keep repeating the
> > same transaction (meaning address/len are unchanged) until all the data
> > was transferred.
> >
> Do you have a spec reference against the DisplayPort 1.1a (last public
> version) spec? My chosen behaviour matches Table 2-50 in the 1.1a spec.

In my copy if DP v1.1 the example in 2-50 just keeps repeating w/ 16 bytes.
So doesn't match what you do. And that's unchanged in v1.2.

DP v1.2 has some extra clarifications for this stuff:
"2.7.7 I2C-overAUX Transaction Clarifications and Implementation Rules

 2.7.7.1.6.4 Upon the Reply of I2C_ACK|AUX_ACK Followed by the Total Number of Data
 Bytes Fewer than LEN+1, to a Request Transaction with MOT Bit Set Either to 0 or 1

 The Source device must:
 o Repeat the identical I2C-read-over-AUX transaction with the updated
   LEN value equal to the original LEN value minus the total number of data
   bytes received so far,
 o Repeat the identical I2C-read-over-AUX transaction with the same LEN
   value as the original value, or,
 o Issue an address-only I2C-over-AUX with MOT bit set to 0 to prompt I2C STOP
   to terminate the current I2C-read-over-AUX transaction.
 It should be noted that when the Source device repeats the same I2C-read-over-AUX
 transaction with the same LEN value as the original value, the Sink device is
 likely to read more data bytes than the Source device needs."

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Simon Farnsworth]

[-- Attachment #1.1: Type: text/plain, Size: 4614 bytes --]

On Monday 26 January 2015 18:11:01 Ville Syrjälä wrote:
> On Mon, Jan 26, 2015 at 03:47:13PM +0000, Simon Farnsworth wrote:
> > On Monday 26 January 2015 17:33:35 Ville Syrjälä wrote:
> > > On Mon, Jan 26, 2015 at 03:22:48PM +0000, Simon Farnsworth 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.
> > > > 
> > > > Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
> > > > ---
> > > > 
> > > > 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>
> > > 
> > > I think that's agaisnt the spec. IIRC you have to keep repeating the
> > > same transaction (meaning address/len are unchanged) until all the data
> > > was transferred.
> > >
> > Do you have a spec reference against the DisplayPort 1.1a (last public
> > version) spec? My chosen behaviour matches Table 2-50 in the 1.1a spec.
> 
> In my copy if DP v1.1 the example in 2-50 just keeps repeating w/ 16 bytes.
> So doesn't match what you do. And that's unchanged in v1.2.
>
Yes, looks like I misread the table; I was more thinking about "what are the
semantics of a 4 byte reply to a 16 byte read?" when I read that bit of the
spec.

> DP v1.2 has some extra clarifications for this stuff:
> "2.7.7 I2C-overAUX Transaction Clarifications and Implementation Rules
> 
>  2.7.7.1.6.4 Upon the Reply of I2C_ACK|AUX_ACK Followed by the Total Number of Data
>  Bytes Fewer than LEN+1, to a Request Transaction with MOT Bit Set Either to 0 or 1
> 
>  The Source device must:
>  o Repeat the identical I2C-read-over-AUX transaction with the updated
>    LEN value equal to the original LEN value minus the total number of data
>    bytes received so far,
>  o Repeat the identical I2C-read-over-AUX transaction with the same LEN
>    value as the original value, or,
>  o Issue an address-only I2C-over-AUX with MOT bit set to 0 to prompt I2C STOP
>    to terminate the current I2C-read-over-AUX transaction.
>  It should be noted that when the Source device repeats the same I2C-read-over-AUX
>  transaction with the same LEN value as the original value, the Sink device is
>  likely to read more data bytes than the Source device needs."
> 
So what would be the best implementation strategy for Linux? Bear in mind
that I don't have the 1.2 spec, nor do I have any devices which give a
partial response to a 16 byte read, so I'm coding blind here, based solely
on the information people are giving me.

The simplest strategy would be to keep repeating with 16 byte reads all the
time, but Thierry's original comment and response to the first patch have
suggested that that's a performance problem waiting to happen, hence the
effort to find a better strategy.

My strategy was intended to spot that there's probably a FIFO in the middle
that's too small, and reduce the transfer size to avoid overflowing the
FIFO. I could make it more complex by remembering that the last read was a
partial read, finishing it off by reducing the read size, then sticking to
the new smaller size, but that's added complexity and I'm not sure of its
value.

Should I simply stick to 16 byte reads, and cope with getting back less data
than I want, or do you think we need the more complex strategy that complies
with DP 1.2?
-- 
Simon Farnsworth
Software Engineer
ONELAN Ltd
http://www.onelan.com

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

On Mon, Jan 26, 2015 at 04:39:29PM +0000, Simon Farnsworth wrote:
> On Monday 26 January 2015 18:11:01 Ville Syrjälä wrote:
> > On Mon, Jan 26, 2015 at 03:47:13PM +0000, Simon Farnsworth wrote:
> > > On Monday 26 January 2015 17:33:35 Ville Syrjälä wrote:
> > > > On Mon, Jan 26, 2015 at 03:22:48PM +0000, Simon Farnsworth 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.
> > > > > 
> > > > > Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
> > > > > ---
> > > > > 
> > > > > 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>
> > > > 
> > > > I think that's agaisnt the spec. IIRC you have to keep repeating the
> > > > same transaction (meaning address/len are unchanged) until all the data
> > > > was transferred.
> > > >
> > > Do you have a spec reference against the DisplayPort 1.1a (last public
> > > version) spec? My chosen behaviour matches Table 2-50 in the 1.1a spec.
> > 
> > In my copy if DP v1.1 the example in 2-50 just keeps repeating w/ 16 bytes.
> > So doesn't match what you do. And that's unchanged in v1.2.
> >
> Yes, looks like I misread the table; I was more thinking about "what are the
> semantics of a 4 byte reply to a 16 byte read?" when I read that bit of the
> spec.
> 
> > DP v1.2 has some extra clarifications for this stuff:
> > "2.7.7 I2C-overAUX Transaction Clarifications and Implementation Rules
> > 
> >  2.7.7.1.6.4 Upon the Reply of I2C_ACK|AUX_ACK Followed by the Total Number of Data
> >  Bytes Fewer than LEN+1, to a Request Transaction with MOT Bit Set Either to 0 or 1
> > 
> >  The Source device must:
> >  o Repeat the identical I2C-read-over-AUX transaction with the updated
> >    LEN value equal to the original LEN value minus the total number of data
> >    bytes received so far,
> >  o Repeat the identical I2C-read-over-AUX transaction with the same LEN
> >    value as the original value, or,
> >  o Issue an address-only I2C-over-AUX with MOT bit set to 0 to prompt I2C STOP
> >    to terminate the current I2C-read-over-AUX transaction.
> >  It should be noted that when the Source device repeats the same I2C-read-over-AUX
> >  transaction with the same LEN value as the original value, the Sink device is
> >  likely to read more data bytes than the Source device needs."
> > 
> So what would be the best implementation strategy for Linux? Bear in mind
> that I don't have the 1.2 spec, nor do I have any devices which give a
> partial response to a 16 byte read, so I'm coding blind here, based solely
> on the information people are giving me.
> 
> The simplest strategy would be to keep repeating with 16 byte reads all the
> time, but Thierry's original comment and response to the first patch have
> suggested that that's a performance problem waiting to happen, hence the
> effort to find a better strategy.
> 
> My strategy was intended to spot that there's probably a FIFO in the middle
> that's too small, and reduce the transfer size to avoid overflowing the
> FIFO. I could make it more complex by remembering that the last read was a
> partial read, finishing it off by reducing the read size, then sticking to
> the new smaller size, but that's added complexity and I'm not sure of its
> value.
> 
> Should I simply stick to 16 byte reads, and cope with getting back less data
> than I want, or do you think we need the more complex strategy that complies
> with DP 1.2?

So I've been experimenting a bit with various dongles here, and sadly I've
not managed to get any one of them to return short reads :(

I did find one that allows changing the speed of the i2c bus, but even if
I reduce it to 1khz there are no short reads, just a lot more defers. The
dongle in question has OUI 001cf8.

However the good news is that EDID reads seem to get faster across the
board with 16 byte messages. How much faster depends on the dongle.

Here are my measurements how long it took to read a single EDID block:
 DP->DVI (OUI 001cf8):	40ms -> 35ms
 DP->VGA (OUI 0022b9):	45ms -> 38ms
 Zotac DP->2xHDMI:	25ms ->  4ms


Oh and this is how I mangled my drm_dp_i2c_xfer():
transferred = 0;
while (msgs[i].len > transferred) {
	msg.buffer = msgs[i].buf + transferred;
	msg.size = min_t(unsigned int, drm_dp_i2c_msg_size,
			 msgs[i].len - transferred);
	err = drm_dp_i2c_do_msg(aux, &msg);
	if (err < 0)
		break;
	WARN_ON(err == 0);
	transferred += err;
}

I made the msg size configurable via a module param just to help me test
this stuff, but I'm thinking we might want to upstream that just to make
it easier to try smaller message sizes if/when people encounter problematic
sinks/dongles.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Jani Nikula]

On Tue, 27 Jan 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> So I've been experimenting a bit with various dongles here, and sadly I've
> not managed to get any one of them to return short reads :(
>
> I did find one that allows changing the speed of the i2c bus, but even if
> I reduce it to 1khz there are no short reads, just a lot more defers. The
> dongle in question has OUI 001cf8.
>
> However the good news is that EDID reads seem to get faster across the
> board with 16 byte messages. How much faster depends on the dongle.
>
> Here are my measurements how long it took to read a single EDID block:
>  DP->DVI (OUI 001cf8):	40ms -> 35ms
>  DP->VGA (OUI 0022b9):	45ms -> 38ms
>  Zotac DP->2xHDMI:	25ms ->  4ms
>
>
> Oh and this is how I mangled my drm_dp_i2c_xfer():
> transferred = 0;
> while (msgs[i].len > transferred) {
> 	msg.buffer = msgs[i].buf + transferred;
> 	msg.size = min_t(unsigned int, drm_dp_i2c_msg_size,
> 			 msgs[i].len - transferred);
> 	err = drm_dp_i2c_do_msg(aux, &msg);
> 	if (err < 0)
> 		break;
> 	WARN_ON(err == 0);
> 	transferred += err;
> }
>
> I made the msg size configurable via a module param just to help me test
> this stuff, but I'm thinking we might want to upstream that just to make
> it easier to try smaller message sizes if/when people encounter problematic
> sinks/dongles.

How about just letting that happen first, to see if and how the problems
occur? If there's a pattern, maybe we can fall back to 1-byte transfers
in those cases (or even add OUI based quirks). I've grown really
hesitant about adding new module parameters, they are ABI we can't
easily remove/regress once added.


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Wed, Jan 28, 2015 at 10:59:06AM +0200, Jani Nikula wrote:
> On Tue, 27 Jan 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > So I've been experimenting a bit with various dongles here, and sadly I've
> > not managed to get any one of them to return short reads :(
> >
> > I did find one that allows changing the speed of the i2c bus, but even if
> > I reduce it to 1khz there are no short reads, just a lot more defers. The
> > dongle in question has OUI 001cf8.
> >
> > However the good news is that EDID reads seem to get faster across the
> > board with 16 byte messages. How much faster depends on the dongle.
> >
> > Here are my measurements how long it took to read a single EDID block:
> >  DP->DVI (OUI 001cf8):	40ms -> 35ms
> >  DP->VGA (OUI 0022b9):	45ms -> 38ms
> >  Zotac DP->2xHDMI:	25ms ->  4ms
> >
> >
> > Oh and this is how I mangled my drm_dp_i2c_xfer():
> > transferred = 0;
> > while (msgs[i].len > transferred) {
> > 	msg.buffer = msgs[i].buf + transferred;
> > 	msg.size = min_t(unsigned int, drm_dp_i2c_msg_size,
> > 			 msgs[i].len - transferred);
> > 	err = drm_dp_i2c_do_msg(aux, &msg);
> > 	if (err < 0)
> > 		break;
> > 	WARN_ON(err == 0);
> > 	transferred += err;
> > }
> >
> > I made the msg size configurable via a module param just to help me test
> > this stuff, but I'm thinking we might want to upstream that just to make
> > it easier to try smaller message sizes if/when people encounter problematic
> > sinks/dongles.
> 
> How about just letting that happen first, to see if and how the problems
> occur? If there's a pattern, maybe we can fall back to 1-byte transfers
> in those cases (or even add OUI based quirks). I've grown really
> hesitant about adding new module parameters, they are ABI we can't
> easily remove/regress once added.

module_param_debug takes care of any such risks imo.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Jani Nikula]

On Wed, 28 Jan 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Jan 28, 2015 at 10:59:06AM +0200, Jani Nikula wrote:
>> On Tue, 27 Jan 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> > So I've been experimenting a bit with various dongles here, and sadly I've
>> > not managed to get any one of them to return short reads :(
>> >
>> > I did find one that allows changing the speed of the i2c bus, but even if
>> > I reduce it to 1khz there are no short reads, just a lot more defers. The
>> > dongle in question has OUI 001cf8.
>> >
>> > However the good news is that EDID reads seem to get faster across the
>> > board with 16 byte messages. How much faster depends on the dongle.
>> >
>> > Here are my measurements how long it took to read a single EDID block:
>> >  DP->DVI (OUI 001cf8):	40ms -> 35ms
>> >  DP->VGA (OUI 0022b9):	45ms -> 38ms
>> >  Zotac DP->2xHDMI:	25ms ->  4ms
>> >
>> >
>> > Oh and this is how I mangled my drm_dp_i2c_xfer():
>> > transferred = 0;
>> > while (msgs[i].len > transferred) {
>> > 	msg.buffer = msgs[i].buf + transferred;
>> > 	msg.size = min_t(unsigned int, drm_dp_i2c_msg_size,
>> > 			 msgs[i].len - transferred);
>> > 	err = drm_dp_i2c_do_msg(aux, &msg);
>> > 	if (err < 0)
>> > 		break;
>> > 	WARN_ON(err == 0);
>> > 	transferred += err;
>> > }
>> >
>> > I made the msg size configurable via a module param just to help me test
>> > this stuff, but I'm thinking we might want to upstream that just to make
>> > it easier to try smaller message sizes if/when people encounter problematic
>> > sinks/dongles.
>> 
>> How about just letting that happen first, to see if and how the problems
>> occur? If there's a pattern, maybe we can fall back to 1-byte transfers
>> in those cases (or even add OUI based quirks). I've grown really
>> hesitant about adding new module parameters, they are ABI we can't
>> easily remove/regress once added.
>
> module_param_debug takes care of any such risks imo.

No such thing, maybe you mean module_param_unsafe?

Jani.


> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Wed, Jan 28, 2015 at 11:33:34AM +0200, Jani Nikula wrote:
> On Wed, 28 Jan 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Jan 28, 2015 at 10:59:06AM +0200, Jani Nikula wrote:
> >> On Tue, 27 Jan 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >> > So I've been experimenting a bit with various dongles here, and sadly I've
> >> > not managed to get any one of them to return short reads :(
> >> >
> >> > I did find one that allows changing the speed of the i2c bus, but even if
> >> > I reduce it to 1khz there are no short reads, just a lot more defers. The
> >> > dongle in question has OUI 001cf8.
> >> >
> >> > However the good news is that EDID reads seem to get faster across the
> >> > board with 16 byte messages. How much faster depends on the dongle.
> >> >
> >> > Here are my measurements how long it took to read a single EDID block:
> >> >  DP->DVI (OUI 001cf8):	40ms -> 35ms
> >> >  DP->VGA (OUI 0022b9):	45ms -> 38ms
> >> >  Zotac DP->2xHDMI:	25ms ->  4ms
> >> >
> >> >
> >> > Oh and this is how I mangled my drm_dp_i2c_xfer():
> >> > transferred = 0;
> >> > while (msgs[i].len > transferred) {
> >> > 	msg.buffer = msgs[i].buf + transferred;
> >> > 	msg.size = min_t(unsigned int, drm_dp_i2c_msg_size,
> >> > 			 msgs[i].len - transferred);
> >> > 	err = drm_dp_i2c_do_msg(aux, &msg);
> >> > 	if (err < 0)
> >> > 		break;
> >> > 	WARN_ON(err == 0);
> >> > 	transferred += err;
> >> > }
> >> >
> >> > I made the msg size configurable via a module param just to help me test
> >> > this stuff, but I'm thinking we might want to upstream that just to make
> >> > it easier to try smaller message sizes if/when people encounter problematic
> >> > sinks/dongles.
> >> 
> >> How about just letting that happen first, to see if and how the problems
> >> occur? If there's a pattern, maybe we can fall back to 1-byte transfers
> >> in those cases (or even add OUI based quirks). I've grown really
> >> hesitant about adding new module parameters, they are ABI we can't
> >> easily remove/regress once added.
> >
> > module_param_debug takes care of any such risks imo.
> 
> No such thing, maybe you mean module_param_unsafe?

Indeed that's what I've meant.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Simon Farnsworth]

[-- Attachment #1.1: Type: text/plain, Size: 1507 bytes --]

On Wednesday 28 January 2015 11:33:34 Jani Nikula wrote:
> On Wed, 28 Jan 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Jan 28, 2015 at 10:59:06AM +0200, Jani Nikula wrote:
> >> On Tue, 27 Jan 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
--snip--
> >> > I made the msg size configurable via a module param just to help me test
> >> > this stuff, but I'm thinking we might want to upstream that just to make
> >> > it easier to try smaller message sizes if/when people encounter problematic
> >> > sinks/dongles.
> >> 
> >> How about just letting that happen first, to see if and how the problems
> >> occur? If there's a pattern, maybe we can fall back to 1-byte transfers
> >> in those cases (or even add OUI based quirks). I've grown really
> >> hesitant about adding new module parameters, they are ABI we can't
> >> easily remove/regress once added.
> >
> > module_param_debug takes care of any such risks imo.
> 
> No such thing, maybe you mean module_param_unsafe?
> 
> Jani.

Changing to module_param_unsafe is trivial. That would taint the kernel if
you play with it, hopefully making it clear that this is not permanent ABI.

I'm now seeing the Bizlink adapters fail after about 45 seconds of
isochronous link up, so it'll be a few days before I do a v4 of this patch,
as I need Datapath's assistance analysing the differences in behaviour
between us and Windows).

-- 
Simon Farnsworth
Software Engineer
ONELAN Ltd
http://www.onelan.com

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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@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?

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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

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

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

[Ville Syrjälä]

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.

> +{
> +	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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Jani Nikula]

On Tue, 10 Feb 2015, Simon Farnsworth <simon.farnsworth@onelan.co.uk> wrote:
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=55228
> Tested-by: Aidan Marks <aidanamarks@gmail.com>

Aidan writes in the bug, "Hi Simon, I'm sorry to report that the v4
patch doesn't work for me at all, it reverts to 1024x768.  v3 was great
though."

BR,
Jani.



-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Wed, Feb 11, 2015 at 03:36:29PM +1000, Dave Airlie wrote:
> On 11 February 2015 at 04:42, Simon Farnsworth
> <simon.farnsworth@onelan.co.uk> wrote:
> > A note:
> >
> > This is *not* enough to bring us to parity with Windows with these
> > adapters. There's also something wrong with our HPD handling that trips them
> > up - I suspect, based on AUX traces from one that's happy, that it's our
> > handling of short HPDs that's imperfect, as the device does a short HPD when
> > it sees the DVI-D HPD sense change.
> >
> 
> We don't handle soft irqs well outside of MST, I think the Apple VGA dongle
> also causes a soft irq when you plug a vga monitor into the dongle, when
>  the dongle is already plugged in.

Yeah we probably need some helper function to handle sst short pulse. The
problem there is that with sst the driver owns the drm connector and not
the helper like with mst, so I'm not sure how we should communicate back
to the driver what has been detected exactly. Maybe just a pile of out
pointers for edid and stuff, or we create an sst_connector_state struct or
something. Of course mst should then use the same for handling sst leafs
so that we can share all the special magic for e.g. the apple vga dongle.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Dave Airlie]

On 11 February 2015 at 04:42, Simon Farnsworth
<simon.farnsworth@onelan.co.uk> wrote:
> A note:
>
> This is *not* enough to bring us to parity with Windows with these
> adapters. There's also something wrong with our HPD handling that trips them
> up - I suspect, based on AUX traces from one that's happy, that it's our
> handling of short HPDs that's imperfect, as the device does a short HPD when
> it sees the DVI-D HPD sense change.
>

We don't handle soft irqs well outside of MST, I think the Apple VGA dongle
also causes a soft irq when you plug a vga monitor into the dongle, when
 the dongle is already plugged in.

Dave.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Simon Farnsworth]

[-- Attachment #1.1: Type: text/plain, Size: 9966 bytes --]

A note:

This is *not* enough to bring us to parity with Windows with these
adapters. There's also something wrong with our HPD handling that trips them
up - I suspect, based on AUX traces from one that's happy, that it's our
handling of short HPDs that's imperfect, as the device does a short HPD when
it sees the DVI-D HPD sense change.

Simon

On Tuesday 10 February 2015 18:38:08 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)
> +{
> +	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
> 

-- 
Simon Farnsworth
Software Engineer
ONELAN Ltd
http://www.onelan.com

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Simon Farnsworth]

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)
+{
+	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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Simon Farnsworth]

[-- Attachment #1.1: Type: text/plain, Size: 3642 bytes --]

On Friday 23 January 2015 22:21:09 Thierry Reding wrote:
> On Fri, Jan 23, 2015 at 09:46:29PM +0200, Ville Syrjälä wrote:
> > On Fri, Jan 23, 2015 at 06:40:38PM +0000, Simon Farnsworth 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. Analysis of the
> > > failure state was provided by Datapath Ltd.
> > > 
> > > Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
> > > ---
> > > Thierry,
> > > 
> > > You put in the comment about "decreased performance", back in December 2013;
> > > would you mind testing that this still works with the devices you tested?
> > > 
> > > Unfortunately, Bizlink are the only game in town for DP->DVI-DL adapters -
> > > and their firmware is prone to giving up on I2C if we look at it
> > > wrongly. Even Apple's device is Bizlink designed.
> > > 
> > >  drivers/gpu/drm/drm_dp_helper.c | 13 +++++--------
> > >  1 file changed, 5 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > > index 79968e3..b4a9d4a 100644
> > > --- a/drivers/gpu/drm/drm_dp_helper.c
> > > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > > @@ -507,16 +507,13 @@ 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.
> > > +		/* 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.
> > >  		 */
> > > -		for (j = 0; j < msgs[i].len; j++) {
> > > +		for (j = 0; j < msgs[i].len; j+=16) {
> > >  			msg.buffer = msgs[i].buf + j;
> > > -			msg.size = 1;
> > > +			msg.size = min(16, msgs[i].len - 16);
> > 
> > I don't think it's quite this simple. The sink is allowed to ACK
> > partial data for multi-byte messages. The code doesn't handle that.
>

It doesn't look challenging to fix that, though - I'll do a v2 with that
fixed. I don't have hardware to test against that I know of; is there anyone
who's got a sink that does partial ACKs that could test for me?

> Also not all hardware may support transferring 16 bytes at a time. How
> does that work with these adapters? Does it mean they can't work on DP
> hardware that can't do 16 byte block transfers?
> 

Correct. 16 bytes or go home, based on testing done at Datapath. Not
entirely coincidentally, this is the behaviour of NVIDIA, AMD and Intel
graphics on Windows and Mac OS X - I suspect that testing was dominated by
"what does Windows do", not by "what does the spec say".

Datapath Ltd tested with a non-Linux source that's only capable of
transferring one byte at a time, and the adapter failed in exactly the same
way as it does with Linux.

-- 
Simon Farnsworth
Software Engineer
ONELAN Ltd
http://www.onelan.com

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Fri, Jan 23, 2015 at 10:21:09PM +0100, Thierry Reding wrote:
> On Fri, Jan 23, 2015 at 09:46:29PM +0200, Ville Syrjälä wrote:
> > On Fri, Jan 23, 2015 at 06:40:38PM +0000, Simon Farnsworth 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. Analysis of the
> > > failure state was provided by Datapath Ltd.
> > > 
> > > Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
> > > ---
> > > Thierry,
> > > 
> > > You put in the comment about "decreased performance", back in December 2013;
> > > would you mind testing that this still works with the devices you tested?
> > > 
> > > Unfortunately, Bizlink are the only game in town for DP->DVI-DL adapters -
> > > and their firmware is prone to giving up on I2C if we look at it
> > > wrongly. Even Apple's device is Bizlink designed.
> > > 
> > >  drivers/gpu/drm/drm_dp_helper.c | 13 +++++--------
> > >  1 file changed, 5 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > > index 79968e3..b4a9d4a 100644
> > > --- a/drivers/gpu/drm/drm_dp_helper.c
> > > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > > @@ -507,16 +507,13 @@ 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.
> > > +		/* 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.
> > >  		 */
> > > -		for (j = 0; j < msgs[i].len; j++) {
> > > +		for (j = 0; j < msgs[i].len; j+=16) {
> > >  			msg.buffer = msgs[i].buf + j;
> > > -			msg.size = 1;
> > > +			msg.size = min(16, msgs[i].len - 16);
> > 
> > I don't think it's quite this simple. The sink is allowed to ACK
> > partial data for multi-byte messages. The code doesn't handle that.
> 
> Also not all hardware may support transferring 16 bytes at a time. How
> does that work with these adapters? Does it mean they can't work on DP
> hardware that can't do 16 byte block transfers?

If we handle short reads then hw which really can only do 1 byte at a time
could just always do that. But yeah that means you can't use these fancy
dp sinks with them I guess.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2726 bytes --]

On Fri, Jan 23, 2015 at 09:46:29PM +0200, Ville Syrjälä wrote:
> On Fri, Jan 23, 2015 at 06:40:38PM +0000, Simon Farnsworth 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. Analysis of the
> > failure state was provided by Datapath Ltd.
> > 
> > Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
> > ---
> > Thierry,
> > 
> > You put in the comment about "decreased performance", back in December 2013;
> > would you mind testing that this still works with the devices you tested?
> > 
> > Unfortunately, Bizlink are the only game in town for DP->DVI-DL adapters -
> > and their firmware is prone to giving up on I2C if we look at it
> > wrongly. Even Apple's device is Bizlink designed.
> > 
> >  drivers/gpu/drm/drm_dp_helper.c | 13 +++++--------
> >  1 file changed, 5 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > index 79968e3..b4a9d4a 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -507,16 +507,13 @@ 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.
> > +		/* 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.
> >  		 */
> > -		for (j = 0; j < msgs[i].len; j++) {
> > +		for (j = 0; j < msgs[i].len; j+=16) {
> >  			msg.buffer = msgs[i].buf + j;
> > -			msg.size = 1;
> > +			msg.size = min(16, msgs[i].len - 16);
> 
> I don't think it's quite this simple. The sink is allowed to ACK
> partial data for multi-byte messages. The code doesn't handle that.

Also not all hardware may support transferring 16 bytes at a time. How
does that work with these adapters? Does it mean they can't work on DP
hardware that can't do 16 byte block transfers?

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

On Fri, Jan 23, 2015 at 06:40:38PM +0000, Simon Farnsworth 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. Analysis of the
> failure state was provided by Datapath Ltd.
> 
> Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
> ---
> Thierry,
> 
> You put in the comment about "decreased performance", back in December 2013;
> would you mind testing that this still works with the devices you tested?
> 
> Unfortunately, Bizlink are the only game in town for DP->DVI-DL adapters -
> and their firmware is prone to giving up on I2C if we look at it
> wrongly. Even Apple's device is Bizlink designed.
> 
>  drivers/gpu/drm/drm_dp_helper.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 79968e3..b4a9d4a 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -507,16 +507,13 @@ 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.
> +		/* 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.
>  		 */
> -		for (j = 0; j < msgs[i].len; j++) {
> +		for (j = 0; j < msgs[i].len; j+=16) {
>  			msg.buffer = msgs[i].buf + j;
> -			msg.size = 1;
> +			msg.size = min(16, msgs[i].len - 16);

I don't think it's quite this simple. The sink is allowed to ACK
partial data for multi-byte messages. The code doesn't handle that.

>  
>  			err = drm_dp_i2c_do_msg(aux, &msg);
>  			if (err < 0)
> -- 
> 2.1.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Simon Farnsworth]

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. Analysis of the
failure state was provided by Datapath Ltd.

Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
---
Thierry,

You put in the comment about "decreased performance", back in December 2013;
would you mind testing that this still works with the devices you tested?

Unfortunately, Bizlink are the only game in town for DP->DVI-DL adapters -
and their firmware is prone to giving up on I2C if we look at it
wrongly. Even Apple's device is Bizlink designed.

 drivers/gpu/drm/drm_dp_helper.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 79968e3..b4a9d4a 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -507,16 +507,13 @@ 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.
+		/* 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.
 		 */
-		for (j = 0; j < msgs[i].len; j++) {
+		for (j = 0; j < msgs[i].len; j+=16) {
 			msg.buffer = msgs[i].buf + j;
-			msg.size = 1;
+			msg.size = min(16, msgs[i].len - 16);
 
 			err = drm_dp_i2c_do_msg(aux, &msg);
 			if (err < 0)
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel