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);
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.