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

[Marek Vasut]

On 2/8/22 22:17, Thomas Watson wrote:
> 
>> 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.

The CI build fails on this patch:

https://source.denx.de/u-boot/custodians/u-boot-usb/-/pipelines/10922