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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 01 Sep 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Sep 01, 2015 at 05:11:45PM +0200, Daniel Vetter wrote:
>> On Tue, Sep 01, 2015 at 02:13:01PM +0300, Ville Syrjälä wrote:
>> > 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 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;
>> > > 
>> > > 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.
>> 
>> Hm right, mixed that up.
>> 
>> > > > +}
>> > > > +
>> > > > +/*
>> > > > + * 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
>> 
>> 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.
>
> 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.

The series pushed to topic/drm-fixes, thanks for the patches and review.

BR,
Jani.




> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel