[U-Boot] [PATCH 0/7] Add a better USB keyboard polling method
Hi Marek,
This time a patch-set for next :)
Currently one can choose between 2 poll methods for usb keyboards, both of which are suboptimal. One option is to use control messages to get reports, which some devices (e.g. my kvm) do not like. The other option is to use interrupt urbs, but usb_submit_int_msg waits for the interrupt packet to show up, meaning that each poll takes 40 ms, slowing anything else down tremendously.
This patch-sets adds a third method (only usable with ehci for now), which makes use of the int_queue concept in the ehci code. This allows us to submit an interrupt message, and then poll for the actual completion of this message giving us much lower latency then even the control message method (effectively this gives us 0 latency), while using standard interrupt messages which seems to keep keyboards much happier.
Regards,
Hans
When periodic_schedules == 0, the schedule is disabled and there is no reason to disable it again.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/usb/host/ehci-hcd.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index 6323c50..20830d7 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -1258,9 +1258,11 @@ create_int_queue(struct usb_device *dev, unsigned long pipe, int queuesize, ALIGN_END_ADDR(struct qTD, result->tds, queuesize));
- if (disable_periodic(ctrl) < 0) { - debug("FATAL: periodic should never fail, but did"); - goto fail3; + if (ctrl->periodic_schedules > 0) { + if (disable_periodic(ctrl) < 0) { + debug("FATAL: periodic should never fail, but did"); + goto fail3; + } }
/* hook up to periodic list */
Preperation patch to use create_int_queue outside of ehci-hcd.c .
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/usb/host/ehci-hcd.c | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index 20830d7..cf3e3c0 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -1163,6 +1163,23 @@ create_int_queue(struct usb_device *dev, unsigned long pipe, int queuesize, struct int_queue *result = NULL; int i;
+ /* + * Interrupt transfers requiring several transactions are not supported + * because bInterval is ignored. + * + * Also, ehci_submit_async() relies on wMaxPacketSize being a power of 2 + * <= PKT_ALIGN if several qTDs are required, while the USB + * specification does not constrain this for interrupt transfers. That + * means that ehci_submit_async() would support interrupt transfers + * requiring several transactions only as long as the transfer size does + * not require more than a single qTD. + */ + if (elementsize > usb_maxpacket(dev, pipe)) { + printf("%s: xfers requiring several transactions are not supported.\n", + __func__); + return NULL; + } + debug("Enter create_int_queue\n"); if (usb_pipetype(pipe) != PIPE_INTERRUPT) { debug("non-interrupt pipe (type=%lu)", usb_pipetype(pipe)); @@ -1384,24 +1401,9 @@ submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer, debug("dev=%p, pipe=%lu, buffer=%p, length=%d, interval=%d", dev, pipe, buffer, length, interval);
- /* - * Interrupt transfers requiring several transactions are not supported - * because bInterval is ignored. - * - * Also, ehci_submit_async() relies on wMaxPacketSize being a power of 2 - * <= PKT_ALIGN if several qTDs are required, while the USB - * specification does not constrain this for interrupt transfers. That - * means that ehci_submit_async() would support interrupt transfers - * requiring several transactions only as long as the transfer size does - * not require more than a single qTD. - */ - if (length > usb_maxpacket(dev, pipe)) { - printf("%s: Interrupt transfers requiring several " - "transactions are not supported.\n", __func__); - return -1; - } - queue = create_int_queue(dev, pipe, 1, length, buffer); + if (!queue) + return -1;
timeout = get_timer(0) + USB_TIMEOUT_MS(pipe); while ((backbuffer = poll_int_queue(dev, queue)) == NULL)
Hi
On Sat, Sep 20, 2014 at 5:01 PM, Hans de Goede hdegoede@redhat.com wrote:
Preperation patch to use create_int_queue outside of ehci-hcd.c .
Signed-off-by: Hans de Goede hdegoede@redhat.com
drivers/usb/host/ehci-hcd.c | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index 20830d7..cf3e3c0 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -1163,6 +1163,23 @@ create_int_queue(struct usb_device *dev, unsigned long pipe, int queuesize, struct int_queue *result = NULL; int i;
/** Interrupt transfers requiring several transactions are not supported* because bInterval is ignored.** Also, ehci_submit_async() relies on wMaxPacketSize being a power of 2* <= PKT_ALIGN if several qTDs are required, while the USB* specification does not constrain this for interrupt transfers. That* means that ehci_submit_async() would support interrupt transfers* requiring several transactions only as long as the transfer size does* not require more than a single qTD.*/if (elementsize > usb_maxpacket(dev, pipe)) {printf("%s: xfers requiring several transactions are not supported.\n",__func__);return NULL;}debug("Enter create_int_queue\n"); if (usb_pipetype(pipe) != PIPE_INTERRUPT) { debug("non-interrupt pipe (type=%lu)", usb_pipetype(pipe));@@ -1384,24 +1401,9 @@ submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer, debug("dev=%p, pipe=%lu, buffer=%p, length=%d, interval=%d", dev, pipe, buffer, length, interval);
/** Interrupt transfers requiring several transactions are not supported* because bInterval is ignored.** Also, ehci_submit_async() relies on wMaxPacketSize being a power of 2* <= PKT_ALIGN if several qTDs are required, while the USB* specification does not constrain this for interrupt transfers. That* means that ehci_submit_async() would support interrupt transfers* requiring several transactions only as long as the transfer size does* not require more than a single qTD.*/if (length > usb_maxpacket(dev, pipe)) {printf("%s: Interrupt transfers requiring several ""transactions are not supported.\n", __func__);return -1;}queue = create_int_queue(dev, pipe, 1, length, buffer);
if (!queue)return -1;
Can you return a more consistent error code?
Michael
timeout = get_timer(0) + USB_TIMEOUT_MS(pipe); while ((backbuffer = poll_int_queue(dev, queue)) == NULL)-- 2.1.0
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Hi,
On 09/20/2014 07:42 PM, Michael Trimarchi wrote:
Hi
On Sat, Sep 20, 2014 at 5:01 PM, Hans de Goede hdegoede@redhat.com wrote:
Preperation patch to use create_int_queue outside of ehci-hcd.c .
Signed-off-by: Hans de Goede hdegoede@redhat.com
drivers/usb/host/ehci-hcd.c | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index 20830d7..cf3e3c0 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -1163,6 +1163,23 @@ create_int_queue(struct usb_device *dev, unsigned long pipe, int queuesize, struct int_queue *result = NULL; int i;
/** Interrupt transfers requiring several transactions are not supported* because bInterval is ignored.** Also, ehci_submit_async() relies on wMaxPacketSize being a power of 2* <= PKT_ALIGN if several qTDs are required, while the USB* specification does not constrain this for interrupt transfers. That* means that ehci_submit_async() would support interrupt transfers* requiring several transactions only as long as the transfer size does* not require more than a single qTD.*/if (elementsize > usb_maxpacket(dev, pipe)) {printf("%s: xfers requiring several transactions are not supported.\n",__func__);return NULL;}debug("Enter create_int_queue\n"); if (usb_pipetype(pipe) != PIPE_INTERRUPT) { debug("non-interrupt pipe (type=%lu)", usb_pipetype(pipe));@@ -1384,24 +1401,9 @@ submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer, debug("dev=%p, pipe=%lu, buffer=%p, length=%d, interval=%d", dev, pipe, buffer, length, interval);
/** Interrupt transfers requiring several transactions are not supported* because bInterval is ignored.** Also, ehci_submit_async() relies on wMaxPacketSize being a power of 2* <= PKT_ALIGN if several qTDs are required, while the USB* specification does not constrain this for interrupt transfers. That* means that ehci_submit_async() would support interrupt transfers* requiring several transactions only as long as the transfer size does* not require more than a single qTD.*/if (length > usb_maxpacket(dev, pipe)) {printf("%s: Interrupt transfers requiring several ""transactions are not supported.\n", __func__);return -1;}queue = create_int_queue(dev, pipe, 1, length, buffer);
if (!queue)return -1;Can you return a more consistent error code?
I'm just moving code around, and returning the same error code as before. Surely changing the error code belongs in another patch ?
Regards,
Hans
On Sunday, September 21, 2014 at 07:53:35 PM, Hans de Goede wrote:
Hi,
[...]
if (length > usb_maxpacket(dev, pipe)) {printf("%s: Interrupt transfers requiring several ""transactions are not supported.\n", __func__);return -1;}queue = create_int_queue(dev, pipe, 1, length, buffer);
if (!queue)return -1;Can you return a more consistent error code?
I'm just moving code around, and returning the same error code as before. Surely changing the error code belongs in another patch ?
Yes, full ACK. This is exactly a prime examply where squashing two fixes into one patch would break bisectability absolutely perfectly.
Best regards, Marek Vasut
Hi Marek
On Sun, Sep 21, 2014 at 9:36 PM, Marek Vasut marex@denx.de wrote:
On Sunday, September 21, 2014 at 07:53:35 PM, Hans de Goede wrote:
Hi,
[...]
if (length > usb_maxpacket(dev, pipe)) {printf("%s: Interrupt transfers requiring several ""transactions are not supported.\n", __func__);return -1;}queue = create_int_queue(dev, pipe, 1, length, buffer);
if (!queue)return -1;Can you return a more consistent error code?
I'm just moving code around, and returning the same error code as before. Surely changing the error code belongs in another patch ?
Yes, full ACK. This is exactly a prime examply where squashing two fixes into one patch would break bisectability absolutely perfectly.
Agree on separated patch, I have just ask if Hans can do in the patches queue. Marek, thanks for the lesson. Anyway seems that in USB part we have already several -1 return.
Michael
Best regards, Marek Vasut
On Sunday, September 21, 2014 at 10:00:24 PM, Michael Trimarchi wrote:
Hi Marek
On Sun, Sep 21, 2014 at 9:36 PM, Marek Vasut marex@denx.de wrote:
On Sunday, September 21, 2014 at 07:53:35 PM, Hans de Goede wrote:
Hi,
[...]
if (length > usb_maxpacket(dev, pipe)) {printf("%s: Interrupt transfers requiring several ""transactions are not supported.\n",__func__); - return -1;
}queue = create_int_queue(dev, pipe, 1, length, buffer);
if (!queue)return -1;Can you return a more consistent error code?
I'm just moving code around, and returning the same error code as before. Surely changing the error code belongs in another patch ?
Yes, full ACK. This is exactly a prime examply where squashing two fixes into one patch would break bisectability absolutely perfectly.
Agree on separated patch, I have just ask if Hans can do in the patches queue. Marek, thanks for the lesson. Anyway seems that in USB part we have already several -1 return.
You know how it goes, patches are welcome ;-)
Best regards, Marek Vasut
Preperation patch to use poll_int_queue outside of ehci-hcd.c .
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/usb/host/ehci-hcd.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index cf3e3c0..e527a7a 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -1106,6 +1106,7 @@ submit_control_msg(struct usb_device *dev, unsigned long pipe, void *buffer, }
struct int_queue { + int elementsize; struct QH *first; struct QH *current; struct QH *last; @@ -1200,6 +1201,7 @@ create_int_queue(struct usb_device *dev, unsigned long pipe, int queuesize, debug("ehci intr queue: out of memory\n"); goto fail1; } + result->elementsize = elementsize; result->first = memalign(USB_DMA_MINALIGN, sizeof(struct QH) * queuesize); if (!result->first) { @@ -1336,6 +1338,11 @@ void *poll_int_queue(struct usb_device *dev, struct int_queue *queue) queue->current++; else queue->current = NULL; + + invalidate_dcache_range((uint32_t)cur->buffer, + ALIGN_END_ADDR(char, cur->buffer, + queue->elementsize)); + debug("Exit poll_int_queue with completed intr transfer. token is %x at %p (first at %p)\n", hc32_to_cpu(cur_td->qt_token), cur, queue->first); return cur->buffer; @@ -1419,9 +1426,6 @@ submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer, return -EINVAL; }
- invalidate_dcache_range((uint32_t)buffer, - ALIGN_END_ADDR(char, buffer, length)); - ret = destroy_int_queue(dev, queue); if (ret < 0) return ret;
Signed-off-by: Hans de Goede hdegoede@redhat.com --- include/usb.h | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/include/usb.h b/include/usb.h index c355fbe..6b29aed 100644 --- a/include/usb.h +++ b/include/usb.h @@ -129,6 +129,8 @@ struct usb_device { unsigned int slot_id; };
+struct int_queue; + /* * You can initialize platform's USB host or device * ports by passing this enum as an argument to @@ -162,6 +164,13 @@ int submit_control_msg(struct usb_device *dev, unsigned long pipe, void *buffer, int submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer, int transfer_len, int interval);
+#ifdef CONFIG_USB_EHCI /* Only the ehci code has pollable int support */ +struct int_queue *create_int_queue(struct usb_device *dev, unsigned long pipe, + int queuesize, int elementsize, void *buffer); +int destroy_int_queue(struct usb_device *dev, struct int_queue *queue); +void *poll_int_queue(struct usb_device *dev, struct int_queue *queue); +#endif + /* Defines */ #define USB_UHCI_VEND_ID 0x8086 #define USB_UHCI_DEV_ID 0x7112
This is not used anywhere, so lets remove it.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- common/usb_kbd.c | 26 -------------------------- 1 file changed, 26 deletions(-)
diff --git a/common/usb_kbd.c b/common/usb_kbd.c index fdc083c..cb869ac 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -116,32 +116,6 @@ 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;
-/* Generic keyboard event polling. */ -void usb_kbd_generic_poll(void) -{ - struct stdio_dev *dev; - struct usb_device *usb_kbd_dev; - struct usb_kbd_pdata *data; - struct usb_interface *iface; - struct usb_endpoint_descriptor *ep; - int pipe; - int maxp; - - /* Get the pointer to USB Keyboard device pointer */ - dev = stdio_get_by_name(DEVNAME); - usb_kbd_dev = (struct usb_device *)dev->priv; - data = usb_kbd_dev->privptr; - iface = &usb_kbd_dev->config.if_desc[0]; - ep = &iface->ep_desc[0]; - pipe = usb_rcvintpipe(usb_kbd_dev, ep->bEndpointAddress); - - /* Submit a interrupt transfer request */ - maxp = usb_maxpacket(usb_kbd_dev, pipe); - usb_submit_int_msg(usb_kbd_dev, pipe, data->new, - min(maxp, USB_KBD_BOOT_REPORT_SIZE), - ep->bInterval); -} - /* Puts character in the queue and sets up the in and out pointer. */ static void usb_kbd_put_queue(struct usb_kbd_pdata *data, char c) {
Instead of looking them up every time we need them.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- common/usb_kbd.c | 34 +++++++++++++--------------------- 1 file changed, 13 insertions(+), 21 deletions(-)
diff --git a/common/usb_kbd.c b/common/usb_kbd.c index cb869ac..85ee1c8 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -99,6 +99,10 @@ static const unsigned char usb_kbd_arrow[] = { #define USB_KBD_BOOT_REPORT_SIZE 8
struct usb_kbd_pdata { + unsigned long intpipe; + int intpktsize; + int intinterval; + uint32_t repeat_delay;
uint32_t usb_in_pointer; @@ -305,23 +309,11 @@ static int usb_kbd_irq(struct usb_device *dev) static inline void usb_kbd_poll_for_event(struct usb_device *dev) { #if defined(CONFIG_SYS_USB_EVENT_POLL) - struct usb_interface *iface; - struct usb_endpoint_descriptor *ep; - struct usb_kbd_pdata *data; - int pipe; - int maxp; - - /* Get the pointer to USB Keyboard device pointer */ - data = dev->privptr; - iface = &dev->config.if_desc[0]; - ep = &iface->ep_desc[0]; - pipe = usb_rcvintpipe(dev, ep->bEndpointAddress); + struct usb_kbd_pdata *data = dev->privptr;
/* Submit a interrupt transfer request */ - maxp = usb_maxpacket(dev, pipe); - usb_submit_int_msg(dev, pipe, &data->new[0], - min(maxp, USB_KBD_BOOT_REPORT_SIZE), - ep->bInterval); + usb_submit_int_msg(dev, data->intpipe, &data->new[0], data->intpktsize, + data->intinterval);
usb_kbd_irq_worker(dev); #elif defined(CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP) @@ -389,7 +381,6 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum) struct usb_interface *iface; struct usb_endpoint_descriptor *ep; struct usb_kbd_pdata *data; - int pipe, maxp;
if (dev->descriptor.bNumConfigurations != 1) return 0; @@ -438,8 +429,10 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum) /* Set IRQ handler */ dev->irq_handle = usb_kbd_irq;
- pipe = usb_rcvintpipe(dev, ep->bEndpointAddress); - maxp = usb_maxpacket(dev, pipe); + data->intpipe = usb_rcvintpipe(dev, ep->bEndpointAddress); + data->intpktsize = min(usb_maxpacket(dev, data->intpipe), + USB_KBD_BOOT_REPORT_SIZE); + data->intinterval = ep->bInterval;
/* We found a USB Keyboard, install it. */ usb_set_protocol(dev, iface->desc.bInterfaceNumber, 0); @@ -448,9 +441,8 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum) usb_set_idle(dev, iface->desc.bInterfaceNumber, REPEAT_RATE, 0);
debug("USB KBD: enable interrupt pipe...\n"); - if (usb_submit_int_msg(dev, pipe, data->new, - min(maxp, USB_KBD_BOOT_REPORT_SIZE), - ep->bInterval) < 0) { + if (usb_submit_int_msg(dev, data->intpipe, data->new, data->intpktsize, + data->intinterval) < 0) { printf("Failed to get keyboard state from device %04x:%04x\n", dev->descriptor.idVendor, dev->descriptor.idProduct); /* Abort, we don't want to use that non-functional keyboard. */
Hi
On Sat, Sep 20, 2014 at 5:01 PM, Hans de Goede hdegoede@redhat.com wrote:
Instead of looking them up every time we need them.
split subject and patch description
Signed-off-by: Hans de Goede hdegoede@redhat.com
common/usb_kbd.c | 34 +++++++++++++--------------------- 1 file changed, 13 insertions(+), 21 deletions(-)
diff --git a/common/usb_kbd.c b/common/usb_kbd.c index cb869ac..85ee1c8 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -99,6 +99,10 @@ static const unsigned char usb_kbd_arrow[] = { #define USB_KBD_BOOT_REPORT_SIZE 8
struct usb_kbd_pdata {
unsigned long intpipe;
unsigned int intpipe ??
Michael
int intpktsize;int intinterval;uint32_t repeat_delay; uint32_t usb_in_pointer;@@ -305,23 +309,11 @@ static int usb_kbd_irq(struct usb_device *dev) static inline void usb_kbd_poll_for_event(struct usb_device *dev) { #if defined(CONFIG_SYS_USB_EVENT_POLL)
struct usb_interface *iface;struct usb_endpoint_descriptor *ep;struct usb_kbd_pdata *data;int pipe;int maxp;/* Get the pointer to USB Keyboard device pointer */data = dev->privptr;iface = &dev->config.if_desc[0];ep = &iface->ep_desc[0];pipe = usb_rcvintpipe(dev, ep->bEndpointAddress);
struct usb_kbd_pdata *data = dev->privptr; /* Submit a interrupt transfer request */
maxp = usb_maxpacket(dev, pipe);usb_submit_int_msg(dev, pipe, &data->new[0],min(maxp, USB_KBD_BOOT_REPORT_SIZE),ep->bInterval);
usb_submit_int_msg(dev, data->intpipe, &data->new[0], data->intpktsize,data->intinterval); usb_kbd_irq_worker(dev);#elif defined(CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP) @@ -389,7 +381,6 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum) struct usb_interface *iface; struct usb_endpoint_descriptor *ep; struct usb_kbd_pdata *data;
int pipe, maxp; if (dev->descriptor.bNumConfigurations != 1) return 0;@@ -438,8 +429,10 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum) /* Set IRQ handler */ dev->irq_handle = usb_kbd_irq;
pipe = usb_rcvintpipe(dev, ep->bEndpointAddress);maxp = usb_maxpacket(dev, pipe);
data->intpipe = usb_rcvintpipe(dev, ep->bEndpointAddress);data->intpktsize = min(usb_maxpacket(dev, data->intpipe),USB_KBD_BOOT_REPORT_SIZE);data->intinterval = ep->bInterval; /* We found a USB Keyboard, install it. */ usb_set_protocol(dev, iface->desc.bInterfaceNumber, 0);@@ -448,9 +441,8 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum) usb_set_idle(dev, iface->desc.bInterfaceNumber, REPEAT_RATE, 0);
debug("USB KBD: enable interrupt pipe...\n");
if (usb_submit_int_msg(dev, pipe, data->new,min(maxp, USB_KBD_BOOT_REPORT_SIZE),ep->bInterval) < 0) {
if (usb_submit_int_msg(dev, data->intpipe, data->new, data->intpktsize,data->intinterval) < 0) { printf("Failed to get keyboard state from device %04x:%04x\n", dev->descriptor.idVendor, dev->descriptor.idProduct); /* Abort, we don't want to use that non-functional keyboard. */-- 2.1.0
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Hi,
On 09/20/2014 07:53 PM, Michael Trimarchi wrote:
Hi
On Sat, Sep 20, 2014 at 5:01 PM, Hans de Goede hdegoede@redhat.com wrote:
Instead of looking them up every time we need them.
split subject and patch description
Signed-off-by: Hans de Goede hdegoede@redhat.com
common/usb_kbd.c | 34 +++++++++++++--------------------- 1 file changed, 13 insertions(+), 21 deletions(-)
diff --git a/common/usb_kbd.c b/common/usb_kbd.c index cb869ac..85ee1c8 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -99,6 +99,10 @@ static const unsigned char usb_kbd_arrow[] = { #define USB_KBD_BOOT_REPORT_SIZE 8
struct usb_kbd_pdata {
unsigned long intpipe;unsigned int intpipe ??
usb "pipe"-s are unsigned long everywhere in u-boot.
Regards,
Hans
Michael
int intpktsize;int intinterval;uint32_t repeat_delay; uint32_t usb_in_pointer;@@ -305,23 +309,11 @@ static int usb_kbd_irq(struct usb_device *dev) static inline void usb_kbd_poll_for_event(struct usb_device *dev) { #if defined(CONFIG_SYS_USB_EVENT_POLL)
struct usb_interface *iface;struct usb_endpoint_descriptor *ep;struct usb_kbd_pdata *data;int pipe;int maxp;/* Get the pointer to USB Keyboard device pointer */data = dev->privptr;iface = &dev->config.if_desc[0];ep = &iface->ep_desc[0];pipe = usb_rcvintpipe(dev, ep->bEndpointAddress);
struct usb_kbd_pdata *data = dev->privptr; /* Submit a interrupt transfer request */
maxp = usb_maxpacket(dev, pipe);usb_submit_int_msg(dev, pipe, &data->new[0],min(maxp, USB_KBD_BOOT_REPORT_SIZE),ep->bInterval);
usb_submit_int_msg(dev, data->intpipe, &data->new[0], data->intpktsize,data->intinterval); usb_kbd_irq_worker(dev);#elif defined(CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP) @@ -389,7 +381,6 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum) struct usb_interface *iface; struct usb_endpoint_descriptor *ep; struct usb_kbd_pdata *data;
int pipe, maxp; if (dev->descriptor.bNumConfigurations != 1) return 0;@@ -438,8 +429,10 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum) /* Set IRQ handler */ dev->irq_handle = usb_kbd_irq;
pipe = usb_rcvintpipe(dev, ep->bEndpointAddress);maxp = usb_maxpacket(dev, pipe);
data->intpipe = usb_rcvintpipe(dev, ep->bEndpointAddress);data->intpktsize = min(usb_maxpacket(dev, data->intpipe),USB_KBD_BOOT_REPORT_SIZE);data->intinterval = ep->bInterval; /* We found a USB Keyboard, install it. */ usb_set_protocol(dev, iface->desc.bInterfaceNumber, 0);@@ -448,9 +441,8 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum) usb_set_idle(dev, iface->desc.bInterfaceNumber, REPEAT_RATE, 0);
debug("USB KBD: enable interrupt pipe...\n");
if (usb_submit_int_msg(dev, pipe, data->new,min(maxp, USB_KBD_BOOT_REPORT_SIZE),ep->bInterval) < 0) {
if (usb_submit_int_msg(dev, data->intpipe, data->new, data->intpktsize,data->intinterval) < 0) { printf("Failed to get keyboard state from device %04x:%04x\n", dev->descriptor.idVendor, dev->descriptor.idProduct); /* Abort, we don't want to use that non-functional keyboard. */-- 2.1.0
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Waiting an interrupt packet to complete in usb_kbd_poll_for_event, causes a 40 ms latency for each call to usb_kbd_testc, which is undesirable.
Using control messages leads to lower (but still not 0) latency, but some devices do not work well with control messages (e.g. my kvm behaves funny with them).
This commit adds support for using the int_queue mechanism which at least the ehci-hcd driver supports. This allows polling with 0 latency, while using interrupt packets.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- common/usb_kbd.c | 37 ++++++++++++++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 3 deletions(-)
diff --git a/common/usb_kbd.c b/common/usb_kbd.c index 85ee1c8..278937c 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -102,6 +102,7 @@ struct usb_kbd_pdata { unsigned long intpipe; int intpktsize; int intinterval; + struct int_queue *intq;
uint32_t repeat_delay;
@@ -324,6 +325,15 @@ static inline void usb_kbd_poll_for_event(struct usb_device *dev) 1, 0, data->new, USB_KBD_BOOT_REPORT_SIZE); if (memcmp(data->old, data->new, USB_KBD_BOOT_REPORT_SIZE)) usb_kbd_irq_worker(dev); +#elif defined(CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE) + struct usb_kbd_pdata *data = dev->privptr; + if (poll_int_queue(dev, data->intq)) { + usb_kbd_irq_worker(dev); + /* We've consumed all queued int packets, create new */ + destroy_int_queue(dev, data->intq); + data->intq = create_int_queue(dev, data->intpipe, 1, + USB_KBD_BOOT_REPORT_SIZE, data->new); + } #endif }
@@ -441,8 +451,14 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum) usb_set_idle(dev, iface->desc.bInterfaceNumber, REPEAT_RATE, 0);
debug("USB KBD: enable interrupt pipe...\n"); +#ifdef CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE + data->intq = create_int_queue(dev, data->intpipe, 1, + USB_KBD_BOOT_REPORT_SIZE, data->new); + if (!data->intq) { +#else if (usb_submit_int_msg(dev, data->intpipe, data->new, data->intpktsize, data->intinterval) < 0) { +#endif printf("Failed to get keyboard state from device %04x:%04x\n", dev->descriptor.idVendor, dev->descriptor.idProduct); /* Abort, we don't want to use that non-functional keyboard. */ @@ -515,13 +531,28 @@ int drv_usb_kbd_init(void) /* Deregister the keyboard. */ int usb_kbd_deregister(int force) { -#ifdef CONFIG_SYS_STDIO_DEREGISTER +#if !defined(CONFIG_SYS_STDIO_DEREGISTER) + return 1; +#elif defined(CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE) + struct stdio_dev *dev; + struct usb_device *usb_kbd_dev; + struct usb_kbd_pdata *data; + + dev = stdio_get_by_name(DEVNAME); + if (dev) { + if (stdio_deregister_dev(dev, force) != 0) + return 1; + usb_kbd_dev = (struct usb_device *)dev->priv; + data = usb_kbd_dev->privptr; + destroy_int_queue(usb_kbd_dev, data->intq); + } + + return 0; +#else int ret = stdio_deregister(DEVNAME, force); if (ret && ret != -ENODEV) return ret;
return 0; -#else - return 1; #endif }
Hi,
On 09/20/2014 05:01 PM, Hans de Goede wrote:
Waiting an interrupt packet to complete in usb_kbd_poll_for_event, causes a 40 ms latency for each call to usb_kbd_testc, which is undesirable.
Using control messages leads to lower (but still not 0) latency, but some devices do not work well with control messages (e.g. my kvm behaves funny with them).
This commit adds support for using the int_queue mechanism which at least the ehci-hcd driver supports. This allows polling with 0 latency, while using interrupt packets.
Signed-off-by: Hans de Goede hdegoede@redhat.com
common/usb_kbd.c | 37 ++++++++++++++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 3 deletions(-)
diff --git a/common/usb_kbd.c b/common/usb_kbd.c index 85ee1c8..278937c 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -102,6 +102,7 @@ struct usb_kbd_pdata { unsigned long intpipe; int intpktsize; int intinterval;
struct int_queue *intq;
uint32_t repeat_delay;
@@ -324,6 +325,15 @@ static inline void usb_kbd_poll_for_event(struct usb_device *dev) 1, 0, data->new, USB_KBD_BOOT_REPORT_SIZE); if (memcmp(data->old, data->new, USB_KBD_BOOT_REPORT_SIZE)) usb_kbd_irq_worker(dev); +#elif defined(CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE)
- struct usb_kbd_pdata *data = dev->privptr;
- if (poll_int_queue(dev, data->intq)) {
usb_kbd_irq_worker(dev);/* We've consumed all queued int packets, create new */destroy_int_queue(dev, data->intq);data->intq = create_int_queue(dev, data->intpipe, 1,USB_KBD_BOOT_REPORT_SIZE, data->new);- }
#endif }
@@ -441,8 +451,14 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum) usb_set_idle(dev, iface->desc.bInterfaceNumber, REPEAT_RATE, 0);
debug("USB KBD: enable interrupt pipe...\n"); +#ifdef CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE
- data->intq = create_int_queue(dev, data->intpipe, 1,
USB_KBD_BOOT_REPORT_SIZE, data->new);- if (!data->intq) {
+#else if (usb_submit_int_msg(dev, data->intpipe, data->new, data->intpktsize, data->intinterval) < 0) { +#endif printf("Failed to get keyboard state from device %04x:%04x\n", dev->descriptor.idVendor, dev->descriptor.idProduct); /* Abort, we don't want to use that non-functional keyboard. */ @@ -515,13 +531,28 @@ int drv_usb_kbd_init(void) /* Deregister the keyboard. */ int usb_kbd_deregister(int force) { -#ifdef CONFIG_SYS_STDIO_DEREGISTER +#if !defined(CONFIG_SYS_STDIO_DEREGISTER)
- return 1;
+#elif defined(CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE)
- struct stdio_dev *dev;
- struct usb_device *usb_kbd_dev;
- struct usb_kbd_pdata *data;
- dev = stdio_get_by_name(DEVNAME);
- if (dev) {
if (stdio_deregister_dev(dev, force) != 0)return 1;usb_kbd_dev = (struct usb_device *)dev->priv;
Hmm I've just realized that this is a use after free for dev. I've fixed this in my local tree. I'll wait for review of the other patches in this set before sending a v2.
Note this is not really a use after free, since stdio_deregister_dev does not free dev, but it reallu should free dev, since stdio_register_dev does a clone of the passed in dev, so stdio_deregister_dev should free it, so as to not leak memory. Once that memory leak gets fixed, this will be a use after free.
Likewise usb_kbd.c should free usb_kbd_pdata and the data->new buffer on deregister. I'll add a fix plugging the memleak in usb_kbd.c to v2 of this set.
Regards,
Hans
On Saturday, September 20, 2014 at 05:01:05 PM, Hans de Goede wrote:
Hi Marek,
This time a patch-set for next :)
Currently one can choose between 2 poll methods for usb keyboards, both of which are suboptimal. One option is to use control messages to get reports, which some devices (e.g. my kvm) do not like. The other option is to use interrupt urbs, but usb_submit_int_msg waits for the interrupt packet to show up, meaning that each poll takes 40 ms, slowing anything else down tremendously.
This patch-sets adds a third method (only usable with ehci for now), which makes use of the int_queue concept in the ehci code. This allows us to submit an interrupt message, and then poll for the actual completion of this message giving us much lower latency then even the control message method (effectively this gives us 0 latency), while using standard interrupt messages which seems to keep keyboards much happier.
I'd be happy to just pick V2 as it would be. The patches look really nice.
Best regards, Marek Vasut
participants (3)
-
Hans de Goede -
Marek Vasut -
Michael Trimarchi