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

[Thomas Watson]

> On Jan 8, 2022, at 8:10 AM, Peter Robinson <pbrobinson at gmail.com> wrote:
> 
> On Wed, Dec 22, 2021 at 11:16 PM Thomas Watson <twatson52 at icloud.com <mailto:twatson52 at icloud.com>> 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>
> Tested-by: Peter Robinson <pbrobinson at gmail.com <mailto:pbrobinson at gmail.com>>
> 
> Tested across a handful of devices booting Fedora via UEFI and LGTM.
>> ---
>> This revision wraps the comment at 80 characters and also should not
>> have been corrupted by my e-mail client.
>> 
>> common/usb_kbd.c | 20 +++++++++++++++-----
>> 1 file changed, 15 insertions(+), 5 deletions(-)
>> 
>> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
>> index afad260d3d..5408c821b4 100644
>> --- a/common/usb_kbd.c
>> +++ b/common/usb_kbd.c
>> @@ -118,7 +118,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 +394,31 @@ 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
>> +
>>        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);
>> }
>> --
>> 2.31.1

Hi, just wanted to know if there were any problems with this. I'd like it to get included as it's a big inconvenience that should be simple to fix.