On Fri, Jun 10, 2011 at 04:51:43PM +0800, Andiry Xu wrote:
> This patch assign devices to different interrupters based on its slot_id.
> However, it's not mandatory that a device should use the same interrupter.
> A device can assign its control transfer to one interrupter and bulk/intr/
> isoc transfer to another interrupter. The interrupter usage policy is
> flexible.
>
> The policy in this patch may be replaced by better approach in the future.
I would like to get a more flexible policy, or at least the APIs to
allow drivers or the USB core flexibility in directing URB traffic to
different interrupters before this gets merged. I'm most concerned
about allowing drivers or the USB core to change interrupt moderation
rate the rings should use. Either that, or the xHCI driver needs to
choose the moderation interval based on the endpoint type and polling
rate. I think we need that, because I have doubts that these patches
alone will improve performance. I don't really want to add this much
optional code without some indication that it will help performance.
What about adding a field to the struct urb to allow the driver
to set the interrupter field? We might have to add some USB core API to
allow the driver to figure out how many interrupters are available. If
the driver doesn't set the field, we can just use the per-device
basis.
I think we'd have to restrict the driver to only using one interrupter
per endpoint, unless streams are enabled. Otherwise I think it breaks
the xhci_td list handling assumption that events are handed back in the
order they are placed on the ring, because we could have a race
condition between two CPUs that receive interrupts for the same
endpoint. You could set an initial interrupter used in the xhci_virt_ep
structure, and then double check it whenever a new URB is queued for
that endpoint.
Then if each URB could have a different interrupter, the UAS driver could
direct different stream transfers to different interrupters.
> Signed-off-by: Andiry Xu <andiry.xu@xxxxxxx>
> ---
> drivers/usb/host/xhci-mem.c | 3 +++
> drivers/usb/host/xhci-ring.c | 22 ++++++++++++++--------
> drivers/usb/host/xhci.h | 2 ++
> 3 files changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index f1f64a4..dd0f27d 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -781,6 +781,9 @@ int xhci_alloc_virt_device(struct xhci_hcd *xhci, int slot_id,
> INIT_LIST_HEAD(&dev->cmd_list);
> dev->udev = udev;
>
> + /* Decide the interrupter target */
> + dev->intr_target = slot_id % xhci->intr_num;
> +
> /* Point to output device context in dcbaa. */
> xhci->dcbaa->dev_context_ptrs[slot_id] = cpu_to_le64(dev->out_ctx->dma);
> xhci_dbg(xhci, "Set slot id %d dcbaa entry %p to 0x%llx\n",
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index fc3c9d8..f3e49af 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -2702,6 +2702,7 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> bool first_trb;
> u64 addr;
> bool more_trbs_coming;
> + int intr;
>
> struct xhci_generic_trb *start_trb;
> int start_cycle;
> @@ -2724,6 +2725,7 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> urb_priv = urb->hcpriv;
> td = urb_priv->td[0];
>
> + intr = xhci->devs[slot_id]->intr_target;
> /*
> * Don't give the first TRB to the hardware (by toggling the cycle bit)
> * until we've finished creating all the other TRBs. The ring's cycle
> @@ -2806,7 +2808,7 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> }
> length_field = TRB_LEN(trb_buff_len) |
> remainder |
> - TRB_INTR_TARGET(0);
> + TRB_INTR_TARGET(intr);
>
> if (num_trbs > 1)
> more_trbs_coming = true;
> @@ -2862,10 +2864,10 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> bool more_trbs_coming;
> int start_cycle;
> u32 field, length_field;
> -
> int running_total, trb_buff_len, ret;
> unsigned int total_packet_count;
> u64 addr;
> + int intr;
>
> if (urb->num_sgs)
> return queue_bulk_sg_tx(xhci, mem_flags, urb, slot_id, ep_index);
> @@ -2910,6 +2912,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> urb_priv = urb->hcpriv;
> td = urb_priv->td[0];
>
> + intr = xhci->devs[slot_id]->intr_target;
> /*
> * Don't give the first TRB to the hardware (by toggling the cycle bit)
> * until we've finished creating all the other TRBs. The ring's cycle
> @@ -2969,7 +2972,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> }
> length_field = TRB_LEN(trb_buff_len) |
> remainder |
> - TRB_INTR_TARGET(0);
> + TRB_INTR_TARGET(intr);
>
> if (num_trbs > 1)
> more_trbs_coming = true;
> @@ -3009,6 +3012,7 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> u32 field, length_field;
> struct urb_priv *urb_priv;
> struct xhci_td *td;
> + int intr;
>
> ep_ring = xhci_urb_to_transfer_ring(xhci, urb);
> if (!ep_ring)
> @@ -3042,6 +3046,7 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> urb_priv = urb->hcpriv;
> td = urb_priv->td[0];
>
> + intr = xhci->devs[slot_id]->intr_target;
> /*
> * Don't give the first TRB to the hardware (by toggling the cycle bit)
> * until we've finished creating all the other TRBs. The ring's cycle
> @@ -3071,7 +3076,7 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> queue_trb(xhci, ep_ring, false, true,
> setup->bRequestType | setup->bRequest << 8 | le16_to_cpu(setup->wValue) << 16,
> le16_to_cpu(setup->wIndex) | le16_to_cpu(setup->wLength) << 16,
> - TRB_LEN(8) | TRB_INTR_TARGET(0),
> + TRB_LEN(8) | TRB_INTR_TARGET(intr),
> /* Immediate data in pointer */
> field);
>
> @@ -3084,7 +3089,7 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
>
> length_field = TRB_LEN(urb->transfer_buffer_length) |
> xhci_td_remainder(urb->transfer_buffer_length) |
> - TRB_INTR_TARGET(0);
> + TRB_INTR_TARGET(intr);
> if (urb->transfer_buffer_length > 0) {
> if (setup->bRequestType & USB_DIR_IN)
> field |= TRB_DIR_IN;
> @@ -3107,7 +3112,7 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> queue_trb(xhci, ep_ring, false, false,
> 0,
> 0,
> - TRB_INTR_TARGET(0),
> + TRB_INTR_TARGET(intr),
> /* Event on completion */
> field | TRB_IOC | TRB_TYPE(TRB_STATUS) | ep_ring->cycle_state);
>
> @@ -3209,7 +3214,7 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> u32 field, length_field;
> int running_total, trb_buff_len, td_len, td_remain_len, ret;
> u64 start_addr, addr;
> - int i, j;
> + int i, j, intr;
> bool more_trbs_coming;
>
> ep_ring = xhci->devs[slot_id]->eps[ep_index].ring;
> @@ -3233,6 +3238,7 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> start_trb = &ep_ring->enqueue->generic;
> start_cycle = ep_ring->cycle_state;
>
> + intr = xhci->devs[slot_id]->intr_target;
> /* Queue the first TRB, even if it's zero-length */
> for (i = 0; i < num_tds; i++) {
> unsigned int total_packet_count;
> @@ -3322,7 +3328,7 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> }
> length_field = TRB_LEN(trb_buff_len) |
> remainder |
> - TRB_INTR_TARGET(0);
> + TRB_INTR_TARGET(intr);
>
> queue_trb(xhci, ep_ring, false, more_trbs_coming,
> lower_32_bits(addr),
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index a18cf6b..84c0edb 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -792,6 +792,8 @@ struct xhci_virt_device {
> int num_rings_cached;
> /* Store xHC assigned device address */
> int address;
> + /* Store the interrupter target */
> + int intr_target;
> #define XHCI_MAX_RINGS_CACHED 31
> struct xhci_virt_ep eps[31];
> struct completion cmd_completion;
> --
> 1.7.1
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
