[PATCH v3] console: usb: kbd: Limit poll frequency to improve performance

[Marek Vasut]

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 at 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?