drm/i915: cope with large i2c transfers
Subsystems (2)
PATCH 1/1 drm/i915: cope with large i2c transfers
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).
---
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(-)
drivers/gpu/drm/i915/i915_reg.h +1 -0
@@ -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)
drivers/gpu/drm/i915/intel_i2c.c +57 -10
100 lines not on this page · raw message
Signature · 5 lines
2.2.0.rc0.207.ga3a616c -- Dmitry
Quoted 3 lines · 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.
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.
Quoted 7 lines
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 <[email protected]>
The code looks good and I've only whitespace and variable scoping gripes, so
Though I am tempted to say we should impose the 256 byte limit for stable@
-Chris
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
Quoted 3 lines · Chris Wilson
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
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel