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

[Simon Farnsworth <simon.farnsworth@onelan.co.uk>]

[-- Attachment #1.1: Type: text/plain, Size: 8879 bytes --]

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

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel