[PATCH] drm/dp: Use large transactions for I2C over AUX
Ville Syrjälä
ville.syrjala at linux.intel.com
Mon Jan 26 08:00:11 PST 2015
On Mon, Jan 26, 2015 at 05:33:35PM +0200, 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.
Actually for an i2c read it seems you can specify the same len or reduce
the len by the amount you already received. Reducing seems more
approriate as the spec goes to state that when repeating the the message
with the original length the sink is likely to read more i2c data than
will be needed. Althoguh maybe it can buffer it so that if the original
amount of data was more than 16 bytes we might get the answer to the
next message quicker. So maybe we should just keep updating the length
with 'min(16, total_bytes - total_bytes_received)' ?
For writes we don't seem to do anything correctly atm. When getting a
defer or ack w/ M value we should issue a I2C_WRITE_STATUS_UPDATE command
to prompt the sink to report back with an update M value. Currently we
seem to just retry with the same write message.
>
> > Note that I've not looked at MST support - I have neither the DP 1.2 spec
> > nor any MST branch devices, so I can't test anything I write or check it
> > against a spec. It looks from the code, however, as if MST has the branch
> > device do the split from a big request into small transactions.
> >
> > drivers/gpu/drm/drm_dp_helper.c | 42 ++++++++++++++++++++++-------------------
> > include/drm/drm_dp_helper.h | 5 +++++
> > 2 files changed, 28 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > index 79968e3..701b201 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -396,11 +396,13 @@ static u32 drm_dp_i2c_functionality(struct i2c_adapter *adapter)
> > * retrying the transaction as appropriate. It is assumed that the
> > * aux->transfer function does not modify anything in the msg other than the
> > * reply field.
> > + *
> > + * Returns bytes transferred on success, or a negative error code on failure.
> > */
> > static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> > {
> > unsigned int retry;
> > - int err;
> > + int ret;
> >
> > /*
> > * DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source device
> > @@ -409,14 +411,14 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> > */
> > for (retry = 0; retry < 7; retry++) {
> > mutex_lock(&aux->hw_mutex);
> > - err = aux->transfer(aux, msg);
> > + ret = aux->transfer(aux, msg);
> > mutex_unlock(&aux->hw_mutex);
> > - if (err < 0) {
> > - if (err == -EBUSY)
> > + if (ret < 0) {
> > + if (ret == -EBUSY)
> > continue;
> >
> > - DRM_DEBUG_KMS("transaction failed: %d\n", err);
> > - return err;
> > + DRM_DEBUG_KMS("transaction failed: %d\n", ret);
> > + return ret;
> > }
> >
> >
> > @@ -457,9 +459,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> > * Both native ACK and I2C ACK replies received. We
> > * can assume the transfer was successful.
> > */
> > - if (err < msg->size)
> > - return -EPROTO;
> > - return 0;
> > + return ret;
> >
> > case DP_AUX_I2C_REPLY_NACK:
> > DRM_DEBUG_KMS("I2C nack\n");
> > @@ -487,6 +487,7 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
> > {
> > struct drm_dp_aux *aux = adapter->algo_data;
> > unsigned int i, j;
> > + int transfer_size = DP_AUX_MAX_PAYLOAD_BYTES;
> > struct drm_dp_aux_msg msg;
> > int err = 0;
> >
> > @@ -507,20 +508,23 @@ 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.
> > + *
> > + * We therefore start by requesting 16 byte transfers. If
> > + * the hardware gives us a partial ACK, we stick to the new
> > + * smaller size from the partial ACK.
> > */
> > - for (j = 0; j < msgs[i].len; j++) {
> > + for (j = 0; j < msgs[i].len; j += transfer_size) {
> > msg.buffer = msgs[i].buf + j;
> > - msg.size = 1;
> > + msg.size = min(transfer_size, msgs[i].len - j);
> >
> > - err = drm_dp_i2c_do_msg(aux, &msg);
> > - if (err < 0)
> > + transfer_size = drm_dp_i2c_do_msg(aux, &msg);
> > + if (transfer_size <= 0) {
> > + err = transfer_size == 0 ? -EPROTO : transfer_size;
> > break;
> > + }
> > }
> > if (err < 0)
> > break;
> > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > index 11f8c84..444d51b 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -42,6 +42,8 @@
> > * 1.2 formally includes both eDP and DPI definitions.
> > */
> >
> > +#define DP_AUX_MAX_PAYLOAD_BYTES 16
> > +
> > #define DP_AUX_I2C_WRITE 0x0
> > #define DP_AUX_I2C_READ 0x1
> > #define DP_AUX_I2C_STATUS 0x2
> > @@ -519,6 +521,9 @@ struct drm_dp_aux_msg {
> > * transactions. The drm_dp_aux_register_i2c_bus() function registers an
> > * I2C adapter that can be passed to drm_probe_ddc(). Upon removal, drivers
> > * should call drm_dp_aux_unregister_i2c_bus() to remove the I2C adapter.
> > + * The I2C adapter uses long transfers by default; if a partial response is
> > + * received, the adapter will drop down to the size given by the partial
> > + * response for this transaction only.
> > *
> > * Note that the aux helper code assumes that the .transfer() function
> > * only modifies the reply field of the drm_dp_aux_msg structure. The
> > --
> > 2.1.0
>
> --
> Ville Syrjälä
> Intel OTC
--
Ville Syrjälä
Intel OTC
More information about the dri-devel
mailing list