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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).