[PATCH] i2c: designware: size the RX FIFO threshold to the queued transfer

[PATCH] i2c: designware: size the RX FIFO threshold to the queued transfer

[Navon John Lukose]

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.0

Re: [PATCH] i2c: designware: size the RX FIFO threshold to the queued transfer

[Mika Westerberg]

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

Re: [PATCH] i2c: designware: size the RX FIFO threshold to the queued transfer

[Navon John Lukose]

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.

Re: [PATCH] i2c: designware: size the RX FIFO threshold to the queued transfer

[Mika Westerberg]

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?

Re: [PATCH] i2c: designware: size the RX FIFO threshold to the queued transfer

[Navon John Lukose]

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.

Re: [PATCH] i2c: designware: size the RX FIFO threshold to the queued transfer

[Mika Westerberg]

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

Re: [PATCH] i2c: designware: size the RX FIFO threshold to the queued transfer

[Navon John Lukose]

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 :)

Re: [PATCH] i2c: designware: size the RX FIFO threshold to the queued transfer

[Andy Shevchenko]

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.

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

Re: [PATCH] i2c: designware: size the RX FIFO threshold to the queued transfer

[Navon John Lukose]

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.

Re: [PATCH] i2c: designware: size the RX FIFO threshold to the queued transfer

[Andy Shevchenko]

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