[PATCH] drm/dp: Adjust i2c-over-aux retry count based on message size and i2c bus speed

[PATCH] drm/dp: Adjust i2c-over-aux retry count based on message size and i2c bus speed

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Calculate the number of retries we should do for each i2c-over-aux
message based on the time it takes to perform the i2c transfer vs. the
aux transfer.

This mostly matches what the DP spec recommends. The numbers didn't come
out exactly the same as the tables in the spec, but I think there's
something fishy about the AUX trasnfer size calculations in the spec.
Also the way the i2c transfer length is calculated is somewhat open to
interpretation.

Note that currently we assume 10 kHz speed for the i2c bus. Some real
world devices (eg. some Apple DP->VGA dongle) fails with less than 16
retries, and that would correspond to something close to 20 kHz, so we
assume 10 kHz to be on the safe side. Ideally we should query/set the
i2c bus speed via DPCD but for now this should at leaast remove the
regression from the 1->16 byte trasnfer size change. And of course if
the sink completes the transfer quicker this shouldn't slow things down
since we don't change the interval between retries.

Fixes a regression with some DP dongles from:
commit 1d002fa720738bcd0bddb9178e9ea0773288e1dd
Author: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
Date:   Tue Feb 10 18:38:08 2015 +0000

    drm/dp: Use large transactions for I2C over AUX

Cc: Simon Farnsworth <simon.farnsworth@onelan.com>
Cc: moosotc@gmail.coma
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91451
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 64 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 62 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 80a02a4..2b6b7f9 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -422,6 +422,61 @@ static u32 drm_dp_i2c_functionality(struct i2c_adapter *adapter)
 	       I2C_FUNC_10BIT_ADDR;
 }
 
+#define AUX_PRECHARGE_LEN 16 /* 10 to 16 */
+#define AUX_SYNC_LEN (16 + 4) /* preamble + AUX_SYNC_END */
+#define AUX_STOP_LEN 4
+#define AUX_CMD_LEN 4
+#define AUX_ADDRESS_LEN 20
+#define AUX_REPLY_PAD_LEN 4
+#define AUX_LENGTH_LEN 8
+
+#define AUX_RESPONSE_TIMEOUT 300
+
+static int drm_dp_aux_req_len(const struct drm_dp_aux_msg *msg)
+{
+	int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
+		AUX_CMD_LEN + AUX_ADDRESS_LEN + AUX_LENGTH_LEN;
+
+	if ((msg->request & DP_AUX_I2C_READ) == 0)
+		len += msg->size * 8;
+
+	return len;
+}
+
+static int drm_dp_aux_reply_len(const struct drm_dp_aux_msg *msg)
+{
+	int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
+		AUX_CMD_LEN + AUX_REPLY_PAD_LEN;
+
+	if (msg->request & DP_AUX_I2C_READ)
+		len += msg->size * 8;
+	else
+		len += 8; /* 0 or 1 data bytes for write reply */
+
+	return len;
+}
+
+#define I2C_START_LEN 1
+#define I2C_STOP_LEN 1
+#define I2C_ADDR_LEN 9 /* ADDRESS + R/W + ACK/NACK */
+#define I2C_DATA_LEN 9 /* DATA + ACK/NACK */
+
+static int drm_dp_i2c_msg_len(const struct drm_dp_aux_msg *msg,
+			      int i2c_speed_khz)
+{
+	return (I2C_START_LEN + I2C_ADDR_LEN + msg->size * I2C_DATA_LEN +
+		I2C_STOP_LEN) * 1000 / i2c_speed_khz;
+}
+
+static int drm_dp_i2c_retry_count(const struct drm_dp_aux_msg *msg,
+			      int i2c_speed_khz)
+{
+	int aux_len = drm_dp_aux_req_len(msg) + drm_dp_aux_reply_len(msg) + AUX_RESPONSE_TIMEOUT;
+	int i2c_len = drm_dp_i2c_msg_len(msg, i2c_speed_khz) - AUX_RESPONSE_TIMEOUT;
+
+	return DIV_ROUND_UP(i2c_len, aux_len);
+}
+
 /*
  * Transfer a single I2C-over-AUX message and handle various error conditions,
  * retrying the transaction as appropriate.  It is assumed that the
@@ -434,13 +489,18 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 {
 	unsigned int retry, defer_i2c;
 	int ret;
-
 	/*
 	 * DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source device
 	 * is required to retry at least seven times upon receiving AUX_DEFER
 	 * before giving up the AUX transaction.
+	 *
+	 * We also try to account for the i2c bus speed.
+	 * FIXME currently assumes 10 kHz as some real world devices seem
+	 * to require it. We should query/set the speed via DPCD if supported.
 	 */
