On Tue, 22 Apr 2014, Russel Hughes wrote:
> > More importantly, the routine sets urb->start_frame to the current
> > value of the frame counter. This is completely wrong; urb->start_frame
> > is supposed to be the (micro-)frame number for when the transfer
> > begins, not when the transfer was submitted.
> >
> > As far as I can tell, the only way to do this correctly is to set the
> > Frame ID field (with SIA = 0) in the first TD of an isochronous stream,
> > and then set SIA = 1 in all the following TDs (see 4.11.2.5). That
> > way, xhci-hcd will know exactly when the stream begins, so it can keep
> > track of the frame in which each URB starts. Dealing with underruns is
> > left as an exercise for the implementer...
> >
>
> Let me know if you want any changes tested using my DAC that reliably
> shows the problem.
Russel, here's a patch you can test. It's only a partial fix for the
problem, because it doesn't handle over/underruns. Still, it would be
nice to see if the patch makes any difference in normal operation.
Even if it doesn't fix the problem, please post a short stretch (a few
hundred lines) from a usbmon trace with the patch installed.
Alan Stern
Index: usb-3.15/drivers/usb/host/xhci-ring.c
===================================================================
--- usb-3.15.orig/drivers/usb/host/xhci-ring.c
+++ usb-3.15/drivers/usb/host/xhci-ring.c
@@ -3153,7 +3153,7 @@ static void check_trb_math(struct urb *u
static void giveback_first_trb(struct xhci_hcd *xhci, int slot_id,
unsigned int ep_index, unsigned int stream_id, int start_cycle,
- struct xhci_generic_trb *start_trb)
+ struct xhci_generic_trb *start_trb, bool ring_doorbell)
{
/*
* Pass all the TRBs to the hardware at once and make sure this write
@@ -3164,7 +3164,8 @@ static void giveback_first_trb(struct xh
start_trb->field[3] |= cpu_to_le32(start_cycle);
else
start_trb->field[3] &= cpu_to_le32(~TRB_CYCLE);
- xhci_ring_ep_doorbell(xhci, slot_id, ep_index, stream_id);
+ if (ring_doorbell)
+ xhci_ring_ep_doorbell(xhci, slot_id, ep_index, stream_id);
}
/*
@@ -3406,7 +3407,7 @@ static int queue_bulk_sg_tx(struct xhci_
check_trb_math(urb, num_trbs, running_total);
giveback_first_trb(xhci, slot_id, ep_index, urb->stream_id,
- start_cycle, start_trb);
+ start_cycle, start_trb, true);
return 0;
}
@@ -3545,7 +3546,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *
check_trb_math(urb, num_trbs, running_total);
giveback_first_trb(xhci, slot_id, ep_index, urb->stream_id,
- start_cycle, start_trb);
+ start_cycle, start_trb, true);
return 0;
}
@@ -3662,7 +3663,7 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *
field | TRB_IOC | TRB_TYPE(TRB_STATUS) | ep_ring->cycle_state);
giveback_first_trb(xhci, slot_id, ep_index, 0,
- start_cycle, start_trb);
+ start_cycle, start_trb, true);
return 0;
}
@@ -3742,8 +3743,10 @@ static unsigned int xhci_get_last_burst_
/* This is for isoc transfer */
static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
- struct urb *urb, int slot_id, unsigned int ep_index)
+ struct urb *urb, int slot_id, unsigned int ep_index,
+ unsigned esit)
{
+ struct xhci_virt_ep *ep;
struct xhci_ring *ep_ring;
struct urb_priv *urb_priv;
struct xhci_td *td;
@@ -3756,8 +3759,11 @@ static int xhci_queue_isoc_tx(struct xhc
u64 start_addr, addr;
int i, j;
bool more_trbs_coming;
+ bool new_isoc_stream;
+ unsigned start_uframe;
- ep_ring = xhci->devs[slot_id]->eps[ep_index].ring;
+ ep = &xhci->devs[slot_id]->eps[ep_index];
+ ep_ring = ep->ring;
num_tds = urb->number_of_packets;
if (num_tds < 1) {
@@ -3765,6 +3771,8 @@ static int xhci_queue_isoc_tx(struct xhc
return -EINVAL;
}
+ new_isoc_stream = list_empty(&urb->ep->urb_list);
+
start_addr = (u64) urb->transfer_dma;
start_trb = &ep_ring->enqueue->generic;
start_cycle = ep_ring->cycle_state;
@@ -3826,10 +3834,6 @@ static int xhci_queue_isoc_tx(struct xhc
field |= ep_ring->cycle_state;
}
- /* Only set interrupt on short packet for IN EPs */
- if (usb_urb_dir_in(urb))
- field |= TRB_ISP;
-
/* Chain all the TRBs together; clear the chain bit in
* the last TRB to indicate it's the last TRB in the
* chain.
@@ -3895,8 +3899,52 @@ static int xhci_queue_isoc_tx(struct xhc
}
xhci_to_hcd(xhci)->self.bandwidth_isoc_reqs++;
+ /*
+ * For new isochronous streams, schedule the transfer to start
+ * in the first slot beyond the Isochronous Scheduling Threshold
+ * (4.14.2.1.4). Otherwise, leave the SIA (Schedule Isochronous ASAP)
+ * bit set in the first TRB. Over/underruns are handled when the
+ * events are received.
+ */
+ if (new_isoc_stream) {
+ unsigned ist = HCS_IST(xhci->hcs_params2);
+ unsigned boundary;
+
+ start_uframe = readl(&xhci->run_regs->microframe_index);
+
+ /* IST is given in frames if bit 3 is set, else in uframes */
+ start_uframe += (ist & 0x8) ? (ist & 0x7) << 3 : ist & 0x7;
+
+ /* Round up to the following frame or ESIT boundary */
+ boundary = max(esit, 8u);
+ start_uframe = (start_uframe + boundary) & (- boundary);
+ start_uframe &= MFINDEX_MASK;
+
+ /* Store the starting frame number in the first TRB */
+ start_trb->field[3] &= cpu_to_le32(~TRB_SIA);
+ start_trb->field[3] |= cpu_to_le32(
+ TRB_FRAME_ID(start_uframe >> 3));
+ } else {
+ start_uframe = ep->next_uframe;
+ }
+ ep->next_uframe = (start_uframe + esit * num_tds) & MFINDEX_MASK;
+
+ urb->start_frame = start_uframe;
+ if (urb->dev->speed == USB_SPEED_FULL)
+ urb->start_frame >>= 3;
+
+ /*
+ * Ring the endpoint doorbell only for the first TRB of a new
+ * isochronous stream. Following TRBs should be queued sufficiently
+ * far in advance (beyond the IST) that no doorbell ring is needed.
+ *
+ * Indeed, if we did ring the doorbell again and an over/underrun
+ * occurred at the same time, we wouldn't know whether or not the
+ * endpoint had been removed from the periodic schedule, because
+ * ringing the doorbell reactivates endpoints (4.14.2.1).
+ */
giveback_first_trb(xhci, slot_id, ep_index, urb->stream_id,
- start_cycle, start_trb);
+ start_cycle, start_trb, new_isoc_stream);
return 0;
cleanup:
/* Clean up a partially enqueued isoc transfer. */
@@ -3935,7 +3983,6 @@ int xhci_queue_isoc_tx_prepare(struct xh
struct xhci_virt_device *xdev;
struct xhci_ring *ep_ring;
struct xhci_ep_ctx *ep_ctx;
- int start_frame;
int xhci_interval;
int ep_interval;
int num_tds, num_trbs, i;
@@ -3958,19 +4005,10 @@ int xhci_queue_isoc_tx_prepare(struct xh
if (ret)
return ret;
- start_frame = readl(&xhci->run_regs->microframe_index);
- start_frame &= 0x3fff;
-
- urb->start_frame = start_frame;
- if (urb->dev->speed == USB_SPEED_LOW ||
- urb->dev->speed == USB_SPEED_FULL)
- urb->start_frame >>= 3;
-
xhci_interval = EP_INTERVAL_TO_UFRAMES(le32_to_cpu(ep_ctx->ep_info));
ep_interval = urb->interval;
/* Convert to microframes */
- if (urb->dev->speed == USB_SPEED_LOW ||
- urb->dev->speed == USB_SPEED_FULL)
+ if (urb->dev->speed == USB_SPEED_FULL)
ep_interval *= 8;
/* FIXME change this to a warning and a suggestion to use the new API
* to set the polling interval (once the API is added).
@@ -3982,13 +4020,13 @@ int xhci_queue_isoc_tx_prepare(struct xh
xhci_interval, xhci_interval == 1 ? "" : "s");
urb->interval = xhci_interval;
/* Convert back to frames for LS/FS devices */
- if (urb->dev->speed == USB_SPEED_LOW ||
- urb->dev->speed == USB_SPEED_FULL)
+ if (urb->dev->speed == USB_SPEED_FULL)
urb->interval /= 8;
}
ep_ring->num_trbs_free_temp = ep_ring->num_trbs_free;
- return xhci_queue_isoc_tx(xhci, mem_flags, urb, slot_id, ep_index);
+ return xhci_queue_isoc_tx(xhci, mem_flags, urb, slot_id, ep_index,
+ xhci_interval);
}
/**** Command Ring Operations ****/
Index: usb-3.15/drivers/usb/host/xhci.h
===================================================================
--- usb-3.15.orig/drivers/usb/host/xhci.h
+++ usb-3.15/drivers/usb/host/xhci.h
@@ -481,6 +481,8 @@ struct xhci_run_regs {
struct xhci_intr_reg ir_set[128];
};
+#define MFINDEX_MASK 0x3fff
+
/**
* struct doorbell_array
*
@@ -887,6 +889,8 @@ struct xhci_virt_ep {
* process the missed tds on the endpoint ring.
*/
bool skip;
+ unsigned next_uframe; /* isochronous scheduling */
+
/* Bandwidth checking storage */
struct xhci_bw_info bw_info;
struct list_head bw_endpoint_list;
@@ -1167,6 +1171,7 @@ enum xhci_setup_dev {
/* Isochronous TRB specific fields */
#define TRB_SIA (1<<31)
+#define TRB_FRAME_ID(p) ((p) << 20)
struct xhci_generic_trb {
__le32 field[4];
--
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
