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;
> > break;
>
> 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