-	for (retry = 0, defer_i2c = 0; retry < (7 + defer_i2c); retry++) {
+	int max_retries = max(7, drm_dp_i2c_retry_count(msg, 10));
+
+	for (retry = 0, defer_i2c = 0; retry < (max_retries + defer_i2c); retry++) {
 		mutex_lock(&aux->hw_mutex);
 		ret = aux->transfer(aux, msg);
 		mutex_unlock(&aux->hw_mutex);
-- 
2.4.6

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

[PATCH] drm/dp: Adjust i2c-over-aux retry count based on message size and i2c bus speed

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Calculate the number of retries we should do for each i2c-over-aux
message based on the time it takes to perform the i2c transfer vs. the
aux transfer.

This mostly matches what the DP spec recommends. The numbers didn't come
out exactly the same as the tables in the spec, but I think there's
something fishy about the AUX trasnfer size calculations in the spec.
Also the way the i2c transfer length is calculated is somewhat open to
interpretation.

Note that currently we assume 10 kHz speed for the i2c bus. Some real
world devices (eg. some Apple DP->VGA dongle) fails with less than 16
retries, and that would correspond to something close to 20 kHz, so we
assume 10 kHz to be on the safe side. Ideally we should query/set the
i2c bus speed via DPCD but for now this should at leaast remove the
regression from the 1->16 byte trasnfer size change. And of course if
the sink completes the transfer quicker this shouldn't slow things down
since we don't change the interval between retries.

Fixes a regression with some DP dongles from:
commit 1d002fa720738bcd0bddb9178e9ea0773288e1dd
Author: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
Date:   Tue Feb 10 18:38:08 2015 +0000

    drm/dp: Use large transactions for I2C over AUX

Cc: Simon Farnsworth <simon.farnsworth@onelan.com>
Cc: moosotc@gmail.com
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91451
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 64 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 62 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 80a02a4..2b6b7f9 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -422,6 +422,61 @@ static u32 drm_dp_i2c_functionality(struct i2c_adapter *adapter)
 	       I2C_FUNC_10BIT_ADDR;
 }
 
+#define AUX_PRECHARGE_LEN 16 /* 10 to 16 */
+#define AUX_SYNC_LEN (16 + 4) /* preamble + AUX_SYNC_END */
+#define AUX_STOP_LEN 4
+#define AUX_CMD_LEN 4
+#define AUX_ADDRESS_LEN 20
+#define AUX_REPLY_PAD_LEN 4
+#define AUX_LENGTH_LEN 8
+
+#define AUX_RESPONSE_TIMEOUT 300
+
+static int drm_dp_aux_req_len(const struct drm_dp_aux_msg *msg)
+{
+	int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
+		AUX_CMD_LEN + AUX_ADDRESS_LEN + AUX_LENGTH_LEN;
+
+	if ((msg->request & DP_AUX_I2C_READ) == 0)
+		len += msg->size * 8;
+
+	return len;
+}
+
+static int drm_dp_aux_reply_len(const struct drm_dp_aux_msg *msg)
+{
+	int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
+		AUX_CMD_LEN + AUX_REPLY_PAD_LEN;
+
+	if (msg->request & DP_AUX_I2C_READ)
+		len += msg->size * 8;
+	else
+		len += 8; /* 0 or 1 data bytes for write reply */
+
+	return len;
+}
+
+#define I2C_START_LEN 1
+#define I2C_STOP_LEN 1
+#define I2C_ADDR_LEN 9 /* ADDRESS + R/W + ACK/NACK */
+#define I2C_DATA_LEN 9 /* DATA + ACK/NACK */
+
+static int drm_dp_i2c_msg_len(const struct drm_dp_aux_msg *msg,
+			      int i2c_speed_khz)
+{
+	return (I2C_START_LEN + I2C_ADDR_LEN + msg->size * I2C_DATA_LEN +
+		I2C_STOP_LEN) * 1000 / i2c_speed_khz;
+}
+
+static int drm_dp_i2c_retry_count(const struct drm_dp_aux_msg *msg,
+			      int i2c_speed_khz)
+{
+	int aux_len = drm_dp_aux_req_len(msg) + drm_dp_aux_reply_len(msg) + AUX_RESPONSE_TIMEOUT;
+	int i2c_len = drm_dp_i2c_msg_len(msg, i2c_speed_khz) - AUX_RESPONSE_TIMEOUT;
+
+	return DIV_ROUND_UP(i2c_len, aux_len);
+}
+
 /*
  * Transfer a single I2C-over-AUX message and handle various error conditions,
  * retrying the transaction as appropriate.  It is assumed that the
@@ -434,13 +489,18 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 {
 	unsigned int retry, defer_i2c;
 	int ret;
-
 	/*
 	 * DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source device
 	 * is required to retry at least seven times upon receiving AUX_DEFER
 	 * before giving up the AUX transaction.
+	 *
+	 * We also try to account for the i2c bus speed.
+	 * FIXME currently assumes 10 kHz as some real world devices seem
+	 * to require it. We should query/set the speed via DPCD if supported.
 	 */
-	for (retry = 0, defer_i2c = 0; retry < (7 + defer_i2c); retry++) {
+	int max_retries = max(7, drm_dp_i2c_retry_count(msg, 10));
+
+	for (retry = 0, defer_i2c = 0; retry < (max_retries + defer_i2c); retry++) {
 		mutex_lock(&aux->hw_mutex);
 		ret = aux->transfer(aux, msg);
 		mutex_unlock(&aux->hw_mutex);
-- 
2.4.6

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

Re: [PATCH] drm/dp: Adjust i2c-over-aux retry count based on message size and i2c bus speed

[moosotc]

ville.syrjala@linux.intel.com writes:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Calculate the number of retries we should do for each i2c-over-aux
> message based on the time it takes to perform the i2c transfer vs. the
> aux transfer.
>
> This mostly matches what the DP spec recommends. The numbers didn't come
> out exactly the same as the tables in the spec, but I think there's
> something fishy about the AUX trasnfer size calculations in the spec.
> Also the way the i2c transfer length is calculated is somewhat open to
> interpretation.
>
> Note that currently we assume 10 kHz speed for the i2c bus. Some real
> world devices (eg. some Apple DP->VGA dongle) fails with less than 16
> retries, and that would correspond to something close to 20 kHz, so we
> assume 10 kHz to be on the safe side. Ideally we should query/set the
> i2c bus speed via DPCD but for now this should at leaast remove the
> regression from the 1->16 byte trasnfer size change. And of course if
> the sink completes the transfer quicker this shouldn't slow things down
> since we don't change the interval between retries.
>
> Fixes a regression with some DP dongles from:
> commit 1d002fa720738bcd0bddb9178e9ea0773288e1dd
> Author: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
> Date:   Tue Feb 10 18:38:08 2015 +0000
>
>     drm/dp: Use large transactions for I2C over AUX
>
> Cc: Simon Farnsworth <simon.farnsworth@onelan.com>
> Cc: moosotc@gmail.com
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91451
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 64 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 62 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 80a02a4..2b6b7f9 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -422,6 +422,61 @@ static u32 drm_dp_i2c_functionality(struct i2c_adapter *adapter)
>  	       I2C_FUNC_10BIT_ADDR;
>  }
>  
> +#define AUX_PRECHARGE_LEN 16 /* 10 to 16 */
> +#define AUX_SYNC_LEN (16 + 4) /* preamble + AUX_SYNC_END */
> +#define AUX_STOP_LEN 4
> +#define AUX_CMD_LEN 4
> +#define AUX_ADDRESS_LEN 20
> +#define AUX_REPLY_PAD_LEN 4
> +#define AUX_LENGTH_LEN 8
> +
> +#define AUX_RESPONSE_TIMEOUT 300
> +
> +static int drm_dp_aux_req_len(const struct drm_dp_aux_msg *msg)
> +{
> +	int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
> +		AUX_CMD_LEN + AUX_ADDRESS_LEN + AUX_LENGTH_LEN;
> +
> +	if ((msg->request & DP_AUX_I2C_READ) == 0)
> +		len += msg->size * 8;
> +
> +	return len;
> +}
> +
> +static int drm_dp_aux_reply_len(const struct drm_dp_aux_msg *msg)
> +{
> +	int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
> +		AUX_CMD_LEN + AUX_REPLY_PAD_LEN;
> +
> +	if (msg->request & DP_AUX_I2C_READ)
> +		len += msg->size * 8;
> +	else
> +		len += 8; /* 0 or 1 data bytes for write reply */
> +
> +	return len;
> +}
> +
> +#define I2C_START_LEN 1
> +#define I2C_STOP_LEN 1
> +#define I2C_ADDR_LEN 9 /* ADDRESS + R/W + ACK/NACK */
> +#define I2C_DATA_LEN 9 /* DATA + ACK/NACK */
> +
> +static int drm_dp_i2c_msg_len(const struct drm_dp_aux_msg *msg,
> +			      int i2c_speed_khz)
> +{
> +	return (I2C_START_LEN + I2C_ADDR_LEN + msg->size * I2C_DATA_LEN +
> +		I2C_STOP_LEN) * 1000 / i2c_speed_khz;
> +}
> +
> +static int drm_dp_i2c_retry_count(const struct drm_dp_aux_msg *msg,
> +			      int i2c_speed_khz)
> +{
> +	int aux_len = drm_dp_aux_req_len(msg) + drm_dp_aux_reply_len(msg) + AUX_RESPONSE_TIMEOUT;
> +	int i2c_len = drm_dp_i2c_msg_len(msg, i2c_speed_khz) - AUX_RESPONSE_TIMEOUT;
> +
> +	return DIV_ROUND_UP(i2c_len, aux_len);
> +}
> +
>  /*
>   * Transfer a single I2C-over-AUX message and handle various error conditions,
>   * retrying the transaction as appropriate.  It is assumed that the
> @@ -434,13 +489,18 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>  {
>  	unsigned int retry, defer_i2c;
>  	int ret;
> -
>  	/*
>  	 * DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source device
>  	 * is required to retry at least seven times upon receiving AUX_DEFER
>  	 * before giving up the AUX transaction.
> +	 *
> +	 * We also try to account for the i2c bus speed.
> +	 * FIXME currently assumes 10 kHz as some real world devices seem
> +	 * to require it. We should query/set the speed via DPCD if supported.
>  	 */
> -	for (retry = 0, defer_i2c = 0; retry < (7 + defer_i2c); retry++) {
> +	int max_retries = max(7, drm_dp_i2c_retry_count(msg, 10));
> +
> +	for (retry = 0, defer_i2c = 0; retry < (max_retries + defer_i2c); retry++) {
>  		mutex_lock(&aux->hw_mutex);
>  		ret = aux->transfer(aux, msg);
>  		mutex_unlock(&aux->hw_mutex);

Tested-by: moosotc@gmail.com

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

Re: [PATCH] drm/dp: Adjust i2c-over-aux retry count based on message size and i2c bus speed

[simon.farnsworth]

On Tuesday 25 August 2015 18:10:14 ville.syrjala@linux.intel.com wrote:


Simon Farnsworth |Senior Software Engineer
Tel: +44(0)1491 411400|Fax: +44(0)1491 579254
Email: simon.farnsworth@onelan.com |Company Website: www.onelan.com

ONELAN Limited
Andersen House, Newtown Road, Henley-on-Thames, Oxfordshire. UK, RG9 1HG

 
 



This email and any files transmitted with it are confidential and intended solely for the intended recipient. If you are not the named addressee you should not disseminate, distribute, copy or alter this email. Any views or opinions presented in this email are solely those of the author and might not represent those of ONELAN Ltd. Warning: Although ONELAN Ltd has taken reasonable precautions to ensure no viruses are present in this email, the company cannot accept responsibility for any loss or damage arising from the use of this email or attachments. 

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Calculate the number of retries we should do for each i2c-over-aux
> message based on the time it takes to perform the i2c transfer vs. the
> aux transfer.
> 
> This mostly matches what the DP spec recommends. The numbers didn't come
> out exactly the same as the tables in the spec, but I think there's
> something fishy about the AUX trasnfer size calculations in the spec.
> Also the way the i2c transfer length is calculated is somewhat open to
> interpretation.
> 
> Note that currently we assume 10 kHz speed for the i2c bus. Some real
> world devices (eg. some Apple DP->VGA dongle) fails with less than 16
> retries, and that would correspond to something close to 20 kHz, so we
> assume 10 kHz to be on the safe side. Ideally we should query/set the
> i2c bus speed via DPCD but for now this should at leaast remove the
> regression from the 1->16 byte trasnfer size change. And of course if
> the sink completes the transfer quicker this shouldn't slow things down
> since we don't change the interval between retries.
> 
> Fixes a regression with some DP dongles from:
> commit 1d002fa720738bcd0bddb9178e9ea0773288e1dd
> Author: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
> Date:   Tue Feb 10 18:38:08 2015 +0000
> 
>     drm/dp: Use large transactions for I2C over AUX
> 
> Cc: Simon Farnsworth <simon.farnsworth@onelan.com>
> Cc: moosotc@gmail.com
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91451
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 64 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 62 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 80a02a4..2b6b7f9 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -422,6 +422,61 @@ static u32 drm_dp_i2c_functionality(struct i2c_adapter *adapter)
>  	       I2C_FUNC_10BIT_ADDR;
>  }
>  
> +#define AUX_PRECHARGE_LEN 16 /* 10 to 16 */

I'd change this to 10 - see below for reasoning.

> +#define AUX_SYNC_LEN (16 + 4) /* preamble + AUX_SYNC_END */
> +#define AUX_STOP_LEN 4
> +#define AUX_CMD_LEN 4
> +#define AUX_ADDRESS_LEN 20
> +#define AUX_REPLY_PAD_LEN 4
> +#define AUX_LENGTH_LEN 8
> +
> +#define AUX_RESPONSE_TIMEOUT 300
> +
> +static int drm_dp_aux_req_len(const struct drm_dp_aux_msg *msg)
> +{
> +	int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
> +		AUX_CMD_LEN + AUX_ADDRESS_LEN + AUX_LENGTH_LEN;
> +
> +	if ((msg->request & DP_AUX_I2C_READ) == 0)
> +		len += msg->size * 8;
> +
> +	return len;
> +}
> +
> +static int drm_dp_aux_reply_len(const struct drm_dp_aux_msg *msg)
> +{
> +	int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
> +		AUX_CMD_LEN + AUX_REPLY_PAD_LEN;
> +
> +	if (msg->request & DP_AUX_I2C_READ)
> +		len += msg->size * 8;
> +	else
> +		len += 8; /* 0 or 1 data bytes for write reply */
> +
> +	return len;
> +}
> +
> +#define I2C_START_LEN 1
> +#define I2C_STOP_LEN 1
> +#define I2C_ADDR_LEN 9 /* ADDRESS + R/W + ACK/NACK */
> +#define I2C_DATA_LEN 9 /* DATA + ACK/NACK */
> +
> +static int drm_dp_i2c_msg_len(const struct drm_dp_aux_msg *msg,
> +			      int i2c_speed_khz)
> +{
> +	return (I2C_START_LEN + I2C_ADDR_LEN + msg->size * I2C_DATA_LEN +
> +		I2C_STOP_LEN) * 1000 / i2c_speed_khz;
> +}
> +

This function caught me out at first - because you're not tracking state
between two AUX transactions, you assume that every DP AUX transaction is
mapped to a single I2C transaction. This then gets you the worst case timings
for the I2C transaction.

> +static int drm_dp_i2c_retry_count(const struct drm_dp_aux_msg *msg,
> +			      int i2c_speed_khz)
> +{
> +	int aux_len = drm_dp_aux_req_len(msg) + drm_dp_aux_reply_len(msg) + AUX_RESPONSE_TIMEOUT;
> +	int i2c_len = drm_dp_i2c_msg_len(msg, i2c_speed_khz) - AUX_RESPONSE_TIMEOUT;
> +
> +	return DIV_ROUND_UP(i2c_len, aux_len);
> +}
> +

The logic here looks wrong to me - aux_len is the worst case time for a
single AUX transaction, while I'd expect it to be the best case time (as it's
the divisor). Similarly, I'd expect i2c_len to be the worst case time for the
I2C transaction, but then you subtract a response timeout from it.

I'd change AUX_PRECHARGE_LEN to 10, so that drm_dp_aux_req_len and
drm_dp_aux_reply_len return best case times for the AUX transaction. Then
this function becomes:

static int drm_dp_i2c_retry_count(const struct drm_dp_aux_msg *msg,
			      int i2c_speed_khz)
{
	int aux_len = drm_dp_aux_req_len(msg) + drm_dp_aux_reply_len(msg);
	int i2c_len = drm_dp_i2c_msg_len(msg, i2c_speed_khz);

	return DIV_ROUND_UP(i2c_len + AUX_RESPONSE_TIMEOUT, aux_len);
}

In other words, retry until the best case timings for the AUX transactions
exceed the worst case timings for the I2C transaction plus the AUX response
timeout.

>  /*
>   * Transfer a single I2C-over-AUX message and handle various error conditions,
>   * retrying the transaction as appropriate.  It is assumed that the
> @@ -434,13 +489,18 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>  {
>  	unsigned int retry, defer_i2c;
>  	int ret;
> -
>  	/*
>  	 * DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source device
>  	 * is required to retry at least seven times upon receiving AUX_DEFER
>  	 * before giving up the AUX transaction.
> +	 *
> +	 * We also try to account for the i2c bus speed.
> +	 * FIXME currently assumes 10 kHz as some real world devices seem
> +	 * to require it. We should query/set the speed via DPCD if supported.
>  	 */
> -	for (retry = 0, defer_i2c = 0; retry < (7 + defer_i2c); retry++) {
> +	int max_retries = max(7, drm_dp_i2c_retry_count(msg, 10));
> +
> +	for (retry = 0, defer_i2c = 0; retry < (max_retries + defer_i2c); retry++) {
>  		mutex_lock(&aux->hw_mutex);
>  		ret = aux->transfer(aux, msg);
>  		mutex_unlock(&aux->hw_mutex);
> 

______________________________________________________________________
This email has been scanned by the Symantec Email Security.cloud service.
For more information please visit http://www.symanteccloud.com
______________________________________________________________________
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/dp: Adjust i2c-over-aux retry count based on message size and i2c bus speed

[Ville Syrjälä]

On Wed, Aug 26, 2015 at 12:11:56PM +0100, simon.farnsworth@onelan.com wrote:
> On Tuesday 25 August 2015 18:10:14 ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Calculate the number of retries we should do for each i2c-over-aux
> > message based on the time it takes to perform the i2c transfer vs. the
> > aux transfer.
> > 
> > This mostly matches what the DP spec recommends. The numbers didn't come
> > out exactly the same as the tables in the spec, but I think there's
> > something fishy about the AUX trasnfer size calculations in the spec.
> > Also the way the i2c transfer length is calculated is somewhat open to
> > interpretation.
> > 
> > Note that currently we assume 10 kHz speed for the i2c bus. Some real
> > world devices (eg. some Apple DP->VGA dongle) fails with less than 16
> > retries, and that would correspond to something close to 20 kHz, so we
> > assume 10 kHz to be on the safe side. Ideally we should query/set the
> > i2c bus speed via DPCD but for now this should at leaast remove the
> > regression from the 1->16 byte trasnfer size change. And of course if
> > the sink completes the transfer quicker this shouldn't slow things down
> > since we don't change the interval between retries.
> > 
> > Fixes a regression with some DP dongles from:
> > commit 1d002fa720738bcd0bddb9178e9ea0773288e1dd
> > Author: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
> > Date:   Tue Feb 10 18:38:08 2015 +0000
> > 
> >     drm/dp: Use large transactions for I2C over AUX
> > 
> > Cc: Simon Farnsworth <simon.farnsworth@onelan.com>
> > Cc: moosotc@gmail.com
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91451
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_dp_helper.c | 64 +++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 62 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > index 80a02a4..2b6b7f9 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -422,6 +422,61 @@ static u32 drm_dp_i2c_functionality(struct i2c_adapter *adapter)
> >  	       I2C_FUNC_10BIT_ADDR;
> >  }
> >  
> > +#define AUX_PRECHARGE_LEN 16 /* 10 to 16 */
> 
> I'd change this to 10 - see below for reasoning.
> 
> > +#define AUX_SYNC_LEN (16 + 4) /* preamble + AUX_SYNC_END */
> > +#define AUX_STOP_LEN 4
> > +#define AUX_CMD_LEN 4
> > +#define AUX_ADDRESS_LEN 20
> > +#define AUX_REPLY_PAD_LEN 4
> > +#define AUX_LENGTH_LEN 8
> > +
> > +#define AUX_RESPONSE_TIMEOUT 300
> > +
> > +static int drm_dp_aux_req_len(const struct drm_dp_aux_msg *msg)
> > +{
> > +	int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
> > +		AUX_CMD_LEN + AUX_ADDRESS_LEN + AUX_LENGTH_LEN;
> > +
> > +	if ((msg->request & DP_AUX_I2C_READ) == 0)
> > +		len += msg->size * 8;
> > +
> > +	return len;
> > +}
> > +
> > +static int drm_dp_aux_reply_len(const struct drm_dp_aux_msg *msg)
> > +{
> > +	int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
> > +		AUX_CMD_LEN + AUX_REPLY_PAD_LEN;
> > +
> > +	if (msg->request & DP_AUX_I2C_READ)
> > +		len += msg->size * 8;
> > +	else
> > +		len += 8; /* 0 or 1 data bytes for write reply */
> > +
> > +	return len;
> > +}
> > +
> > +#define I2C_START_LEN 1
> > +#define I2C_STOP_LEN 1
> > +#define I2C_ADDR_LEN 9 /* ADDRESS + R/W + ACK/NACK */
> > +#define I2C_DATA_LEN 9 /* DATA + ACK/NACK */
> > +
> > +static int drm_dp_i2c_msg_len(const struct drm_dp_aux_msg *msg,
> > +			      int i2c_speed_khz)
> > +{
> > +	return (I2C_START_LEN + I2C_ADDR_LEN + msg->size * I2C_DATA_LEN +
> > +		I2C_STOP_LEN) * 1000 / i2c_speed_khz;
> > +}
> > +
> 
> This function caught me out at first - because you're not tracking state
> between two AUX transactions, you assume that every DP AUX transaction is
> mapped to a single I2C transaction. This then gets you the worst case timings
> for the I2C transaction.

Yeah I did consider making it more accurate but then thought it's easier
to just assume worst case behaviour.

> 
> > +static int drm_dp_i2c_retry_count(const struct drm_dp_aux_msg *msg,
> > +			      int i2c_speed_khz)
> > +{
> > +	int aux_len = drm_dp_aux_req_len(msg) + drm_dp_aux_reply_len(msg) + AUX_RESPONSE_TIMEOUT;
> > +	int i2c_len = drm_dp_i2c_msg_len(msg, i2c_speed_khz) - AUX_RESPONSE_TIMEOUT;
> > +
> > +	return DIV_ROUND_UP(i2c_len, aux_len);
> > +}
> > +
> 
> The logic here looks wrong to me - aux_len is the worst case time for a
> single AUX transaction, while I'd expect it to be the best case time (as it's
> the divisor). Similarly, I'd expect i2c_len to be the worst case time for the
> I2C transaction, but then you subtract a response timeout from it.
> 
> I'd change AUX_PRECHARGE_LEN to 10, so that drm_dp_aux_req_len and
> drm_dp_aux_reply_len return best case times for the AUX transaction. Then
> this function becomes:
> 
> static int drm_dp_i2c_retry_count(const struct drm_dp_aux_msg *msg,
> 			      int i2c_speed_khz)
> {
> 	int aux_len = drm_dp_aux_req_len(msg) + drm_dp_aux_reply_len(msg);
> 	int i2c_len = drm_dp_i2c_msg_len(msg, i2c_speed_khz);
> 
> 	return DIV_ROUND_UP(i2c_len + AUX_RESPONSE_TIMEOUT, aux_len);
> }
> 
> In other words, retry until the best case timings for the AUX transactions
> exceed the worst case timings for the I2C transaction plus the AUX response
> timeout.

Yeah that would make sense. It did cross my mind, but I kept the
calculation as the spec had it (more or less). But it's hard to argue
with the logic of comparing the worst case i2c to the best case aux, so
I'll resping the patch with that change. I'll also need to remove the
'len += 8' from the aux write reply estimate.

> 
> >  /*
> >   * Transfer a single I2C-over-AUX message and handle various error conditions,
> >   * retrying the transaction as appropriate.  It is assumed that the
> > @@ -434,13 +489,18 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> >  {
> >  	unsigned int retry, defer_i2c;
> >  	int ret;
> > -
> >  	/*
> >  	 * DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source device
> >  	 * is required to retry at least seven times upon receiving AUX_DEFER
> >  	 * before giving up the AUX transaction.
> > +	 *
> > +	 * We also try to account for the i2c bus speed.
> > +	 * FIXME currently assumes 10 kHz as some real world devices seem
> > +	 * to require it. We should query/set the speed via DPCD if supported.
> >  	 */
> > -	for (retry = 0, defer_i2c = 0; retry < (7 + defer_i2c); retry++) {
> > +	int max_retries = max(7, drm_dp_i2c_retry_count(msg, 10));
> > +
> > +	for (retry = 0, defer_i2c = 0; retry < (max_retries + defer_i2c); retry++) {
> >  		mutex_lock(&aux->hw_mutex);
> >  		ret = aux->transfer(aux, msg);
> >  		mutex_unlock(&aux->hw_mutex);
> > 
> 
> ______________________________________________________________________
> This email has been scanned by the Symantec Email Security.cloud service.
> For more information please visit http://www.symanteccloud.com
> ______________________________________________________________________

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

[PATCH 1/3] drm/dp: Define AUX_RETRY_INTERVAL as 500 us

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently we react to native and i2c defers by waiting either 400-500 us
or 500-600 us, depending on which code path we take. Consolidate them
all to one define AUX_RETRY_INTERVAL which defines the minimum interval.
Since we've been using two different intervals pick the longer of them
and define AUX_RETRY_INTERVAL as 500 us. For the maximum just use
AUX_RETRY_INTERVAL+100 us.

I want to have a define for this so that I can use it when calculating
the estimated duration of i2c-over-aux transfers. Without a define it
would be very easy to change the sleep duration and neglect to update
the i2c-over-aux estimates.

Cc: Simon Farnsworth <simon.farnsworth@onelan.com>
Cc: moosotc@gmail.com
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 80a02a4..7069e54 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -159,6 +159,8 @@ int drm_dp_bw_code_to_link_rate(u8 link_bw)
 }
 EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate);
 
+#define AUX_RETRY_INTERVAL 500 /* us */
+
 /**
  * DOC: dp helpers
  *
@@ -213,7 +215,7 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
 			return -EIO;
 
 		case DP_AUX_NATIVE_REPLY_DEFER:
-			usleep_range(400, 500);
+			usleep_range(AUX_RETRY_INTERVAL, AUX_RETRY_INTERVAL + 100);
 			break;
 		}
 	}
@@ -476,7 +478,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 			 * For now just defer for long enough to hopefully be
 			 * safe for all use-cases.
 			 */
-			usleep_range(500, 600);
+			usleep_range(AUX_RETRY_INTERVAL, AUX_RETRY_INTERVAL + 100);
 			continue;
 
 		default:
@@ -506,7 +508,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 			aux->i2c_defer_count++;
 			if (defer_i2c < 7)
 				defer_i2c++;
-			usleep_range(400, 500);
+			usleep_range(AUX_RETRY_INTERVAL, AUX_RETRY_INTERVAL + 100);
 			continue;
 
 		default:
-- 
2.4.6

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

[PATCH v2 2/3] drm/dp: Adjust i2c-over-aux retry count based on message size and i2c bus speed

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Calculate the number of retries we should do for each i2c-over-aux
message based on the time it takes to perform the i2c transfer vs. the
aux transfer. We assume the shortest possible length for the aux
transfer, and the longest possible (exluding clock stretching) for the
i2c transfer.

The DP spec has some examples on how to calculate this, but we don't
calculate things quite the same way. The spec doesn't account for the
retry interval (assumes immediate retry on defer), and doesn't assume
the best/worst case behaviour as we do.

Note that currently we assume 10 kHz speed for the i2c bus. Some real
world devices (eg. some Apple DP->VGA dongle) fails with less than 16
retries. and that would correspond to something close to 15 kHz (with
our method of calculating things) But let's just go for 10 kHz to be
on the safe side. Ideally we should query/set the i2c bus speed via
DPCD but for now this should at leaast remove the regression from the
1->16 byte trasnfer size change. And of course if the sink completes
the transfer quicker this shouldn't slow things down since we don't
change the interval between retries.

I did a few experiments with a DP->DVI dongle I have that allows you
to change the i2c bus speed. Here are the results of me changing the
actual bus speed and the assumed bus speed and seeing when we start
to fail the operation:

actual i2c khz          assumed i2c khz         max retries
1                       1 ok -> 2 fail          211 ok -> 106 fail
5                       8 ok -> 9 fail          27 ok -> 24 fail
10                      17 ok -> 18 fail        13 ok -> 12 fail
100                     210 ok -> 211 fail      2 ok -> 1 fail

So based on that we have a fairly decent safety margin baked into
the formula to calculate the max number of retries.

Fixes a regression with some DP dongles from:
commit 1d002fa720738bcd0bddb9178e9ea0773288e1dd
Author: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
Date:   Tue Feb 10 18:38:08 2015 +0000

    drm/dp: Use large transactions for I2C over AUX

v2: Use best case for AUX and worst case for i2c (Simon Farnsworth)
    Add a define our AUX retry interval and account for it

Cc: Simon Farnsworth <simon.farnsworth@onelan.com>
Cc: moosotc@gmail.com
Tested-by: moosotc@gmail.com
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91451
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 81 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 79 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 7069e54..23b9fcc 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -424,6 +424,78 @@ static u32 drm_dp_i2c_functionality(struct i2c_adapter *adapter)
 	       I2C_FUNC_10BIT_ADDR;
 }
 
+#define AUX_PRECHARGE_LEN 10 /* 10 to 16 */
+#define AUX_SYNC_LEN (16 + 4) /* preamble + AUX_SYNC_END */
+#define AUX_STOP_LEN 4
+#define AUX_CMD_LEN 4
+#define AUX_ADDRESS_LEN 20
+#define AUX_REPLY_PAD_LEN 4
+#define AUX_LENGTH_LEN 8
+
+/*
+ * Calculate the length of the AUX request/reply. Gives the "best"
+ * case estimate, ie. successful while as short as possible.
+ */
+static int drm_dp_aux_req_len(const struct drm_dp_aux_msg *msg)
+{
+	int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
+		AUX_CMD_LEN + AUX_ADDRESS_LEN + AUX_LENGTH_LEN;
+
+	if ((msg->request & DP_AUX_I2C_READ) == 0)
+		len += msg->size * 8;
+
+	return len;
+}
+
+static int drm_dp_aux_reply_len(const struct drm_dp_aux_msg *msg)
+{
+	int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
+		AUX_CMD_LEN + AUX_REPLY_PAD_LEN;
+
+	/*
+	 * For read we expect what was asked. For writes there will
+	 * be 0 or 1 data bytes. Assume 0 for the "best" case.
+	 */
+	if (msg->request & DP_AUX_I2C_READ)
+		len += msg->size * 8;
+
+	return len;
+}
+
+#define I2C_START_LEN 1
+#define I2C_STOP_LEN 1
+#define I2C_ADDR_LEN 9 /* ADDRESS + R/W + ACK/NACK */
+#define I2C_DATA_LEN 9 /* DATA + ACK/NACK */
+
+/*
+ * Calculate the length of the i2c transfer (in AUX clocks)
+ * assuming the i2c bus speed is as specified. Gives the the
+ * "worst" case estimate, ie. successful while as long as possible.
+ * Doesn't account the the "MOT" bit, and instead assumes each
+ * message includes a START, ADDRESS and STOP. Neither does it
+ * account for additional random variables such as clock stretching.
+ */
+static int drm_dp_i2c_msg_len(const struct drm_dp_aux_msg *msg,
+			      int i2c_speed_khz)
+{
+	return (I2C_START_LEN + I2C_ADDR_LEN + msg->size * I2C_DATA_LEN +
+		I2C_STOP_LEN) * 1000 / i2c_speed_khz;
+}
+
+/*
+ * Deterine how many retries should be attempted to successfully transfer
+ * the specified message, based on the estimated durations of the
+ * i2c and AUX transfers.
+ */
+static int drm_dp_i2c_retry_count(const struct drm_dp_aux_msg *msg,
+			      int i2c_speed_khz)
+{
+	int aux_len = drm_dp_aux_req_len(msg) + drm_dp_aux_reply_len(msg);
+	int i2c_len = drm_dp_i2c_msg_len(msg, i2c_speed_khz);
+
+	return DIV_ROUND_UP(i2c_len, aux_len + AUX_RETRY_INTERVAL);
+}
+
 /*
  * Transfer a single I2C-over-AUX message and handle various error conditions,
  * retrying the transaction as appropriate.  It is assumed that the
@@ -436,13 +508,18 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 {
 	unsigned int retry, defer_i2c;
 	int ret;
-
 	/*
 	 * DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source device
 	 * is required to retry at least seven times upon receiving AUX_DEFER
 	 * before giving up the AUX transaction.
+	 *
+	 * We also try to account for the i2c bus speed.
+	 * FIXME currently assumes 10 kHz as some real world devices seem
+	 * to require it. We should query/set the speed via DPCD if supported.
 	 */
-	for (retry = 0, defer_i2c = 0; retry < (7 + defer_i2c); retry++) {
+	int max_retries = max(7, drm_dp_i2c_retry_count(msg, 10));
+
+	for (retry = 0, defer_i2c = 0; retry < (max_retries + defer_i2c); retry++) {
 		mutex_lock(&aux->hw_mutex);
 		ret = aux->transfer(aux, msg);
 		mutex_unlock(&aux->hw_mutex);
-- 
2.4.6

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

Re: [PATCH v2 2/3] drm/dp: Adjust i2c-over-aux retry count based on message size and i2c bus speed

[Daniel Vetter]

On Wed, Aug 26, 2015 at 10:55:06PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Calculate the number of retries we should do for each i2c-over-aux
> message based on the time it takes to perform the i2c transfer vs. the
> aux transfer. We assume the shortest possible length for the aux
> transfer, and the longest possible (exluding clock stretching) for the
> i2c transfer.
> 
> The DP spec has some examples on how to calculate this, but we don't
> calculate things quite the same way. The spec doesn't account for the
> retry interval (assumes immediate retry on defer), and doesn't assume
> the best/worst case behaviour as we do.
> 
> Note that currently we assume 10 kHz speed for the i2c bus. Some real
> world devices (eg. some Apple DP->VGA dongle) fails with less than 16
> retries. and that would correspond to something close to 15 kHz (with
> our method of calculating things) But let's just go for 10 kHz to be
> on the safe side. Ideally we should query/set the i2c bus speed via
> DPCD but for now this should at leaast remove the regression from the
> 1->16 byte trasnfer size change. And of course if the sink completes
> the transfer quicker this shouldn't slow things down since we don't
> change the interval between retries.
> 
> I did a few experiments with a DP->DVI dongle I have that allows you
> to change the i2c bus speed. Here are the results of me changing the
> actual bus speed and the assumed bus speed and seeing when we start
> to fail the operation:
> 
> actual i2c khz          assumed i2c khz         max retries
> 1                       1 ok -> 2 fail          211 ok -> 106 fail
> 5                       8 ok -> 9 fail          27 ok -> 24 fail
> 10                      17 ok -> 18 fail        13 ok -> 12 fail
> 100                     210 ok -> 211 fail      2 ok -> 1 fail
> 
> So based on that we have a fairly decent safety margin baked into
> the formula to calculate the max number of retries.
> 
> Fixes a regression with some DP dongles from:
> commit 1d002fa720738bcd0bddb9178e9ea0773288e1dd
> Author: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
> Date:   Tue Feb 10 18:38:08 2015 +0000
> 
>     drm/dp: Use large transactions for I2C over AUX
> 
> v2: Use best case for AUX and worst case for i2c (Simon Farnsworth)
>     Add a define our AUX retry interval and account for it
> 
> Cc: Simon Farnsworth <simon.farnsworth@onelan.com>
> Cc: moosotc@gmail.com
> Tested-by: moosotc@gmail.com
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91451
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 81 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 79 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 7069e54..23b9fcc 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -424,6 +424,78 @@ static u32 drm_dp_i2c_functionality(struct i2c_adapter *adapter)
>  	       I2C_FUNC_10BIT_ADDR;
>  }
>  
> +#define AUX_PRECHARGE_LEN 10 /* 10 to 16 */
> +#define AUX_SYNC_LEN (16 + 4) /* preamble + AUX_SYNC_END */
> +#define AUX_STOP_LEN 4
> +#define AUX_CMD_LEN 4
> +#define AUX_ADDRESS_LEN 20
> +#define AUX_REPLY_PAD_LEN 4
> +#define AUX_LENGTH_LEN 8
> +
> +/*
> + * Calculate the length of the AUX request/reply. Gives the "best"
> + * case estimate, ie. successful while as short as possible.
> + */
> +static int drm_dp_aux_req_len(const struct drm_dp_aux_msg *msg)
> +{
> +	int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
> +		AUX_CMD_LEN + AUX_ADDRESS_LEN + AUX_LENGTH_LEN;
> +
> +	if ((msg->request & DP_AUX_I2C_READ) == 0)
> +		len += msg->size * 8;
> +
> +	return len;
> +}
> +
> +static int drm_dp_aux_reply_len(const struct drm_dp_aux_msg *msg)
> +{
> +	int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
> +		AUX_CMD_LEN + AUX_REPLY_PAD_LEN;
> +
> +	/*
> +	 * For read we expect what was asked. For writes there will
> +	 * be 0 or 1 data bytes. Assume 0 for the "best" case.
> +	 */
> +	if (msg->request & DP_AUX_I2C_READ)
> +		len += msg->size * 8;
> +
> +	return len;
> +}
> +
> +#define I2C_START_LEN 1
> +#define I2C_STOP_LEN 1
> +#define I2C_ADDR_LEN 9 /* ADDRESS + R/W + ACK/NACK */
> +#define I2C_DATA_LEN 9 /* DATA + ACK/NACK */
> +
> +/*
> + * Calculate the length of the i2c transfer (in AUX clocks)
> + * assuming the i2c bus speed is as specified. Gives the the
> + * "worst" case estimate, ie. successful while as long as possible.
> + * Doesn't account the the "MOT" bit, and instead assumes each
> + * message includes a START, ADDRESS and STOP. Neither does it
> + * account for additional random variables such as clock stretching.
> + */
> +static int drm_dp_i2c_msg_len(const struct drm_dp_aux_msg *msg,
> +			      int i2c_speed_khz)
> +{
> +	return (I2C_START_LEN + I2C_ADDR_LEN + msg->size * I2C_DATA_LEN +
> +		I2C_STOP_LEN) * 1000 / i2c_speed_khz;

This doesn't seem to compute the lenght, but the time a transfer takes, in
usec (if I haven't screwed up my numbers ...). Also DIV_ROUND_DOWN to make
the defensiveness clear?

> +}
> +
> +/*
> + * Deterine how many retries should be attempted to successfully transfer

Deteri_m_e.

> + * the specified message, based on the estimated durations of the
> + * i2c and AUX transfers.
> + */
> +static int drm_dp_i2c_retry_count(const struct drm_dp_aux_msg *msg,
> +			      int i2c_speed_khz)
> +{
> +	int aux_len = drm_dp_aux_req_len(msg) + drm_dp_aux_reply_len(msg);
> +	int i2c_len = drm_dp_i2c_msg_len(msg, i2c_speed_khz);
> +
> +	return DIV_ROUND_UP(i2c_len, aux_len + AUX_RETRY_INTERVAL);

So assuming I get things right i2c_len is actually i2c_time_us, but
aux_len is still just a length (and since the bus runs at 2MHz it doesn't
seem to just add up correctly). But AUX_RETRY_INTERVAL is in usec (and I
expected an usec/usec division for the ration). I think there's a
aux_time_us = DIV_ROUND_UP(aux_len * 1000 * 1000 / aux_speed_HZ) missing.

Cheers, Daniel

> +}
> +
>  /*
>   * Transfer a single I2C-over-AUX message and handle various error conditions,
>   * retrying the transaction as appropriate.  It is assumed that the
> @@ -436,13 +508,18 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>  {
>  	unsigned int retry, defer_i2c;
>  	int ret;
> -
>  	/*
>  	 * DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source device
>  	 * is required to retry at least seven times upon receiving AUX_DEFER
>  	 * before giving up the AUX transaction.
> +	 *
> +	 * We also try to account for the i2c bus speed.
> +	 * FIXME currently assumes 10 kHz as some real world devices seem
> +	 * to require it. We should query/set the speed via DPCD if supported.
>  	 */
> -	for (retry = 0, defer_i2c = 0; retry < (7 + defer_i2c); retry++) {
> +	int max_retries = max(7, drm_dp_i2c_retry_count(msg, 10));
> +
> +	for (retry = 0, defer_i2c = 0; retry < (max_retries + defer_i2c); retry++) {
>  		mutex_lock(&aux->hw_mutex);
>  		ret = aux->transfer(aux, msg);
>  		mutex_unlock(&aux->hw_mutex);
> -- 
> 2.4.6
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/3] drm/dp: Adjust i2c-over-aux retry count based on message size and i2c bus speed

[Jani Nikula]

On Tue, 01 Sep 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Aug 26, 2015 at 10:55:06PM +0300, ville.syrjala@linux.intel.com wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> 
>> Calculate the number of retries we should do for each i2c-over-aux
>> message based on the time it takes to perform the i2c transfer vs. the
>> aux transfer. We assume the shortest possible length for the aux
>> transfer, and the longest possible (exluding clock stretching) for the
>> i2c transfer.
>> 
>> The DP spec has some examples on how to calculate this, but we don't
>> calculate things quite the same way. The spec doesn't account for the
>> retry interval (assumes immediate retry on defer), and doesn't assume
>> the best/worst case behaviour as we do.
>> 
>> Note that currently we assume 10 kHz speed for the i2c bus. Some real
>> world devices (eg. some Apple DP->VGA dongle) fails with less than 16
>> retries. and that would correspond to something close to 15 kHz (with
>> our method of calculating things) But let's just go for 10 kHz to be
>> on the safe side. Ideally we should query/set the i2c bus speed via
>> DPCD but for now this should at leaast remove the regression from the
>> 1->16 byte trasnfer size change. And of course if the sink completes
>> the transfer quicker this shouldn't slow things down since we don't
>> change the interval between retries.
>> 
>> I did a few experiments with a DP->DVI dongle I have that allows you
>> to change the i2c bus speed. Here are the results of me changing the
>> actual bus speed and the assumed bus speed and seeing when we start
>> to fail the operation:
>> 
>> actual i2c khz          assumed i2c khz         max retries
>> 1                       1 ok -> 2 fail          211 ok -> 106 fail
>> 5                       8 ok -> 9 fail          27 ok -> 24 fail
>> 10                      17 ok -> 18 fail        13 ok -> 12 fail
>> 100                     210 ok -> 211 fail      2 ok -> 1 fail
>> 
>> So based on that we have a fairly decent safety margin baked into
>> the formula to calculate the max number of retries.
>> 
>> Fixes a regression with some DP dongles from:
>> commit 1d002fa720738bcd0bddb9178e9ea0773288e1dd
>> Author: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
>> Date:   Tue Feb 10 18:38:08 2015 +0000
>> 
>>     drm/dp: Use large transactions for I2C over AUX
>> 
>> v2: Use best case for AUX and worst case for i2c (Simon Farnsworth)
>>     Add a define our AUX retry interval and account for it
>> 
>> Cc: Simon Farnsworth <simon.farnsworth@onelan.com>
>> Cc: moosotc@gmail.com
>> Tested-by: moosotc@gmail.com
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91451
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>  drivers/gpu/drm/drm_dp_helper.c | 81 ++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 79 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
>> index 7069e54..23b9fcc 100644
>> --- a/drivers/gpu/drm/drm_dp_helper.c
>> +++ b/drivers/gpu/drm/drm_dp_helper.c
>> @@ -424,6 +424,78 @@ static u32 drm_dp_i2c_functionality(struct i2c_adapter *adapter)
>>  	       I2C_FUNC_10BIT_ADDR;
>>  }
>>  
>> +#define AUX_PRECHARGE_LEN 10 /* 10 to 16 */
>> +#define AUX_SYNC_LEN (16 + 4) /* preamble + AUX_SYNC_END */
>> +#define AUX_STOP_LEN 4
>> +#define AUX_CMD_LEN 4
>> +#define AUX_ADDRESS_LEN 20
>> +#define AUX_REPLY_PAD_LEN 4
>> +#define AUX_LENGTH_LEN 8
>> +
>> +/*
>> + * Calculate the length of the AUX request/reply. Gives the "best"
>> + * case estimate, ie. successful while as short as possible.
>> + */
>> +static int drm_dp_aux_req_len(const struct drm_dp_aux_msg *msg)
>> +{
>> +	int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
>> +		AUX_CMD_LEN + AUX_ADDRESS_LEN + AUX_LENGTH_LEN;
>> +
>> +	if ((msg->request & DP_AUX_I2C_READ) == 0)
>> +		len += msg->size * 8;
>> +
>> +	return len;
>> +}
>> +
>> +static int drm_dp_aux_reply_len(const struct drm_dp_aux_msg *msg)
>> +{
>> +	int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
>> +		AUX_CMD_LEN + AUX_REPLY_PAD_LEN;
>> +
>> +	/*
>> +	 * For read we expect what was asked. For writes there will
>> +	 * be 0 or 1 data bytes. Assume 0 for the "best" case.
>> +	 */
>> +	if (msg->request & DP_AUX_I2C_READ)
>> +		len += msg->size * 8;
>> +
>> +	return len;
>> +}
>> +
>> +#define I2C_START_LEN 1
>> +#define I2C_STOP_LEN 1
>> +#define I2C_ADDR_LEN 9 /* ADDRESS + R/W + ACK/NACK */
>> +#define I2C_DATA_LEN 9 /* DATA + ACK/NACK */
>> +
>> +/*
>> + * Calculate the length of the i2c transfer (in AUX clocks)
>> + * assuming the i2c bus speed is as specified. Gives the the
>> + * "worst" case estimate, ie. successful while as long as possible.
>> + * Doesn't account the the "MOT" bit, and instead assumes each
>> + * message includes a START, ADDRESS and STOP. Neither does it
>> + * account for additional random variables such as clock stretching.
>> + */
>> +static int drm_dp_i2c_msg_len(const struct drm_dp_aux_msg *msg,
>> +			      int i2c_speed_khz)
>> +{
>> +	return (I2C_START_LEN + I2C_ADDR_LEN + msg->size * I2C_DATA_LEN +
>> +		I2C_STOP_LEN) * 1000 / i2c_speed_khz;
>
> This doesn't seem to compute the lenght, but the time a transfer takes, in
> usec (if I haven't screwed up my numbers ...). Also DIV_ROUND_DOWN to make
> the defensiveness clear?
>
>> +}
>> +
>> +/*
>> + * Deterine how many retries should be attempted to successfully transfer
>
> Deteri_m_e.

Determine. :)

>
>> + * the specified message, based on the estimated durations of the
>> + * i2c and AUX transfers.
>> + */
>> +static int drm_dp_i2c_retry_count(const struct drm_dp_aux_msg *msg,
>> +			      int i2c_speed_khz)
>> +{
>> +	int aux_len = drm_dp_aux_req_len(msg) + drm_dp_aux_reply_len(msg);
>> +	int i2c_len = drm_dp_i2c_msg_len(msg, i2c_speed_khz);
>> +
>> +	return DIV_ROUND_UP(i2c_len, aux_len + AUX_RETRY_INTERVAL);
>
> So assuming I get things right i2c_len is actually i2c_time_us, but
> aux_len is still just a length (and since the bus runs at 2MHz it doesn't
> seem to just add up correctly). But AUX_RETRY_INTERVAL is in usec (and I
> expected an usec/usec division for the ration). I think there's a
> aux_time_us = DIV_ROUND_UP(aux_len * 1000 * 1000 / aux_speed_HZ) missing.
>
> Cheers, Daniel
>
>> +}
>> +
>>  /*
>>   * Transfer a single I2C-over-AUX message and handle various error conditions,
>>   * retrying the transaction as appropriate.  It is assumed that the
>> @@ -436,13 +508,18 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>>  {
>>  	unsigned int retry, defer_i2c;
>>  	int ret;
>> -
>>  	/*
>>  	 * DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source device
>>  	 * is required to retry at least seven times upon receiving AUX_DEFER
>>  	 * before giving up the AUX transaction.
>> +	 *
>> +	 * We also try to account for the i2c bus speed.
>> +	 * FIXME currently assumes 10 kHz as some real world devices seem
>> +	 * to require it. We should query/set the speed via DPCD if supported.
>>  	 */
>> -	for (retry = 0, defer_i2c = 0; retry < (7 + defer_i2c); retry++) {
>> +	int max_retries = max(7, drm_dp_i2c_retry_count(msg, 10));
>> +
>> +	for (retry = 0, defer_i2c = 0; retry < (max_retries + defer_i2c); retry++) {
>>  		mutex_lock(&aux->hw_mutex);
>>  		ret = aux->transfer(aux, msg);
>>  		mutex_unlock(&aux->hw_mutex);
>> -- 
>> 2.4.6
>> 
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
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 v2 2/3] drm/dp: Adjust i2c-over-aux retry count based on message size and i2c bus speed

[Ville Syrjälä]

On Tue, Sep 01, 2015 at 11:14:43AM +0200, Daniel Vetter wrote:
> On Wed, Aug 26, 2015 at 10:55:06PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Calculate the number of retries we should do for each i2c-over-aux
> > message based on the time it takes to perform the i2c transfer vs. the
> > aux transfer. We assume the shortest possible length for the aux
> > transfer, and the longest possible (exluding clock stretching) for the
> > i2c transfer.
> > 
> > The DP spec has some examples on how to calculate this, but we don't
> > calculate things quite the same way. The spec doesn't account for the
> > retry interval (assumes immediate retry on defer), and doesn't assume
> > the best/worst case behaviour as we do.
> > 
> > Note that currently we assume 10 kHz speed for the i2c bus. Some real
> > world devices (eg. some Apple DP->VGA dongle) fails with less than 16
> > retries. and that would correspond to something close to 15 kHz (with
> > our method of calculating things) But let's just go for 10 kHz to be
> > on the safe side. Ideally we should query/set the i2c bus speed via
> > DPCD but for now this should at leaast remove the regression from the
> > 1->16 byte trasnfer size change. And of course if the sink completes
> > the transfer quicker this shouldn't slow things down since we don't
> > change the interval between retries.
> > 
> > I did a few experiments with a DP->DVI dongle I have that allows you
> > to change the i2c bus speed. Here are the results of me changing the
> > actual bus speed and the assumed bus speed and seeing when we start
> > to fail the operation:
> > 
> > actual i2c khz          assumed i2c khz         max retries
> > 1                       1 ok -> 2 fail          211 ok -> 106 fail
> > 5                       8 ok -> 9 fail          27 ok -> 24 fail
> > 10                      17 ok -> 18 fail        13 ok -> 12 fail
> > 100                     210 ok -> 211 fail      2 ok -> 1 fail
> > 
> > So based on that we have a fairly decent safety margin baked into
> > the formula to calculate the max number of retries.
> > 
> > Fixes a regression with some DP dongles from:
> > commit 1d002fa720738bcd0bddb9178e9ea0773288e1dd
> > Author: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
> > Date:   Tue Feb 10 18:38:08 2015 +0000
> > 
> >     drm/dp: Use large transactions for I2C over AUX
> > 
> > v2: Use best case for AUX and worst case for i2c (Simon Farnsworth)
> >     Add a define our AUX retry interval and account for it
> > 
> > Cc: Simon Farnsworth <simon.farnsworth@onelan.com>
> > Cc: moosotc@gmail.com
> > Tested-by: moosotc@gmail.com
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91451
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_dp_helper.c | 81 ++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 79 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > index 7069e54..23b9fcc 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -424,6 +424,78 @@ static u32 drm_dp_i2c_functionality(struct i2c_adapter *adapter)
> >  	       I2C_FUNC_10BIT_ADDR;
> >  }
> >  
> > +#define AUX_PRECHARGE_LEN 10 /* 10 to 16 */
> > +#define AUX_SYNC_LEN (16 + 4) /* preamble + AUX_SYNC_END */
> > +#define AUX_STOP_LEN 4
> > +#define AUX_CMD_LEN 4
> > +#define AUX_ADDRESS_LEN 20
> > +#define AUX_REPLY_PAD_LEN 4
> > +#define AUX_LENGTH_LEN 8
> > +
> > +/*
> > + * Calculate the length of the AUX request/reply. Gives the "best"
> > + * case estimate, ie. successful while as short as possible.
> > + */
> > +static int drm_dp_aux_req_len(const struct drm_dp_aux_msg *msg)
> > +{
> > +	int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
> > +		AUX_CMD_LEN + AUX_ADDRESS_LEN + AUX_LENGTH_LEN;
> > +
> > +	if ((msg->request & DP_AUX_I2C_READ) == 0)
> > +		len += msg->size * 8;
> > +
> > +	return len;
> > +}
> > +
> > +static int drm_dp_aux_reply_len(const struct drm_dp_aux_msg *msg)
> > +{
> > +	int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
> > +		AUX_CMD_LEN + AUX_REPLY_PAD_LEN;
> > +
> > +	/*
> > +	 * For read we expect what was asked. For writes there will
> > +	 * be 0 or 1 data bytes. Assume 0 for the "best" case.
> > +	 */
> > +	if (msg->request & DP_AUX_I2C_READ)
> > +		len += msg->size * 8;
> > +
> > +	return len;
> > +}
> > +
> > +#define I2C_START_LEN 1
> > +#define I2C_STOP_LEN 1
> > +#define I2C_ADDR_LEN 9 /* ADDRESS + R/W + ACK/NACK */
> > +#define I2C_DATA_LEN 9 /* DATA + ACK/NACK */
> > +
> > +/*
> > + * Calculate the length of the i2c transfer (in AUX clocks)
> > + * assuming the i2c bus speed is as specified. Gives the the
> > + * "worst" case estimate, ie. successful while as long as possible.
> > + * Doesn't account the the "MOT" bit, and instead assumes each
> > + * message includes a START, ADDRESS and STOP. Neither does it
> > + * account for additional random variables such as clock stretching.
> > + */
> > +static int drm_dp_i2c_msg_len(const struct drm_dp_aux_msg *msg,
> > +			      int i2c_speed_khz)
> > +{
> > +	return (I2C_START_LEN + I2C_ADDR_LEN + msg->size * I2C_DATA_LEN +
> > +		I2C_STOP_LEN) * 1000 / i2c_speed_khz;
> 
> This doesn't seem to compute the lenght, but the time a transfer takes, in
> usec (if I haven't screwed up my numbers ...). Also DIV_ROUND_DOWN to make
> the defensiveness clear?

Well it started out as length, and then I figured I'll just shove the
i2c->aux unit conversion in here. It's a length in time if you will ;)

And it should rather be DIV_ROUND_UP() since we want the "maximum" here.

> 
> > +}
> > +
> > +/*
> > + * Deterine how many retries should be attempted to successfully transfer
> 
> Deteri_m_e.
> 
> > + * the specified message, based on the estimated durations of the
> > + * i2c and AUX transfers.
> > + */
> > +static int drm_dp_i2c_retry_count(const struct drm_dp_aux_msg *msg,
> > +			      int i2c_speed_khz)
> > +{
> > +	int aux_len = drm_dp_aux_req_len(msg) + drm_dp_aux_reply_len(msg);
> > +	int i2c_len = drm_dp_i2c_msg_len(msg, i2c_speed_khz);
> > +
> > +	return DIV_ROUND_UP(i2c_len, aux_len + AUX_RETRY_INTERVAL);
> 
> So assuming I get things right i2c_len is actually i2c_time_us, but
> aux_len is still just a length (and since the bus runs at 2MHz it doesn't
> seem to just add up correctly).

AUX bitrate is 1MHz

> But AUX_RETRY_INTERVAL is in usec (and I
> expected an usec/usec division for the ration). I think there's a
> aux_time_us = DIV_ROUND_UP(aux_len * 1000 * 1000 / aux_speed_HZ) missing.
> 
> Cheers, Daniel
> 
> > +}
> > +
> >  /*
> >   * Transfer a single I2C-over-AUX message and handle various error conditions,
> >   * retrying the transaction as appropriate.  It is assumed that the
> > @@ -436,13 +508,18 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> >  {
> >  	unsigned int retry, defer_i2c;
> >  	int ret;
> > -
> >  	/*
> >  	 * DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source device
> >  	 * is required to retry at least seven times upon receiving AUX_DEFER
> >  	 * before giving up the AUX transaction.
> > +	 *
> > +	 * We also try to account for the i2c bus speed.
> > +	 * FIXME currently assumes 10 kHz as some real world devices seem
> > +	 * to require it. We should query/set the speed via DPCD if supported.
> >  	 */
> > -	for (retry = 0, defer_i2c = 0; retry < (7 + defer_i2c); retry++) {
> > +	int max_retries = max(7, drm_dp_i2c_retry_count(msg, 10));
> > +
> > +	for (retry = 0, defer_i2c = 0; retry < (max_retries + defer_i2c); retry++) {
> >  		mutex_lock(&aux->hw_mutex);
> >  		ret = aux->transfer(aux, msg);
> >  		mutex_unlock(&aux->hw_mutex);
> > -- 
> > 2.4.6
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

Re: [PATCH v2 2/3] drm/dp: Adjust i2c-over-aux retry count based on message size and i2c bus speed

[Daniel Vetter]

On Tue, Sep 01, 2015 at 02:13:01PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 01, 2015 at 11:14:43AM +0200, Daniel Vetter wrote:
> > On Wed, Aug 26, 2015 at 10:55:06PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Calculate the number of retries we should do for each i2c-over-aux
> > > message based on the time it takes to perform the i2c transfer vs. the
> > > aux transfer. We assume the shortest possible length for the aux
> > > transfer, and the longest possible (exluding clock stretching) for the
> > > i2c transfer.
> > > 
> > > The DP spec has some examples on how to calculate this, but we don't
> > > calculate things quite the same way. The spec doesn't account for the
> > > retry interval (assumes immediate retry on defer), and doesn't assume
> > > the best/worst case behaviour as we do.
> > > 
> > > Note that currently we assume 10 kHz speed for the i2c bus. Some real
> > > world devices (eg. some Apple DP->VGA dongle) fails with less than 16
> > > retries. and that would correspond to something close to 15 kHz (with
> > > our method of calculating things) But let's just go for 10 kHz to be
> > > on the safe side. Ideally we should query/set the i2c bus speed via
> > > DPCD but for now this should at leaast remove the regression from the
> > > 1->16 byte trasnfer size change. And of course if the sink completes
> > > the transfer quicker this shouldn't slow things down since we don't
> > > change the interval between retries.
> > > 
> > > I did a few experiments with a DP->DVI dongle I have that allows you
> > > to change the i2c bus speed. Here are the results of me changing the
> > > actual bus speed and the assumed bus speed and seeing when we start
> > > to fail the operation:
> > > 
> > > actual i2c khz          assumed i2c khz         max retries
> > > 1                       1 ok -> 2 fail          211 ok -> 106 fail
> > > 5                       8 ok -> 9 fail          27 ok -> 24 fail
> > > 10                      17 ok -> 18 fail        13 ok -> 12 fail
> > > 100                     210 ok -> 211 fail      2 ok -> 1 fail
> > > 
> > > So based on that we have a fairly decent safety margin baked into
> > > the formula to calculate the max number of retries.
> > > 
> > > Fixes a regression with some DP dongles from:
> > > commit 1d002fa720738bcd0bddb9178e9ea0773288e1dd
> > > Author: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
> > > Date:   Tue Feb 10 18:38:08 2015 +0000
> > > 
> > >     drm/dp: Use large transactions for I2C over AUX
> > > 
> > > v2: Use best case for AUX and worst case for i2c (Simon Farnsworth)
> > >     Add a define our AUX retry interval and account for it
> > > 
> > > Cc: Simon Farnsworth <simon.farnsworth@onelan.com>
> > > Cc: moosotc@gmail.com
> > > Tested-by: moosotc@gmail.com
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91451
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_dp_helper.c | 81 ++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 79 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > > index 7069e54..23b9fcc 100644
> > > --- a/drivers/gpu/drm/drm_dp_helper.c
> > > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > > @@ -424,6 +424,78 @@ static u32 drm_dp_i2c_functionality(struct i2c_adapter *adapter)
> > >  	       I2C_FUNC_10BIT_ADDR;
> > >  }
> > >  
> > > +#define AUX_PRECHARGE_LEN 10 /* 10 to 16 */
> > > +#define AUX_SYNC_LEN (16 + 4) /* preamble + AUX_SYNC_END */
> > > +#define AUX_STOP_LEN 4
> > > +#define AUX_CMD_LEN 4
> > > +#define AUX_ADDRESS_LEN 20
> > > +#define AUX_REPLY_PAD_LEN 4
> > > +#define AUX_LENGTH_LEN 8
> > > +
> > > +/*
> > > + * Calculate the length of the AUX request/reply. Gives the "best"
> > > + * case estimate, ie. successful while as short as possible.
> > > + */
> > > +static int drm_dp_aux_req_len(const struct drm_dp_aux_msg *msg)
> > > +{
> > > +	int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
> > > +		AUX_CMD_LEN + AUX_ADDRESS_LEN + AUX_LENGTH_LEN;
> > > +
> > > +	if ((msg->request & DP_AUX_I2C_READ) == 0)
> > > +		len += msg->size * 8;
> > > +
> > > +	return len;
> > > +}
> > > +
> > > +static int drm_dp_aux_reply_len(const struct drm_dp_aux_msg *msg)
> > > +{
> > > +	int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
> > > +		AUX_CMD_LEN + AUX_REPLY_PAD_LEN;
> > > +
> > > +	/*
> > > +	 * For read we expect what was asked. For writes there will
> > > +	 * be 0 or 1 data bytes. Assume 0 for the "best" case.
> > > +	 */
> > > +	if (msg->request & DP_AUX_I2C_READ)
> > > +		len += msg->size * 8;
> > > +
> > > +	return len;
> > > +}
> > > +
> > > +#define I2C_START_LEN 1
> > > +#define I2C_STOP_LEN 1
> > > +#define I2C_ADDR_LEN 9 /* ADDRESS + R/W + ACK/NACK */
> > > +#define I2C_DATA_LEN 9 /* DATA + ACK/NACK */
> > > +
> > > +/*
> > > + * Calculate the length of the i2c transfer (in AUX clocks)
> > > + * assuming the i2c bus speed is as specified. Gives the the
> > > + * "worst" case estimate, ie. successful while as long as possible.
> > > + * Doesn't account the the "MOT" bit, and instead assumes each
> > > + * message includes a START, ADDRESS and STOP. Neither does it
> > > + * account for additional random variables such as clock stretching.
> > > + */
> > > +static int drm_dp_i2c_msg_len(const struct drm_dp_aux_msg *msg,
> > > +			      int i2c_speed_khz)
> > > +{
> > > +	return (I2C_START_LEN + I2C_ADDR_LEN + msg->size * I2C_DATA_LEN +
> > > +		I2C_STOP_LEN) * 1000 / i2c_speed_khz;
> > 
> > This doesn't seem to compute the lenght, but the time a transfer takes, in
> > usec (if I haven't screwed up my numbers ...). Also DIV_ROUND_DOWN to make
> > the defensiveness clear?
> 
> Well it started out as length, and then I figured I'll just shove the
> i2c->aux unit conversion in here. It's a length in time if you will ;)
> 
> And it should rather be DIV_ROUND_UP() since we want the "maximum" here.

Hm right, mixed that up.

> > > +}
> > > +
> > > +/*
> > > + * Deterine how many retries should be attempted to successfully transfer
> > 
> > Deteri_m_e.
> > 
> > > + * the specified message, based on the estimated durations of the
> > > + * i2c and AUX transfers.
> > > + */
> > > +static int drm_dp_i2c_retry_count(const struct drm_dp_aux_msg *msg,
> > > +			      int i2c_speed_khz)
> > > +{
> > > +	int aux_len = drm_dp_aux_req_len(msg) + drm_dp_aux_reply_len(msg);
> > > +	int i2c_len = drm_dp_i2c_msg_len(msg, i2c_speed_khz);
> > > +
> > > +	return DIV_ROUND_UP(i2c_len, aux_len + AUX_RETRY_INTERVAL);
> > 
> > So assuming I get things right i2c_len is actually i2c_time_us, but
> > aux_len is still just a length (and since the bus runs at 2MHz it doesn't
> > seem to just add up correctly).
> 
> AUX bitrate is 1MHz

Right I remembered only that we need to put in the clock divider for 2MHz
for intel dp aux transfers and got confused by that hw requirement.

Still a comment that the aux clock is 1MHz plus renaming aux_len to
aux_time_usecs would be great for clarity.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/3] drm/dp: Adjust i2c-over-aux retry count based on message size and i2c bus speed

[Daniel Vetter]

On Tue, Sep 01, 2015 at 05:11:45PM +0200, Daniel Vetter wrote:
> On Tue, Sep 01, 2015 at 02:13:01PM +0300, Ville Syrjälä wrote:
> > On Tue, Sep 01, 2015 at 11:14:43AM +0200, Daniel Vetter wrote:
> > > On Wed, Aug 26, 2015 at 10:55:06PM +0300, ville.syrjala@linux.intel.com wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > Calculate the number of retries we should do for each i2c-over-aux
> > > > message based on the time it takes to perform the i2c transfer vs. the
> > > > aux transfer. We assume the shortest possible length for the aux
> > > > transfer, and the longest possible (exluding clock stretching) for the
> > > > i2c transfer.
> > > > 
> > > > The DP spec has some examples on how to calculate this, but we don't
> > > > calculate things quite the same way. The spec doesn't account for the
> > > > retry interval (assumes immediate retry on defer), and doesn't assume
> > > > the best/worst case behaviour as we do.
> > > > 
> > > > Note that currently we assume 10 kHz speed for the i2c bus. Some real
> > > > world devices (eg. some Apple DP->VGA dongle) fails with less than 16
> > > > retries. and that would correspond to something close to 15 kHz (with
> > > > our method of calculating things) But let's just go for 10 kHz to be
> > > > on the safe side. Ideally we should query/set the i2c bus speed via
> > > > DPCD but for now this should at leaast remove the regression from the
> > > > 1->16 byte trasnfer size change. And of course if the sink completes
> > > > the transfer quicker this shouldn't slow things down since we don't
> > > > change the interval between retries.
> > > > 
> > > > I did a few experiments with a DP->DVI dongle I have that allows you
> > > > to change the i2c bus speed. Here are the results of me changing the
> > > > actual bus speed and the assumed bus speed and seeing when we start
> > > > to fail the operation:
> > > > 
> > > > actual i2c khz          assumed i2c khz         max retries
> > > > 1                       1 ok -> 2 fail          211 ok -> 106 fail
> > > > 5                       8 ok -> 9 fail          27 ok -> 24 fail
> > > > 10                      17 ok -> 18 fail        13 ok -> 12 fail
> > > > 100                     210 ok -> 211 fail      2 ok -> 1 fail
> > > > 
> > > > So based on that we have a fairly decent safety margin baked into
> > > > the formula to calculate the max number of retries.
> > > > 
> > > > Fixes a regression with some DP dongles from:
> > > > commit 1d002fa720738bcd0bddb9178e9ea0773288e1dd
> > > > Author: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
> > > > Date:   Tue Feb 10 18:38:08 2015 +0000
> > > > 
> > > >     drm/dp: Use large transactions for I2C over AUX
> > > > 
> > > > v2: Use best case for AUX and worst case for i2c (Simon Farnsworth)
> > > >     Add a define our AUX retry interval and account for it
> > > > 
> > > > Cc: Simon Farnsworth <simon.farnsworth@onelan.com>
> > > > Cc: moosotc@gmail.com
> > > > Tested-by: moosotc@gmail.com
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91451
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_dp_helper.c | 81 ++++++++++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 79 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > > > index 7069e54..23b9fcc 100644
> > > > --- a/drivers/gpu/drm/drm_dp_helper.c
> > > > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > > > @@ -424,6 +424,78 @@ static u32 drm_dp_i2c_functionality(struct i2c_adapter *adapter)
> > > >  	       I2C_FUNC_10BIT_ADDR;
> > > >  }
> > > >  
> > > > +#define AUX_PRECHARGE_LEN 10 /* 10 to 16 */
> > > > +#define AUX_SYNC_LEN (16 + 4) /* preamble + AUX_SYNC_END */
> > > > +#define AUX_STOP_LEN 4
> > > > +#define AUX_CMD_LEN 4
> > > > +#define AUX_ADDRESS_LEN 20
> > > > +#define AUX_REPLY_PAD_LEN 4
> > > > +#define AUX_LENGTH_LEN 8
> > > > +
> > > > +/*
> > > > + * Calculate the length of the AUX request/reply. Gives the "best"
> > > > + * case estimate, ie. successful while as short as possible.
> > > > + */
> > > > +static int drm_dp_aux_req_len(const struct drm_dp_aux_msg *msg)
> > > > +{
> > > > +	int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
> > > > +		AUX_CMD_LEN + AUX_ADDRESS_LEN + AUX_LENGTH_LEN;
> > > > +
> > > > +	if ((msg->request & DP_AUX_I2C_READ) == 0)
> > > > +		len += msg->size * 8;
> > > > +
> > > > +	return len;
> > > > +}
> > > > +
> > > > +static int drm_dp_aux_reply_len(const struct drm_dp_aux_msg *msg)
> > > > +{
> > > > +	int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
> > > > +		AUX_CMD_LEN + AUX_REPLY_PAD_LEN;
> > > > +
> > > > +	/*
> > > > +	 * For read we expect what was asked. For writes there will
> > > > +	 * be 0 or 1 data bytes. Assume 0 for the "best" case.
> > > > +	 */
> > > > +	if (msg->request & DP_AUX_I2C_READ)
> > > > +		len += msg->size * 8;
> > > > +
> > > > +	return len;
> > > > +}
> > > > +
> > > > +#define I2C_START_LEN 1
> > > > +#define I2C_STOP_LEN 1
> > > > +#define I2C_ADDR_LEN 9 /* ADDRESS + R/W + ACK/NACK */
> > > > +#define I2C_DATA_LEN 9 /* DATA + ACK/NACK */
> > > > +
> > > > +/*
> > > > + * Calculate the length of the i2c transfer (in AUX clocks)
> > > > + * assuming the i2c bus speed is as specified. Gives the the
> > > > + * "worst" case estimate, ie. successful while as long as possible.
> > > > + * Doesn't account the the "MOT" bit, and instead assumes each
> > > > + * message includes a START, ADDRESS and STOP. Neither does it
> > > > + * account for additional random variables such as clock stretching.
> > > > + */
> > > > +static int drm_dp_i2c_msg_len(const struct drm_dp_aux_msg *msg,
> > > > +			      int i2c_speed_khz)
> > > > +{
> > > > +	return (I2C_START_LEN + I2C_ADDR_LEN + msg->size * I2C_DATA_LEN +
> > > > +		I2C_STOP_LEN) * 1000 / i2c_speed_khz;
> > > 
> > > This doesn't seem to compute the lenght, but the time a transfer takes, in
> > > usec (if I haven't screwed up my numbers ...). Also DIV_ROUND_DOWN to make
> > > the defensiveness clear?
> > 
> > Well it started out as length, and then I figured I'll just shove the
> > i2c->aux unit conversion in here. It's a length in time if you will ;)
> > 
> > And it should rather be DIV_ROUND_UP() since we want the "maximum" here.
> 
> Hm right, mixed that up.
> 
> > > > +}
> > > > +
> > > > +/*
> > > > + * Deterine how many retries should be attempted to successfully transfer
> > > 
> > > Deteri_m_e.
> > > 
> > > > + * the specified message, based on the estimated durations of the
> > > > + * i2c and AUX transfers.
> > > > + */
> > > > +static int drm_dp_i2c_retry_count(const struct drm_dp_aux_msg *msg,
> > > > +			      int i2c_speed_khz)
> > > > +{
> > > > +	int aux_len = drm_dp_aux_req_len(msg) + drm_dp_aux_reply_len(msg);
> > > > +	int i2c_len = drm_dp_i2c_msg_len(msg, i2c_speed_khz);
> > > > +
> > > > +	return DIV_ROUND_UP(i2c_len, aux_len + AUX_RETRY_INTERVAL);
> > > 
> > > So assuming I get things right i2c_len is actually i2c_time_us, but
> > > aux_len is still just a length (and since the bus runs at 2MHz it doesn't
> > > seem to just add up correctly).
> > 
> > AUX bitrate is 1MHz
> 
> Right I remembered only that we need to put in the clock divider for 2MHz
> for intel dp aux transfers and got confused by that hw requirement.
> 
> Still a comment that the aux clock is 1MHz plus renaming aux_len to
> aux_time_usecs would be great for clarity.

Forgot to add: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> for the
entire series if you rename the functions/variables where approriate to
make it clear that you compute usecs and add a comment about the 1MHz aux
clock as reminder for people like me here in this patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/3] drm/dp: Adjust i2c-over-aux retry count based on message size and i2c bus speed

[Jani Nikula]

On Tue, 01 Sep 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Sep 01, 2015 at 05:11:45PM +0200, Daniel Vetter wrote:
>> On Tue, Sep 01, 2015 at 02:13:01PM +0300, Ville Syrjälä wrote:
>> > On Tue, Sep 01, 2015 at 11:14:43AM +0200, Daniel Vetter wrote:
>> > > On Wed, Aug 26, 2015 at 10:55:06PM +0300, ville.syrjala@linux.intel.com wrote:
>> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > > > 
>> > > > Calculate the number of retries we should do for each i2c-over-aux
>> > > > message based on the time it takes to perform the i2c transfer vs. the
>> > > > aux transfer. We assume the shortest possible length for the aux
>> > > > transfer, and the longest possible (exluding clock stretching) for the
>> > > > i2c transfer.
>> > > > 
>> > > > The DP spec has some examples on how to calculate this, but we don't
>> > > > calculate things quite the same way. The spec doesn't account for the
>> > > > retry interval (assumes immediate retry on defer), and doesn't assume
>> > > > the best/worst case behaviour as we do.
>> > > > 
>> > > > Note that currently we assume 10 kHz speed for the i2c bus. Some real
>> > > > world devices (eg. some Apple DP->VGA dongle) fails with less than 16
>> > > > retries. and that would correspond to something close to 15 kHz (with
>> > > > our method of calculating things) But let's just go for 10 kHz to be
>> > > > on the safe side. Ideally we should query/set the i2c bus speed via
>> > > > DPCD but for now this should at leaast remove the regression from the
>> > > > 1->16 byte trasnfer size change. And of course if the sink completes
>> > > > the transfer quicker this shouldn't slow things down since we don't
>> > > > change the interval between retries.
>> > > > 
>> > > > I did a few experiments with a DP->DVI dongle I have that allows you
>> > > > to change the i2c bus speed. Here are the results of me changing the
>> > > > actual bus speed and the assumed bus speed and seeing when we start
>> > > > to fail the operation:
>> > > > 
>> > > > actual i2c khz          assumed i2c khz         max retries
>> > > > 1                       1 ok -> 2 fail          211 ok -> 106 fail
>> > > > 5                       8 ok -> 9 fail          27 ok -> 24 fail
>> > > > 10                      17 ok -> 18 fail        13 ok -> 12 fail
>> > > > 100                     210 ok -> 211 fail      2 ok -> 1 fail
>> > > > 
>> > > > So based on that we have a fairly decent safety margin baked into
>> > > > the formula to calculate the max number of retries.
>> > > > 
>> > > > Fixes a regression with some DP dongles from:
>> > > > commit 1d002fa720738bcd0bddb9178e9ea0773288e1dd
>> > > > Author: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
>> > > > Date:   Tue Feb 10 18:38:08 2015 +0000
>> > > > 
>> > > >     drm/dp: Use large transactions for I2C over AUX
>> > > > 
>> > > > v2: Use best case for AUX and worst case for i2c (Simon Farnsworth)
>> > > >     Add a define our AUX retry interval and account for it
>> > > > 
>> > > > Cc: Simon Farnsworth <simon.farnsworth@onelan.com>
>> > > > Cc: moosotc@gmail.com
>> > > > Tested-by: moosotc@gmail.com
>> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91451
>> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > > > ---
>> > > >  drivers/gpu/drm/drm_dp_helper.c | 81 ++++++++++++++++++++++++++++++++++++++++-
>> > > >  1 file changed, 79 insertions(+), 2 deletions(-)
>> > > > 
>> > > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
>> > > > index 7069e54..23b9fcc 100644
>> > > > --- a/drivers/gpu/drm/drm_dp_helper.c
>> > > > +++ b/drivers/gpu/drm/drm_dp_helper.c
>> > > > @@ -424,6 +424,78 @@ static u32 drm_dp_i2c_functionality(struct i2c_adapter *adapter)
>> > > >  	       I2C_FUNC_10BIT_ADDR;
>> > > >  }
>> > > >  
>> > > > +#define AUX_PRECHARGE_LEN 10 /* 10 to 16 */
>> > > > +#define AUX_SYNC_LEN (16 + 4) /* preamble + AUX_SYNC_END */
>> > > > +#define AUX_STOP_LEN 4
>> > > > +#define AUX_CMD_LEN 4
>> > > > +#define AUX_ADDRESS_LEN 20
>> > > > +#define AUX_REPLY_PAD_LEN 4
>> > > > +#define AUX_LENGTH_LEN 8
>> > > > +
>> > > > +/*
>> > > > + * Calculate the length of the AUX request/reply. Gives the "best"
>> > > > + * case estimate, ie. successful while as short as possible.
>> > > > + */
>> > > > +static int drm_dp_aux_req_len(const struct drm_dp_aux_msg *msg)
>> > > > +{
>> > > > +	int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
>> > > > +		AUX_CMD_LEN + AUX_ADDRESS_LEN + AUX_LENGTH_LEN;
>> > > > +
>> > > > +	if ((msg->request & DP_AUX_I2C_READ) == 0)
>> > > > +		len += msg->size * 8;
>> > > > +
>> > > > +	return len;
>> > > > +}
>> > > > +
>> > > > +static int drm_dp_aux_reply_len(const struct drm_dp_aux_msg *msg)
>> > > > +{
>> > > > +	int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
>> > > > +		AUX_CMD_LEN + AUX_REPLY_PAD_LEN;
>> > > > +
>> > > > +	/*
>> > > > +	 * For read we expect what was asked. For writes there will
>> > > > +	 * be 0 or 1 data bytes. Assume 0 for the "best" case.
>> > > > +	 */
>> > > > +	if (msg->request & DP_AUX_I2C_READ)
>> > > > +		len += msg->size * 8;
>> > > > +
>> > > > +	return len;
>> > > > +}
>> > > > +
>> > > > +#define I2C_START_LEN 1
>> > > > +#define I2C_STOP_LEN 1
>> > > > +#define I2C_ADDR_LEN 9 /* ADDRESS + R/W + ACK/NACK */
>> > > > +#define I2C_DATA_LEN 9 /* DATA + ACK/NACK */
>> > > > +
>> > > > +/*
>> > > > + * Calculate the length of the i2c transfer (in AUX clocks)
>> > > > + * assuming the i2c bus speed is as specified. Gives the the
>> > > > + * "worst" case estimate, ie. successful while as long as possible.
>> > > > + * Doesn't account the the "MOT" bit, and instead assumes each
>> > > > + * message includes a START, ADDRESS and STOP. Neither does it
>> > > > + * account for additional random variables such as clock stretching.
>> > > > + */
>> > > > +static int drm_dp_i2c_msg_len(const struct drm_dp_aux_msg *msg,
>> > > > +			      int i2c_speed_khz)
>> > > > +{
>> > > > +	return (I2C_START_LEN + I2C_ADDR_LEN + msg->size * I2C_DATA_LEN +
>> > > > +		I2C_STOP_LEN) * 1000 / i2c_speed_khz;
>> > > 
>> > > This doesn't seem to compute the lenght, but the time a transfer takes, in
>> > > usec (if I haven't screwed up my numbers ...). Also DIV_ROUND_DOWN to make
>> > > the defensiveness clear?
>> > 
>> > Well it started out as length, and then I figured I'll just shove the
>> > i2c->aux unit conversion in here. It's a length in time if you will ;)
>> > 
>> > And it should rather be DIV_ROUND_UP() since we want the "maximum" here.
>> 
>> Hm right, mixed that up.
>> 
>> > > > +}
>> > > > +
>> > > > +/*
>> > > > + * Deterine how many retries should be attempted to successfully transfer
>> > > 
>> > > Deteri_m_e.
>> > > 
>> > > > + * the specified message, based on the estimated durations of the
>> > > > + * i2c and AUX transfers.
>> > > > + */
>> > > > +static int drm_dp_i2c_retry_count(const struct drm_dp_aux_msg *msg,
>> > > > +			      int i2c_speed_khz)
>> > > > +{
>> > > > +	int aux_len = drm_dp_aux_req_len(msg) + drm_dp_aux_reply_len(msg);
>> > > > +	int i2c_len = drm_dp_i2c_msg_len(msg, i2c_speed_khz);
>> > > > +
>> > > > +	return DIV_ROUND_UP(i2c_len, aux_len + AUX_RETRY_INTERVAL);
>> > > 
>> > > So assuming I get things right i2c_len is actually i2c_time_us, but
>> > > aux_len is still just a length (and since the bus runs at 2MHz it doesn't
>> > > seem to just add up correctly).
>> > 
>> > AUX bitrate is 1MHz
>> 
>> Right I remembered only that we need to put in the clock divider for 2MHz
>> for intel dp aux transfers and got confused by that hw requirement.
>> 
>> Still a comment that the aux clock is 1MHz plus renaming aux_len to
>> aux_time_usecs would be great for clarity.
>
> Forgot to add: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> for the
> entire series if you rename the functions/variables where approriate to
> make it clear that you compute usecs and add a comment about the 1MHz aux
> clock as reminder for people like me here in this patch.

The series pushed to topic/drm-fixes, thanks for the patches and review.

BR,
Jani.




> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[PATCH v3 2/3] drm/dp: Adjust i2c-over-aux retry count based on message size and i2c bus speed

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Calculate the number of retries we should do for each i2c-over-aux
message based on the time it takes to perform the i2c transfer vs. the
aux transfer. We assume the shortest possible length for the aux
transfer, and the longest possible (exluding clock stretching) for the
i2c transfer.

The DP spec has some examples on how to calculate this, but we don't
calculate things quite the same way. The spec doesn't account for the
retry interval (assumes immediate retry on defer), and doesn't assume
the best/worst case behaviour as we do.

Note that currently we assume 10 kHz speed for the i2c bus. Some real
world devices (eg. some Apple DP->VGA dongle) fails with less than 16
retries. and that would correspond to something close to 15 kHz (with
our method of calculating things) But let's just go for 10 kHz to be
on the safe side. Ideally we should query/set the i2c bus speed via
DPCD but for now this should at leaast remove the regression from the
1->16 byte trasnfer size change. And of course if the sink completes
the transfer quicker this shouldn't slow things down since we don't
change the interval between retries.

I did a few experiments with a DP->DVI dongle I have that allows you
to change the i2c bus speed. Here are the results of me changing the
actual bus speed and the assumed bus speed and seeing when we start
to fail the operation:

actual i2c khz          assumed i2c khz         max retries
1                       1 ok -> 2 fail          211 ok -> 106 fail
5                       8 ok -> 9 fail          27 ok -> 24 fail
10                      17 ok -> 18 fail        13 ok -> 12 fail
100                     210 ok -> 211 fail      2 ok -> 1 fail

So based on that we have a fairly decent safety margin baked into
the formula to calculate the max number of retries.

Fixes a regression with some DP dongles from:
commit 1d002fa720738bcd0bddb9178e9ea0773288e1dd
Author: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
Date:   Tue Feb 10 18:38:08 2015 +0000

    drm/dp: Use large transactions for I2C over AUX

v2: Use best case for AUX and worst case for i2c (Simon Farnsworth)
    Add a define our AUX retry interval and account for it
v3: Make everything usecs to avoid confusion about units (Daniel)
    Add a comment reminding people about the AUX bitrate (Daniel)
    Use DIV_ROUND_UP() since we're after the "worst" case for i2c

Cc: Simon Farnsworth <simon.farnsworth@onelan.com>
Cc: moosotc@gmail.com
Tested-by: moosotc@gmail.com
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91451
Reviewed-by: Simon Farnsworth <simon.farnsworth@onelan.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 84 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 82 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 7069e54..214a4c6 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -424,6 +424,81 @@ static u32 drm_dp_i2c_functionality(struct i2c_adapter *adapter)
 	       I2C_FUNC_10BIT_ADDR;
 }
 
+#define AUX_PRECHARGE_LEN 10 /* 10 to 16 */
+#define AUX_SYNC_LEN (16 + 4) /* preamble + AUX_SYNC_END */
+#define AUX_STOP_LEN 4
+#define AUX_CMD_LEN 4
+#define AUX_ADDRESS_LEN 20
+#define AUX_REPLY_PAD_LEN 4
+#define AUX_LENGTH_LEN 8
+
+/*
+ * Calculate the duration of the AUX request/reply in usec. Gives the
+ * "best" case estimate, ie. successful while as short as possible.
+ */
+static int drm_dp_aux_req_duration(const struct drm_dp_aux_msg *msg)
+{
+	int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
+		AUX_CMD_LEN + AUX_ADDRESS_LEN + AUX_LENGTH_LEN;
+
+	if ((msg->request & DP_AUX_I2C_READ) == 0)
+		len += msg->size * 8;
+
+	return len;
+}
+
+static int drm_dp_aux_reply_duration(const struct drm_dp_aux_msg *msg)
+{
+	int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
+		AUX_CMD_LEN + AUX_REPLY_PAD_LEN;
+
+	/*
+	 * For read we expect what was asked. For writes there will
+	 * be 0 or 1 data bytes. Assume 0 for the "best" case.
+	 */
+	if (msg->request & DP_AUX_I2C_READ)
+		len += msg->size * 8;
+
+	return len;
+}
+
+#define I2C_START_LEN 1
+#define I2C_STOP_LEN 1
+#define I2C_ADDR_LEN 9 /* ADDRESS + R/W + ACK/NACK */
+#define I2C_DATA_LEN 9 /* DATA + ACK/NACK */
+
+/*
+ * Calculate the length of the i2c transfer in usec, assuming
+ * the i2c bus speed is as specified. Gives the the "worst"
+ * case estimate, ie. successful while as long as possible.
+ * Doesn't account the the "MOT" bit, and instead assumes each
+ * message includes a START, ADDRESS and STOP. Neither does it
+ * account for additional random variables such as clock stretching.
+ */
+static int drm_dp_i2c_msg_duration(const struct drm_dp_aux_msg *msg,
+				   int i2c_speed_khz)
+{
+	/* AUX bitrate is 1MHz, i2c bitrate as specified */
+	return DIV_ROUND_UP((I2C_START_LEN + I2C_ADDR_LEN +
+			     msg->size * I2C_DATA_LEN +
+			     I2C_STOP_LEN) * 1000, i2c_speed_khz);
+}
+
+/*
+ * Deterine how many retries should be attempted to successfully transfer
+ * the specified message, based on the estimated durations of the
+ * i2c and AUX transfers.
+ */
+static int drm_dp_i2c_retry_count(const struct drm_dp_aux_msg *msg,
+			      int i2c_speed_khz)
+{
+	int aux_time_us = drm_dp_aux_req_duration(msg) +
+		drm_dp_aux_reply_duration(msg);
+	int i2c_time_us = drm_dp_i2c_msg_duration(msg, i2c_speed_khz);
+
+	return DIV_ROUND_UP(i2c_time_us, aux_time_us + AUX_RETRY_INTERVAL);
+}
+
 /*
  * Transfer a single I2C-over-AUX message and handle various error conditions,
  * retrying the transaction as appropriate.  It is assumed that the
@@ -436,13 +511,18 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 {
 	unsigned int retry, defer_i2c;
 	int ret;
-
 	/*
 	 * DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source device
 	 * is required to retry at least seven times upon receiving AUX_DEFER
 	 * before giving up the AUX transaction.
+	 *
+	 * We also try to account for the i2c bus speed.
+	 * FIXME currently assumes 10 kHz as some real world devices seem
+	 * to require it. We should query/set the speed via DPCD if supported.
 	 */
-	for (retry = 0, defer_i2c = 0; retry < (7 + defer_i2c); retry++) {
+	int max_retries = max(7, drm_dp_i2c_retry_count(msg, 10));
+
+	for (retry = 0, defer_i2c = 0; retry < (max_retries + defer_i2c); retry++) {
 		mutex_lock(&aux->hw_mutex);
 		ret = aux->transfer(aux, msg);
 		mutex_unlock(&aux->hw_mutex);
-- 
2.4.6

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

Re: [PATCH v3 2/3] drm/dp: Adjust i2c-over-aux retry count based on message size and i2c bus speed

[moosotc]

ville.syrjala@linux.intel.com writes:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Calculate the number of retries we should do for each i2c-over-aux
> message based on the time it takes to perform the i2c transfer vs. the
> aux transfer. We assume the shortest possible length for the aux
> transfer, and the longest possible (exluding clock stretching) for the
> i2c transfer.
>
> The DP spec has some examples on how to calculate this, but we don't
> calculate things quite the same way. The spec doesn't account for the
> retry interval (assumes immediate retry on defer), and doesn't assume
> the best/worst case behaviour as we do.
>
> Note that currently we assume 10 kHz speed for the i2c bus. Some real
> world devices (eg. some Apple DP->VGA dongle) fails with less than 16
> retries. and that would correspond to something close to 15 kHz (with
> our method of calculating things) But let's just go for 10 kHz to be
> on the safe side. Ideally we should query/set the i2c bus speed via
> DPCD but for now this should at leaast remove the regression from the
                                     ^ extra a
> 1->16 byte trasnfer size change. And of course if the sink completes
> the transfer quicker this shouldn't slow things down since we don't
> change the interval between retries.
>
> I did a few experiments with a DP->DVI dongle I have that allows you
> to change the i2c bus speed. Here are the results of me changing the
> actual bus speed and the assumed bus speed and seeing when we start
> to fail the operation:
>
> actual i2c khz          assumed i2c khz         max retries
> 1                       1 ok -> 2 fail          211 ok -> 106 fail
> 5                       8 ok -> 9 fail          27 ok -> 24 fail
> 10                      17 ok -> 18 fail        13 ok -> 12 fail
> 100                     210 ok -> 211 fail      2 ok -> 1 fail
>
> So based on that we have a fairly decent safety margin baked into

[..snip..]

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

[PATCH 3/3] drm/dp: Add dp_aux_i2c_speed_khz module param to set the assume i2c bus speed

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

To help with debugging i2c-over-aux issues, add a module parameter than
can be used to tweak the assumed i2c bus speed, and thus the maximum
number of retries we will do for each aux message.

Cc: Simon Farnsworth <simon.farnsworth@onelan.com>
Cc: moosotc@gmail.com
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 23b9fcc..ee5cd86 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -497,6 +497,15 @@ static int drm_dp_i2c_retry_count(const struct drm_dp_aux_msg *msg,
 }
 
 /*
+ * FIXME currently assumes 10 kHz as some real world devices seem
+ * to require it. We should query/set the speed via DPCD if supported.
+ */
+static int dp_aux_i2c_speed_khz __read_mostly = 10;
+module_param_unsafe(dp_aux_i2c_speed_khz, int, 0644);
+MODULE_PARM_DESC(dp_aux_i2c_speed_khz,
+		 "Assumed speed of the i2c bus in kHz, (1-400, default 10)");
+
+/*
  * Transfer a single I2C-over-AUX message and handle various error conditions,
  * retrying the transaction as appropriate.  It is assumed that the
  * aux->transfer function does not modify anything in the msg other than the
@@ -514,10 +523,8 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 	 * before giving up the AUX transaction.
 	 *
 	 * We also try to account for the i2c bus speed.
-	 * FIXME currently assumes 10 kHz as some real world devices seem
-	 * to require it. We should query/set the speed via DPCD if supported.
 	 */
-	int max_retries = max(7, drm_dp_i2c_retry_count(msg, 10));
+	int max_retries = max(7, drm_dp_i2c_retry_count(msg, dp_aux_i2c_speed_khz));
 
 	for (retry = 0, defer_i2c = 0; retry < (max_retries + defer_i2c); retry++) {
 		mutex_lock(&aux->hw_mutex);
-- 
2.4.6

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

Re: [PATCH 3/3] drm/dp: Add dp_aux_i2c_speed_khz module param to set the assume i2c bus speed

[Simon Farnsworth]

All three patches are:

Reviewed-by: Simon Farnsworth <simon.farnsworth@onelan.com>

I'd consider making the upper limit of AUX_RETRY_INTERVAL a define as well, but 
that's just nit-picking and not worth respinning the patches.

Simon

On Wednesday 26 Aug 2015 22:55:07 ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> To help with debugging i2c-over-aux issues, add a module parameter than
> can be used to tweak the assumed i2c bus speed, and thus the maximum
> number of retries we will do for each aux message.
> 
> Cc: Simon Farnsworth <simon.farnsworth@onelan.com>
> Cc: moosotc@gmail.com
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c
> b/drivers/gpu/drm/drm_dp_helper.c index 23b9fcc..ee5cd86 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -497,6 +497,15 @@ static int drm_dp_i2c_retry_count(const struct
> drm_dp_aux_msg *msg, }
> 
>  /*
> + * FIXME currently assumes 10 kHz as some real world devices seem
> + * to require it. We should query/set the speed via DPCD if supported.
> + */
> +static int dp_aux_i2c_speed_khz __read_mostly = 10;
> +module_param_unsafe(dp_aux_i2c_speed_khz, int, 0644);
> +MODULE_PARM_DESC(dp_aux_i2c_speed_khz,
> +		 "Assumed speed of the i2c bus in kHz, (1-400, default 10)");
> +
> +/*
>   * Transfer a single I2C-over-AUX message and handle various error
> conditions, * retrying the transaction as appropriate.  It is assumed that
> the * aux->transfer function does not modify anything in the msg other than
> the @@ -514,10 +523,8 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux
> *aux, struct drm_dp_aux_msg *msg) * before giving up the AUX transaction.
>  	 *
>  	 * We also try to account for the i2c bus speed.
> -	 * FIXME currently assumes 10 kHz as some real world devices seem
> -	 * to require it. We should query/set the speed via DPCD if supported.
>  	 */
> -	int max_retries = max(7, drm_dp_i2c_retry_count(msg, 10));
> +	int max_retries = max(7, drm_dp_i2c_retry_count(msg,
> dp_aux_i2c_speed_khz));
> 
>  	for (retry = 0, defer_i2c = 0; retry < (max_retries + defer_i2c); 
retry++)
> { mutex_lock(&aux->hw_mutex);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel