diff mbox series

[v2,5/7] HID: ft260: improve i2c large reads performance

Message ID 20220928144854.5580-6-michael.zaidman@gmail.com
State New
Headers show
Series HID: ft260: fixes and performance improvements | expand

Commit Message

Michael Zaidman Sept. 28, 2022, 2:48 p.m. UTC
The patch increases the read buffer size to 128 bytes. It reduces the
number of ft260_i2c_read calls by three and improves the large data
chunks' read performance by about 10%.

Before:

$ sudo ./i2cperf -d 2 -o 2 -s 128 -r 0-0xff 13 0x51 -S

  Read block via i2ctransfer by chunks
  -------------------------------------------------------------------
  data rate(bps)  efficiency(%)  data size(B)  total IOs   IO size(B)
  -------------------------------------------------------------------
  43990           85             256           2           128

Kernel log:

[  +1.464346] ft260_i2c_write_read: off 0x0 rlen 128 wlen 2
[  +0.000002] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x0
[  +0.001653] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000188] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000002] ft260_i2c_read: rep 0xc2 addr 0x51 len 128 rlen 60 flag 0x3
[  +0.008609] ft260_raw_event: i2c resp: rep 0xde len 60
[  +0.000157] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000002] ft260_i2c_read: rep 0xc2 addr 0x51 len 68 rlen 60 flag 0x0
[  +0.008840] ft260_raw_event: i2c resp: rep 0xde len 60
[  +0.000203] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000002] ft260_i2c_read: rep 0xc2 addr 0x51 len 8 rlen 8 flag 0x4
[  +0.002794] ft260_raw_event: i2c resp: rep 0xd1 len 8
[  +0.000201] ft260_xfer_status: bus_status 0x20, clock 100
[  +0.015171] ft260_i2c_write_read: off 0x80 rlen 128 wlen 2
[  +0.000002] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x0
[  +0.000764] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000231] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000148] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000002] ft260_i2c_read: rep 0xc2 addr 0x51 len 128 rlen 60 flag 0x3
[  +0.008488] ft260_raw_event: i2c resp: rep 0xde len 60
[  +0.000205] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000002] ft260_i2c_read: rep 0xc2 addr 0x51 len 68 rlen 60 flag 0x0
[  +0.008795] ft260_raw_event: i2c resp: rep 0xde len 60
[  +0.000176] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000001] ft260_i2c_read: rep 0xc2 addr 0x51 len 8 rlen 8 flag 0x4
[  +0.002819] ft260_raw_event: i2c resp: rep 0xd1 len 8
[  +0.000167] ft260_xfer_status: bus_status 0x20, clock 100

After:

$ sudo ./i2cperf -d 2 -o 2 -s 128 -r 0-0xff 13 0x51 -S

  Read block via i2ctransfer by chunks
  -------------------------------------------------------------------
  data rate(bps)  efficiency(%)  data size(B)  total IOs   IO size(B)
  -------------------------------------------------------------------
  49316           85             256           2           128

Kernel log:

[  +1.447360] ft260_i2c_write_read: off 0x0 rlen 128 wlen 2
[  +0.000002] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x0
[  +0.001633] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000190] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000001] ft260_i2c_read: rep 0xc2 addr 0x51 len 128 rlen 128 flag 0x7
[  +0.008617] ft260_raw_event: i2c resp: rep 0xde len 60
[  +0.008033] ft260_raw_event: i2c resp: rep 0xde len 60
[  +0.000954] ft260_raw_event: i2c resp: rep 0xd1 len 8
[  +0.000208] ft260_xfer_status: bus_status 0x20, clock 100
[  +0.015853] ft260_i2c_write_read: off 0x80 rlen 128 wlen 2
[  +0.000002] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x0
[  +0.001182] ft260_xfer_status: bus_status 0x41, clock 100
[  +0.000180] ft260_xfer_status: bus_status 0x40, clock 100
[  +0.000001] ft260_i2c_read: rep 0xc2 addr 0x51 len 128 rlen 128 flag 0x7
[  +0.008575] ft260_raw_event: i2c resp: rep 0xde len 60
[  +0.008014] ft260_raw_event: i2c resp: rep 0xde len 60
[  +0.001001] ft260_raw_event: i2c resp: rep 0xd1 len 8
[  +0.000223] ft260_xfer_status: bus_status 0x20, clock 100

Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com>
Tested-by: Guillaume Champagne <champagne.guillaume.c@gmail.com>
---
 drivers/hid/hid-ft260.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

Comments

Enrik Berkhan Oct. 4, 2022, 6:15 p.m. UTC | #1
Hi Michael,

On Wed, 2022-09-28 at 17:48 +0300, Michael Zaidman wrote:
> After:
> 
> $ sudo ./i2cperf -d 2 -o 2 -s 128 -r 0-0xff 13 0x51 -S
> 
>   Read block via i2ctransfer by chunks
>   -------------------------------------------------------------------
>   data rate(bps)  efficiency(%)  data size(B)  total IOs   IO size(B)
>   -------------------------------------------------------------------
>   49316           85             256           2           128
> 
> Kernel log:
> 
> [  +1.447360] ft260_i2c_write_read: off 0x0 rlen 128 wlen 2
> [  +0.000002] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x0
> [  +0.001633] ft260_xfer_status: bus_status 0x41, clock 100
> [  +0.000190] ft260_xfer_status: bus_status 0x40, clock 100
> [  +0.000001] ft260_i2c_read: rep 0xc2 addr 0x51 len 128 rlen 128 flag 0x7
> [  +0.008617] ft260_raw_event: i2c resp: rep 0xde len 60
> [  +0.008033] ft260_raw_event: i2c resp: rep 0xde len 60
> [  +0.000954] ft260_raw_event: i2c resp: rep 0xd1 len 8

