From: Mathias Nyman <mathias.nyman@linux.intel.com>
To: dylan_robinson@motu.com
Cc: linux-usb@vger.kernel.org, mathias.nyman@intel.com,
michal.pecio@gmail.com, stern@rowland.harvard.edu,
Mathias Nyman <mathias.nyman@linux.intel.com>
Subject: [RFT PATCH] xhci: fix frame id calculation for isoc transfer
Date: Mon, 11 May 2026 22:21:05 +0300 [thread overview]
Message-ID: <20260511192105.3756809-1-mathias.nyman@linux.intel.com> (raw)
In-Reply-To: <CA+Df+jeoj_QL6XNzUyP963HF7ngCWoAs0MjqQXPr3Fa6VW3rXw@mail.gmail.com>
Always calculate estimated start frame, and set urb->start_frame
Fix valid frame window start and end calculation to match xhci
spec 4.11.2.5
Don't inclease frame id with 1 if a URB mod transfer is queued late.
Queue it with next expected frame ID but print a message if URB is
next expected frame id does not fir valid frame window range (URB mid
transfer is queued late)
This patch doesn't switch to URB_ISO_ASAP /SIA scheduling if URB is queued
late, not sure if that would help as it only moves the frame id glitch
problem forward, unless _every_ URB queued after a late URB is forced to
use SIA, which again would defeat the point of Frame ID use.
get rid of the annoying 'xep' and 'xdev' variables, xhci driver uses
ep and vdev naming everywhere else
Contains some FIXMEs, patch for initial testing purposes
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
drivers/usb/host/xhci-mem.c | 1 +
drivers/usb/host/xhci-ring.c | 189 ++++++++++++++---------------------
drivers/usb/host/xhci.h | 7 +-
3 files changed, 80 insertions(+), 117 deletions(-)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 997fe90f54e5..58714e10773f 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1499,6 +1499,7 @@ int xhci_endpoint_init(struct xhci_hcd *xhci,
return -ENOMEM;
virt_dev->eps[ep_index].skip = false;
+ virt_dev->eps[ep_index].next_uframe = -1;
ep_ring = virt_dev->eps[ep_index].new_ring;
xhci_ring_init(xhci, ep_ring);
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index e47e644b296e..e63dc6920685 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3977,80 +3977,64 @@ static int xhci_ist_microframes(struct xhci_hcd *xhci)
}
/*
- * Calculates Frame ID field of the isochronous TRB identifies the
- * target frame that the Interval associated with this Isochronous
- * Transfer Descriptor will start on. Refer to 4.11.2.5 in 1.1 spec.
+ * Set the urb->start_frame of the URB.
*
- * Returns actual frame id on success, negative value on error.
+ * Returns microframe index of first TD
*/
-static int xhci_get_isoc_frame_id(struct xhci_hcd *xhci,
- struct urb *urb, int index)
+static int xhci_get_isoc_start_frame(struct xhci_hcd *xhci, struct urb *urb,
+ struct xhci_virt_ep *ep)
{
- int start_frame, ist, ret = 0;
- int start_frame_id, end_frame_id, current_frame_id;
+ int curr_frame, start_uframe;
+ int win_start, win_end;
+ bool frame_unit;
+ bool in_range;
+ int uinterval;
+ u32 mfindex;
+ int ist;
- if (urb->dev->speed == USB_SPEED_LOW ||
- urb->dev->speed == USB_SPEED_FULL)
- start_frame = urb->start_frame + index * urb->interval;
- else
- start_frame = (urb->start_frame + index * urb->interval) >> 3;
+ /* check if urb uses frame units instead of microframes */
+ frame_unit = (urb->dev->speed == USB_SPEED_FULL ||
+ urb->dev->speed == USB_SPEED_LOW);
+
+ uinterval = urb->interval;
+ if (frame_unit)
+ uinterval *= 8;
+ /* get current microframe index and isoc sheduling threshold */
+ mfindex = readl(&xhci->run_regs->microframe_index);
ist = xhci_ist_microframes(xhci);
- /* Software shall not schedule an Isoch TD with a Frame ID value that
- * is less than the Start Frame ID or greater than the End Frame ID,
- * where:
- *
- * End Frame ID = (Current MFINDEX register value + 895 ms.) MOD 2048
- * Start Frame ID = (Current MFINDEX register value + IST + 1) MOD 2048
- *
- * Both the End Frame ID and Start Frame ID values are calculated
- * in microframes. When software determines the valid Frame ID value;
- * The End Frame ID value should be rounded down to the nearest Frame
- * boundary, and the Start Frame ID value should be rounded up to the
- * nearest Frame boundary.
- */
- current_frame_id = readl(&xhci->run_regs->microframe_index);
- start_frame_id = roundup(current_frame_id + ist + 1, 8);
- end_frame_id = rounddown(current_frame_id + 895 * 8, 8);
+ /* calculate valid frame window, in frame units, see xhci 4.11.2.5 */
+ curr_frame = MFINDEX_TO_FRAME(mfindex);
+ win_start = (curr_frame + DIV_ROUND_UP_POW2(ist, 8) + 1) % MAX_FRAMES;
+ win_end = (curr_frame + 895) % MAX_FRAMES;
- start_frame &= 0x7ff;
- start_frame_id = (start_frame_id >> 3) & 0x7ff;
- end_frame_id = (end_frame_id >> 3) & 0x7ff;
+ /* are we mid stream? is URB is queued inside the valid frame window */
+ // FIXME, used to check if !list_empty(&ep_ring->td_list)), is that reliable
+ if (ep->next_uframe >= 0) {
+ u32 frame = ep->next_uframe / 8;
- if (start_frame_id < end_frame_id) {
- if (start_frame > end_frame_id ||
- start_frame < start_frame_id)
- ret = -EINVAL;
- } else if (start_frame_id > end_frame_id) {
- if ((start_frame > end_frame_id &&
- start_frame < start_frame_id))
- ret = -EINVAL;
- } else {
- ret = -EINVAL;
- }
+ in_range = frame >= win_start && frame <= win_end;
+ /* valid frame window end wrapped around */
+ if (win_start > win_end)
+ in_range = frame >= win_start || frame <= win_end;
- if (index == 0) {
- if (ret == -EINVAL || start_frame == start_frame_id) {
- start_frame = start_frame_id + 1;
- if (urb->dev->speed == USB_SPEED_LOW ||
- urb->dev->speed == USB_SPEED_FULL)
- urb->start_frame = start_frame;
- else
- urb->start_frame = start_frame << 3;
- ret = 0;
- }
- }
+ start_uframe = ep->next_uframe;
- if (ret) {
- xhci_warn(xhci, "Frame ID %d (reg %d, index %d) beyond range (%d, %d)\n",
- start_frame, current_frame_id, index,
- start_frame_id, end_frame_id);
- xhci_warn(xhci, "Ignore frame ID field, use SIA bit instead\n");
- return ret;
+ if (!in_range)
+ xhci_warn(xhci, "Frame ID %d for Isoc URB %p is outside range %d-%d\n",
+ frame, urb, win_start, win_end);
+ } else {
+ /* align first URB to next interval boundary, or at last to full frame */
+ start_uframe = mfindex + ist + XHCI_CFC_DELAY;
+ start_uframe = roundup(start_uframe, 8);
+ start_uframe = roundup(start_uframe, uinterval);
}
- return start_frame;
+ /* set urb->start_frame */
+ urb->start_frame = frame_unit ? start_uframe / 8 : start_uframe;
+
+ return start_uframe;
}
/* Check if we should generate event interrupt for a TD in an isoc URB */
@@ -4089,10 +4073,12 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
u64 start_addr, addr;
int i, j;
bool more_trbs_coming;
- struct xhci_virt_ep *xep;
+ struct xhci_virt_ep *ep;
int frame_id;
+ int uinterval = urb->interval;
+ int start_uframe;
- xep = &xhci->devs[slot_id]->eps[ep_index];
+ ep = &xhci->devs[slot_id]->eps[ep_index];
ep_ring = xhci->devs[slot_id]->eps[ep_index].ring;
ir = xhci->interrupters[0];
@@ -4106,12 +4092,17 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
start_cycle = ep_ring->cycle_state;
urb_priv = urb->hcpriv;
+
+ if (urb->dev->speed == USB_SPEED_FULL || urb->dev->speed == USB_SPEED_LOW)
+ uinterval = urb->interval * 8;
+
+ start_uframe = xhci_get_isoc_start_frame(xhci, urb, ep);
+
/* Queue the TRBs for each TD, even if they are zero-length */
for (i = 0; i < num_tds; i++) {
unsigned int total_pkt_count, max_pkt;
unsigned int burst_count, last_burst_pkt_count;
u32 sia_frame_id;
-
first_trb = true;
running_total = 0;
addr = start_addr + urb->iso_frame_desc[i].offset;
@@ -4137,14 +4128,14 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
goto cleanup;
}
td = &urb_priv->td[i];
- /* use SIA as default, if frame id is used overwrite it */
+
sia_frame_id = TRB_SIA;
- if (!(urb->transfer_flags & URB_ISO_ASAP) &&
- (xhci->hcc_params & HCC_CFC)) {
- frame_id = xhci_get_isoc_frame_id(xhci, urb, i);
- if (frame_id >= 0)
- sia_frame_id = TRB_FRAME_ID(frame_id);
+ if (!(urb->transfer_flags & URB_ISO_ASAP) && (xhci->hcc_params & HCC_CFC)) {
+ frame_id = (start_uframe + i * uinterval) / 8;
+ frame_id %= MAX_FRAMES;
+ sia_frame_id = TRB_FRAME_ID(frame_id);
}
+
/*
* Set isoc specific data for the first TRB in a TD.
* Prevent HW from getting the TRBs by keeping the cycle state
@@ -4156,7 +4147,7 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
(i ? ep_ring->cycle_state : !start_cycle);
/* xhci 1.1 with ETE uses TD_Size field for TBC, old is Rsvdz */
- if (!xep->use_extended_tbc)
+ if (!ep->use_extended_tbc)
field |= TRB_TBC(burst_count);
/* fill the rest of the TRB fields, and remaining normal TRBs */
@@ -4198,7 +4189,7 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
TRB_INTR_TARGET(0);
/* xhci 1.1 with ETE uses TD Size field for TBC */
- if (first_trb && xep->use_extended_tbc)
+ if (first_trb && ep->use_extended_tbc)
length_field |= TRB_TD_SIZE_TBC(burst_count);
else
length_field |= TRB_TD_SIZE(remainder);
@@ -4223,9 +4214,7 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
}
}
- /* store the next frame id */
- if (xhci->hcc_params & HCC_CFC)
- xep->next_frame_id = urb->start_frame + num_tds * urb->interval;
+ ep->next_uframe = (start_uframe + num_tds * uinterval) % MAX_UFRAMES;
if (xhci_to_hcd(xhci)->self.bandwidth_isoc_reqs == 0) {
if (xhci->quirks & XHCI_AMD_PLL_FIX)
@@ -4249,7 +4238,7 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
*/
urb_priv->td[0].end_trb = ep_ring->enqueue;
/* Every TRB except the first & last will have its cycle bit flipped. */
- td_to_noop(xhci, xep, &urb_priv->td[0], true);
+ td_to_noop(xhci, ep, &urb_priv->td[0], true);
/* Reset the ring enqueue back to the first TRB and its cycle bit. */
ep_ring->enqueue = urb_priv->td[0].start_trb;
@@ -4269,19 +4258,17 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
int xhci_queue_isoc_tx_prepare(struct xhci_hcd *xhci, gfp_t mem_flags,
struct urb *urb, int slot_id, unsigned int ep_index)
{
- struct xhci_virt_device *xdev;
+ struct xhci_virt_device *vdev;
struct xhci_ring *ep_ring;
struct xhci_ep_ctx *ep_ctx;
- int start_frame;
+ struct xhci_virt_ep *ep;
int num_tds, num_trbs, i;
int ret;
- struct xhci_virt_ep *xep;
- int ist;
- xdev = xhci->devs[slot_id];
- xep = &xhci->devs[slot_id]->eps[ep_index];
- ep_ring = xdev->eps[ep_index].ring;
- ep_ctx = xhci_get_ep_ctx(xhci, xdev->out_ctx, ep_index);
+ vdev = xhci->devs[slot_id];
+ ep = &xhci->devs[slot_id]->eps[ep_index];
+ ep_ring = vdev->eps[ep_index].ring;
+ ep_ctx = xhci_get_ep_ctx(xhci, vdev->out_ctx, ep_index);
num_trbs = 0;
num_tds = urb->number_of_packets;
@@ -4302,38 +4289,8 @@ int xhci_queue_isoc_tx_prepare(struct xhci_hcd *xhci, gfp_t mem_flags,
*/
check_interval(urb, ep_ctx);
- /* Calculate the start frame and put it in urb->start_frame. */
- if ((xhci->hcc_params & HCC_CFC) && !list_empty(&ep_ring->td_list)) {
- if (GET_EP_CTX_STATE(ep_ctx) == EP_STATE_RUNNING) {
- urb->start_frame = xep->next_frame_id;
- goto skip_start_over;
- }
- }
-
- start_frame = readl(&xhci->run_regs->microframe_index);
- start_frame &= 0x3fff;
- /*
- * Round up to the next frame and consider the time before trb really
- * gets scheduled by hardare.
- */
- ist = xhci_ist_microframes(xhci);
- start_frame += ist + XHCI_CFC_DELAY;
- start_frame = roundup(start_frame, 8);
-
- /*
- * Round up to the next ESIT (Endpoint Service Interval Time) if ESIT
- * is greate than 8 microframes.
- */
- if (urb->dev->speed == USB_SPEED_LOW ||
- urb->dev->speed == USB_SPEED_FULL) {
- start_frame = roundup(start_frame, urb->interval << 3);
- urb->start_frame = start_frame >> 3;
- } else {
- start_frame = roundup(start_frame, urb->interval);
- urb->start_frame = start_frame;
- }
-
-skip_start_over:
+ if (GET_EP_CTX_STATE(ep_ctx) != EP_STATE_RUNNING)
+ ep->next_uframe = -1;
return xhci_queue_isoc_tx(xhci, mem_flags, urb, slot_id, ep_index);
}
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index aeecd301f207..bcb7b8c877db 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -287,6 +287,11 @@ struct xhci_run_regs {
struct xhci_intr_reg ir_set[1024];
};
+/* Bits [13:3] of the microframe index equals the 1ms frame index */
+#define MFINDEX_TO_FRAME(p) (((p) >> 3) & 0x7ff)
+#define MAX_FRAMES 2048
+#define MAX_UFRAMES (MAX_FRAMES * 8)
+
/**
* struct doorbell_array
*
@@ -696,7 +701,7 @@ struct xhci_virt_ep {
struct list_head bw_endpoint_list;
unsigned long stop_time;
/* Isoch Frame ID checking storage */
- int next_frame_id;
+ int next_uframe;
/* Use new Isoch TRB layout needed for extended TBC support */
bool use_extended_tbc;
/* set if this endpoint is controlled via sideband access*/
--
2.43.0
next prev parent reply other threads:[~2026-05-11 19:21 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-11-04 21:25 [Bug 220748] New: usb: xhci_queue_isoc_tx_prepare ignore start_frame and always assumes URB_ISO_ASAP is set bugzilla-daemon
2025-11-05 8:40 ` [Bug 220748] " bugzilla-daemon
2025-11-05 8:48 ` bugzilla-daemon
2025-11-05 8:53 ` bugzilla-daemon
2025-11-05 9:06 ` bugzilla-daemon
2025-11-05 15:47 ` bugzilla-daemon
2025-11-05 18:28 ` bugzilla-daemon
2025-11-05 18:30 ` bugzilla-daemon
2025-11-05 20:16 ` bugzilla-daemon
2025-11-05 21:18 ` bugzilla-daemon
2025-11-06 3:52 ` bugzilla-daemon
2025-11-06 8:27 ` bugzilla-daemon
2025-11-06 8:35 ` bugzilla-daemon
2025-11-06 15:03 ` bugzilla-daemon
2025-11-08 10:33 ` bugzilla-daemon
2025-11-08 16:09 ` bugzilla-daemon
2025-11-10 10:23 ` bugzilla-daemon
2025-11-10 10:42 ` bugzilla-daemon
2026-05-04 23:53 ` bugzilla-daemon
2026-05-04 23:54 ` bugzilla-daemon
2026-05-05 1:14 ` bugzilla-daemon
2026-05-05 9:59 ` bugzilla-daemon
2026-05-05 17:09 ` bugzilla-daemon
2026-05-05 17:10 ` bugzilla-daemon
2026-05-05 17:10 ` bugzilla-daemon
2026-05-05 17:13 ` bugzilla-daemon
2026-05-06 13:32 ` bugzilla-daemon
2026-05-06 15:03 ` bugzilla-daemon
2026-05-07 2:38 ` Alan Stern
2026-05-07 16:17 ` Dylan Robinson
2026-05-07 17:24 ` Alan Stern
2026-05-07 21:16 ` Dylan Robinson
2026-05-08 3:02 ` Alan Stern
2026-05-08 17:20 ` Dylan Robinson
2026-05-09 1:25 ` Alan Stern
2026-05-09 22:12 ` Michal Pecio
2026-05-10 12:39 ` Dylan Robinson
2026-05-11 19:21 ` Mathias Nyman [this message]
2026-05-11 19:36 ` [RFT PATCH] xhci: fix frame id calculation for isoc transfer Mathias Nyman
2026-05-07 21:54 ` [Bug 220748] usb: xhci_queue_isoc_tx_prepare ignore start_frame and always assumes URB_ISO_ASAP is set Michal Pecio
2026-05-08 3:09 ` Alan Stern
2026-05-08 9:41 ` Michal Pecio
2026-05-08 14:54 ` Alan Stern
2026-05-08 21:39 ` Dylan Robinson
2026-05-09 11:10 ` Michal Pecio
2026-05-09 20:18 ` Dylan Robinson
2026-05-11 19:15 ` bugzilla-daemon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260511192105.3756809-1-mathias.nyman@linux.intel.com \
--to=mathias.nyman@linux.intel.com \
--cc=dylan_robinson@motu.com \
--cc=linux-usb@vger.kernel.org \
--cc=mathias.nyman@intel.com \
--cc=michal.pecio@gmail.com \
--cc=stern@rowland.harvard.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox