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);
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);
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.
I'd change this to 10 - see below for reasoning.
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.
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.
______________________________________________________________________ This email has been scanned by the Symantec Email Security.cloud service. For more information please visit http://www.symanteccloud.com ______________________________________________________________________
On Wed, Aug 26, 2015 at 12:11:56PM +0100, simon.farnsworth@onelan.com wrote:
Yeah I did consider making it more accurate but then thought it's easier to just assume worst case behaviour.
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.
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:
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);
On Wed, Aug 26, 2015 at 10:55:06PM +0300, ville.syrjala@linux.intel.com wrote:
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?
Deteri_m_e.
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
On Tue, 01 Sep 2015, Daniel Vetter daniel@ffwll.ch wrote:
Determine. :)
On Tue, Sep 01, 2015 at 11:14:43AM +0200, Daniel Vetter wrote:
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.
AUX bitrate is 1MHz
On Tue, Sep 01, 2015 at 02:13:01PM +0300, Ville Syrjälä wrote:
Hm right, mixed that up.
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
On Tue, Sep 01, 2015 at 05:11:45PM +0200, Daniel Vetter wrote:
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
On Tue, 01 Sep 2015, Daniel Vetter daniel@ffwll.ch wrote:
The series pushed to topic/drm-fixes, thanks for the patches and review.
BR, Jani.
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);
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);
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:
retry++)
{ mutex_lock(&aux->hw_mutex);
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Jani Nikula -
moosotc＠gmail.com -
Simon Farnsworth -
simon.farnsworth＠onelan.com -
Ville Syrjälä -
ville.syrjala＠linux.intel.com