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

[moosotc@gmail.com]

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