[PATCH] drm/dp: Use large transactions for I2C over AUX

[Ville Syrjälä]

On Mon, Jan 26, 2015 at 04:39:29PM +0000, Simon Farnsworth wrote:
> On Monday 26 January 2015 18:11:01 Ville Syrjälä wrote:
> > On Mon, Jan 26, 2015 at 03:47:13PM +0000, Simon Farnsworth wrote:
> > > On Monday 26 January 2015 17:33:35 Ville Syrjälä wrote:
> > > > On Mon, Jan 26, 2015 at 03:22:48PM +0000, Simon Farnsworth wrote:
> > > > > DisplayPort to DVI-D Dual Link adapters designed by Bizlink have bugs in
> > > > > their I2C over AUX implementation. They work fine with Windows, but fail
> > > > > with Linux.
> > > > > 
> > > > > It turns out that they cannot keep an I2C transaction open unless the
> > > > > previous read was 16 bytes; shorter reads can only be followed by a zero
> > > > > byte transfer ending the I2C transaction.
> > > > > 
> > > > > Copy Windows's behaviour, and read 16 bytes at a time. If we get a short
> > > > > reply, assume that there's a hardware bottleneck, and shrink our read size
> > > > > to match.
> > > > > 
> > > > > Signed-off-by: Simon Farnsworth <simon.farnsworth at onelan.co.uk>
> > > > > ---
> > > > > 
> > > > > v2 changes, after feedback from Thierry and Ville:
> > > > > 
> > > > >  * Handle short replies. I've decided (arbitrarily) that a short reply
> > > > >    results in us dropping back to the newly chosen size for the rest of this
> > > > >    I2C transaction. Thus, given an attempt to read the first 16 bytes of
> > > > >    EDID, and a sink that only does 4 bytes of buffering, we will see the
> > > > >    following AUX transfers for the EDID read (after address is set):
> > > > > 
> > > > >    <set address, block etc>
> > > > >    Read 16 bytes from I2C over AUX.
> > > > >    Reply with 4 bytes
> > > > >    Read 4 bytes
> > > > >    Reply with 4 bytes
> > > > >    Read 4 bytes
> > > > >    Reply with 4 bytes
> > > > >    Read 4 bytes
> > > > >    Reply with 4 bytes
> > > > >    <end I2C transaction>
> > > > 
> > > > I think that's agaisnt the spec. IIRC you have to keep repeating the
> > > > same transaction (meaning address/len are unchanged) until all the data
> > > > was transferred.
> > > >
> > > Do you have a spec reference against the DisplayPort 1.1a (last public
> > > version) spec? My chosen behaviour matches Table 2-50 in the 1.1a spec.
> > 
> > In my copy if DP v1.1 the example in 2-50 just keeps repeating w/ 16 bytes.
> > So doesn't match what you do. And that's unchanged in v1.2.
> >
> Yes, looks like I misread the table; I was more thinking about "what are the
> semantics of a 4 byte reply to a 16 byte read?" when I read that bit of the
> spec.
> 
> > DP v1.2 has some extra clarifications for this stuff:
> > "2.7.7 I2C-overAUX Transaction Clarifications and Implementation Rules
> > 
> >  2.7.7.1.6.4 Upon the Reply of I2C_ACK|AUX_ACK Followed by the Total Number of Data
> >  Bytes Fewer than LEN+1, to a Request Transaction with MOT Bit Set Either to 0 or 1
> > 
> >  The Source device must:
> >  o Repeat the identical I2C-read-over-AUX transaction with the updated
> >    LEN value equal to the original LEN value minus the total number of data
> >    bytes received so far,
> >  o Repeat the identical I2C-read-over-AUX transaction with the same LEN
> >    value as the original value, or,
> >  o Issue an address-only I2C-over-AUX with MOT bit set to 0 to prompt I2C STOP
> >    to terminate the current I2C-read-over-AUX transaction.
> >  It should be noted that when the Source device repeats the same I2C-read-over-AUX
> >  transaction with the same LEN value as the original value, the Sink device is
> >  likely to read more data bytes than the Source device needs."
> > 
> So what would be the best implementation strategy for Linux? Bear in mind
> that I don't have the 1.2 spec, nor do I have any devices which give a
> partial response to a 16 byte read, so I'm coding blind here, based solely
> on the information people are giving me.
> 
> The simplest strategy would be to keep repeating with 16 byte reads all the
> time, but Thierry's original comment and response to the first patch have
> suggested that that's a performance problem waiting to happen, hence the
> effort to find a better strategy.
> 
> My strategy was intended to spot that there's probably a FIFO in the middle
> that's too small, and reduce the transfer size to avoid overflowing the
> FIFO. I could make it more complex by remembering that the last read was a
> partial read, finishing it off by reducing the read size, then sticking to
> the new smaller size, but that's added complexity and I'm not sure of its
> value.
> 
> Should I simply stick to 16 byte reads, and cope with getting back less data
> than I want, or do you think we need the more complex strategy that complies
> with DP 1.2?

So I've been experimenting a bit with various dongles here, and sadly I've
not managed to get any one of them to return short reads :(

I did find one that allows changing the speed of the i2c bus, but even if
I reduce it to 1khz there are no short reads, just a lot more defers. The
dongle in question has OUI 001cf8.

However the good news is that EDID reads seem to get faster across the
board with 16 byte messages. How much faster depends on the dongle.

Here are my measurements how long it took to read a single EDID block:
 DP->DVI (OUI 001cf8):	40ms -> 35ms
 DP->VGA (OUI 0022b9):	45ms -> 38ms
 Zotac DP->2xHDMI:	25ms ->  4ms


Oh and this is how I mangled my drm_dp_i2c_xfer():
transferred = 0;
while (msgs[i].len > transferred) {
	msg.buffer = msgs[i].buf + transferred;
	msg.size = min_t(unsigned int, drm_dp_i2c_msg_size,
			 msgs[i].len - transferred);
	err = drm_dp_i2c_do_msg(aux, &msg);
	if (err < 0)
		break;
	WARN_ON(err == 0);
	transferred += err;
}

I made the msg size configurable via a module param just to help me test
this stuff, but I'm thinking we might want to upstream that just to make
it easier to try smaller message sizes if/when people encounter problematic
sinks/dongles.

-- 
Ville Syrjälä
Intel OTC