| Message ID | 20211222225124.7154-1-twatson52@icloud.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | [v2] console: usb: kbd: Limit poll frequency to improve performance | expand |
> From: Thomas Watson <twatson52@icloud.com> > Date: Wed, 22 Dec 2021 16:51:26 -0600 > > 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@icloud.com> > --- > This revision wraps the comment at 80 characters and also should not > have been corrupted by my e-mail client. This fixes a *major* annoyance with using the EFI boot path while having a USB keyboard plugged in, and the approach seems entirely reasonable to me and addresses the concerns raised by Heinrich in an earlier attempt to fix this issue: https://patchwork.ozlabs.org/project/uboot/patch/20210708082250.11833-1-mchang@suse.com/ Tested-by: Mark Kettenis <kettenis@openbsd.org> Reviewed-by: Mark Kettenis <kettenis@openbsd.org> > > 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 > >
> From: Thomas Watson <twatson52@icloud.com> > Date: Wed, 22 Dec 2021 16:51:26 -0600 > > 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@icloud.com> > --- > This revision wraps the comment at 80 characters and also should not > have been corrupted by my e-mail client. This fixes a *major* annoyance with using the EFI boot path while having a USB keyboard plugged in, and the approach seems entirely reasonable to me and addresses the concerns raised by Heinrich in an earlier attempt to fix this issue: https://patchwork.ozlabs.org/project/uboot/patch/20210708082250.11833-1-mchang@suse.com/ Tested-by: Mark Kettenis <kettenis@openbsd.org> Reviewed-by: Mark Kettenis <kettenis@openbsd.org> > > 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 > >
On Wed, Dec 22, 2021 at 11:16 PM Thomas Watson <twatson52@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@icloud.com> Tested-by: Peter Robinson <pbrobinson@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 >
> On Jan 8, 2022, at 8:10 AM, Peter Robinson <pbrobinson@gmail.com> wrote: > > On Wed, Dec 22, 2021 at 11:16 PM Thomas Watson <twatson52@icloud.com <mailto:twatson52@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@icloud.com> > Tested-by: Peter Robinson <pbrobinson@gmail.com <mailto:pbrobinson@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.
On 2/8/22 22:17, Thomas Watson wrote: > >> On Jan 8, 2022, at 8:10 AM, Peter Robinson <pbrobinson@gmail.com> wrote: >> >> On Wed, Dec 22, 2021 at 11:16 PM Thomas Watson <twatson52@icloud.com <mailto:twatson52@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@icloud.com> >> Tested-by: Peter Robinson <pbrobinson@gmail.com <mailto:pbrobinson@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
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); }
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@icloud.com> --- 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(-)