From: Michael Zaidman <michael.zaidman@gmail.com>
To: David Laight <David.Laight@aculab.com>
Cc: "jikos@kernel.org" <jikos@kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-input@vger.kernel.org" <linux-input@vger.kernel.org>,
Guillaume Champagne <champagne.guillaume.c@gmail.com>
Subject: Re: [PATCH v2 2/7] HID: ft260: improve i2c write performance
Date: Wed, 28 Sep 2022 23:27:34 +0300 [thread overview]
Message-ID: <YzSuNlP5LlvSdYYT@michael-VirtualBox> (raw)
In-Reply-To: <f9be53a647cd4b698ed820a70b67b7d4@AcuMS.aculab.com>
On Wed, Sep 28, 2022 at 03:50:36PM +0000, David Laight wrote:
> From: Michael Zaidman
> > Sent: 28 September 2022 15:49
> >
> > The patch improves i2c writing performance by about 30 percent by revising
> > the sleep time in the ft260_hid_output_report_check_status() in the
> > following ways:
>
> Spinning in kernel for several milliseconds isn't friendly at all.
>
> David
>
We do not sleep if the estimated I2C transfer time is below 2 ms since the
first xfer check frequently takes about 1.5 ms on the real HW (i7-4790K @ 4.0GHz).
That means the condition is usually satisfied already on the first 1-3 checks,
as can be seen in example #1.
On the other hand, sending a large amount of data at a low I2C clock rate
saturates the internal FT260 buffer and causes hiccups in status readiness,
as shown in exemple #2. Aligning the status checking wait time to the worst
case significantly reduces the performance.
Exemple #1
$ sudo ./i2cperf -f 2 -o 2 -s 16 -r 0-0xff 13 0x51 -S
Fill block with increment via i2ctransfer by chunks
-------------------------------------------------------------------
data rate(bps) efficiency(%) data size(B) total IOs IO size(B)
-------------------------------------------------------------------
49823 73 256 16 16
[Sep28 21:40] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[ +0.005031] ft260_xfer_status: bus_status 0x20, clock 100
[ +0.016531] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[ +0.001710] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000227] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.011805] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[ +0.000747] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000189] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.014932] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[ +0.000806] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000223] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000178] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.010921] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[ +0.000704] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000181] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000228] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000182] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.014243] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[ +0.001220] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000143] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.011332] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[ +0.001533] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000183] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.011865] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[ +0.000934] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000197] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.014102] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[ +0.001722] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000181] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.014000] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[ +0.000845] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000172] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.013294] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[ +0.001499] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000190] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.011524] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[ +0.001238] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000291] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000182] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.013632] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[ +0.000967] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000184] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.013505] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[ +0.001302] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000140] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.012673] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[ +0.001147] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000140] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000217] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.012426] ft260_i2c_write: rep 0xd4 addr 0x51 off 0 len 18 wlen 18 flag 0x6 d[0] 0x0
[ +0.001172] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000192] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000192] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000184] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000211] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000193] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000194] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000197] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000175] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000226] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000132] ft260_xfer_status: bus_status 0x20, clock 100
Exemple #2
$ sudo ./i2cperf -f 2 -o 2 -s 128 -r 0-0x1ff 13 0x51 -S
Fill block with increment via i2ctransfer by chunks
-------------------------------------------------------------------
data rate(bps) efficiency(%) data size(B) total IOs IO size(B)
-------------------------------------------------------------------
65553 86 512 4 128
[Sep28 22:04] ft260_i2c_write: rep 0xde addr 0x51 off 0 len 130 wlen 60 flag 0x2 d[0] 0x0
[ +0.005707] ft260_hid_output_report_check_status: wait 4260 usec, len 64
[ +0.000846] ft260_xfer_status: bus_status 0x20, clock 100
[ +0.000002] ft260_i2c_write: rep 0xde addr 0x51 off 60 len 70 wlen 60 flag 0x0 d[0] 0x0
[ +0.005155] ft260_hid_output_report_check_status: wait 4260 usec, len 64
[ +0.000245] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.000002] ft260_i2c_write: rep 0xd2 addr 0x51 off 120 len 10 wlen 10 flag 0x4 d[0] 0x0
[ +0.001593] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.016725] ft260_i2c_write: rep 0xde addr 0x51 off 0 len 130 wlen 60 flag 0x2 d[0] 0x0
[ +0.005433] ft260_hid_output_report_check_status: wait 4260 usec, len 64
[ +0.000240] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000193] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.000001] ft260_i2c_write: rep 0xde addr 0x51 off 60 len 70 wlen 60 flag 0x0 d[0] 0x0
[ +0.005547] ft260_hid_output_report_check_status: wait 4260 usec, len 64
[ +0.000185] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.000002] ft260_i2c_write: rep 0xd2 addr 0x51 off 120 len 10 wlen 10 flag 0x4 d[0] 0x0
[ +0.001575] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000235] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000186] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000195] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000160] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.014875] ft260_i2c_write: rep 0xde addr 0x51 off 0 len 130 wlen 60 flag 0x2 d[0] 0x1
[ +0.005596] ft260_hid_output_report_check_status: wait 4260 usec, len 64
[ +0.000230] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000188] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.000002] ft260_i2c_write: rep 0xde addr 0x51 off 60 len 70 wlen 60 flag 0x0 d[0] 0x1
[ +0.005568] ft260_hid_output_report_check_status: wait 4260 usec, len 64
[ +0.000190] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000175] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000172] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.000002] ft260_i2c_write: rep 0xd2 addr 0x51 off 120 len 10 wlen 10 flag 0x4 d[0] 0x1
[ +0.001287] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000226] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000176] ft260_xfer_status: bus_status 0x41, clock 100]
[ +0.000234] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000201] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000173] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000223] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000180] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000218] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000195] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000178] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000129] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000128] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000130] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.014159] ft260_i2c_write: rep 0xde addr 0x51 off 0 len 130 wlen 60 flag 0x2 d[0] 0x1
[ +0.005546] ft260_hid_output_report_check_status: wait 4260 usec, len 64
[ +0.000196] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000183] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000251] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000141] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000191] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000181] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000225] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.000001] ft260_i2c_write: rep 0xde addr 0x51 off 60 len 70 wlen 60 flag 0x0 d[0] 0x1
[ +0.005693] ft260_hid_output_report_check_status: wait 4260 usec, len 64
[ +0.000180] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000227] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000151] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.000002] ft260_i2c_write: rep 0xd2 addr 0x51 off 120 len 10 wlen 10 flag 0x4 d[0] 0x1
[ +0.001321] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000185] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000179] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000234] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000162] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000237] ft260_xfer_status: bus_status 0x40, clock 100
> >
> > 1. Reduce the sleep time and start to poll earlier:
> >
> > Before:
> > $ sudo ./i2cperf -f 2 -o 2 -s 32 -r 0-0xff 13 0x51 -S
> >
> > Fill block with increment via i2ctransfer by chunks
> > -------------------------------------------------------------------
> > data rate(bps) efficiency(%) data size(B) total IOs IO size(B)
> > -------------------------------------------------------------------
> > 40510 80 256 8 32
> >
> > After:
> > $ sudo ./i2cperf -f 2 -o 2 -s 32 -r 0-0xff 13 0x51 -S
> >
> > Fill block with increment via i2ctransfer by chunks
> > -------------------------------------------------------------------
> > data rate(bps) efficiency(%) data size(B) total IOs IO size(B)
> > -------------------------------------------------------------------
> > 52584 80 256 8 32
> >
> > 2. Do not sleep when the calculated sleep time is below 2 ms:
> >
> > Before:
> > $ sudo ./i2cperf -f 2 -o 2 -s 16 -r 0-0xff 13 0x51 -S
> >
> > Fill block with increment via i2ctransfer by chunks
> > -------------------------------------------------------------------
> > data rate(bps) efficiency(%) data size(B) total IOs IO size(B)
> > -------------------------------------------------------------------
> > 26707 73 256 16 16
> >
> > After:
> > $ sudo ./i2cperf -f 2 -o 2 -s 16 -r 0-0xff 13 0x51 -S
> >
> > Fill block with increment via i2ctransfer by chunks
> > -------------------------------------------------------------------
> > data rate(bps) efficiency(%) data size(B) total IOs IO size(B)
> > -------------------------------------------------------------------
> > 37034 73 256 16 16
> >
> > Link to the i2cperf - https://github.com/MichaelZaidman/i2cperf
> >
> > Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com>
> > Tested-by: Guillaume Champagne <champagne.guillaume.c@gmail.com>
> > ---
> > drivers/hid/hid-ft260.c | 14 +++++++++-----
> > 1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> > index a35201d68b15..44106cadd746 100644
> > --- a/drivers/hid/hid-ft260.c
> > +++ b/drivers/hid/hid-ft260.c
> > @@ -345,7 +345,7 @@ static int ft260_hid_output_report(struct hid_device *hdev, u8 *data,
> > static int ft260_hid_output_report_check_status(struct ft260_device *dev,
> > u8 *data, int len)
> > {
> > - int ret, usec, try = 3;
> > + int ret, usec, try = 100;
> > struct hid_device *hdev = dev->hdev;
> >
> > ret = ft260_hid_output_report(hdev, data, len);
> > @@ -356,10 +356,14 @@ static int ft260_hid_output_report_check_status(struct ft260_device *dev,
> > return ret;
> > }
> >
> > - /* transfer time = 1 / clock(KHz) * 10 bits * bytes */
> > - usec = 10000 / dev->clock * len;
> > - usleep_range(usec, usec + 100);
> > - ft260_dbg("wait %d usec, len %d\n", usec, len);
> > + /* transfer time = 1 / clock(KHz) * 9 bits * bytes */
> > + usec = len * 9000 / dev->clock;
> > + if (usec > 2000) {
> > + usec -= 1500;
> > + usleep_range(usec, usec + 100);
> > + ft260_dbg("wait %d usec, len %d\n", usec, len);
> > + }
> > +
> > do {
> > ret = ft260_xfer_status(dev);
> > if (ret != -EAGAIN)
> > --
> > 2.34.1
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
next prev parent reply other threads:[~2022-09-28 20:27 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-28 14:48 [PATCH v2 0/7] HID: ft260: fixes and performance improvements Michael Zaidman
2022-09-28 14:48 ` [PATCH v2 1/7] HID: ft260: ft260_xfer_status routine cleanup Michael Zaidman
2022-09-28 14:48 ` [PATCH v2 2/7] HID: ft260: improve i2c write performance Michael Zaidman
2022-09-28 15:50 ` David Laight
2022-09-28 20:27 ` Michael Zaidman [this message]
2022-09-28 14:48 ` [PATCH v2 3/7] HID: ft260: support i2c writes larger than HID report size Michael Zaidman
2022-09-28 14:48 ` [PATCH v2 4/7] HID: ft260: support i2c reads greater " Michael Zaidman
2022-10-04 18:11 ` Enrik Berkhan
2022-10-05 14:50 ` Michael Zaidman
2022-09-28 14:48 ` [PATCH v2 5/7] HID: ft260: improve i2c large reads performance Michael Zaidman
2022-10-04 18:15 ` Enrik Berkhan
2022-10-05 14:34 ` Michael Zaidman
2022-10-05 18:19 ` Enrik Berkhan
2022-09-28 14:48 ` [PATCH v2 6/7] HID: ft260: do not populate /dev/hidraw device Michael Zaidman
2022-09-28 14:48 ` [PATCH v2 7/7] HID: ft260: skip unexpected HID input reports Michael Zaidman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YzSuNlP5LlvSdYYT@michael-VirtualBox \
--to=michael.zaidman@gmail.com \
--cc=David.Laight@aculab.com \
--cc=champagne.guillaume.c@gmail.com \
--cc=jikos@kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.