Re: [PATCH v3 2/6] usb: dwc3: gadget: cancel requests instead of release after missed isoc

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

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