* [Qemu-devel] [PATCH 1/8] usb-redir: Split usb_handle_interrupt_data into separate in/out functions
2012-11-06 14:08 [Qemu-devel] usb: Move interrupt handling from poll to async handling Hans de Goede
@ 2012-11-06 14:08 ` Hans de Goede
2012-11-06 14:08 ` [Qemu-devel] [PATCH 2/8] usb-redir: Store interrupt receiving status in the bufp-queue Hans de Goede
` (7 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: Hans de Goede @ 2012-11-06 14:08 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Hans de Goede, qemu-devel
No functional changes.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
hw/usb/redirect.c | 136 ++++++++++++++++++++++++++++--------------------------
1 file changed, 71 insertions(+), 65 deletions(-)
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index d9236c5..9052ef8 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -618,80 +618,82 @@ static void usbredir_handle_bulk_data(USBRedirDevice *dev, USBPacket *p,
p->status = USB_RET_ASYNC;
}
-static void usbredir_handle_interrupt_data(USBRedirDevice *dev,
- USBPacket *p, uint8_t ep)
+static void usbredir_handle_interrupt_in_data(USBRedirDevice *dev,
+ USBPacket *p, uint8_t ep)
{
- if (ep & USB_DIR_IN) {
- /* Input interrupt endpoint, buffered packet input */
- struct buf_packet *intp;
- int status, len;
-
- if (!dev->endpoint[EP2I(ep)].interrupt_started &&
- !dev->endpoint[EP2I(ep)].interrupt_error) {
- struct usb_redir_start_interrupt_receiving_header start_int = {
- .endpoint = ep,
- };
- /* No id, we look at the ep when receiving a status back */
- usbredirparser_send_start_interrupt_receiving(dev->parser, 0,
- &start_int);
- usbredirparser_do_write(dev->parser);
- DPRINTF("interrupt recv started ep %02X\n", ep);
- dev->endpoint[EP2I(ep)].interrupt_started = 1;
- /* We don't really want to drop interrupt packets ever, but
- having some upper limit to how much we buffer is good. */
- dev->endpoint[EP2I(ep)].bufpq_target_size = 1000;
- dev->endpoint[EP2I(ep)].bufpq_dropping_packets = 0;
- }
+ /* Input interrupt endpoint, buffered packet input */
+ struct buf_packet *intp;
+ int status, len;
- intp = QTAILQ_FIRST(&dev->endpoint[EP2I(ep)].bufpq);
- if (intp == NULL) {
- DPRINTF2("interrupt-token-in ep %02X, no intp\n", ep);
- /* Check interrupt_error for stream errors */
- status = dev->endpoint[EP2I(ep)].interrupt_error;
- dev->endpoint[EP2I(ep)].interrupt_error = 0;
- if (status) {
- usbredir_handle_status(dev, p, status);
- } else {
- p->status = USB_RET_NAK;
- }
- return;
- }
- DPRINTF("interrupt-token-in ep %02X status %d len %d\n", ep,
- intp->status, intp->len);
+ if (!dev->endpoint[EP2I(ep)].interrupt_started &&
+ !dev->endpoint[EP2I(ep)].interrupt_error) {
+ struct usb_redir_start_interrupt_receiving_header start_int = {
+ .endpoint = ep,
+ };
+ /* No id, we look at the ep when receiving a status back */
+ usbredirparser_send_start_interrupt_receiving(dev->parser, 0,
+ &start_int);
+ usbredirparser_do_write(dev->parser);
+ DPRINTF("interrupt recv started ep %02X\n", ep);
+ dev->endpoint[EP2I(ep)].interrupt_started = 1;
+ /* We don't really want to drop interrupt packets ever, but
+ having some upper limit to how much we buffer is good. */
+ dev->endpoint[EP2I(ep)].bufpq_target_size = 1000;
+ dev->endpoint[EP2I(ep)].bufpq_dropping_packets = 0;
+ }
- status = intp->status;
- len = intp->len;
- if (len > p->iov.size) {
- ERROR("received int data is larger then packet ep %02X\n", ep);
- len = p->iov.size;
- status = usb_redir_babble;
+ intp = QTAILQ_FIRST(&dev->endpoint[EP2I(ep)].bufpq);
+ if (intp == NULL) {
+ DPRINTF2("interrupt-token-in ep %02X, no intp\n", ep);
+ /* Check interrupt_error for stream errors */
+ status = dev->endpoint[EP2I(ep)].interrupt_error;
+ dev->endpoint[EP2I(ep)].interrupt_error = 0;
+ if (status) {
+ usbredir_handle_status(dev, p, status);
+ } else {
+ p->status = USB_RET_NAK;
}
- usb_packet_copy(p, intp->data, len);
- bufp_free(dev, intp, ep);
- usbredir_handle_status(dev, p, status);
- } else {
- /* Output interrupt endpoint, normal async operation */
- struct usb_redir_interrupt_packet_header interrupt_packet;
- uint8_t buf[p->iov.size];
+ return;
+ }
+ DPRINTF("interrupt-token-in ep %02X status %d len %d\n", ep,
+ intp->status, intp->len);
- DPRINTF("interrupt-out ep %02X len %zd id %"PRIu64"\n", ep,
- p->iov.size, p->id);
+ status = intp->status;
+ len = intp->len;
+ if (len > p->iov.size) {
+ ERROR("received int data is larger then packet ep %02X\n", ep);
+ len = p->iov.size;
+ status = usb_redir_babble;
+ }
+ usb_packet_copy(p, intp->data, len);
+ bufp_free(dev, intp, ep);
+ usbredir_handle_status(dev, p, status);
+}
- if (usbredir_already_in_flight(dev, p->id)) {
- p->status = USB_RET_ASYNC;
- return;
- }
+static void usbredir_handle_interrupt_out_data(USBRedirDevice *dev,
+ USBPacket *p, uint8_t ep)
+{
+ /* Output interrupt endpoint, normal async operation */
+ struct usb_redir_interrupt_packet_header interrupt_packet;
+ uint8_t buf[p->iov.size];
- interrupt_packet.endpoint = ep;
- interrupt_packet.length = p->iov.size;
+ DPRINTF("interrupt-out ep %02X len %zd id %"PRIu64"\n", ep,
+ p->iov.size, p->id);
- usb_packet_copy(p, buf, p->iov.size);
- usbredir_log_data(dev, "interrupt data out:", buf, p->iov.size);
- usbredirparser_send_interrupt_packet(dev->parser, p->id,
- &interrupt_packet, buf, p->iov.size);
- usbredirparser_do_write(dev->parser);
+ if (usbredir_already_in_flight(dev, p->id)) {
p->status = USB_RET_ASYNC;
+ return;
}
+
+ interrupt_packet.endpoint = ep;
+ interrupt_packet.length = p->iov.size;
+
+ usb_packet_copy(p, buf, p->iov.size);
+ usbredir_log_data(dev, "interrupt data out:", buf, p->iov.size);
+ usbredirparser_send_interrupt_packet(dev->parser, p->id,
+ &interrupt_packet, buf, p->iov.size);
+ usbredirparser_do_write(dev->parser);
+ p->status = USB_RET_ASYNC;
}
static void usbredir_stop_interrupt_receiving(USBRedirDevice *dev,
@@ -737,7 +739,11 @@ static void usbredir_handle_data(USBDevice *udev, USBPacket *p)
usbredir_handle_bulk_data(dev, p, ep);
break;
case USB_ENDPOINT_XFER_INT:
- usbredir_handle_interrupt_data(dev, p, ep);
+ if (ep & USB_DIR_IN) {
+ usbredir_handle_interrupt_in_data(dev, p, ep);
+ } else {
+ usbredir_handle_interrupt_out_data(dev, p, ep);
+ }
break;
default:
ERROR("handle_data ep %02X has unknown type %d\n", ep,
--
1.7.12.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH 2/8] usb-redir: Store interrupt receiving status in the bufp-queue
2012-11-06 14:08 [Qemu-devel] usb: Move interrupt handling from poll to async handling Hans de Goede
2012-11-06 14:08 ` [Qemu-devel] [PATCH 1/8] usb-redir: Split usb_handle_interrupt_data into separate in/out functions Hans de Goede
@ 2012-11-06 14:08 ` Hans de Goede
2012-11-07 9:51 ` Paolo Bonzini
2012-11-06 14:08 ` [Qemu-devel] [PATCH 3/8] usb-redir: Only add actually in flight packets to the in flight queue Hans de Goede
` (6 subsequent siblings)
8 siblings, 1 reply; 15+ messages in thread
From: Hans de Goede @ 2012-11-06 14:08 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Hans de Goede, qemu-devel
Since we handle interrupt out async, and not buffered like iso-out, there is
no need for a separate status flag, instead store any reported error status
into the bufp queue.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
hw/usb/redirect.c | 24 +++++++++---------------
1 file changed, 9 insertions(+), 15 deletions(-)
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 9052ef8..b61bb6e 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -58,11 +58,11 @@ struct endp_data {
uint8_t type;
uint8_t interval;
uint8_t interface; /* bInterfaceNumber this ep belongs to */
+ uint8_t zero; /* Was interrupt_error, now always 0 for migration compat */
uint16_t max_packet_size; /* In bytes, not wMaxPacketSize format !! */
uint8_t iso_started;
uint8_t iso_error; /* For reporting iso errors to the HC */
uint8_t interrupt_started;
- uint8_t interrupt_error;
uint8_t bufpq_prefilled;
uint8_t bufpq_dropping_packets;
QTAILQ_HEAD(, buf_packet) bufpq;
@@ -626,7 +626,7 @@ static void usbredir_handle_interrupt_in_data(USBRedirDevice *dev,
int status, len;
if (!dev->endpoint[EP2I(ep)].interrupt_started &&
- !dev->endpoint[EP2I(ep)].interrupt_error) {
+ QTAILQ_EMPTY(&dev->endpoint[EP2I(ep)].bufpq)) {
struct usb_redir_start_interrupt_receiving_header start_int = {
.endpoint = ep,
};
@@ -645,14 +645,7 @@ static void usbredir_handle_interrupt_in_data(USBRedirDevice *dev,
intp = QTAILQ_FIRST(&dev->endpoint[EP2I(ep)].bufpq);
if (intp == NULL) {
DPRINTF2("interrupt-token-in ep %02X, no intp\n", ep);
- /* Check interrupt_error for stream errors */
- status = dev->endpoint[EP2I(ep)].interrupt_error;
- dev->endpoint[EP2I(ep)].interrupt_error = 0;
- if (status) {
- usbredir_handle_status(dev, p, status);
- } else {
- p->status = USB_RET_NAK;
- }
+ p->status = USB_RET_NAK;
return;
}
DPRINTF("interrupt-token-in ep %02X status %d len %d\n", ep,
@@ -708,7 +701,6 @@ static void usbredir_stop_interrupt_receiving(USBRedirDevice *dev,
DPRINTF("interrupt recv stopped ep %02X\n", ep);
dev->endpoint[EP2I(ep)].interrupt_started = 0;
}
- dev->endpoint[EP2I(ep)].interrupt_error = 0;
usbredir_free_bufpq(dev, ep);
}
@@ -1513,20 +1505,22 @@ static void usbredir_interrupt_receiving_status(void *priv, uint64_t id,
{
USBRedirDevice *dev = priv;
uint8_t ep = interrupt_receiving_status->endpoint;
+ uint8_t status = interrupt_receiving_status->status;
DPRINTF("interrupt recv status %d ep %02X id %"PRIu64"\n",
interrupt_receiving_status->status, ep, id);
- if (!dev->dev.attached || !dev->endpoint[EP2I(ep)].interrupt_started) {
+ if (!dev->dev.attached || !dev->endpoint[EP2I(ep)].interrupt_started ||
+ status == usb_redir_success) {
return;
}
- dev->endpoint[EP2I(ep)].interrupt_error =
- interrupt_receiving_status->status;
if (interrupt_receiving_status->status == usb_redir_stall) {
DPRINTF("interrupt receiving stopped by peer ep %02X\n", ep);
dev->endpoint[EP2I(ep)].interrupt_started = 0;
+ status = usb_redir_ioerror;
}
+ bufp_alloc(dev, NULL, 0, status, ep);
}
static void usbredir_bulk_streams_status(void *priv, uint64_t id,
@@ -1833,7 +1827,7 @@ static const VMStateDescription usbredir_ep_vmstate = {
VMSTATE_UINT8(iso_started, struct endp_data),
VMSTATE_UINT8(iso_error, struct endp_data),
VMSTATE_UINT8(interrupt_started, struct endp_data),
- VMSTATE_UINT8(interrupt_error, struct endp_data),
+ VMSTATE_UINT8(zero, struct endp_data), /* Was interrupt_error */
VMSTATE_UINT8(bufpq_prefilled, struct endp_data),
VMSTATE_UINT8(bufpq_dropping_packets, struct endp_data),
{
--
1.7.12.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH 2/8] usb-redir: Store interrupt receiving status in the bufp-queue
2012-11-06 14:08 ` [Qemu-devel] [PATCH 2/8] usb-redir: Store interrupt receiving status in the bufp-queue Hans de Goede
@ 2012-11-07 9:51 ` Paolo Bonzini
0 siblings, 0 replies; 15+ messages in thread
From: Paolo Bonzini @ 2012-11-07 9:51 UTC (permalink / raw)
To: Hans de Goede; +Cc: Gerd Hoffmann, qemu-devel
Il 06/11/2012 15:08, Hans de Goede ha scritto:
> Since we handle interrupt out async, and not buffered like iso-out, there is
> no need for a separate status flag, instead store any reported error status
> into the bufp queue.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> hw/usb/redirect.c | 24 +++++++++---------------
> 1 file changed, 9 insertions(+), 15 deletions(-)
>
> diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> index 9052ef8..b61bb6e 100644
> --- a/hw/usb/redirect.c
> +++ b/hw/usb/redirect.c
> @@ -58,11 +58,11 @@ struct endp_data {
> uint8_t type;
> uint8_t interval;
> uint8_t interface; /* bInterfaceNumber this ep belongs to */
> + uint8_t zero; /* Was interrupt_error, now always 0 for migration compat */
No need for this, use VMSTATE_UNUSED.
But if you do this, you need to bump the migration version because the
old version will not be able to retrieve the interrupt_error value. Can
you somewhat figure out it in the pre_load callback instead?
Hmm... well, migration from usbredir is new in 1.3, so you do have time
to change the protocol. You do not need zero, you do not need
VMSTATE_UNUSED. But you'd better do this change as well in a certain
downstream you care about, otherwise you'll have the same problem soon.
Paolo
> uint16_t max_packet_size; /* In bytes, not wMaxPacketSize format !! */
> uint8_t iso_started;
> uint8_t iso_error; /* For reporting iso errors to the HC */
> uint8_t interrupt_started;
> - uint8_t interrupt_error;
> uint8_t bufpq_prefilled;
> uint8_t bufpq_dropping_packets;
> QTAILQ_HEAD(, buf_packet) bufpq;
> @@ -626,7 +626,7 @@ static void usbredir_handle_interrupt_in_data(USBRedirDevice *dev,
> int status, len;
>
> if (!dev->endpoint[EP2I(ep)].interrupt_started &&
> - !dev->endpoint[EP2I(ep)].interrupt_error) {
> + QTAILQ_EMPTY(&dev->endpoint[EP2I(ep)].bufpq)) {
> struct usb_redir_start_interrupt_receiving_header start_int = {
> .endpoint = ep,
> };
> @@ -645,14 +645,7 @@ static void usbredir_handle_interrupt_in_data(USBRedirDevice *dev,
> intp = QTAILQ_FIRST(&dev->endpoint[EP2I(ep)].bufpq);
> if (intp == NULL) {
> DPRINTF2("interrupt-token-in ep %02X, no intp\n", ep);
> - /* Check interrupt_error for stream errors */
> - status = dev->endpoint[EP2I(ep)].interrupt_error;
> - dev->endpoint[EP2I(ep)].interrupt_error = 0;
> - if (status) {
> - usbredir_handle_status(dev, p, status);
> - } else {
> - p->status = USB_RET_NAK;
> - }
> + p->status = USB_RET_NAK;
> return;
> }
> DPRINTF("interrupt-token-in ep %02X status %d len %d\n", ep,
> @@ -708,7 +701,6 @@ static void usbredir_stop_interrupt_receiving(USBRedirDevice *dev,
> DPRINTF("interrupt recv stopped ep %02X\n", ep);
> dev->endpoint[EP2I(ep)].interrupt_started = 0;
> }
> - dev->endpoint[EP2I(ep)].interrupt_error = 0;
> usbredir_free_bufpq(dev, ep);
> }
>
> @@ -1513,20 +1505,22 @@ static void usbredir_interrupt_receiving_status(void *priv, uint64_t id,
> {
> USBRedirDevice *dev = priv;
> uint8_t ep = interrupt_receiving_status->endpoint;
> + uint8_t status = interrupt_receiving_status->status;
>
> DPRINTF("interrupt recv status %d ep %02X id %"PRIu64"\n",
> interrupt_receiving_status->status, ep, id);
>
> - if (!dev->dev.attached || !dev->endpoint[EP2I(ep)].interrupt_started) {
> + if (!dev->dev.attached || !dev->endpoint[EP2I(ep)].interrupt_started ||
> + status == usb_redir_success) {
> return;
> }
>
> - dev->endpoint[EP2I(ep)].interrupt_error =
> - interrupt_receiving_status->status;
> if (interrupt_receiving_status->status == usb_redir_stall) {
> DPRINTF("interrupt receiving stopped by peer ep %02X\n", ep);
> dev->endpoint[EP2I(ep)].interrupt_started = 0;
> + status = usb_redir_ioerror;
> }
> + bufp_alloc(dev, NULL, 0, status, ep);
> }
>
> static void usbredir_bulk_streams_status(void *priv, uint64_t id,
> @@ -1833,7 +1827,7 @@ static const VMStateDescription usbredir_ep_vmstate = {
> VMSTATE_UINT8(iso_started, struct endp_data),
> VMSTATE_UINT8(iso_error, struct endp_data),
> VMSTATE_UINT8(interrupt_started, struct endp_data),
> - VMSTATE_UINT8(interrupt_error, struct endp_data),
> + VMSTATE_UINT8(zero, struct endp_data), /* Was interrupt_error */
> VMSTATE_UINT8(bufpq_prefilled, struct endp_data),
> VMSTATE_UINT8(bufpq_dropping_packets, struct endp_data),
> {
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 3/8] usb-redir: Only add actually in flight packets to the in flight queue
2012-11-06 14:08 [Qemu-devel] usb: Move interrupt handling from poll to async handling Hans de Goede
2012-11-06 14:08 ` [Qemu-devel] [PATCH 1/8] usb-redir: Split usb_handle_interrupt_data into separate in/out functions Hans de Goede
2012-11-06 14:08 ` [Qemu-devel] [PATCH 2/8] usb-redir: Store interrupt receiving status in the bufp-queue Hans de Goede
@ 2012-11-06 14:08 ` Hans de Goede
2012-11-06 14:08 ` [Qemu-devel] [PATCH 4/8] usb-redir: Handle interrupt packets async Hans de Goede
` (5 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: Hans de Goede @ 2012-11-06 14:08 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Hans de Goede, qemu-devel
Packets which have queued up, but not yet handed over to the device, are
*not* in flight.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
hw/usb/redirect.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index b61bb6e..ad601d8 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -352,7 +352,9 @@ static void usbredir_fill_already_in_flight_from_ep(USBRedirDevice *dev,
if (p->combined && p != p->combined->first) {
continue;
}
- packet_id_queue_add(&dev->already_in_flight, p->id);
+ if (p->state == USB_PACKET_ASYNC) {
+ packet_id_queue_add(&dev->already_in_flight, p->id);
+ }
}
}
--
1.7.12.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH 4/8] usb-redir: Handle interrupt packets async
2012-11-06 14:08 [Qemu-devel] usb: Move interrupt handling from poll to async handling Hans de Goede
` (2 preceding siblings ...)
2012-11-06 14:08 ` [Qemu-devel] [PATCH 3/8] usb-redir: Only add actually in flight packets to the in flight queue Hans de Goede
@ 2012-11-06 14:08 ` Hans de Goede
2012-11-06 14:08 ` [Qemu-devel] [PATCH 5/8] ehci: Lower timer freq when there are no iso packets in the periodic schedule Hans de Goede
` (4 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: Hans de Goede @ 2012-11-06 14:08 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Hans de Goede, qemu-devel
This allows to hcd code to slow down its timer, rather then having to
poll us every ms.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
hw/usb/redirect.c | 64 ++++++++++++++++++++++++++++++++++++++++---------------
1 file changed, 47 insertions(+), 17 deletions(-)
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index ad601d8..5367876 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -68,6 +68,7 @@ struct endp_data {
QTAILQ_HEAD(, buf_packet) bufpq;
int32_t bufpq_size;
int32_t bufpq_target_size;
+ USBPacket *pending_async_packet;
};
struct PacketIdQueueEntry {
@@ -323,12 +324,23 @@ static void packet_id_queue_empty(struct PacketIdQueue *q)
static void usbredir_cancel_packet(USBDevice *udev, USBPacket *p)
{
USBRedirDevice *dev = DO_UPCAST(USBRedirDevice, dev, udev);
+ int ep_idx;
if (p->combined) {
usb_combined_packet_cancel(udev, p);
return;
}
+ ep_idx = p->ep->nr;
+ if (p->pid == USB_TOKEN_IN) {
+ ep_idx += 16;
+ }
+ if (dev->endpoint[ep_idx].pending_async_packet) {
+ assert(dev->endpoint[ep_idx].pending_async_packet == p);
+ dev->endpoint[ep_idx].pending_async_packet = NULL;
+ return;
+ }
+
packet_id_queue_add(&dev->cancelled, p->id);
usbredirparser_send_cancel_data_packet(dev->parser, p->id);
usbredirparser_do_write(dev->parser);
@@ -366,8 +378,12 @@ static void usbredir_fill_already_in_flight(USBRedirDevice *dev)
usbredir_fill_already_in_flight_from_ep(dev, &udev->ep_ctl);
for (ep = 0; ep < USB_MAX_ENDPOINTS; ep++) {
- usbredir_fill_already_in_flight_from_ep(dev, &udev->ep_in[ep]);
- usbredir_fill_already_in_flight_from_ep(dev, &udev->ep_out[ep]);
+ if (udev->ep_in[ep].type == USB_ENDPOINT_XFER_BULK) {
+ usbredir_fill_already_in_flight_from_ep(dev, &udev->ep_in[ep]);
+ }
+ if (udev->ep_out[ep].type == USB_ENDPOINT_XFER_BULK) {
+ usbredir_fill_already_in_flight_from_ep(dev, &udev->ep_out[ep]);
+ }
}
}
@@ -620,12 +636,25 @@ static void usbredir_handle_bulk_data(USBRedirDevice *dev, USBPacket *p,
p->status = USB_RET_ASYNC;
}
+static void usbredir_interrupt_in_complete(USBRedirDevice *dev, USBPacket *p,
+ uint8_t ep, int status, uint8_t *data, int len)
+{
+ DPRINTF("interrupt-token-in ep %02X status %d len %d\n", ep, status, len);
+
+ if (len > p->iov.size) {
+ ERROR("received int data is larger then packet ep %02X\n", ep);
+ len = p->iov.size;
+ status = usb_redir_babble;
+ }
+ usb_packet_copy(p, data, len);
+ usbredir_handle_status(dev, p, status);
+}
+
static void usbredir_handle_interrupt_in_data(USBRedirDevice *dev,
USBPacket *p, uint8_t ep)
{
/* Input interrupt endpoint, buffered packet input */
struct buf_packet *intp;
- int status, len;
if (!dev->endpoint[EP2I(ep)].interrupt_started &&
QTAILQ_EMPTY(&dev->endpoint[EP2I(ep)].bufpq)) {
@@ -647,22 +676,15 @@ static void usbredir_handle_interrupt_in_data(USBRedirDevice *dev,
intp = QTAILQ_FIRST(&dev->endpoint[EP2I(ep)].bufpq);
if (intp == NULL) {
DPRINTF2("interrupt-token-in ep %02X, no intp\n", ep);
- p->status = USB_RET_NAK;
+ assert(dev->endpoint[EP2I(ep)].pending_async_packet == NULL);
+ dev->endpoint[EP2I(ep)].pending_async_packet = p;
+ p->status = USB_RET_ASYNC;
return;
}
- DPRINTF("interrupt-token-in ep %02X status %d len %d\n", ep,
- intp->status, intp->len);
- status = intp->status;
- len = intp->len;
- if (len > p->iov.size) {
- ERROR("received int data is larger then packet ep %02X\n", ep);
- len = p->iov.size;
- status = usb_redir_babble;
- }
- usb_packet_copy(p, intp->data, len);
+ usbredir_interrupt_in_complete(dev, p, ep, intp->status,
+ intp->data, intp->len);
bufp_free(dev, intp, ep);
- usbredir_handle_status(dev, p, status);
}
static void usbredir_handle_interrupt_out_data(USBRedirDevice *dev,
@@ -1661,8 +1683,16 @@ static void usbredir_interrupt_packet(void *priv, uint64_t id,
return;
}
- /* bufp_alloc also adds the packet to the ep queue */
- bufp_alloc(dev, data, data_len, interrupt_packet->status, ep);
+ if (dev->endpoint[EP2I(ep)].pending_async_packet) {
+ USBPacket *p = dev->endpoint[EP2I(ep)].pending_async_packet;
+ usbredir_interrupt_in_complete(dev, p, ep,
+ interrupt_packet->status, data, data_len);
+ usb_packet_complete(&dev->dev, p);
+ dev->endpoint[EP2I(ep)].pending_async_packet = NULL;
+ } else {
+ /* bufp_alloc also adds the packet to the ep queue */
+ bufp_alloc(dev, data, data_len, interrupt_packet->status, ep);
+ }
} else {
USBPacket *p = usbredir_find_packet_by_id(dev, ep, id);
if (p) {
--
1.7.12.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH 5/8] ehci: Lower timer freq when there are no iso packets in the periodic schedule
2012-11-06 14:08 [Qemu-devel] usb: Move interrupt handling from poll to async handling Hans de Goede
` (3 preceding siblings ...)
2012-11-06 14:08 ` [Qemu-devel] [PATCH 4/8] usb-redir: Handle interrupt packets async Hans de Goede
@ 2012-11-06 14:08 ` Hans de Goede
2012-11-06 14:08 ` [Qemu-devel] [PATCH 6/8] hid: Change idle handling to use a timer Hans de Goede
` (3 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: Hans de Goede @ 2012-11-06 14:08 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Hans de Goede, qemu-devel
If there are no iso packets in the periodic schedule, then there can only
be interrupt packets there, and for these usb-devices either return nak,
meaning that the executing state will get passed every frame, causing
async_stepdown to stay 0, or they are handled async, and then ehci_frame_timer
will get called immediately from a bh, thus not introducing any latency
to async handled interrupt packets.
The advantage of this is that when we only have async handled interrupt
packets in the periodic schedule, async_stepdown can do its work and
significantly lower the frequency at which the frame_timer runs.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
hw/usb/hcd-ehci.c | 18 ++++++++++++++----
hw/usb/hcd-ehci.h | 1 +
2 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index ee6c9ae..b804474 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -114,6 +114,7 @@
#define BUFF_SIZE 5*4096 // Max bytes to transfer per transaction
#define MAX_QH 100 // Max allowable queue heads in a chain
#define MIN_FR_PER_TICK 3 // Min frames to process when catching up
+#define ISO_ACTIVE_COUNT 64
/* Internal periodic / asynchronous schedule state machine states
*/
@@ -1325,6 +1326,8 @@ static int ehci_process_itd(EHCIState *ehci,
uint32_t i, len, pid, dir, devaddr, endp;
uint32_t pg, off, ptr1, ptr2, max, mult;
+ ehci->iso_active_counter = ISO_ACTIVE_COUNT;
+
dir =(itd->bufptr[1] & ITD_BUFPTR_DIRECTION);
devaddr = get_field(itd->bufptr[0], ITD_BUFPTR_DEVADDR);
endp = get_field(itd->bufptr[0], ITD_BUFPTR_EP);
@@ -2157,9 +2160,12 @@ static void ehci_frame_timer(void *opaque)
ns_elapsed = t_now - ehci->last_run_ns;
frames = ns_elapsed / FRAME_TIMER_NS;
+ if (ehci->async_stepdown < ehci->maxframes / 2) {
+ ehci->async_stepdown++;
+ }
+
if (ehci_periodic_enabled(ehci) || ehci->pstate != EST_INACTIVE) {
need_timer++;
- ehci->async_stepdown = 0;
if (frames > ehci->maxframes) {
skipped_frames = frames - ehci->maxframes;
@@ -2183,14 +2189,18 @@ static void ehci_frame_timer(void *opaque)
break;
}
}
+ if (ehci->iso_active_counter) {
+ ehci->iso_active_counter--;
+ }
ehci_update_frindex(ehci, 1);
ehci_advance_periodic_state(ehci);
ehci->last_run_ns += FRAME_TIMER_NS;
}
- } else {
- if (ehci->async_stepdown < ehci->maxframes / 2) {
- ehci->async_stepdown++;
+
+ if (ehci->iso_active_counter) {
+ ehci->async_stepdown = 0;
}
+ } else {
ehci_update_frindex(ehci, frames);
ehci->last_run_ns += FRAME_TIMER_NS * frames;
}
diff --git a/hw/usb/hcd-ehci.h b/hw/usb/hcd-ehci.h
index d8078f4..ed9d89c 100644
--- a/hw/usb/hcd-ehci.h
+++ b/hw/usb/hcd-ehci.h
@@ -311,6 +311,7 @@ struct EHCIState {
uint64_t last_run_ns;
uint32_t async_stepdown;
+ uint32_t iso_active_counter;
bool int_req_by_async;
};
--
1.7.12.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH 6/8] hid: Change idle handling to use a timer
2012-11-06 14:08 [Qemu-devel] usb: Move interrupt handling from poll to async handling Hans de Goede
` (4 preceding siblings ...)
2012-11-06 14:08 ` [Qemu-devel] [PATCH 5/8] ehci: Lower timer freq when there are no iso packets in the periodic schedule Hans de Goede
@ 2012-11-06 14:08 ` Hans de Goede
2012-11-06 14:08 ` [Qemu-devel] [PATCH 7/8] usb-hid: Move from NAK/polling to async packet handling Hans de Goede
` (2 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: Hans de Goede @ 2012-11-06 14:08 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Hans de Goede, qemu-devel
This is a preparation patch for moving away from a polling model into async
packet handling for usb-hid.
As an added advantage this remove 1000 calls / sec to
qemu_get_clock_ns(vm_clock) while idle-time is set to 0.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
hw/hid.c | 43 +++++++++++++++++++++++++++++++++++++------
hw/hid.h | 5 +++--
hw/usb/dev-hid.c | 8 +++-----
3 files changed, 43 insertions(+), 13 deletions(-)
diff --git a/hw/hid.c b/hw/hid.c
index 03761ab..f8c5393 100644
--- a/hw/hid.c
+++ b/hw/hid.c
@@ -71,12 +71,38 @@ static const uint8_t hid_usage_keys[0x100] = {
bool hid_has_events(HIDState *hs)
{
- return hs->n > 0;
+ return hs->n > 0 || hs->idle_pending;
}
-void hid_set_next_idle(HIDState *hs, int64_t curtime)
+static void hid_idle_timer(void *opaque)
{
- hs->next_idle_clock = curtime + (get_ticks_per_sec() * hs->idle * 4) / 1000;
+ HIDState *hs = opaque;
+
+ hs->idle_pending = true;
+ hs->event(hs);
+}
+
+static void hid_del_idle_timer(HIDState *hs)
+{
+ if (hs->idle_timer) {
+ qemu_del_timer(hs->idle_timer);
+ qemu_free_timer(hs->idle_timer);
+ hs->idle_timer = NULL;
+ }
+}
+
+void hid_set_next_idle(HIDState *hs)
+{
+ if (hs->idle) {
+ uint64_t expire_time = qemu_get_clock_ns(vm_clock) +
+ get_ticks_per_sec() * hs->idle * 4 / 1000;
+ if (!hs->idle_timer) {
+ hs->idle_timer = qemu_new_timer_ns(vm_clock, hid_idle_timer, hs);
+ }
+ qemu_mod_timer_ns(hs->idle_timer, expire_time);
+ } else {
+ hid_del_idle_timer(hs);
+ }
}
static void hid_pointer_event_clear(HIDPointerEvent *e, int buttons)
@@ -232,6 +258,8 @@ int hid_pointer_poll(HIDState *hs, uint8_t *buf, int len)
int index;
HIDPointerEvent *e;
+ hs->idle_pending = false;
+
hid_pointer_activate(hs);
/* When the buffer is empty, return the last event. Relative
@@ -319,6 +347,8 @@ int hid_pointer_poll(HIDState *hs, uint8_t *buf, int len)
int hid_keyboard_poll(HIDState *hs, uint8_t *buf, int len)
{
+ hs->idle_pending = false;
+
if (len < 2) {
return 0;
}
@@ -377,6 +407,8 @@ void hid_reset(HIDState *hs)
hs->n = 0;
hs->protocol = 1;
hs->idle = 0;
+ hs->idle_pending = false;
+ hid_del_idle_timer(hs);
}
void hid_free(HIDState *hs)
@@ -390,6 +422,7 @@ void hid_free(HIDState *hs)
qemu_remove_mouse_event_handler(hs->ptr.eh_entry);
break;
}
+ hid_del_idle_timer(hs);
}
void hid_init(HIDState *hs, int kind, HIDEventFunc event)
@@ -412,9 +445,7 @@ static int hid_post_load(void *opaque, int version_id)
{
HIDState *s = opaque;
- if (s->idle) {
- hid_set_next_idle(s, qemu_get_clock_ns(vm_clock));
- }
+ hid_set_next_idle(s);
return 0;
}
diff --git a/hw/hid.h b/hw/hid.h
index 5315cf7..1cae633 100644
--- a/hw/hid.h
+++ b/hw/hid.h
@@ -43,7 +43,8 @@ struct HIDState {
int kind;
int32_t protocol;
uint8_t idle;
- int64_t next_idle_clock;
+ bool idle_pending;
+ QEMUTimer *idle_timer;
HIDEventFunc event;
};
@@ -52,7 +53,7 @@ void hid_reset(HIDState *hs);
void hid_free(HIDState *hs);
bool hid_has_events(HIDState *hs);
-void hid_set_next_idle(HIDState *hs, int64_t curtime);
+void hid_set_next_idle(HIDState *hs);
void hid_pointer_activate(HIDState *hs);
int hid_pointer_poll(HIDState *hs, uint8_t *buf, int len);
int hid_keyboard_poll(HIDState *hs, uint8_t *buf, int len);
diff --git a/hw/usb/dev-hid.c b/hw/usb/dev-hid.c
index 55266b1..aa59ec4 100644
--- a/hw/usb/dev-hid.c
+++ b/hw/usb/dev-hid.c
@@ -439,7 +439,7 @@ static void usb_hid_handle_control(USBDevice *dev, USBPacket *p,
break;
case SET_IDLE:
hs->idle = (uint8_t) (value >> 8);
- hid_set_next_idle(hs, qemu_get_clock_ns(vm_clock));
+ hid_set_next_idle(hs);
if (hs->kind == HID_MOUSE || hs->kind == HID_TABLET) {
hid_pointer_activate(hs);
}
@@ -461,16 +461,14 @@ static void usb_hid_handle_data(USBDevice *dev, USBPacket *p)
switch (p->pid) {
case USB_TOKEN_IN:
if (p->ep->nr == 1) {
- int64_t curtime = qemu_get_clock_ns(vm_clock);
if (hs->kind == HID_MOUSE || hs->kind == HID_TABLET) {
hid_pointer_activate(hs);
}
- if (!hid_has_events(hs) &&
- (!hs->idle || hs->next_idle_clock - curtime > 0)) {
+ if (!hid_has_events(hs)) {
p->status = USB_RET_NAK;
return;
}
- hid_set_next_idle(hs, curtime);
+ hid_set_next_idle(hs);
if (hs->kind == HID_MOUSE || hs->kind == HID_TABLET) {
len = hid_pointer_poll(hs, buf, p->iov.size);
} else if (hs->kind == HID_KEYBOARD) {
--
1.7.12.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH 7/8] usb-hid: Move from NAK/polling to async packet handling
2012-11-06 14:08 [Qemu-devel] usb: Move interrupt handling from poll to async handling Hans de Goede
` (5 preceding siblings ...)
2012-11-06 14:08 ` [Qemu-devel] [PATCH 6/8] hid: Change idle handling to use a timer Hans de Goede
@ 2012-11-06 14:08 ` Hans de Goede
2012-11-08 15:35 ` Gerd Hoffmann
2012-11-06 14:08 ` [Qemu-devel] [PATCH 8/8] usb-hid: Allow connecting to a USB-2 device Hans de Goede
2012-11-06 22:05 ` [Qemu-devel] usb: Move interrupt handling from poll to async handling Gerd Hoffmann
8 siblings, 1 reply; 15+ messages in thread
From: Hans de Goede @ 2012-11-06 14:08 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Hans de Goede, qemu-devel
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
hw/usb/dev-hid.c | 44 ++++++++++++++++++++++++++++++++++----------
1 file changed, 34 insertions(+), 10 deletions(-)
diff --git a/hw/usb/dev-hid.c b/hw/usb/dev-hid.c
index aa59ec4..69f89ff 100644
--- a/hw/usb/dev-hid.c
+++ b/hw/usb/dev-hid.c
@@ -45,6 +45,7 @@
typedef struct USBHIDState {
USBDevice dev;
USBEndpoint *intr;
+ USBPacket *async_packet_pending;
HIDState hid;
} USBHIDState;
@@ -357,10 +358,30 @@ static const uint8_t qemu_keyboard_hid_report_descriptor[] = {
0xc0, /* End Collection */
};
+static void usb_data_in_complete(HIDState *hs, USBPacket *p)
+{
+ uint8_t buf[p->iov.size];
+ int len = 0;
+
+ hid_set_next_idle(hs);
+ if (hs->kind == HID_MOUSE || hs->kind == HID_TABLET) {
+ len = hid_pointer_poll(hs, buf, p->iov.size);
+ } else if (hs->kind == HID_KEYBOARD) {
+ len = hid_keyboard_poll(hs, buf, p->iov.size);
+ }
+ usb_packet_copy(p, buf, len);
+}
+
static void usb_hid_changed(HIDState *hs)
{
USBHIDState *us = container_of(hs, USBHIDState, hid);
+ if (us->async_packet_pending) {
+ us->async_packet_pending->status = USB_RET_SUCCESS; /* Clear ASYNC */
+ usb_data_in_complete(hs, us->async_packet_pending);
+ usb_packet_complete(&us->dev, us->async_packet_pending);
+ us->async_packet_pending = NULL;
+ }
usb_wakeup(us->intr);
}
@@ -455,8 +476,6 @@ static void usb_hid_handle_data(USBDevice *dev, USBPacket *p)
{
USBHIDState *us = DO_UPCAST(USBHIDState, dev, dev);
HIDState *hs = &us->hid;
- uint8_t buf[p->iov.size];
- int len = 0;
switch (p->pid) {
case USB_TOKEN_IN:
@@ -465,16 +484,12 @@ static void usb_hid_handle_data(USBDevice *dev, USBPacket *p)
hid_pointer_activate(hs);
}
if (!hid_has_events(hs)) {
- p->status = USB_RET_NAK;
+ assert(us->async_packet_pending == NULL);
+ us->async_packet_pending = p;
+ p->status = USB_RET_ASYNC;
return;
}
- hid_set_next_idle(hs);
- if (hs->kind == HID_MOUSE || hs->kind == HID_TABLET) {
- len = hid_pointer_poll(hs, buf, p->iov.size);
- } else if (hs->kind == HID_KEYBOARD) {
- len = hid_keyboard_poll(hs, buf, p->iov.size);
- }
- usb_packet_copy(p, buf, len);
+ usb_data_in_complete(hs, p);
} else {
goto fail;
}
@@ -487,6 +502,14 @@ static void usb_hid_handle_data(USBDevice *dev, USBPacket *p)
}
}
+static void usb_hid_cancel_packet(USBDevice *dev, USBPacket *p)
+{
+ USBHIDState *us = DO_UPCAST(USBHIDState, dev, dev);
+
+ assert(us->async_packet_pending == p);
+ us->async_packet_pending = NULL;
+}
+
static void usb_hid_handle_destroy(USBDevice *dev)
{
USBHIDState *us = DO_UPCAST(USBHIDState, dev, dev);
@@ -560,6 +583,7 @@ static void usb_hid_class_initfn(ObjectClass *klass, void *data)
uc->handle_control = usb_hid_handle_control;
uc->handle_data = usb_hid_handle_data;
uc->handle_destroy = usb_hid_handle_destroy;
+ uc->cancel_packet = usb_hid_cancel_packet;
}
static void usb_tablet_class_initfn(ObjectClass *klass, void *data)
--
1.7.12.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH 8/8] usb-hid: Allow connecting to a USB-2 device
2012-11-06 14:08 [Qemu-devel] usb: Move interrupt handling from poll to async handling Hans de Goede
` (6 preceding siblings ...)
2012-11-06 14:08 ` [Qemu-devel] [PATCH 7/8] usb-hid: Move from NAK/polling to async packet handling Hans de Goede
@ 2012-11-06 14:08 ` Hans de Goede
2012-11-07 9:47 ` Paolo Bonzini
2012-11-08 15:36 ` Gerd Hoffmann
2012-11-06 22:05 ` [Qemu-devel] usb: Move interrupt handling from poll to async handling Gerd Hoffmann
8 siblings, 2 replies; 15+ messages in thread
From: Hans de Goede @ 2012-11-06 14:08 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Hans de Goede, qemu-devel
Our ehci code has is capable of significantly lowering the wakeup rate
for the hcd emulation while the device is idle. It is possible to add
similar code ot the uhci emulation, but that simply is not there atm,
and there is no reason why a (virtual) usb device can not be a USB-2 device.
Making usb-hid devices connect to the emulated ehci controller instead
of the emulated uhci controller on vms which have both lowers the cpuload
for a fully idle vm from 20% to 2-3% (on my laptop).
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
hw/usb/dev-hid.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/hw/usb/dev-hid.c b/hw/usb/dev-hid.c
index 69f89ff..96ba0c0 100644
--- a/hw/usb/dev-hid.c
+++ b/hw/usb/dev-hid.c
@@ -97,7 +97,7 @@ static const USBDescIface desc_iface_mouse = {
.bEndpointAddress = USB_DIR_IN | 0x01,
.bmAttributes = USB_ENDPOINT_XFER_INT,
.wMaxPacketSize = 4,
- .bInterval = 0x0a,
+ .bInterval = 8,
},
},
};
@@ -127,7 +127,7 @@ static const USBDescIface desc_iface_tablet = {
.bEndpointAddress = USB_DIR_IN | 0x01,
.bmAttributes = USB_ENDPOINT_XFER_INT,
.wMaxPacketSize = 8,
- .bInterval = 0x0a,
+ .bInterval = 8,
},
},
};
@@ -158,7 +158,7 @@ static const USBDescIface desc_iface_keyboard = {
.bEndpointAddress = USB_DIR_IN | 0x01,
.bmAttributes = USB_ENDPOINT_XFER_INT,
.wMaxPacketSize = 8,
- .bInterval = 0x0a,
+ .bInterval = 8,
},
},
};
@@ -224,6 +224,7 @@ static const USBDesc desc_mouse = {
.iSerialNumber = STR_SERIALNUMBER,
},
.full = &desc_device_mouse,
+ .high = &desc_device_mouse,
.str = desc_strings,
};
@@ -237,6 +238,7 @@ static const USBDesc desc_tablet = {
.iSerialNumber = STR_SERIALNUMBER,
},
.full = &desc_device_tablet,
+ .high = &desc_device_tablet,
.str = desc_strings,
};
@@ -250,6 +252,7 @@ static const USBDesc desc_keyboard = {
.iSerialNumber = STR_SERIALNUMBER,
},
.full = &desc_device_keyboard,
+ .high = &desc_device_keyboard,
.str = desc_strings,
};
--
1.7.12.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH 8/8] usb-hid: Allow connecting to a USB-2 device
2012-11-06 14:08 ` [Qemu-devel] [PATCH 8/8] usb-hid: Allow connecting to a USB-2 device Hans de Goede
@ 2012-11-07 9:47 ` Paolo Bonzini
2012-11-08 15:36 ` Gerd Hoffmann
1 sibling, 0 replies; 15+ messages in thread
From: Paolo Bonzini @ 2012-11-07 9:47 UTC (permalink / raw)
To: Hans de Goede; +Cc: Gerd Hoffmann, qemu-devel
Il 06/11/2012 15:08, Hans de Goede ha scritto:
> Our ehci code has is capable of significantly lowering the wakeup rate
> for the hcd emulation while the device is idle. It is possible to add
> similar code ot the uhci emulation, but that simply is not there atm,
> and there is no reason why a (virtual) usb device can not be a USB-2 device.
>
> Making usb-hid devices connect to the emulated ehci controller instead
> of the emulated uhci controller on vms which have both lowers the cpuload
> for a fully idle vm from 20% to 2-3% (on my laptop).
You need this to be dependent on the machine version. Otherwise the USB
paths may change and migration will break.
Paolo
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> hw/usb/dev-hid.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/hw/usb/dev-hid.c b/hw/usb/dev-hid.c
> index 69f89ff..96ba0c0 100644
> --- a/hw/usb/dev-hid.c
> +++ b/hw/usb/dev-hid.c
> @@ -97,7 +97,7 @@ static const USBDescIface desc_iface_mouse = {
> .bEndpointAddress = USB_DIR_IN | 0x01,
> .bmAttributes = USB_ENDPOINT_XFER_INT,
> .wMaxPacketSize = 4,
> - .bInterval = 0x0a,
> + .bInterval = 8,
> },
> },
> };
> @@ -127,7 +127,7 @@ static const USBDescIface desc_iface_tablet = {
> .bEndpointAddress = USB_DIR_IN | 0x01,
> .bmAttributes = USB_ENDPOINT_XFER_INT,
> .wMaxPacketSize = 8,
> - .bInterval = 0x0a,
> + .bInterval = 8,
> },
> },
> };
> @@ -158,7 +158,7 @@ static const USBDescIface desc_iface_keyboard = {
> .bEndpointAddress = USB_DIR_IN | 0x01,
> .bmAttributes = USB_ENDPOINT_XFER_INT,
> .wMaxPacketSize = 8,
> - .bInterval = 0x0a,
> + .bInterval = 8,
> },
> },
> };
> @@ -224,6 +224,7 @@ static const USBDesc desc_mouse = {
> .iSerialNumber = STR_SERIALNUMBER,
> },
> .full = &desc_device_mouse,
> + .high = &desc_device_mouse,
> .str = desc_strings,
> };
>
> @@ -237,6 +238,7 @@ static const USBDesc desc_tablet = {
> .iSerialNumber = STR_SERIALNUMBER,
> },
> .full = &desc_device_tablet,
> + .high = &desc_device_tablet,
> .str = desc_strings,
> };
>
> @@ -250,6 +252,7 @@ static const USBDesc desc_keyboard = {
> .iSerialNumber = STR_SERIALNUMBER,
> },
> .full = &desc_device_keyboard,
> + .high = &desc_device_keyboard,
> .str = desc_strings,
> };
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH 8/8] usb-hid: Allow connecting to a USB-2 device
2012-11-06 14:08 ` [Qemu-devel] [PATCH 8/8] usb-hid: Allow connecting to a USB-2 device Hans de Goede
2012-11-07 9:47 ` Paolo Bonzini
@ 2012-11-08 15:36 ` Gerd Hoffmann
2012-11-12 11:19 ` Hans de Goede
1 sibling, 1 reply; 15+ messages in thread
From: Gerd Hoffmann @ 2012-11-08 15:36 UTC (permalink / raw)
To: Hans de Goede; +Cc: qemu-devel
On 11/06/12 15:08, Hans de Goede wrote:
> Our ehci code has is capable of significantly lowering the wakeup rate
> for the hcd emulation while the device is idle. It is possible to add
> similar code ot the uhci emulation, but that simply is not there atm,
> and there is no reason why a (virtual) usb device can not be a USB-2 device.
We should probably add usb-tablet2 (+friends) devices as this is a
guest-visible change. We could also try tricks with properties but I
guess separate devices are easier.
cheers,
Gerd
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 8/8] usb-hid: Allow connecting to a USB-2 device
2012-11-08 15:36 ` Gerd Hoffmann
@ 2012-11-12 11:19 ` Hans de Goede
0 siblings, 0 replies; 15+ messages in thread
From: Hans de Goede @ 2012-11-12 11:19 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: qemu-devel
Hi,
On 11/08/2012 04:36 PM, Gerd Hoffmann wrote:
> On 11/06/12 15:08, Hans de Goede wrote:
>> Our ehci code has is capable of significantly lowering the wakeup rate
>> for the hcd emulation while the device is idle. It is possible to add
>> similar code ot the uhci emulation, but that simply is not there atm,
>> and there is no reason why a (virtual) usb device can not be a USB-2 device.
>
> We should probably add usb-tablet2 (+friends) devices as this is a
> guest-visible change. We could also try tricks with properties but I
> guess separate devices are easier.
A quick status update on this patchset.
a) I promised to resend it without this patch Friday afternoon at kvm-forum,
which I went to do directly after saying goodbye, but when I unsuspended my
laptop it broke... (looks like the cable between the motherboard and screen
has a loose contact), so it got delayed a bit.
b) While working on fixing all the comments on the set yesterday to send a v2,
I realized at the very last moment that handling interrupts packet async,
rather then by nakking them, causes problems with migration, as the following
can happen:
1) hcd submits a packet to device, device returns USB_RET_ASYNC
2) device gets some data, fills the packet, calls complete() on it
3) For ehci complete() calls a bh, which immediately walks the async
schedule, but if not enough time has passed since the last bh, to
process periodic frames, or if the interrupt endpoint has an interval
of more then 1 ms, and this periodic frame it is not scheduled, the
completion will not get written back to guest memory!
4) If we now migrate, the completion never gets written back to guest
memory and the interrupt packet is effectively lost
Notice that the same can happen with non async interrupt packets on the uhci
code... I was planning on modifying that to be more like the ehci code anyways
to lower the CPU-load (amount of wakeups), but that will not necessarily fix
this, so this needs some more thinking.
I hope to have a v2 of the wakeup reduction for interrupt endpoints patchset
by the end of the day ...
Regards,
Hans
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] usb: Move interrupt handling from poll to async handling
2012-11-06 14:08 [Qemu-devel] usb: Move interrupt handling from poll to async handling Hans de Goede
` (7 preceding siblings ...)
2012-11-06 14:08 ` [Qemu-devel] [PATCH 8/8] usb-hid: Allow connecting to a USB-2 device Hans de Goede
@ 2012-11-06 22:05 ` Gerd Hoffmann
8 siblings, 0 replies; 15+ messages in thread
From: Gerd Hoffmann @ 2012-11-06 22:05 UTC (permalink / raw)
To: Hans de Goede; +Cc: qemu-devel
On 11/06/12 15:08, Hans de Goede wrote:
> This patchsets move interrupt handling for usb-redir and usb-hid devices
> over from polling to async handling. Together with a patch to allow
> async stepdown in ehci when the periodic schedule only contains interrupt
> queues, and a patch to allow usb-hid devices to connect to ehci, this
> fixes the usb-tablet causing a crazy cpu load for idle guests.
>
> With this patchset the cpuload for a fully idle vm with usb-tablet
> is lowered from 20% to 2-3% (on my laptop).
Not looked at it (yet) in detail, just merged quickly into a test
branch, then found it blowing up in an assert with a win7 guest when
moving the mouse for the first time after boot. So something is fishy
in the hid patches I guess ...
cheers,
Gerd
^ permalink raw reply [flat|nested] 15+ messages in thread