On Thu, Jan 29, 2015 at 02:24:09PM +0000, Simon Farnsworth wrote:
> 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.
My proposed code wouldn't reduce the transfer size in that case due to
the err<msg.size check. So it only considers shrinking the transfer size
when the reply came back with less data than was requested.
>
> 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
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
