[PATCH] drm/i915: cope with large i2c transfers

[PATCH] drm/i915: cope with large i2c transfers

[Dmitry Torokhov]

The hardware is limited to 2^9 - 1 (511) bytes transfers, and current
driver has no protections in case users attempt to do larger transfers.
The code will just stomp over status register and mayhem ensues.

Let's split larger transfers into digestable chunks. Doing this allows
Atmel MXT driver on Pixel 1 function properly (it hasn't since commit
9d8dc3e529a19e427fd379118acd132520935c5d "Input: atmel_mxt_ts -
implement T44 message handling" which tries to consume multiple
touchscreen/touchpad reports in a single transaction).

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

I think I've tracked the main reason for the Pixel Touchpad breakage on
newer kernels. The newer driver tries to consume several reports form
touchscreen, and tries to read ~600 bytes at startup. i915 does not
check the message limits and messes up GMBUS register, causing "phantom"
reads on another i2c channel where the touchpad is connected.

With i915 splitting the data into smaller chunks for transfer my Pixel
reliably detects both touchpad and touchscreen on 4.0.

Might be worth considering for stable too...

Thanks,
Dmitry

 drivers/gpu/drm/i915/i915_reg.h  |  1 +
 drivers/gpu/drm/i915/intel_i2c.c | 67 ++++++++++++++++++++++++++++++++++------
 2 files changed, 58 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index b522eb6..dc45dbf 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1807,6 +1807,7 @@ enum skl_disp_power_wells {
 #define   GMBUS_CYCLE_INDEX	(2<<25)
 #define   GMBUS_CYCLE_STOP	(4<<25)
 #define   GMBUS_BYTE_COUNT_SHIFT 16
+#define   GMBUS_BYTE_COUNT_MAX   ((1U << 9) - 1)
 #define   GMBUS_SLAVE_INDEX_SHIFT 8
 #define   GMBUS_SLAVE_ADDR_SHIFT 1
 #define   GMBUS_SLAVE_READ	(1<<0)
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index b31088a..1c3b4ae 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -270,18 +270,17 @@ gmbus_wait_idle(struct drm_i915_private *dev_priv)
 }
 
 static int
-gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
-		u32 gmbus1_index)
+gmbus_xfer_read_chunk(struct drm_i915_private *dev_priv,
+		      unsigned short addr, u8 *buf, unsigned int len,
+		      u32 gmbus1_index)
 {
 	int reg_offset = dev_priv->gpio_mmio_base;
-	u16 len = msg->len;
-	u8 *buf = msg->buf;
 
 	I915_WRITE(GMBUS1 + reg_offset,
 		   gmbus1_index |
 		   GMBUS_CYCLE_WAIT |
 		   (len << GMBUS_BYTE_COUNT_SHIFT) |
-		   (msg->addr << GMBUS_SLAVE_ADDR_SHIFT) |
+		   (addr << GMBUS_SLAVE_ADDR_SHIFT) |
 		   GMBUS_SLAVE_READ | GMBUS_SW_RDY);
 	while (len) {
 		int ret;
@@ -303,11 +302,36 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
 }
 
 static int
-gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
+gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
+		u32 gmbus1_index)
 {
-	int reg_offset = dev_priv->gpio_mmio_base;
-	u16 len = msg->len;
 	u8 *buf = msg->buf;
+	unsigned int rx_size = msg->len;
+	unsigned int len;
+	int ret;
+
+	do {
+		len = min(rx_size, GMBUS_BYTE_COUNT_MAX);
+
+		ret = gmbus_xfer_read_chunk(dev_priv, msg->addr,
+					    buf, len, gmbus1_index);
+		if (ret)
+			return ret;
+
+		rx_size -= len;
+		buf += len;
+	} while (rx_size != 0);
+
+	return 0;
+}
+
+
+static int
+gmbus_xfer_write_chunk(struct drm_i915_private *dev_priv,
+		       unsigned short addr, u8 *buf, unsigned int len)
+{
+	int reg_offset = dev_priv->gpio_mmio_base;
+	unsigned int chunk_size = len;
 	u32 val, loop;
 
 	val = loop = 0;
@@ -319,8 +343,8 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
 	I915_WRITE(GMBUS3 + reg_offset, val);
 	I915_WRITE(GMBUS1 + reg_offset,
 		   GMBUS_CYCLE_WAIT |
-		   (msg->len << GMBUS_BYTE_COUNT_SHIFT) |
-		   (msg->addr << GMBUS_SLAVE_ADDR_SHIFT) |
+		   (chunk_size << GMBUS_BYTE_COUNT_SHIFT) |
+		   (addr << GMBUS_SLAVE_ADDR_SHIFT) |
 		   GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
 	while (len) {
 		int ret;
@@ -337,6 +361,29 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
 		if (ret)
 			return ret;
 	}
+
+	return 0;
+}
+
+static int
+gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
+{
+	u8 *buf = msg->buf;
+	unsigned int tx_size = msg->len;
+	unsigned int len;
+	int ret;
+
+	do {
+		len = min(tx_size, GMBUS_BYTE_COUNT_MAX);
+
+		ret = gmbus_xfer_write_chunk(dev_priv, msg->addr, buf, len);
+		if (ret)
+			return ret;
+
+		buf += len;
+		tx_size -= len;
+	} while (tx_size != 0);
+
 	return 0;
 }
 
-- 
2.2.0.rc0.207.ga3a616c


-- 
Dmitry

Re: [PATCH] drm/i915: cope with large i2c transfers

[Chris Wilson]

On Mon, Apr 20, 2015 at 03:11:16PM -0700, Dmitry Torokhov wrote:
> The hardware is limited to 2^9 - 1 (511) bytes transfers, and current
> driver has no protections in case users attempt to do larger transfers.
> The code will just stomp over status register and mayhem ensues.

Interesting. The spec says supports "0 to 256 byte" transfers in the
introduction, but the register just says bits 16:24 are a byte counter
without the extra limitation. I suspect the introductory blurb is
inaccurate and it is implementated as a simple counter as demonstrated
here. We should get that clarified in the specs just in case it was once
limited to 256 bytes.
 
> Let's split larger transfers into digestable chunks. Doing this allows
> Atmel MXT driver on Pixel 1 function properly (it hasn't since commit
> 9d8dc3e529a19e427fd379118acd132520935c5d "Input: atmel_mxt_ts -
> implement T44 message handling" which tries to consume multiple
> touchscreen/touchpad reports in a single transaction).
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

The code looks good and I've only whitespace and variable scoping
gripes, so
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Though I am tempted to say we should impose the 256 byte limit for
stable@
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: cope with large i2c transfers

[Linus Torvalds]

On Tue, Apr 21, 2015 at 12:24 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> Though I am tempted to say we should impose the 256 byte limit for
> stable@

If the docs say 256, I'd suggest using that not just for stable, but
for anything. Maybe 511 bytes work everywhere and the docs are just
wrong. And maybe it doesn't. I'd rather start out conservative, and if
somebody can show that it really matters, and that 511 bytes really is
always safe, we can do it then. But I don't imagine that the
difference between "chunk it up to max 511 bytes" is really noticeably
faster than "chunk it up to 256 bytes max".

               Linus