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

[<simon.farnsworth@onelan.com>]

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