Re: [Qemu-devel] [PATCH 12/14] usb-core: usb3 streams

[Hans de Goede]

<resend with the list added to the to field>

Hi,

On 02/21/2013 10:59 AM, Gerd Hoffmann wrote:
> This patch adds support for usb3 streams to the usb subsystem core.
> This is just adding a streams field / parameter in a number of places.

Hmm, I was expecting epqueues to become a per stream thing with streams,
ie an error on one stream should only lead to cancelling queued packets
from that stream, and like wise submitting a packet for stream 2, should
not wait for a previous packet submitted for stream 1 to complete, which
it will with the one epqueue for all streams model (I know pipelining
works around this, but it still shows the concept of 1 queue for
all the streams us wrong IMHO).

Regards,

Hans



> Signed-off-by: Gerd Hoffmann <address@hidden>
> ---
>   hw/usb.h                      |   10 ++++++----
>   hw/usb/core.c                 |   10 ++++++----
>   hw/usb/dev-bluetooth.c        |    2 +-
>   hw/usb/dev-hid.c              |    2 +-
>   hw/usb/dev-hub.c              |   10 +++++-----
>   hw/usb/dev-network.c          |    2 +-
>   hw/usb/dev-smartcard-reader.c |    2 +-
>   hw/usb/dev-uas.c              |    2 +-
>   hw/usb/dev-wacom.c            |    4 ++--
>   hw/usb/hcd-ehci.c             |    7 ++++---
>   hw/usb/hcd-musb.c             |    2 +-
>   hw/usb/hcd-ohci.c             |    4 ++--
>   hw/usb/hcd-uhci.c             |    2 +-
>   hw/usb/hcd-xhci.c             |    9 +++++----
>   14 files changed, 37 insertions(+), 31 deletions(-)
>
> diff --git a/hw/usb.h b/hw/usb.h
> index 0d09e02..382496c 100644
> --- a/hw/usb.h
> +++ b/hw/usb.h
> @@ -361,6 +361,7 @@ struct USBPacket {
>       int pid;
>       uint64_t id;
>       USBEndpoint *ep;
> +    unsigned int stream;
>       QEMUIOVector iov;
>       uint64_t parameter; /* control transfers */
>       bool short_not_ok;
> @@ -383,8 +384,9 @@ struct USBCombinedPacket {
>   void usb_packet_init(USBPacket *p);
>   void usb_packet_set_state(USBPacket *p, USBPacketState state);
>   void usb_packet_check_state(USBPacket *p, USBPacketState expected);
> -void usb_packet_setup(USBPacket *p, int pid, USBEndpoint *ep, uint64_t id,
> -                      bool short_not_ok, bool int_req);
> +void usb_packet_setup(USBPacket *p, int pid,
> +                      USBEndpoint *ep, unsigned int stream,
> +                      uint64_t id, bool short_not_ok, bool int_req);
>   void usb_packet_addbuf(USBPacket *p, void *ptr, size_t len);
>   int usb_packet_map(USBPacket *p, QEMUSGList *sgl);
>   void usb_packet_unmap(USBPacket *p, QEMUSGList *sgl);
> @@ -430,7 +432,7 @@ void usb_attach(USBPort *port);
>   void usb_detach(USBPort *port);
>   void usb_port_reset(USBPort *port);
>   void usb_device_reset(USBDevice *dev);
> -void usb_wakeup(USBEndpoint *ep);
> +void usb_wakeup(USBEndpoint *ep, unsigned int stream);
>   void usb_generic_async_ctrl_complete(USBDevice *s, USBPacket *p);
>   int set_usb_string(uint8_t *buf, const char *str);
>
> @@ -489,7 +491,7 @@ struct USBBus {
>   struct USBBusOps {
>       int (*register_companion)(USBBus *bus, USBPort *ports[],
>                                 uint32_t portcount, uint32_t firstport);
> -    void (*wakeup_endpoint)(USBBus *bus, USBEndpoint *ep);
> +    void (*wakeup_endpoint)(USBBus *bus, USBEndpoint *ep, unsigned int stream);
>   };
>
>   void usb_bus_new(USBBus *bus, USBBusOps *ops, DeviceState *host);
> diff --git a/hw/usb/core.c b/hw/usb/core.c
> index 674fef8..15a150a 100644
> --- a/hw/usb/core.c
> +++ b/hw/usb/core.c
> @@ -71,7 +71,7 @@ void usb_device_reset(USBDevice *dev)
>       usb_device_handle_reset(dev);
>   }
>
> -void usb_wakeup(USBEndpoint *ep)
> +void usb_wakeup(USBEndpoint *ep, unsigned int stream)
>   {
>       USBDevice *dev = ep->dev;
>       USBBus *bus = usb_bus_from_device(dev);
> @@ -80,7 +80,7 @@ void usb_wakeup(USBEndpoint *ep)
>           dev->port->ops->wakeup(dev->port);
>       }
>       if (bus->ops->wakeup_endpoint) {
> -        bus->ops->wakeup_endpoint(bus, ep);
> +        bus->ops->wakeup_endpoint(bus, ep, stream);
>       }
>   }
>
> @@ -545,14 +545,16 @@ void usb_packet_set_state(USBPacket *p, USBPacketState 
> state)
>       p->state = state;
>   }
>
> -void usb_packet_setup(USBPacket *p, int pid, USBEndpoint *ep, uint64_t id,
> -                      bool short_not_ok, bool int_req)
> +void usb_packet_setup(USBPacket *p, int pid,
> +                      USBEndpoint *ep, unsigned int stream,
> +                      uint64_t id, bool short_not_ok, bool int_req)
>   {
>       assert(!usb_packet_is_inflight(p));
>       assert(p->iov.iov != NULL);
>       p->id = id;
>       p->pid = pid;
>       p->ep = ep;
> +    p->stream = stream;
>       p->status = USB_RET_SUCCESS;
>       p->actual_length = 0;
>       p->parameter = 0;
> diff --git a/hw/usb/dev-bluetooth.c b/hw/usb/dev-bluetooth.c
> index adbf9d4..0f8aa48 100644
> --- a/hw/usb/dev-bluetooth.c
> +++ b/hw/usb/dev-bluetooth.c
> @@ -478,7 +478,7 @@ static void usb_bt_out_hci_packet_event(void *opaque,
>       struct USBBtState *s = (struct USBBtState *) opaque;
>
>       if (s->evt.len == 0) {
> -        usb_wakeup(s->intr);
> +        usb_wakeup(s->intr, 0);
>       }
>       usb_bt_fifo_enqueue(&s->evt, data, len);
>   }
> diff --git a/hw/usb/dev-hid.c b/hw/usb/dev-hid.c
> index 29b6481..9701048 100644
> --- a/hw/usb/dev-hid.c
> +++ b/hw/usb/dev-hid.c
> @@ -423,7 +423,7 @@ static void usb_hid_changed(HIDState *hs)
>   {
>       USBHIDState *us = container_of(hs, USBHIDState, hid);
>
> -    usb_wakeup(us->intr);
> +    usb_wakeup(us->intr, 0);
>   }
>
>   static void usb_hid_handle_reset(USBDevice *dev)
> diff --git a/hw/usb/dev-hub.c b/hw/usb/dev-hub.c
> index 79f2f46..504c98c 100644
> --- a/hw/usb/dev-hub.c
> +++ b/hw/usb/dev-hub.c
> @@ -164,7 +164,7 @@ static void usb_hub_attach(USBPort *port1)
>       } else {
>           port->wPortStatus &= ~PORT_STAT_LOW_SPEED;
>       }
> -    usb_wakeup(s->intr);
> +    usb_wakeup(s->intr, 0);
>   }
>
>   static void usb_hub_detach(USBPort *port1)
> @@ -173,7 +173,7 @@ static void usb_hub_detach(USBPort *port1)
>       USBHubPort *port = &s->ports[port1->index];
>
>       trace_usb_hub_detach(s->dev.addr, port1->index + 1);
> -    usb_wakeup(s->intr);
> +    usb_wakeup(s->intr, 0);
>
>       /* Let upstream know the device on this port is gone */
>       s->dev.port->ops->child_detach(s->dev.port, port1->dev);
> @@ -184,7 +184,7 @@ static void usb_hub_detach(USBPort *port1)
>           port->wPortStatus &= ~PORT_STAT_ENABLE;
>           port->wPortChange |= PORT_STAT_C_ENABLE;
>       }
> -    usb_wakeup(s->intr);
> +    usb_wakeup(s->intr, 0);
>   }
>
>   static void usb_hub_child_detach(USBPort *port1, USBDevice *child)
> @@ -202,7 +202,7 @@ static void usb_hub_wakeup(USBPort *port1)
>
>       if (port->wPortStatus & PORT_STAT_SUSPEND) {
>           port->wPortChange |= PORT_STAT_C_SUSPEND;
> -        usb_wakeup(s->intr);
> +        usb_wakeup(s->intr, 0);
>       }
>   }
>
> @@ -364,7 +364,7 @@ static void usb_hub_handle_control(USBDevice *dev, 
> USBPacket *p,
>                       port->wPortChange |= PORT_STAT_C_RESET;
>                       /* set enable bit */
>                       port->wPortStatus |= PORT_STAT_ENABLE;
> -                    usb_wakeup(s->intr);
> +                    usb_wakeup(s->intr, 0);
>                   }
>                   break;
>               case PORT_POWER:
> diff --git a/hw/usb/dev-network.c b/hw/usb/dev-network.c
> index a01a5e7..c08718b 100644
> --- a/hw/usb/dev-network.c
> +++ b/hw/usb/dev-network.c
> @@ -855,7 +855,7 @@ static void *rndis_queue_response(USBNetState *s, unsigned 
> int length)
>               g_malloc0(sizeof(struct rndis_response) + length);
>
>       if (QTAILQ_EMPTY(&s->rndis_resp)) {
> -        usb_wakeup(s->intr);
> +        usb_wakeup(s->intr, 0);
>       }
>
>       QTAILQ_INSERT_TAIL(&s->rndis_resp, r, entries);
> diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
> index 979a473..caebc1c 100644
> --- a/hw/usb/dev-smartcard-reader.c
> +++ b/hw/usb/dev-smartcard-reader.c
> @@ -839,7 +839,7 @@ static void ccid_on_slot_change(USBCCIDState *s, bool full)
>           s->bmSlotICCState |= SLOT_0_CHANGED_MASK;
>       }
>       s->notify_slot_change = true;
> -    usb_wakeup(s->intr);
> +    usb_wakeup(s->intr, 0);
>   }
>
>   static void ccid_write_data_block_error(
> diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
> index d904d1a..316c388 100644
> --- a/hw/usb/dev-uas.c
> +++ b/hw/usb/dev-uas.c
> @@ -276,7 +276,7 @@ static void usb_uas_queue_status(UASDevice *uas, UASStatus 
> *st, int length)
>       } else {
>           USBEndpoint *ep = usb_ep_get(&uas->dev, USB_TOKEN_IN,
>                                        UAS_PIPE_ID_STATUS);
> -        usb_wakeup(ep);
> +        usb_wakeup(ep, 0);
>       }
>   }
>
> diff --git a/hw/usb/dev-wacom.c b/hw/usb/dev-wacom.c
> index ab9fa2e..3be5cde 100644
> --- a/hw/usb/dev-wacom.c
> +++ b/hw/usb/dev-wacom.c
> @@ -138,7 +138,7 @@ static void usb_mouse_event(void *opaque,
>       s->dz += dz1;
>       s->buttons_state = buttons_state;
>       s->changed = 1;
> -    usb_wakeup(s->intr);
> +    usb_wakeup(s->intr, 0);
>   }
>
>   static void usb_wacom_event(void *opaque,
> @@ -152,7 +152,7 @@ static void usb_wacom_event(void *opaque,
>       s->dz += dz;
>       s->buttons_state = buttons_state;
>       s->changed = 1;
> -    usb_wakeup(s->intr);
> +    usb_wakeup(s->intr, 0);
>   }
>
>   static inline int int_clamp(int val, int vmin, int vmax)
> diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> index 7040659..5176251 100644
> --- a/hw/usb/hcd-ehci.c
> +++ b/hw/usb/hcd-ehci.c
> @@ -874,7 +874,8 @@ static int ehci_register_companion(USBBus *bus, USBPort 
> *ports[],
>       return 0;
>   }
>
> -static void ehci_wakeup_endpoint(USBBus *bus, USBEndpoint *ep)
> +static void ehci_wakeup_endpoint(USBBus *bus, USBEndpoint *ep,
> +                                 unsigned int stream)
>   {
>       EHCIState *s = container_of(bus, EHCIState, bus);
>       uint32_t portsc = s->portsc[ep->dev->port->index];
> @@ -1420,7 +1421,7 @@ static int ehci_execute(EHCIPacket *p, const char *action)
>           }
>
>           spd = (p->pid == USB_TOKEN_IN && NLPTR_TBIT(p->qtd.altnext) == 0);
> -        usb_packet_setup(&p->packet, p->pid, ep, p->qtdaddr, spd,
> +        usb_packet_setup(&p->packet, p->pid, ep, 0, p->qtdaddr, spd,
>                            (p->qtd.token & QTD_TOKEN_IOC) != 0);
>           usb_packet_map(&p->packet, &p->sgl);
>           p->async = EHCI_ASYNC_INITIALIZED;
> @@ -1493,7 +1494,7 @@ static int ehci_process_itd(EHCIState *ehci,
>               dev = ehci_find_device(ehci, devaddr);
>               ep = usb_ep_get(dev, pid, endp);
>               if (ep && ep->type == USB_ENDPOINT_XFER_ISOC) {
> -                usb_packet_setup(&ehci->ipacket, pid, ep, addr, false,
> +                usb_packet_setup(&ehci->ipacket, pid, ep, 0, addr, false,
>                                    (itd->transact[i] & ITD_XACT_IOC) != 0);
>                   usb_packet_map(&ehci->ipacket, &ehci->isgl);
>                   usb_handle_packet(dev, &ehci->ipacket);
> diff --git a/hw/usb/hcd-musb.c b/hw/usb/hcd-musb.c
> index 64e9e83..7968e17 100644
> --- a/hw/usb/hcd-musb.c
> +++ b/hw/usb/hcd-musb.c
> @@ -625,7 +625,7 @@ static void musb_packet(MUSBState *s, MUSBEndPoint *ep,
>       /* A wild guess on the FADDR semantics... */
>       dev = usb_find_device(&s->port, ep->faddr[idx]);
>       uep = usb_ep_get(dev, pid, ep->type[idx] & 0xf);
> -    usb_packet_setup(&ep->packey[dir].p, pid, uep,
> +    usb_packet_setup(&ep->packey[dir].p, pid, uep, 0,
>                        (dev->addr << 16) | (uep->nr << 8) | pid, false, true);
>       usb_packet_addbuf(&ep->packey[dir].p, ep->buf[idx], len);
>       ep->packey[dir].ep = ep;
> diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
> index dd9967b..51241cd 100644
> --- a/hw/usb/hcd-ohci.c
> +++ b/hw/usb/hcd-ohci.c
> @@ -830,7 +830,7 @@ static int ohci_service_iso_td(OHCIState *ohci, struct 
> ohci_ed *ed,
>                          OHCI_BM(iso_td.flags, TD_DI) == 0;
>           dev = ohci_find_device(ohci, OHCI_BM(ed->flags, ED_FA));
>           ep = usb_ep_get(dev, pid, OHCI_BM(ed->flags, ED_EN));
> -        usb_packet_setup(&ohci->usb_packet, pid, ep, addr, false, int_req);
> +        usb_packet_setup(&ohci->usb_packet, pid, ep, 0, addr, false, int_req);
>           usb_packet_addbuf(&ohci->usb_packet, ohci->usb_buf, len);
>           usb_handle_packet(dev, &ohci->usb_packet);
>           if (ohci->usb_packet.status == USB_RET_ASYNC) {
> @@ -1034,7 +1034,7 @@ static int ohci_service_td(OHCIState *ohci, struct 
> ohci_ed *ed)
>           }
>           dev = ohci_find_device(ohci, OHCI_BM(ed->flags, ED_FA));
>           ep = usb_ep_get(dev, pid, OHCI_BM(ed->flags, ED_EN));
> -        usb_packet_setup(&ohci->usb_packet, pid, ep, addr, !flag_r,
> +        usb_packet_setup(&ohci->usb_packet, pid, ep, 0, addr, !flag_r,
>                            OHCI_BM(td.flags, TD_DI) == 0);
>           usb_packet_addbuf(&ohci->usb_packet, ohci->usb_buf, pktlen);
>           usb_handle_packet(dev, &ohci->usb_packet);
> diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
> index 60645aa..f8c4286 100644
> --- a/hw/usb/hcd-uhci.c
> +++ b/hw/usb/hcd-uhci.c
> @@ -879,7 +879,7 @@ static int uhci_handle_td(UHCIState *s, UHCIQueue *q, 
> uint32_t qh_addr,
>
>       max_len = ((td->token >> 21) + 1) & 0x7ff;
>       spd = (pid == USB_TOKEN_IN && (td->ctrl & TD_CTRL_SPD) != 0);
> -    usb_packet_setup(&async->packet, pid, q->ep, td_addr, spd,
> +    usb_packet_setup(&async->packet, pid, q->ep, 0, td_addr, spd,
>                        (td->ctrl & TD_CTRL_IOC) != 0);
>       qemu_sglist_add(&async->sgl, td->buffer, max_len);
>       usb_packet_map(&async->packet, &async->sgl);
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index 5fb0c48..b8247f3 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -1518,8 +1518,8 @@ static int xhci_setup_packet(XHCITransfer *xfer)
>       }
>
>       xhci_xfer_create_sgl(xfer, dir == USB_TOKEN_IN); /* Also sets int_req */
> -    usb_packet_setup(&xfer->packet, dir, ep, xfer->trbs[0].addr, false,
> -                     xfer->int_req);
> +    usb_packet_setup(&xfer->packet, dir, ep, 0,
> +                     xfer->trbs[0].addr, false, xfer->int_req);
>       usb_packet_map(&xfer->packet, &xfer->sgl);
>       DPRINTF("xhci: setup packet pid 0x%x addr %d ep %d\n",
>               xfer->packet.pid, dev->addr, ep->nr);
> @@ -1977,7 +1977,7 @@ static TRBCCode xhci_address_slot(XHCIState *xhci, 
> unsigned int slotid,
>           DPRINTF("xhci: device address is %d\n", slot->devaddr);
>           usb_device_reset(dev);
>           usb_packet_setup(&p, USB_TOKEN_OUT,
> -                         usb_ep_get(dev, USB_TOKEN_OUT, 0),
> +                         usb_ep_get(dev, USB_TOKEN_OUT, 0), 0,
>                            0, false, false);
>           usb_device_handle_control(dev, &p,
>                                     DeviceOutRequest | USB_REQ_SET_ADDRESS,
> @@ -3033,7 +3033,8 @@ static int xhci_find_epid(USBEndpoint *ep)
>       }
>   }
>
> -static void xhci_wakeup_endpoint(USBBus *bus, USBEndpoint *ep)
> +static void xhci_wakeup_endpoint(USBBus *bus, USBEndpoint *ep,
> +                                 unsigned int stream)
>   {
>       XHCIState *xhci = container_of(bus, XHCIState, bus);
>       int slotid;