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

[Thomas Watson]

> On Feb 10, 2022, at 3:42 PM, Marek Vasut <marex at 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 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?

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.