On Tue, 01 Sep 2015, Daniel Vetter <daniel@xxxxxxxx> wrote: > On Wed, Aug 26, 2015 at 10:55:06PM +0300, ville.syrjala@xxxxxxxxxxxxxxx wrote: >> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> >> >> 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@xxxxxxxxxxxx> >> 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@xxxxxxxxxx> >> Cc: moosotc@xxxxxxxxx >> Tested-by: moosotc@xxxxxxxxx >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91451 >> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> >> --- >> 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. Determine. :) > >> + * 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@xxxxxxxxxxxxxxxxxxxxx >> http://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
