[PATCH v3] console: usb: kbd: Limit poll frequency to improve performance
Using the XHCI driver, the function `usb_kbd_poll_for_event` takes 30-40ms to run. The exact time is dependent on the polling interval the keyboard requests in its descriptor, and likely cannot be significantly reduced without major rework to the XHCI driver.
The U-Boot EFI console service sets a timer to poll the keyboard every 5 microseconds, and this timer is checked every time a block is read off disk. The net effect is that, on my system, loading a ~40MiB kernel and initrd takes about 62 seconds with a slower keyboard and 53 seconds with a faster one, with the vast majority of the time spent polling the keyboard.
To solve this problem, this patch adds a 20ms delay between consecutive calls to `usb_kbd_poll_for_event`. This is sufficient to reduce the total loading time to under half a second for both keyboards, and does not impact the perceived keystroke latency.
Signed-off-by: Thomas Watson twatson52@icloud.com --- This revision fixes a sandbox test failure by honoring the test's request to skip delays.
common/usb_kbd.c | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-)
diff --git a/common/usb_kbd.c b/common/usb_kbd.c index afad260d3d..352d86fb2e 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -17,6 +17,9 @@ #include <stdio_dev.h> #include <watchdog.h> #include <asm/byteorder.h> +#ifdef CONFIG_SANDBOX +#include <asm/state.h> +#endif
#include <usb.h>
@@ -118,7 +121,7 @@ struct usb_kbd_pdata { extern int __maybe_unused net_busy_flag;
/* The period of time between two calls of usb_kbd_testc(). */ -static unsigned long __maybe_unused kbd_testc_tms; +static unsigned long kbd_testc_tms;
/* Puts character in the queue and sets up the in and out pointer. */ static void usb_kbd_put_queue(struct usb_kbd_pdata *data, u8 c) @@ -394,21 +397,39 @@ static int usb_kbd_testc(struct stdio_dev *sdev) struct usb_device *usb_kbd_dev; struct usb_kbd_pdata *data;
+ /* + * Polling the keyboard for an event can take dozens of milliseconds. + * Add a delay between polls to avoid blocking activity which polls + * rapidly, like the UEFI console timer. + */ + unsigned long poll_delay = CONFIG_SYS_HZ / 50; + #ifdef CONFIG_CMD_NET /* * If net_busy_flag is 1, NET transfer is running, * then we check key-pressed every second (first check may be * less than 1 second) to improve TFTP booting performance. */ - if (net_busy_flag && (get_timer(kbd_testc_tms) < CONFIG_SYS_HZ)) - return 0; - kbd_testc_tms = get_timer(0); + if (net_busy_flag) + poll_delay = CONFIG_SYS_HZ; +#endif + +#ifdef CONFIG_SANDBOX + /* + * Skip delaying polls if a test requests it. + */ + if (state_get_skip_delays()) + poll_delay = 0; #endif + dev = stdio_get_by_name(sdev->name); usb_kbd_dev = (struct usb_device *)dev->priv; data = usb_kbd_dev->privptr;
- usb_kbd_poll_for_event(usb_kbd_dev); + if (get_timer(kbd_testc_tms) >= poll_delay) { + usb_kbd_poll_for_event(usb_kbd_dev); + kbd_testc_tms = get_timer(0); + }
return !(data->usb_in_pointer == data->usb_out_pointer); }
On 2/10/22 22:13, Thomas Watson wrote:
Using the XHCI driver, the function `usb_kbd_poll_for_event` takes 30-40ms to run. The exact time is dependent on the polling interval the keyboard requests in its descriptor, and likely cannot be significantly reduced without major rework to the XHCI driver.
The U-Boot EFI console service sets a timer to poll the keyboard every 5 microseconds, and this timer is checked every time a block is read off disk. The net effect is that, on my system, loading a ~40MiB kernel and initrd takes about 62 seconds with a slower keyboard and 53 seconds with a faster one, with the vast majority of the time spent polling the keyboard.
To solve this problem, this patch adds a 20ms delay between consecutive calls to `usb_kbd_poll_for_event`. This is sufficient to reduce the total loading time to under half a second for both keyboards, and does not impact the perceived keystroke latency.
Signed-off-by: Thomas Watson twatson52@icloud.com
This revision fixes a sandbox test failure by honoring the test's request to skip delays.
common/usb_kbd.c | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-)
diff --git a/common/usb_kbd.c b/common/usb_kbd.c index afad260d3d..352d86fb2e 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -17,6 +17,9 @@ #include <stdio_dev.h> #include <watchdog.h> #include <asm/byteorder.h> +#ifdef CONFIG_SANDBOX +#include <asm/state.h> +#endif
#include <usb.h>
@@ -118,7 +121,7 @@ struct usb_kbd_pdata { extern int __maybe_unused net_busy_flag;
/* The period of time between two calls of usb_kbd_testc(). */ -static unsigned long __maybe_unused kbd_testc_tms; +static unsigned long kbd_testc_tms;
/* Puts character in the queue and sets up the in and out pointer. */ static void usb_kbd_put_queue(struct usb_kbd_pdata *data, u8 c) @@ -394,21 +397,39 @@ static int usb_kbd_testc(struct stdio_dev *sdev) struct usb_device *usb_kbd_dev; struct usb_kbd_pdata *data;
- /*
* Polling the keyboard for an event can take dozens of milliseconds.* Add a delay between polls to avoid blocking activity which polls* rapidly, like the UEFI console timer.*/- unsigned long poll_delay = CONFIG_SYS_HZ / 50;
- #ifdef CONFIG_CMD_NET /*
*/
- If net_busy_flag is 1, NET transfer is running,
- then we check key-pressed every second (first check may be
- less than 1 second) to improve TFTP booting performance.
- if (net_busy_flag && (get_timer(kbd_testc_tms) < CONFIG_SYS_HZ))
return 0;- kbd_testc_tms = get_timer(0);
- if (net_busy_flag)
poll_delay = CONFIG_SYS_HZ;+#endif
+#ifdef CONFIG_SANDBOX
- /*
* Skip delaying polls if a test requests it.*/- if (state_get_skip_delays())
poll_delay = 0;
Is this architecture-specific ifdef really needed in common code?
On Feb 10, 2022, at 3:42 PM, Marek Vasut marex@denx.de wrote:
On 2/10/22 22:13, Thomas Watson wrote:
Using the XHCI driver, the function `usb_kbd_poll_for_event` takes 30-40ms to run. The exact time is dependent on the polling interval the keyboard requests in its descriptor, and likely cannot be significantly reduced without major rework to the XHCI driver. The U-Boot EFI console service sets a timer to poll the keyboard every 5 microseconds, and this timer is checked every time a block is read off disk. The net effect is that, on my system, loading a ~40MiB kernel and initrd takes about 62 seconds with a slower keyboard and 53 seconds with a faster one, with the vast majority of the time spent polling the keyboard. To solve this problem, this patch adds a 20ms delay between consecutive calls to `usb_kbd_poll_for_event`. This is sufficient to reduce the total loading time to under half a second for both keyboards, and does not impact the perceived keystroke latency. Signed-off-by: Thomas Watson twatson52@icloud.com
This revision fixes a sandbox test failure by honoring the test's request to skip delays. common/usb_kbd.c | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/common/usb_kbd.c b/common/usb_kbd.c index afad260d3d..352d86fb2e 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -17,6 +17,9 @@ #include <stdio_dev.h> #include <watchdog.h> #include <asm/byteorder.h> +#ifdef CONFIG_SANDBOX +#include <asm/state.h> +#endif #include <usb.h> @@ -118,7 +121,7 @@ struct usb_kbd_pdata { extern int __maybe_unused net_busy_flag; /* The period of time between two calls of usb_kbd_testc(). */ -static unsigned long __maybe_unused kbd_testc_tms; +static unsigned long kbd_testc_tms; /* Puts character in the queue and sets up the in and out pointer. */ static void usb_kbd_put_queue(struct usb_kbd_pdata *data, u8 c) @@ -394,21 +397,39 @@ static int usb_kbd_testc(struct stdio_dev *sdev) struct usb_device *usb_kbd_dev; struct usb_kbd_pdata *data;
- /*
* Polling the keyboard for an event can take dozens of milliseconds.* Add a delay between polls to avoid blocking activity which polls* rapidly, like the UEFI console timer.*/- unsigned long poll_delay = CONFIG_SYS_HZ / 50;
#ifdef CONFIG_CMD_NET /* * If net_busy_flag is 1, NET transfer is running, * then we check key-pressed every second (first check may be * less than 1 second) to improve TFTP booting performance. */
- if (net_busy_flag && (get_timer(kbd_testc_tms) < CONFIG_SYS_HZ))
return 0;- kbd_testc_tms = get_timer(0);
- if (net_busy_flag)
poll_delay = CONFIG_SYS_HZ;+#endif
+#ifdef CONFIG_SANDBOX
- /*
* Skip delaying polls if a test requests it.*/- if (state_get_skip_delays())
poll_delay = 0;#endif
Is this architecture-specific ifdef really needed in common code?
I modeled this check off a similar one around line 184 of common/usb_hub.c which effectively sets some get_timer() based delay values to 0 like I propose here. Checking this pre-existing flag here seems like a less invasive option to me than modifying the core timer code to also check the flag.
Modifying the test to add a delay would also theoretically be possible, but I am not sure how to accomplish that and it would make the test take longer.
On 2/10/22 23:00, Thomas Watson wrote:
On Feb 10, 2022, at 3:42 PM, Marek Vasut marex@denx.de wrote:
On 2/10/22 22:13, Thomas Watson wrote:
Using the XHCI driver, the function `usb_kbd_poll_for_event` takes 30-40ms to run. The exact time is dependent on the polling interval the keyboard requests in its descriptor, and likely cannot be significantly reduced without major rework to the XHCI driver. The U-Boot EFI console service sets a timer to poll the keyboard every 5 microseconds, and this timer is checked every time a block is read off disk. The net effect is that, on my system, loading a ~40MiB kernel and initrd takes about 62 seconds with a slower keyboard and 53 seconds with a faster one, with the vast majority of the time spent polling the keyboard. To solve this problem, this patch adds a 20ms delay between consecutive calls to `usb_kbd_poll_for_event`. This is sufficient to reduce the total loading time to under half a second for both keyboards, and does not impact the perceived keystroke latency. Signed-off-by: Thomas Watson twatson52@icloud.com
This revision fixes a sandbox test failure by honoring the test's request to skip delays. common/usb_kbd.c | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/common/usb_kbd.c b/common/usb_kbd.c index afad260d3d..352d86fb2e 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -17,6 +17,9 @@ #include <stdio_dev.h> #include <watchdog.h> #include <asm/byteorder.h> +#ifdef CONFIG_SANDBOX +#include <asm/state.h> +#endif #include <usb.h> @@ -118,7 +121,7 @@ struct usb_kbd_pdata { extern int __maybe_unused net_busy_flag; /* The period of time between two calls of usb_kbd_testc(). */ -static unsigned long __maybe_unused kbd_testc_tms; +static unsigned long kbd_testc_tms; /* Puts character in the queue and sets up the in and out pointer. */ static void usb_kbd_put_queue(struct usb_kbd_pdata *data, u8 c) @@ -394,21 +397,39 @@ static int usb_kbd_testc(struct stdio_dev *sdev) struct usb_device *usb_kbd_dev; struct usb_kbd_pdata *data;
- /*
* Polling the keyboard for an event can take dozens of milliseconds.* Add a delay between polls to avoid blocking activity which polls* rapidly, like the UEFI console timer.*/- unsigned long poll_delay = CONFIG_SYS_HZ / 50;
- #ifdef CONFIG_CMD_NET /*
*/
- If net_busy_flag is 1, NET transfer is running,
- then we check key-pressed every second (first check may be
- less than 1 second) to improve TFTP booting performance.
- if (net_busy_flag && (get_timer(kbd_testc_tms) < CONFIG_SYS_HZ))
return 0;- kbd_testc_tms = get_timer(0);
- if (net_busy_flag)
poll_delay = CONFIG_SYS_HZ;+#endif
+#ifdef CONFIG_SANDBOX
- /*
* Skip delaying polls if a test requests it.*/- if (state_get_skip_delays())
poll_delay = 0;Is this architecture-specific ifdef really needed in common code?
I modeled this check off a similar one around line 184 of common/usb_hub.c which effectively sets some get_timer() based delay values to 0 like I propose here. Checking this pre-existing flag here seems like a less invasive option to me than modifying the core timer code to also check the flag.
Modifying the test to add a delay would also theoretically be possible, but I am not sure how to accomplish that and it would make the test take longer.
I CCed Simon, maybe he can help out.
Hi Marek,
On Thu, 10 Feb 2022 at 14:42, Marek Vasut marex@denx.de wrote:
On 2/10/22 22:13, Thomas Watson wrote:
Using the XHCI driver, the function `usb_kbd_poll_for_event` takes 30-40ms to run. The exact time is dependent on the polling interval the keyboard requests in its descriptor, and likely cannot be significantly reduced without major rework to the XHCI driver.
The U-Boot EFI console service sets a timer to poll the keyboard every 5 microseconds, and this timer is checked every time a block is read off disk. The net effect is that, on my system, loading a ~40MiB kernel and initrd takes about 62 seconds with a slower keyboard and 53 seconds with a faster one, with the vast majority of the time spent polling the keyboard.
To solve this problem, this patch adds a 20ms delay between consecutive calls to `usb_kbd_poll_for_event`. This is sufficient to reduce the total loading time to under half a second for both keyboards, and does not impact the perceived keystroke latency.
Signed-off-by: Thomas Watson twatson52@icloud.com
This revision fixes a sandbox test failure by honoring the test's request to skip delays.
common/usb_kbd.c | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-)
We don't want to delay the CI tests which take enough time as it is. So time needs to be faked.
It could be an if() though, rather than #if, perhaps?
Regards, Simon
On 2/11/22 00:02, Simon Glass wrote:
Hi Marek,
On Thu, 10 Feb 2022 at 14:42, Marek Vasut marex@denx.de wrote:
On 2/10/22 22:13, Thomas Watson wrote:
Using the XHCI driver, the function `usb_kbd_poll_for_event` takes 30-40ms to run. The exact time is dependent on the polling interval the keyboard requests in its descriptor, and likely cannot be significantly reduced without major rework to the XHCI driver.
The U-Boot EFI console service sets a timer to poll the keyboard every 5 microseconds, and this timer is checked every time a block is read off disk. The net effect is that, on my system, loading a ~40MiB kernel and initrd takes about 62 seconds with a slower keyboard and 53 seconds with a faster one, with the vast majority of the time spent polling the keyboard.
To solve this problem, this patch adds a 20ms delay between consecutive calls to `usb_kbd_poll_for_event`. This is sufficient to reduce the total loading time to under half a second for both keyboards, and does not impact the perceived keystroke latency.
Signed-off-by: Thomas Watson twatson52@icloud.com
This revision fixes a sandbox test failure by honoring the test's request to skip delays.
common/usb_kbd.c | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-)
We don't want to delay the CI tests which take enough time as it is. So time needs to be faked.
It could be an if() though, rather than #if, perhaps?
Since there are multiple instances of the ifdef already , subsequent patch to clean them up would be fine.
On Feb 10, 2022, at 5:04 PM, Marek Vasut marex@denx.de wrote:
On 2/11/22 00:02, Simon Glass wrote:
Hi Marek, On Thu, 10 Feb 2022 at 14:42, Marek Vasut marex@denx.de wrote:
On 2/10/22 22:13, Thomas Watson wrote:
Using the XHCI driver, the function `usb_kbd_poll_for_event` takes 30-40ms to run. The exact time is dependent on the polling interval the keyboard requests in its descriptor, and likely cannot be significantly reduced without major rework to the XHCI driver.
The U-Boot EFI console service sets a timer to poll the keyboard every 5 microseconds, and this timer is checked every time a block is read off disk. The net effect is that, on my system, loading a ~40MiB kernel and initrd takes about 62 seconds with a slower keyboard and 53 seconds with a faster one, with the vast majority of the time spent polling the keyboard.
To solve this problem, this patch adds a 20ms delay between consecutive calls to `usb_kbd_poll_for_event`. This is sufficient to reduce the total loading time to under half a second for both keyboards, and does not impact the perceived keystroke latency.
Signed-off-by: Thomas Watson twatson52@icloud.com
This revision fixes a sandbox test failure by honoring the test's request to skip delays.
common/usb_kbd.c | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-)
We don't want to delay the CI tests which take enough time as it is. So time needs to be faked. It could be an if() though, rather than #if, perhaps?
Since there are multiple instances of the ifdef already , subsequent patch to clean them up would be fine.
That function `state_get_skip_delays()` and that state.h header file in general is only defined in the sandbox architecture, so I don't think it's possible to remove the #ifdef unless that is changed.
()Hi Thomas,
On Thu, 10 Feb 2022 at 18:43, Thomas Watson twatson52@icloud.com wrote:
On Feb 10, 2022, at 5:04 PM, Marek Vasut marex@denx.de wrote:
On 2/11/22 00:02, Simon Glass wrote:
Hi Marek, On Thu, 10 Feb 2022 at 14:42, Marek Vasut marex@denx.de wrote:
On 2/10/22 22:13, Thomas Watson wrote:
Using the XHCI driver, the function `usb_kbd_poll_for_event` takes 30-40ms to run. The exact time is dependent on the polling interval the keyboard requests in its descriptor, and likely cannot be significantly reduced without major rework to the XHCI driver.
The U-Boot EFI console service sets a timer to poll the keyboard every 5 microseconds, and this timer is checked every time a block is read off disk. The net effect is that, on my system, loading a ~40MiB kernel and initrd takes about 62 seconds with a slower keyboard and 53 seconds with a faster one, with the vast majority of the time spent polling the keyboard.
To solve this problem, this patch adds a 20ms delay between consecutive calls to `usb_kbd_poll_for_event`. This is sufficient to reduce the total loading time to under half a second for both keyboards, and does not impact the perceived keystroke latency.
Signed-off-by: Thomas Watson twatson52@icloud.com
This revision fixes a sandbox test failure by honoring the test's request to skip delays.
common/usb_kbd.c | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-)
We don't want to delay the CI tests which take enough time as it is. So time needs to be faked. It could be an if() though, rather than #if, perhaps?
Since there are multiple instances of the ifdef already , subsequent patch to clean them up would be fine.
That function `state_get_skip_delays()` and that state.h header file in general is only defined in the sandbox architecture, so I don't think it's possible to remove the #ifdef unless that is changed.
We can add it to a generic header, e.g. timer_skip_delays(), with a null implementation for other boards and a call to state_get_skip_delays() for sandbox. We have done that in a few other cases. I agree we should avoid arch-specific code.
Regards, Simon
participants (3)
-
Marek Vasut -
Simon Glass -
Thomas Watson