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

[Marek Vasut]

On 2/10/22 23:00, Thomas Watson wrote:
> 
>> 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.

I CCed Simon, maybe he can help out.