[PATCH] drm/dp: Adjust i2c-over-aux retry count based on message size and i2c bus speed
Ville Syrjälä
ville.syrjala at linux.intel.com
Wed Aug 26 04:43:25 PDT 2015
On Wed, Aug 26, 2015 at 12:11:56PM +0100, simon.farnsworth at onelan.com wrote:
> On Tuesday 25 August 2015 18:10:14 ville.syrjala at linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala at 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 at 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 at onelan.com>
> > Cc: moosotc at gmail.com
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91451
> > Signed-off-by: Ville Syrjälä <ville.syrjala at 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 */
>
> I'd change this to 10 - see below for reasoning.
>
> > +#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;
> > +}
> > +
>
> 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.
Yeah I did consider making it more accurate but then thought it's easier
to just assume worst case behaviour.
>
> > +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);
> > +}
> > +
>
> 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.
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.
>
> > /*
> > * 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);
> >
>
> ______________________________________________________________________
> This email has been scanned by the Symantec Email Security.cloud service.
> For more information please visit http://www.symanteccloud.com
> ______________________________________________________________________
--
Ville Syrjälä
Intel OTC
More information about the dri-devel
mailing list