| Message ID | 1422038438-9323-1-git-send-email-simon.farnsworth@onelan.co.uk (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Jan 23, 2015 at 06:40:38PM +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. Analysis of the > failure state was provided by Datapath Ltd. > > Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk> > --- > Thierry, > > You put in the comment about "decreased performance", back in December 2013; > would you mind testing that this still works with the devices you tested? > > Unfortunately, Bizlink are the only game in town for DP->DVI-DL adapters - > and their firmware is prone to giving up on I2C if we look at it > wrongly. Even Apple's device is Bizlink designed. > > drivers/gpu/drm/drm_dp_helper.c | 13 +++++-------- > 1 file changed, 5 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > index 79968e3..b4a9d4a 100644 > --- a/drivers/gpu/drm/drm_dp_helper.c > +++ b/drivers/gpu/drm/drm_dp_helper.c > @@ -507,16 +507,13 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, > err = drm_dp_i2c_do_msg(aux, &msg); > if (err < 0) > break; > - /* > - * Many hardware implementations support FIFOs larger than a > - * single byte, but it has been empirically determined that > - * transferring data in larger chunks can actually lead to > - * decreased performance. Therefore each message is simply > - * transferred byte-by-byte. > + /* Bizlink designed DP->DVI-D Dual Link adapters require the > + * I2C over AUX packets to be as large as possible. If not, > + * the I2C transactions never succeed. > */ > - for (j = 0; j < msgs[i].len; j++) { > + for (j = 0; j < msgs[i].len; j+=16) { > msg.buffer = msgs[i].buf + j; > - msg.size = 1; > + msg.size = min(16, msgs[i].len - 16); I don't think it's quite this simple. The sink is allowed to ACK partial data for multi-byte messages. The code doesn't handle that. > > err = drm_dp_i2c_do_msg(aux, &msg); > if (err < 0) > -- > 2.1.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Jan 23, 2015 at 09:46:29PM +0200, Ville Syrjälä wrote: > On Fri, Jan 23, 2015 at 06:40:38PM +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. Analysis of the > > failure state was provided by Datapath Ltd. > > > > Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk> > > --- > > Thierry, > > > > You put in the comment about "decreased performance", back in December 2013; > > would you mind testing that this still works with the devices you tested? > > > > Unfortunately, Bizlink are the only game in town for DP->DVI-DL adapters - > > and their firmware is prone to giving up on I2C if we look at it > > wrongly. Even Apple's device is Bizlink designed. > > > > drivers/gpu/drm/drm_dp_helper.c | 13 +++++-------- > > 1 file changed, 5 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > > index 79968e3..b4a9d4a 100644 > > --- a/drivers/gpu/drm/drm_dp_helper.c > > +++ b/drivers/gpu/drm/drm_dp_helper.c > > @@ -507,16 +507,13 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, > > err = drm_dp_i2c_do_msg(aux, &msg); > > if (err < 0) > > break; > > - /* > > - * Many hardware implementations support FIFOs larger than a > > - * single byte, but it has been empirically determined that > > - * transferring data in larger chunks can actually lead to > > - * decreased performance. Therefore each message is simply > > - * transferred byte-by-byte. > > + /* Bizlink designed DP->DVI-D Dual Link adapters require the > > + * I2C over AUX packets to be as large as possible. If not, > > + * the I2C transactions never succeed. > > */ > > - for (j = 0; j < msgs[i].len; j++) { > > + for (j = 0; j < msgs[i].len; j+=16) { > > msg.buffer = msgs[i].buf + j; > > - msg.size = 1; > > + msg.size = min(16, msgs[i].len - 16); > > I don't think it's quite this simple. The sink is allowed to ACK > partial data for multi-byte messages. The code doesn't handle that. Also not all hardware may support transferring 16 bytes at a time. How does that work with these adapters? Does it mean they can't work on DP hardware that can't do 16 byte block transfers? Thierry
On Fri, Jan 23, 2015 at 10:21:09PM +0100, Thierry Reding wrote: > On Fri, Jan 23, 2015 at 09:46:29PM +0200, Ville Syrjälä wrote: > > On Fri, Jan 23, 2015 at 06:40:38PM +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. Analysis of the > > > failure state was provided by Datapath Ltd. > > > > > > Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk> > > > --- > > > Thierry, > > > > > > You put in the comment about "decreased performance", back in December 2013; > > > would you mind testing that this still works with the devices you tested? > > > > > > Unfortunately, Bizlink are the only game in town for DP->DVI-DL adapters - > > > and their firmware is prone to giving up on I2C if we look at it > > > wrongly. Even Apple's device is Bizlink designed. > > > > > > drivers/gpu/drm/drm_dp_helper.c | 13 +++++-------- > > > 1 file changed, 5 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > > > index 79968e3..b4a9d4a 100644 > > > --- a/drivers/gpu/drm/drm_dp_helper.c > > > +++ b/drivers/gpu/drm/drm_dp_helper.c > > > @@ -507,16 +507,13 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, > > > err = drm_dp_i2c_do_msg(aux, &msg); > > > if (err < 0) > > > break; > > > - /* > > > - * Many hardware implementations support FIFOs larger than a > > > - * single byte, but it has been empirically determined that > > > - * transferring data in larger chunks can actually lead to > > > - * decreased performance. Therefore each message is simply > > > - * transferred byte-by-byte. > > > + /* Bizlink designed DP->DVI-D Dual Link adapters require the > > > + * I2C over AUX packets to be as large as possible. If not, > > > + * the I2C transactions never succeed. > > > */ > > > - for (j = 0; j < msgs[i].len; j++) { > > > + for (j = 0; j < msgs[i].len; j+=16) { > > > msg.buffer = msgs[i].buf + j; > > > - msg.size = 1; > > > + msg.size = min(16, msgs[i].len - 16); > > > > I don't think it's quite this simple. The sink is allowed to ACK > > partial data for multi-byte messages. The code doesn't handle that. > > Also not all hardware may support transferring 16 bytes at a time. How > does that work with these adapters? Does it mean they can't work on DP > hardware that can't do 16 byte block transfers? If we handle short reads then hw which really can only do 1 byte at a time could just always do that. But yeah that means you can't use these fancy dp sinks with them I guess. -Daniel
On Friday 23 January 2015 22:21:09 Thierry Reding wrote: > On Fri, Jan 23, 2015 at 09:46:29PM +0200, Ville Syrjälä wrote: > > On Fri, Jan 23, 2015 at 06:40:38PM +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. Analysis of the > > > failure state was provided by Datapath Ltd. > > > > > > Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk> > > > --- > > > Thierry, > > > > > > You put in the comment about "decreased performance", back in December 2013; > > > would you mind testing that this still works with the devices you tested? > > > > > > Unfortunately, Bizlink are the only game in town for DP->DVI-DL adapters - > > > and their firmware is prone to giving up on I2C if we look at it > > > wrongly. Even Apple's device is Bizlink designed. > > > > > > drivers/gpu/drm/drm_dp_helper.c | 13 +++++-------- > > > 1 file changed, 5 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > > > index 79968e3..b4a9d4a 100644 > > > --- a/drivers/gpu/drm/drm_dp_helper.c > > > +++ b/drivers/gpu/drm/drm_dp_helper.c > > > @@ -507,16 +507,13 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, > > > err = drm_dp_i2c_do_msg(aux, &msg); > > > if (err < 0) > > > break; > > > - /* > > > - * Many hardware implementations support FIFOs larger than a > > > - * single byte, but it has been empirically determined that > > > - * transferring data in larger chunks can actually lead to > > > - * decreased performance. Therefore each message is simply > > > - * transferred byte-by-byte. > > > + /* Bizlink designed DP->DVI-D Dual Link adapters require the > > > + * I2C over AUX packets to be as large as possible. If not, > > > + * the I2C transactions never succeed. > > > */ > > > - for (j = 0; j < msgs[i].len; j++) { > > > + for (j = 0; j < msgs[i].len; j+=16) { > > > msg.buffer = msgs[i].buf + j; > > > - msg.size = 1; > > > + msg.size = min(16, msgs[i].len - 16); > > > > I don't think it's quite this simple. The sink is allowed to ACK > > partial data for multi-byte messages. The code doesn't handle that. > It doesn't look challenging to fix that, though - I'll do a v2 with that fixed. I don't have hardware to test against that I know of; is there anyone who's got a sink that does partial ACKs that could test for me? > Also not all hardware may support transferring 16 bytes at a time. How > does that work with these adapters? Does it mean they can't work on DP > hardware that can't do 16 byte block transfers? > Correct. 16 bytes or go home, based on testing done at Datapath. Not entirely coincidentally, this is the behaviour of NVIDIA, AMD and Intel graphics on Windows and Mac OS X - I suspect that testing was dominated by "what does Windows do", not by "what does the spec say". Datapath Ltd tested with a non-Linux source that's only capable of transferring one byte at a time, and the adapter failed in exactly the same way as it does with Linux.
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 79968e3..b4a9d4a 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -507,16 +507,13 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, err = drm_dp_i2c_do_msg(aux, &msg); if (err < 0) break; - /* - * Many hardware implementations support FIFOs larger than a - * single byte, but it has been empirically determined that - * transferring data in larger chunks can actually lead to - * decreased performance. Therefore each message is simply - * transferred byte-by-byte. + /* Bizlink designed DP->DVI-D Dual Link adapters require the + * I2C over AUX packets to be as large as possible. If not, + * the I2C transactions never succeed. */ - for (j = 0; j < msgs[i].len; j++) { + for (j = 0; j < msgs[i].len; j+=16) { msg.buffer = msgs[i].buf + j; - msg.size = 1; + msg.size = min(16, msgs[i].len - 16); err = drm_dp_i2c_do_msg(aux, &msg); if (err < 0)
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. Analysis of the failure state was provided by Datapath Ltd. Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk> --- Thierry, You put in the comment about "decreased performance", back in December 2013; would you mind testing that this still works with the devices you tested? Unfortunately, Bizlink are the only game in town for DP->DVI-DL adapters - and their firmware is prone to giving up on I2C if we look at it wrongly. Even Apple's device is Bizlink designed. drivers/gpu/drm/drm_dp_helper.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)