As the ft260 can pack up to 60 bytes into one report, would it make
sense to use a multiple-of-60 size (120 or 180)? Might reduce overhead
by another tiny bit ...

Cheers,
Enrik
Michael Zaidman Oct. 5, 2022, 2:34 p.m. UTC | #2
On Tue, Oct 04, 2022 at 08:15:56PM +0200, Enrik Berkhan wrote:
> Hi Michael,
> 
> On Wed, 2022-09-28 at 17:48 +0300, Michael Zaidman wrote:
> > After:
> > 
> > $ sudo ./i2cperf -d 2 -o 2 -s 128 -r 0-0xff 13 0x51 -S
> > 
> >   Read block via i2ctransfer by chunks
> >   -------------------------------------------------------------------
> >   data rate(bps)  efficiency(%)  data size(B)  total IOs   IO size(B)
> >   -------------------------------------------------------------------
> >   49316           85             256           2           128
> > 
> > Kernel log:
> > 
> > [  +1.447360] ft260_i2c_write_read: off 0x0 rlen 128 wlen 2
> > [  +0.000002] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x0
> > [  +0.001633] ft260_xfer_status: bus_status 0x41, clock 100
> > [  +0.000190] ft260_xfer_status: bus_status 0x40, clock 100
> > [  +0.000001] ft260_i2c_read: rep 0xc2 addr 0x51 len 128 rlen 128 flag 0x7
> > [  +0.008617] ft260_raw_event: i2c resp: rep 0xde len 60
> > [  +0.008033] ft260_raw_event: i2c resp: rep 0xde len 60
> > [  +0.000954] ft260_raw_event: i2c resp: rep 0xd1 len 8
> 
> As the ft260 can pack up to 60 bytes into one report, would it make
> sense to use a multiple-of-60 size (120 or 180)? Might reduce overhead
> by another tiny bit ...
> 
> Cheers,
> Enrik
> 
The size of the Read IO to perform is given to the driver by the upper
layer. So it's up to him how to align the IO request size.

When we read from the EEPROM, we want to issue the read requests with
EEPROM page size granularity. The I2C EEPROMs page sizes are usually a
power of 2 aligned.

Please see the examples of reading 4K bytes from the 24C512 EEPROM
first by the read requests of EEPROM page size granularity of 128 bytes
and the second time of the 120 bytes (a multiple of 60 bytes granularity).
In the power of 2 aligned cases, we issued lesser Read IOs (I2C combined
transactions - write address read data) than when we did it with the 60
bytes alignment. Hence the performance gain.

$ sudo ./i2cperf -d 2 -o 2 -s 128 -r 0-0xfff 13 0x51 -S
  Read block via i2ctransfer by chunks
  -------------------------------------------------------------------
  data rate(bps)  efficiency(%)  data size(B)  total IOs   IO size(B)
  -------------------------------------------------------------------
  49581           85             4096          32          128

$ sudo ./i2cperf -d 2 -o 2 -s 120 -r 0-0xfff 13 0x51 -S
  Read block via i2ctransfer by chunks
  -------------------------------------------------------------------
  data rate(bps)  efficiency(%)  data size(B)  total IOs   IO size(B)
  -------------------------------------------------------------------
  48816           85             4096          35          120

Thanks,
Michael

>
Enrik Berkhan Oct. 5, 2022, 6:19 p.m. UTC | #3
Hi Michael,

On Wed, 2022-10-05 at 17:34 +0300, Michael Zaidman wrote:
> On Tue, Oct 04, 2022 at 08:15:56PM +0200, Enrik Berkhan wrote:
> > As the ft260 can pack up to 60 bytes into one report, would it make
> > sense to use a multiple-of-60 size (120 or 180)? Might reduce overhead
> > by another tiny bit ...
> > 
> > Cheers,
> > Enrik
> > 
> The size of the Read IO to perform is given to the driver by the upper
> layer. So it's up to him how to align the IO request size.
> 
> When we read from the EEPROM, we want to issue the read requests with
> EEPROM page size granularity. The I2C EEPROMs page sizes are usually a
> power of 2 aligned.

Understood! I only thought about the HID report sizes. With EEPROMs
etc. in mind, it makes perfect sense to prefer power of 2 sizes.

Thanks for also providing the test results.

Cheers,
Enrik
diff mbox series

Patch

diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index cb8f1782d1f0..c7c3a9d395a2 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -30,12 +30,8 @@  MODULE_PARM_DESC(debug, "Toggle FT260 debugging messages");
 
 #define FT260_REPORT_MAX_LENGTH (64)
 #define FT260_I2C_DATA_REPORT_ID(len) (FT260_I2C_REPORT_MIN + (len - 1) / 4)
-/*
- * The input report format assigns 62 bytes for the data payload, but ft260
- * returns 60 and 2 in two separate transactions. To minimize transfer time
- * in reading chunks mode, set the maximum read payload length to 60 bytes.
- */
-#define FT260_RD_DATA_MAX (60)
+
+#define FT260_RD_DATA_MAX (128)
 #define FT260_WR_DATA_MAX (60)
 
 /*