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

[Daniel Vetter <daniel@ffwll.ch>]

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