On Thu, Oct 20, 2022, Thinh Nguyen wrote:
On Thu, Oct 20, 2022, Jeff Vanhoof wrote:
Hi Thinh,
On Wed, Oct 19, 2022 at 11:06:08PM +0000, Thinh Nguyen wrote:
Hi,
On Wed, Oct 19, 2022, Jeff Vanhoof wrote:
Hi Thinh, On Wed, Oct 19, 2022 at 07:08:27PM +0000, Thinh Nguyen wrote:
On Wed, Oct 19, 2022, Jeff Vanhoof wrote:
<snip>
From what I can gather from the log, with the current changes it seems that after a missed isoc event few requests are staying longer than expected in the started_list (not getting reclaimed) and this is preventing the transmission from stopping/starting again, and opening the door for continuous stream of missed isoc events that cause what appears to the user as a frozen video.
So one thought, if IOC bit is not set every frame, but IMI bit is, when a missed isoc related interrupt occurs it seems likely that more than one trb request will need to be reclaimed, but the current set of changes is not handling this.
In the good transfer case this issue seems to be taken care of since the IOC bit is not set every frame and the reclaimation will loop through every item in the started_list and only stop if there are no additional trbs or if one has
It should stop at the request that associated with the interrupt event, whether it's because of IMI or IOC.
In this case I was concerned that if multipled queued reqs did not have IOC bit set, but there was a missed isoc on one of the last reqs, whether or not we would reclaim all of the requests up to the missed isoc related req. I'm not sure if my concern is valid or not.
There should be no problem. If there's an interrupt event indicating a TRB completion, the driver will give back all the requests up to the request associated with the interrupt event, and the controller will continue processing the remaining TRBs. On the next TRB completion event, the driver will again give back all the requests up to the request associated with that event.
I was testing with the following patch you suggested:
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 61fba2b7389b..8352f4b5dd9f 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -3657,6 +3657,10 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, if (event->status & DEPEVT_STATUS_SHORT && !chain) return 1;
- if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
(event->status & DEPEVT_STATUS_MISSED_ISOC) && !chain)return 1;- if ((trb->ctrl & DWC3_TRB_CTRL_IOC) || (trb->ctrl & DWC3_TRB_CTRL_LST)) return 1;
At this time the IMI bit was set for every frame. With these changes it appeared in case of missed isoc that sometimes not all requests would be reclaimed (enqueued != dequeued even 100ms after the last interrupt was handled). If the 1st req in the started_list was fine (IMI set, but not IOC), and a later req was the one actually missed, because of this status check the reclaimation could stop early and not clean up to the appropriate req. As
Oops. You're right.
suggested yesterday, I also tried only setting the IMI bit when no_interrupt is not set, however I was still seeing the complete freezes. After analyzing this issue a bit, I have updated the diff to look more like this:
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index dfaf9ac24c4f..bb800a81815b 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -1230,8 +1230,9 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb, trb->ctrl = DWC3_TRBCTL_ISOCHRONOUS; }
/* always enable Interrupt on Missed ISOC */trb->ctrl |= DWC3_TRB_CTRL_ISP_IMI;
/* enable Interrupt on Missed ISOC */if ((!no_interrupt && !chain) || must_interrupt)trb->ctrl |= DWC3_TRB_CTRL_ISP_IMI;Either all or none of the TRBs of a request is set with IMI, and not some.
case USB_ENDPOINT_XFER_BULK: @@ -3195,6 +3196,11 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, if (event->status & DEPEVT_STATUS_SHORT && !chain) return 1;
- if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
(event->status & DEPEVT_STATUS_MISSED_ISOC) && !chain&& (trb->ctrl & DWC3_TRB_CTRL_ISP_IMI))return 1;- if ((trb->ctrl & DWC3_TRB_CTRL_IOC) || (trb->ctrl & DWC3_TRB_CTRL_LST)) return 1;
Where the trb must have the IMI set before returning early. This seemed to make the freezes recoverable.
Can you try this revised change:
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 61fba2b7389b..a69d8c28d86b 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -3654,7 +3654,7 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, if ((trb->ctrl & DWC3_TRB_CTRL_HWO) && status != -ESHUTDOWN) return 1;
- if (event->status & DEPEVT_STATUS_SHORT && !chain)
I accidentally deleted a couple of lines here.
- if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC && !chain) return 1;
if ((trb->ctrl & DWC3_TRB_CTRL_IOC) ||
I meant to do this:
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 61fba2b7389b..cb65371572ee 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -3657,6 +3657,9 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, if (event->status & DEPEVT_STATUS_SHORT && !chain) return 1;
+ if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC && !chain) + return 1; + if ((trb->ctrl & DWC3_TRB_CTRL_IOC) || (trb->ctrl & DWC3_TRB_CTRL_LST)) return 1; @@ -3673,6 +3676,7 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep, struct scatterlist *s; unsigned int num_queued = req->num_queued_sgs; unsigned int i; + bool missed_isoc = false; int ret = 0;
for_each_sg(sg, s, num_queued, i) { @@ -3681,12 +3685,18 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep, req->sg = sg_next(s); req->num_queued_sgs--;
+ if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC) + missed_isoc = true; + ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req, trb, event, status, true); if (ret) break; }
+ if (missed_isoc) + ret = 1; + return ret; }
BR, Thinh