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

[Simon Glass]

()Hi Thomas,

On Thu, 10 Feb 2022 at 18:43, Thomas Watson <twatson52 at icloud.com> wrote:
>
>
> On Feb 10, 2022, at 5:04 PM, Marek Vasut <marex at denx.de> wrote:
>
> On 2/11/22 00:02, Simon Glass wrote:
>
> Hi Marek,
> On Thu, 10 Feb 2022 at 14:42, 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(-)
>
> We don't want to delay the CI tests which take enough time as it is.
> So time needs to be faked.
> It could be an if() though, rather than #if, perhaps?
>
>
> Since there are multiple instances of the ifdef already , subsequent patch to clean them up would be fine.
>
>
> That function `state_get_skip_delays()` and that state.h header file in general
> is only defined in the sandbox architecture, so I don't think it's possible to
> remove the #ifdef unless that is changed.

We can add it to a generic header, e.g. timer_skip_delays(), with a
null implementation for other boards and a call to
state_get_skip_delays() for sandbox. We have done that in a few other
cases. I agree we should avoid arch-specific code.

Regards,
Simon