| Message ID | 1372326323-8661-1-git-send-email-jilin@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
On 06/27/2013 03:45 AM, Jim Lin wrote: > TFTP booting is observed a little bit slow, especially when a USB > keyboard is installed. > The fix is to move polling to every second if we sense that other task > like TFTP boot is running. > > diff --git a/common/usb_kbd.c b/common/usb_kbd.c > +#ifdef CONFIG_USBKB_TESTC_PERIOD > + /* > + * T is the time between two calls of usb_kbd_testc(). > + * If CONFIG_USBKB_TESTC_PERIOD ms < T < 1000 ms, > + * it implies other task like TFTP boot is running, > + * then we reduce polling to every second > + * to improve TFTP booting performance. > + */ > + if ((get_timer(kbd_testc_tms) >= > + (CONFIG_USBKB_TESTC_PERIOD * CONFIG_SYS_HZ / 1000)) && > + (get_timer(kbd_testc_tms) < CONFIG_SYS_HZ)) > + return 0; > + else > + kbd_testc_tms = get_timer(0); > +#endif I have a hard time understanding why the fact that "some other task is running" implies anything at all re: how often usb_kbd_testc() would be called. It's quite possible that "some other task" is extremely fine-grained, and calls usb_kbd_testc() every 0.1ms, and would be severely negatively affected by usb_kbd_testc() taking a long time to execute. Conversly, it's quite possible that "some other task" is quite granular, and calls usb_kbd_testc() a wide intervals, say every 200ms. So, I think this change keys of entirely the wrong thing. Shouldn't the TFTP process (or use of USB networking?) or other long-running tasks that do check for keyboard IO simply set some flag to indicate to usb_kbd_testc() that it should run at a reduced rate, or even just have those long-running processses call usb_kbd_testc() at a reduced rate themselves?
On Fri, 2013-06-28 at 02:20 +0800, Stephen Warren wrote: > On 06/27/2013 03:45 AM, Jim Lin wrote: > > TFTP booting is observed a little bit slow, especially when a USB > > keyboard is installed. > > The fix is to move polling to every second if we sense that other task > > like TFTP boot is running. > > > > > diff --git a/common/usb_kbd.c b/common/usb_kbd.c > > > +#ifdef CONFIG_USBKB_TESTC_PERIOD > > + /* > > + * T is the time between two calls of usb_kbd_testc(). > > + * If CONFIG_USBKB_TESTC_PERIOD ms < T < 1000 ms, > > + * it implies other task like TFTP boot is running, > > + * then we reduce polling to every second > > + * to improve TFTP booting performance. > > + */ > > + if ((get_timer(kbd_testc_tms) >= > > + (CONFIG_USBKB_TESTC_PERIOD * CONFIG_SYS_HZ / 1000)) && > > + (get_timer(kbd_testc_tms) < CONFIG_SYS_HZ)) > > + return 0; > > + else > > + kbd_testc_tms = get_timer(0); > > +#endif > > I have a hard time understanding why the fact that "some other task is > running" implies anything at all re: how often usb_kbd_testc() would be > called. In my case it takes about 95 ms on Tegra20 and Tegra114 for usb_kbd_testc() to be called periodically. So I set CONFIG_USBKB_TESTC_PERIOD to 100. Like I said, if CONFIG_USBKB_TESTC_PERIOD ms < T < 1000 ms we reduce polling (send command to USB keyboard to check is there any key pressed) to every second. > > It's quite possible that "some other task" is extremely fine-grained, > and calls usb_kbd_testc() every 0.1ms, and would be severely negatively > affected by usb_kbd_testc() taking a long time to execute. > > Conversly, it's quite possible that "some other task" is quite granular, > and calls usb_kbd_testc() a wide intervals, say every 200ms. > > So, I think this change keys of entirely the wrong thing. > > Shouldn't the TFTP process (or use of USB networking?) or other > long-running tasks that do check for keyboard IO simply set some flag to > indicate to usb_kbd_testc() that it should run at a reduced rate, or > even just have those long-running processses call usb_kbd_testc() at a > reduced rate themselves? To fix in usb_kbd_testc() is easier because this issue happens only when USB keyboard is installed and CONFIG_USB_KEYBOARD is defined. -- nvpublic
On 06/27/2013 09:59 PM, Jim Lin wrote: > On Fri, 2013-06-28 at 02:20 +0800, Stephen Warren wrote: >> On 06/27/2013 03:45 AM, Jim Lin wrote: >>> TFTP booting is observed a little bit slow, especially when a USB >>> keyboard is installed. >>> The fix is to move polling to every second if we sense that other task >>> like TFTP boot is running. >>> >> >>> diff --git a/common/usb_kbd.c b/common/usb_kbd.c >> >>> +#ifdef CONFIG_USBKB_TESTC_PERIOD >>> + /* >>> + * T is the time between two calls of usb_kbd_testc(). >>> + * If CONFIG_USBKB_TESTC_PERIOD ms < T < 1000 ms, >>> + * it implies other task like TFTP boot is running, >>> + * then we reduce polling to every second >>> + * to improve TFTP booting performance. >>> + */ >>> + if ((get_timer(kbd_testc_tms) >= >>> + (CONFIG_USBKB_TESTC_PERIOD * CONFIG_SYS_HZ / 1000)) && >>> + (get_timer(kbd_testc_tms) < CONFIG_SYS_HZ)) >>> + return 0; >>> + else >>> + kbd_testc_tms = get_timer(0); >>> +#endif >> >> I have a hard time understanding why the fact that "some other task is >> running" implies anything at all re: how often usb_kbd_testc() would be >> called. > In my case it takes about 95 ms on Tegra20 and Tegra114 for > usb_kbd_testc() to be called periodically. > So I set CONFIG_USBKB_TESTC_PERIOD to 100. > Like I said, if CONFIG_USBKB_TESTC_PERIOD ms < T < 1000 ms > we reduce polling (send command to USB keyboard to check is there > any key pressed) to every second. OK, so I think how this works is: If nothing is happening, then usb_kbd_testc() is repeatedly called back-to-back with no delay between. So, if the time between two calls to usb_kbd_testc() is much longer than the time it takes to execute it once, then something else is going on, and hence the code should skip some calls to usb_kbd_testc(). If that's how this works, then why require CONFIG_USBKBD_TESTC_PERIOD to be set? Why not simply measure the time between when usb_kbd_testc() returns, and when it is re-entered? If it's very short, nothing else is happening. If it's very long, something else is happening. That is a far more direct measurement, and is immune to e.g. CPU frequency differences in a way that a static value for CONFIG_USBKBD_TESTC_PERIOD is not. Also, any kind of time measurement doesn't solve the issue I mentioned re: how granular the other task is. Finally, if you're sitting at the command-prompt, is usb_kbd_testc() used at all? How does regular typing using a USB keyboard interact with this code; will typing react fast, but CTRL-C react slowly?
On Fri, 2013-06-28 at 13:09 +0800, Stephen Warren wrote: > On 06/27/2013 09:59 PM, Jim Lin wrote: > > On Fri, 2013-06-28 at 02:20 +0800, Stephen Warren wrote: > >> On 06/27/2013 03:45 AM, Jim Lin wrote: > >>> TFTP booting is observed a little bit slow, especially when a USB > >>> keyboard is installed. > >>> The fix is to move polling to every second if we sense that other task > >>> like TFTP boot is running. > >>> > >> > >>> diff --git a/common/usb_kbd.c b/common/usb_kbd.c > >> > >>> +#ifdef CONFIG_USBKB_TESTC_PERIOD > >>> + /* > >>> + * T is the time between two calls of usb_kbd_testc(). > >>> + * If CONFIG_USBKB_TESTC_PERIOD ms < T < 1000 ms, > >>> + * it implies other task like TFTP boot is running, > >>> + * then we reduce polling to every second > >>> + * to improve TFTP booting performance. > >>> + */ > >>> + if ((get_timer(kbd_testc_tms) >= > >>> + (CONFIG_USBKB_TESTC_PERIOD * CONFIG_SYS_HZ / 1000)) && > >>> + (get_timer(kbd_testc_tms) < CONFIG_SYS_HZ)) > >>> + return 0; > >>> + else > >>> + kbd_testc_tms = get_timer(0); > >>> +#endif > >> > >> I have a hard time understanding why the fact that "some other task is > >> running" implies anything at all re: how often usb_kbd_testc() would be > >> called. > > In my case it takes about 95 ms on Tegra20 and Tegra114 for > > usb_kbd_testc() to be called periodically. > > So I set CONFIG_USBKB_TESTC_PERIOD to 100. > > Like I said, if CONFIG_USBKB_TESTC_PERIOD ms < T < 1000 ms > > we reduce polling (send command to USB keyboard to check is there > > any key pressed) to every second. > > OK, so I think how this works is: If nothing is happening, then > usb_kbd_testc() is repeatedly called back-to-back with no delay between. > So, if the time between two calls to usb_kbd_testc() is much longer than > the time it takes to execute it once, then something else is going on, > and hence the code should skip some calls to usb_kbd_testc(). > > If that's how this works, then why require CONFIG_USBKBD_TESTC_PERIOD to > be set? Why not simply measure the time between when usb_kbd_testc() > returns, and when it is re-entered? If it's very short, nothing else is > happening. If it's very long, something else is happening. That is a far > more direct measurement, and is immune to e.g. CPU frequency differences Good idea and will be introduced in next revision. > in a way that a static value for CONFIG_USBKBD_TESTC_PERIOD is not. > > Also, any kind of time measurement doesn't solve the issue I mentioned > re: how granular the other task is. > > Finally, if you're sitting at the command-prompt, is usb_kbd_testc() > used at all? How does regular typing using a USB keyboard interact with > this code; will typing react fast, but CTRL-C react slowly? They should react at same rate. -- nvpublic
diff --git a/common/usb_kbd.c b/common/usb_kbd.c index 3174b5e..25bf677 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -121,6 +121,11 @@ struct usb_kbd_pdata { uint8_t flags; }; +#ifdef CONFIG_USBKB_TESTC_PERIOD +/* The period of time between two calls of usb_kbd_testc(). */ +static unsigned long kbd_testc_tms; +#endif + /* Generic keyboard event polling. */ void usb_kbd_generic_poll(void) { @@ -366,6 +371,21 @@ static int usb_kbd_testc(void) struct usb_device *usb_kbd_dev; struct usb_kbd_pdata *data; +#ifdef CONFIG_USBKB_TESTC_PERIOD + /* + * T is the time between two calls of usb_kbd_testc(). + * If CONFIG_USBKB_TESTC_PERIOD ms < T < 1000 ms, + * it implies other task like TFTP boot is running, + * then we reduce polling to every second + * to improve TFTP booting performance. + */ + if ((get_timer(kbd_testc_tms) >= + (CONFIG_USBKB_TESTC_PERIOD * CONFIG_SYS_HZ / 1000)) && + (get_timer(kbd_testc_tms) < CONFIG_SYS_HZ)) + return 0; + else + kbd_testc_tms = get_timer(0); +#endif dev = stdio_get_by_name(DEVNAME); usb_kbd_dev = (struct usb_device *)dev->priv; data = usb_kbd_dev->privptr; diff --git a/doc/README.usb b/doc/README.usb index b4c3ef5..ff4ee6f 100644 --- a/doc/README.usb +++ b/doc/README.usb @@ -80,7 +80,20 @@ CONFIG_USB_UHCI defines the lowlevel part.A lowlevel part must be defined CONFIG_USB_KEYBOARD enables the USB Keyboard CONFIG_USB_STORAGE enables the USB storage devices CONFIG_USB_HOST_ETHER enables USB ethernet adapter support - +CONFIG_USBKB_TESTC_PERIOD defines the average time (in ms) between two calls + of usb_kbd_testc() when TFTP boot is not running + In u-boot console, usb_kbd_testc() will be called + periodically. + When this configuration is defined and other task like + TFTP boot is running, USB keyboard will be polled less + frequently and will be polled every second. + This is to improve TFTP booting performance when USB + keyboard is installed. + In u-boot console, no impact on keyboard input if TFTP + boot is not running. + Example: + Define the following in configuration header file + #define CONFIG_USBKB_TESTC_PERIOD 100 USB Host Networking ===================
TFTP booting is observed a little bit slow, especially when a USB keyboard is installed. The fix is to move polling to every second if we sense that other task like TFTP boot is running. Signed-off-by: Jim Lin <jilin@nvidia.com> --- Changes in v2: 1. Change configuration name from CONFIG_CTRLC_POLL_MS to CONFIG_CTRLC_POLL_S. 2. New code will be executed only when CONFIG_CTRLC_POLL_S is defined in configuration header file. 3. Add description in README.console. Changes in v3: 1. Move changes to common/usb_kbd.c and doc/README.usb 2. Rename config setting to CONFIG_USBKB_TESTC_PERIOD. 3. Remove slow response on USB-keyboard input when TFTP boot is not running. common/usb_kbd.c | 20 ++++++++++++++++++++ doc/README.usb | 15 ++++++++++++++- 2 files changed, 34 insertions(+), 1 deletions(-)