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 AUXv2: 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