drivers/i2c/busses/i2c-designware-master.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
i2c_dw_xfer_msg() leaves DW_IC_RX_TL at the 0 that i2c_dw_configure_mode()
writes, so the controller raises RX_FULL once per received byte. Program
RX_TL from the reads already queued, capped at half the FIFO until the last
message is queued, so there is room for the rest.
On an Arrow Lake-H LPSS core with rx_fifo_depth 32, a 22-byte HID report
costs 2.00 interrupts instead of 21.43, and a 452-byte descriptor 0.14
interrupts per byte instead of 1.64. Mean HIDIOCGINPUT latency rises from
2522 to 3268 us; holding /dev/cpu_dma_latency at 0 removes 94% of that.
Assisted-by: LLM
Signed-off-by: Navon John Lukose <navonjohnlukose@gmail.com>
---
Tested on my machine, a Lenovo Yoga Pro 7 14IAH10, and one controller, an
Intel LPSS core with rx_fifo_depth 32, where it has been the daily driver
for the past five days.
The half-FIFO cap matches DW_IC_TX_TL. Untested: a RECV_LEN continuation
longer than one byte, and cores with rx_fifo_depth below 4.
The two interrupts left on a short read are a TX_EMPTY that only runs the
state machine and one RX_FULL coalesced with STOP_DET.
psys power fell about 1.25 W in one run. It is out of the changelog because
the figure depends on how deep the platform idles.
drivers/i2c/busses/i2c-designware-master.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index a1bcc37..f4ad207 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -378,7 +378,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
{
struct i2c_msg *msgs = dev->msgs;
u32 intr_mask;
- int tx_limit, rx_limit;
+ int tx_limit, rx_limit, rx_tl;
u32 buf_len = dev->tx_buf_len;
u8 *buf = dev->tx_buf;
bool need_restart = false;
@@ -484,6 +484,19 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
if (dev->msg_err)
intr_mask = 0;
+ /*
+ * Size the RX FIFO threshold to the reads already queued, so the
+ * controller raises one RX_FULL for the whole burst instead of one per
+ * received byte. While messages remain to be queued, cap it so RX_FULL
+ * still arrives in time to drain the FIFO and let the next TX_EMPTY
+ * queue the rest.
+ */
+ rx_tl = dev->rx_outstanding;
+ if (dev->msg_write_idx < dev->msgs_num)
+ rx_tl = min_t(int, rx_tl, dev->rx_fifo_depth / 2);
+
+ regmap_write(dev->map, DW_IC_RX_TL, rx_tl ? rx_tl - 1 : 0);
+
__i2c_dw_write_intr_mask(dev, intr_mask);
}
--
2.55.0On Sun, Sep 20, 2026 at 04:56:47AM +0530, Navon John Lukose wrote: > i2c_dw_xfer_msg() leaves DW_IC_RX_TL at the 0 that i2c_dw_configure_mode() > writes, so the controller raises RX_FULL once per received byte. Program > RX_TL from the reads already queued, capped at half the FIFO until the last > message is queued, so there is room for the rest. > > On an Arrow Lake-H LPSS core with rx_fifo_depth 32, a 22-byte HID report > costs 2.00 interrupts instead of 21.43, and a 452-byte descriptor 0.14 > interrupts per byte instead of 1.64. Mean HIDIOCGINPUT latency rises from > 2522 to 3268 us; holding /dev/cpu_dma_latency at 0 removes 94% of that. Nobody will go with 0 in cpu_dma_latency. It basically kills the idea of CPU power states. If you need a dynamic run-time PM QoS, patch the HID subsystem to make those when they want to heavily communicate with the HW. There is also the issue of the interrupt locality. Can you try to make that first and see how much power saving you will get? -- With Best Regards, Andy Shevchenko
On Tue, Sep 22, 2026 at 12:57:31PM +0300, Andy Shevchenko wrote: > Nobody will go with 0 in cpu_dma_latency. It basically kills the idea of CPU > power states. Agreed, and the patch does not ask anyone to. Holding it at 0 was how I measured where the added latency comes from. Mika raised the same point and there is more detail in that subthread: https://lore.kernel.org/all/20260921170456.53354-1-navonjohnlukose@gmail.com/ > If you need a dynamic run-time PM QoS, patch the HID subsystem to > make those when they want to heavily communicate with the HW. A QoS request would bound the latency, AFAIK. It would not change that the controller raises RX_FULL once per received byte, which is what this patch is about, so I do not think one blocks the other. > There is also the issue of the interrupt locality. I am not sure what the i2c driver can do about that. What did you have in mind? Worth noting i2c_dw_configure_mode() sets TX_TL to half the FIFO and RX_TL to 0 on adjacent lines, so a transmit interrupt moves up to half a FIFO and a receive interrupt moves one byte, which is what this patch changes.
On Tue, Sep 22, 2026 at 05:42:49PM +0530, Navon John Lukose wrote: > On Tue, Sep 22, 2026 at 12:57:31PM +0300, Andy Shevchenko wrote: > > Nobody will go with 0 in cpu_dma_latency. It basically kills the idea of CPU > > power states. > > Agreed, and the patch does not ask anyone to. Holding it at 0 was how I > measured where the added latency comes from. Mika raised the same point and > there is more detail in that subthread: > > https://lore.kernel.org/all/20260921170456.53354-1-navonjohnlukose@gmail.com/ > > > If you need a dynamic run-time PM QoS, patch the HID subsystem to > > make those when they want to heavily communicate with the HW. > > A QoS request would bound the latency, AFAIK. It would not change that the > controller raises RX_FULL once per received byte, which is what this patch > is about, so I do not think one blocks the other. > > > There is also the issue of the interrupt locality. > > I am not sure what the i2c driver can do about that. What did you have in > mind? i2c-hid > Worth noting i2c_dw_configure_mode() sets TX_TL to half the FIFO and > RX_TL to 0 on adjacent lines, so a transmit interrupt moves up to half a > FIFO and a receive interrupt moves one byte, which is what this patch > changes. -- With Best Regards, Andy Shevchenko
Hi, On Sun, Sep 20, 2026 at 04:56:47AM +0530, Navon John Lukose wrote: > i2c_dw_xfer_msg() leaves DW_IC_RX_TL at the 0 that i2c_dw_configure_mode() > writes, so the controller raises RX_FULL once per received byte. Program > RX_TL from the reads already queued, capped at half the FIFO until the last > message is queued, so there is room for the rest. > > On an Arrow Lake-H LPSS core with rx_fifo_depth 32, a 22-byte HID report > costs 2.00 interrupts instead of 21.43, and a 452-byte descriptor 0.14 > interrupts per byte instead of 1.64. Mean HIDIOCGINPUT latency rises from > 2522 to 3268 us; holding /dev/cpu_dma_latency at 0 removes 94% of that. I'm slightly worried about the latency increase here and the fact that HIDIOCGINPUT is pretty much same as HID_REQ_GET_REPORT so it's not only hidraw that is affected but everything else using HID_REQ_GET_REPORT as well. We cannot expect regular user knows or should be using /dev/cpu_dma_latency (and it may be bad thing to keep the CPUs from using certain C-states).
On Mon, Sep 21, 2026 at 02:05:26PM +0200, Mika Westerberg wrote:
> I'm slightly worried about the latency increase here and the fact that
> HIDIOCGINPUT is pretty much same as HID_REQ_GET_REPORT so it's not only
> hidraw that is affected but everything else using HID_REQ_GET_REPORT as
> well.
You are right, it is every HID_REQ_GET_REPORT user, not just hidraw.
The main motivation for this patch was cutting the wakeups from the
interrupts, and I should have made that clearer. The /dev/cpu_dma_latency
line was just to measure where the extra latency comes from.
What the wakeups cost, measured with a 21-byte HID GET_REPORT at 142 Hz
pinned to one CPU, display off, 32 reps per arm in randomized order:
psys W
idle 1.87 +/- 0.28
patched 2.53 +/- 0.17
stock 4.11 +/- 0.44
Against the idle floor the polling costs 2.24 W without the patch and
0.66 W with it. Paired saving 1.58 W, 95% CI [1.42, 1.75].
So the added exit latency is the package idling deeper than it can when
the controller interrupts once per received byte.
Hi, On Mon, Sep 21, 2026 at 10:34:56PM +0530, Navon John Lukose wrote: > On Mon, Sep 21, 2026 at 02:05:26PM +0200, Mika Westerberg wrote: > > I'm slightly worried about the latency increase here and the fact that > > HIDIOCGINPUT is pretty much same as HID_REQ_GET_REPORT so it's not only > > hidraw that is affected but everything else using HID_REQ_GET_REPORT as > > well. > > You are right, it is every HID_REQ_GET_REPORT user, not just hidraw. > > The main motivation for this patch was cutting the wakeups from the > interrupts, and I should have made that clearer. The /dev/cpu_dma_latency > line was just to measure where the extra latency comes from. > > What the wakeups cost, measured with a 21-byte HID GET_REPORT at 142 Hz > pinned to one CPU, display off, 32 reps per arm in randomized order: > > psys W > idle 1.87 +/- 0.28 > patched 2.53 +/- 0.17 > stock 4.11 +/- 0.44 > > Against the idle floor the polling costs 2.24 W without the patch and > 0.66 W with it. Paired saving 1.58 W, 95% CI [1.42, 1.75]. > > So the added exit latency is the package idling deeper than it can when > the controller interrupts once per received byte. Yes power savings are always good but in this case it also affects the latency visible to the user so we don't want devices like touchpads become "sluggish" either. Do you see any such issues with the devices when you have this enabled and not using /dev/cpu_dma_latency or any other interface to limit the CPU low power states?
On Tue, Sep 22, 2026 at 08:50:41AM +0200, Mika Westerberg wrote: > Yes power savings are always good but in this case it also affects the > latency visible to the user so we don't want devices like touchpads become > "sluggish" either. Do you see any such issues with the devices when you > have this enabled and not using /dev/cpu_dma_latency or any other interface > to limit the CPU low power states? No. I have been running it as my only kernel for a week with nothing set on cpu_dma_latency, and I cannot tell the difference on the touchpad. This patch increases latency by 0.7 ms on the mean, which I don't think is easily noticeable by humans. Against that, the i2c traffic alone cost 2.24 W on the machine I measured, more than its entire 1.87 W idle draw. However, if anyone tests and can feel the difference, please let me know. Also, i2c_dw_configure_mode() sets TX_TL to half the FIFO and RX_TL to 0 on adjacent lines, so a transmit interrupt moves up to half a FIFO of data and a receive interrupt moves one byte, which is what this patch changes.
Hi, On Tue, Sep 22, 2026 at 05:42:38PM +0530, Navon John Lukose wrote: > On Tue, Sep 22, 2026 at 08:50:41AM +0200, Mika Westerberg wrote: > > Yes power savings are always good but in this case it also affects the > > latency visible to the user so we don't want devices like touchpads become > > "sluggish" either. Do you see any such issues with the devices when you > > have this enabled and not using /dev/cpu_dma_latency or any other interface > > to limit the CPU low power states? > > No. I have been running it as my only kernel for a week with nothing set on > cpu_dma_latency, and I cannot tell the difference on the touchpad. > > This patch increases latency by 0.7 ms on the mean, which I don't think > is easily noticeable by humans. Against that, the i2c traffic alone cost > 2.24 W on the machine I measured, more than its entire 1.87 W idle draw. > > However, if anyone tests and can feel the difference, please let me know. Yeah I don't think 0.7ms is much but we have things like touchscreens and the like with various report sizes so at least it would be good to check if those have any issues. Unfortunately I don't have any of such devices anymore but I asked our validation to check if they have something that could be used to test this. > Also, i2c_dw_configure_mode() sets TX_TL to half the FIFO and RX_TL to 0 > on adjacent lines, so a transmit interrupt moves up to half a FIFO of > data and a receive interrupt moves one byte, which is what this patch > changes. Right and there is no explanation why 0 but it has been working fine so far ;-)
On Tue, Sep 22, 2026 at 02:29:02PM +0200, Mika Westerberg wrote:
> Yeah I don't think 0.7ms is much but we have things like touchscreens and
> the like with various report sizes so at least it would be good to check if
> those have any issues.
>
> Unfortunately I don't have any of such devices anymore but I asked our
> validation to check if they have something that could be used to test this.
I have a Goodix touchscreen on a second controller on the same machine, so
I ran it at three report sizes. GET_REPORT at 100 Hz, 2000 transfers per
cell, three interleaved passes, ~normal desktop use in the background:
bytes interrupts/report mean latency
7 4.17 -> 2.00 1776 -> 2093 us
21 10.06 -> 2.00 2131 -> 2676 us
64 63.97 -> 3.21 2303 -> 3512 us
64 bytes is past rx_fifo_depth on this part, so that is also the first
exercise of the half-FIFO cap.
So the latency is not a fixed 0.7 ms as I implied earlier, it grows with
the report size. Breaking down where it goes, with kprobes on i2c_dw_xfer
and i2c_dw_isr, for 64 bytes:
segment stock patched
xfer start -> first ISR 78 146
first ISR -> last ISR 2157 2748
last ISR -> xfer return 101 147
i2c-hid and ioctl overhead 12 49
84% of it is between the first and last interrupt. Stock's 65 interrupts
are 33 us apart, too close together for the CPU to idle deeply, so each
wakeup is cheap. Patched leaves 3 gaps of around 900 us, deep enough to
idle properly, and pays the exit latency on each one.
So the power saving and the added latency are the same effect seen from
either end. Lowering the cap trades one back for the other at roughly the
same rate, so if the latency needs bounding (I personally think it is
overkill) it wants a QoS request as Andy suggested.
I can send the raw data and the scripts if needed.
Assisted-by: LLM # for the kprobe measurements :)
Hi, On Tue, Sep 22, 2026 at 06:53:34PM +0530, Navon John Lukose wrote: > On Tue, Sep 22, 2026 at 02:29:02PM +0200, Mika Westerberg wrote: > > Yeah I don't think 0.7ms is much but we have things like touchscreens and > > the like with various report sizes so at least it would be good to check if > > those have any issues. > > > > Unfortunately I don't have any of such devices anymore but I asked our > > validation to check if they have something that could be used to test this. Turns out they don't have touch screens either :( > I have a Goodix touchscreen on a second controller on the same machine, so > I ran it at three report sizes. GET_REPORT at 100 Hz, 2000 transfers per > cell, three interleaved passes, ~normal desktop use in the background: > > bytes interrupts/report mean latency > 7 4.17 -> 2.00 1776 -> 2093 us > 21 10.06 -> 2.00 2131 -> 2676 us > 64 63.97 -> 3.21 2303 -> 3512 us > > 64 bytes is past rx_fifo_depth on this part, so that is also the first > exercise of the half-FIFO cap. Okay thanks for checking this. If the latency stays in low single digit ms I think that's fine. > So the latency is not a fixed 0.7 ms as I implied earlier, it grows with > the report size. Breaking down where it goes, with kprobes on i2c_dw_xfer > and i2c_dw_isr, for 64 bytes: > > segment stock patched > xfer start -> first ISR 78 146 > first ISR -> last ISR 2157 2748 > last ISR -> xfer return 101 147 > i2c-hid and ioctl overhead 12 49 > > 84% of it is between the first and last interrupt. Stock's 65 interrupts > are 33 us apart, too close together for the CPU to idle deeply, so each > wakeup is cheap. Patched leaves 3 gaps of around 900 us, deep enough to > idle properly, and pays the exit latency on each one. > > So the power saving and the added latency are the same effect seen from > either end. Lowering the cap trades one back for the other at roughly the > same rate, so if the latency needs bounding (I personally think it is > overkill) it wants a QoS request as Andy suggested. I agree. Just wanted to check that there are no user visible (negative) side-effects. So no objections from me, Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
© 2016 - 2026 Red Hat, Inc.