On Thursday 29 January 2015 15:30:36 you wrote:
> On Tue, Jan 27, 2015 at 03:43:49PM +0000, Simon Farnsworth wrote:
--snip--
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c
> > b/drivers/gpu/drm/drm_dp_helper.c
> > index 79968e3..3db116c 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;
>
> The s/err/ret/ seems a bit superfluous, but OTOH it does make sense
> since it's no longer just an error value. At first glance it just
> confused me a bit since I wasn't expecting that many changes to this
> function.
>
I'm going to hang onto that change - it makes things clearer when you read
the function outside the context of this patch.
> >
> > case DP_AUX_I2C_REPLY_NACK:
> > DRM_DEBUG_KMS("I2C nack\n");
> > @@ -482,14 +482,67 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux,
> > struct drm_dp_aux_msg *msg)
> > return -EREMOTEIO;
> > }
> >
> > +/*
> > + * Keep retrying drm_dp_i2c_do_msg until all data has been transferred.
> > + *
> > + * Returns an error code on failure, or a recommended transfer size on
> > success.
> > + */
> > +static int drm_dp_i2c_drain_msg(struct drm_dp_aux *aux, struct
> > drm_dp_aux_msg *msg)
> > +{
> > + int ret;
> > + struct drm_dp_aux_msg drain_msg;
> > + int drain_bytes;
> > +
> > + ret = drm_dp_i2c_do_msg(aux, msg);
> > +
> > + if (ret == msg->size || ret <= 0)
> > + return ret == 0 ? -EPROTO : ret;
> > +
> > + /*
> > + * We had a partial reply. Drain out the rest of the bytes we
> > + * originally requested, then return the size of the partial
> > + * reply. In theory, this should match DP 1.2's desired behaviour
> > + * for I2C over AUX.
> > + */
>
> The spec is a bit self contradictory, but there is a section which seems
> to suggest this. It's only mentioned for reads mind you, but I don't see
> it causing harm for writes either. Not that we actually handle
> partial/deferred writes correctly at the moment.
>
> > + DRM_DEBUG_KMS("Partial I2C reply: requested %zu bytes got %d bytes\n",
> > msg->size, ret);
> > + drain_msg = *msg;
> > + drain_msg.size -= ret;
> > + drain_msg.buffer += ret;
> > + while (drain_msg.size != 0) {
> > + drain_bytes = drm_dp_i2c_do_msg(aux, &drain_msg);
> > + if (drain_bytes <= 0)
> > + return drain_bytes == 0 ? -EPROTO : drain_bytes;
> > + drain_msg.size -= drain_bytes;
> > + drain_msg.buffer += drain_bytes;
> > + }
>
> Somehow I don't like the duplicated code end up having here. So
> putting it all in a single loop would seem nicer to me. Maybe
> something along these lines?
>
> struct drm_dp_aux_msg msg = *orig_msg;
> int ret = msg.size;
> while (msg.size > 0) {
> int err = drm_dp_i2c_do_msg(aux, &msg);
> if (err <= 0)
> return err == 0 ? -EPROTO : err;
>
> if (err < msg.size && err < ret) {
> DRM_DEBUG_KMS("Partial I2C reply: requested %zu
> bytes got %d bytes\n", msg.size, err);
> ret = err;
> }
>
> msg.size -= err;
> msg.buffer += err;
> }
>
> It would also reduce the returned preferred transfer size further if we
> (for whatever reason) got an even shorter reply while we're draining.
>
I'm not sure that that's the right behaviour, though. If we assume a 3 byte
FIFO in a device that does partial reads, we ask for 16 bytes, causing a
partial response that's 3 bytes long. We then drain out the remaining 13
bytes of the initial request (in case it's set up a 16 byte I2C transfer),
and the last of the reads is guaranteed to be 1 byte long.
We then shrink to 1 byte transfers, when the device would be capable of 3
byte transfers, and could possibly perform better with 3 byte transfers
rather than 1.
Having said that, this is all hypothetical until we find devices that do
partial replies - no-one's been able to find such a device so far.
I'll have a think and see if I can come up with a way to get the behaviour I
want with less code duplication - I might be able to do something by using a
sentinel value to spot first time round the loop.
>
> > + return ret;
> > +}
> > +
> > +/*
> > + * 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. Hence the default is maximum.
> > + */
> > +static int dp_aux_i2c_transfer_size __read_mostly =
> > DP_AUX_MAX_PAYLOAD_BYTES;
> > +module_param_named(dp_aux_i2c_transfer_size, dp_aux_i2c_transfer_size,
> > int, 0600);
> > +MODULE_PARM_DESC(dp_aux_i2c_transfer_size,
> > + "Number of bytes to transfer in a single I2C over DP AUX CH
> > message, (1-16, default 16)");
> > +
> > static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg
> > *msgs,
> > int num)
> > {
> > struct drm_dp_aux *aux = adapter->algo_data;
> > unsigned int i, j;
> > + int transfer_size;
> > struct drm_dp_aux_msg msg;
> > int err = 0;
> >
> > + if (dp_aux_i2c_transfer_size < 1 || dp_aux_i2c_transfer_size >
> > DP_AUX_MAX_PAYLOAD_BYTES) {
> > + DRM_ERROR("dp_aux_i2c_transfer_size invalid - changing from %d
> > to %d\n",
> > + dp_aux_i2c_transfer_size, DP_AUX_MAX_PAYLOAD_BYTES);
> > + dp_aux_i2c_transfer_size = DP_AUX_MAX_PAYLOAD_BYTES;
> > + }
>
> The invalid values could be rejected with custom struct
> kernel_param_ops, but maybe that's a bit overkill. If not that, then
> I'm not sure the error message really has that much value. So I'm thinking
> we could just 'clamp(..., 1, DP_AUX_MAX_PAYLOAD_BYTES)' here.
>
I'll remove the message for v4, as well as marking the parameter unsafe with
module_param_named_unsafe, and use clamp instead of if().
> > +
> > memset(&msg, 0, sizeof(msg));
> >
> > for (i = 0; i < num; i++) {
> > @@ -507,20 +560,20 @@ 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.
> > + /* We want each transaction to be as large as possible, but
> > + * we'll go to smaller sizes if the hardware gives us a
> > + * short reply.
> > */
> > - for (j = 0; j < msgs[i].len; j++) {
> > + transfer_size = dp_aux_i2c_transfer_size;
> > + for (j = 0; j < msgs[i].len; j += msg.size) {
> > msg.buffer = msgs[i].buf + j;
> > - msg.size = 1;
> > + msg.size = min((unsigned)transfer_size, msgs[i].len -
> > j);
>
> Could make transfer_size unsigned in the first place.
>
> >
> > - err = drm_dp_i2c_do_msg(aux, &msg);
> > - if (err < 0)
> > + transfer_size = drm_dp_i2c_drain_msg(aux, &msg);
> > + if (transfer_size < 0) {
> > + err = transfer_size;
> > break;
> > + }
>
> Maybe this is a bit more straight forward?
>
> err = drm_dp_i2c_drain_msg(aux, &msg);
> if (err < 0)
> break;
> transfer_size = err;
>
That and making transfer_size unsigned seems like a good combination. Will
adopt for v4.
--
Simon Farnsworth
Software Engineer
ONELAN Ltd
http://www.onelan.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: This is a digitally signed message part.
URL:
<http://lists.freedesktop.org/archives/dri-devel/attachments/20150129/ecf4d954/attachment.sig>
