RE: [RFC] Sorting out dwc3 ISOC endpoints once and for all

[John Youn <John.Youn@synopsys.com>]

> -----Original Message-----
> From: Thinh Nguyen
> Sent: Monday, June 17, 2019 12:09 PM
> To: John Youn <johnyoun@synopsys.com>; Felipe Balbi
> <felipe.balbi@linux.intel.com>; John Youn <John.Youn@synopsys.com>
> Cc: linux-usb@vger.kernel.org
> Subject: Re: [RFC] Sorting out dwc3 ISOC endpoints once and for all
> 
> Hi,
> 
> John Youn wrote:
> >> -----Original Message-----
> >> From: Felipe Balbi <felipe.balbi@linux.intel.com>
> >> Sent: Friday, June 7, 2019 2:50 AM
> >> To: John Youn <John.Youn@synopsys.com>
> >> Cc: linux-usb@vger.kernel.org
> >> Subject: [RFC] Sorting out dwc3 ISOC endpoints once and for all
> >>
> > ++ Thinh
> >
> > Hi Felipe,
> >
> > Sorry, missed this e-mail.
> >
> >> Now that we have dwc3_gadget_start_isoc_quirk() which figures out the
> >> correct combination for the top-most 2 bits in the frame number, why
> >> don't we just use that to start isochronous transfers and never, again,
> >> have Bus Expiry problems?
> > We should only see expiry problems on the affected versions with incorrect
> top-2 bits right?
> >
> >> I mean something along the lines of below diff (completely untested):
> >>
> >> modified   drivers/usb/dwc3/gadget.c
> >> @@ -1369,9 +1369,8 @@ static int dwc3_gadget_start_isoc_quirk(struct
> >> dwc3_ep *dep)
> >>  	else if (test0 && test1)
> >>  		dep->combo_num = 0;
> >>
> >> -	dep->frame_number &= 0x3fff;
> >>  	dep->frame_number |= dep->combo_num << 14;
> >> -	dep->frame_number += max_t(u32, 4, dep->interval);
> >> +	dep->frame_number = DWC3_ALIGN_FRAME(dep, 1);
> >>
> >>  	/* Reinitialize test variables */
> >>  	dep->start_cmd_status = 0;
> >> @@ -1383,33 +1382,16 @@ static int dwc3_gadget_start_isoc_quirk(struct
> >> dwc3_ep *dep)
> >>  static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
> >>  {
> >>  	struct dwc3 *dwc = dep->dwc;
> >> -	int ret;
> >> -	int i;
> >>
> >>  	if (list_empty(&dep->pending_list)) {
> >>  		dep->flags |= DWC3_EP_PENDING_REQUEST;
> >>  		return -EAGAIN;
> >>  	}
> >>
> >> -	if (!dwc->dis_start_transfer_quirk && dwc3_is_usb31(dwc) &&
> >> -	    (dwc->revision <= DWC3_USB31_REVISION_160A ||
> >> -	     (dwc->revision == DWC3_USB31_REVISION_170A &&
> >> -	      dwc->version_type >= DWC31_VERSIONTYPE_EA01 &&
> >> -	      dwc->version_type <= DWC31_VERSIONTYPE_EA06))) {
> >> -
> >> -		if (dwc->gadget.speed <= USB_SPEED_HIGH && dep->direction)
> >> -			return dwc3_gadget_start_isoc_quirk(dep);
> >> -	}
> >> -
> >> -	for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
> >> -		dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1);
> >> +	dep->frame_number = __dwc3_gadget_get_frame(dwc);
> >> +	dep->frame_number = DWC3_ALIGN_FRAME(dep, 1);
> >>
> >> -		ret = __dwc3_gadget_kick_transfer(dep);
> >> -		if (ret != -EAGAIN)
> >> -			break;
> >> -	}
> >> -
> >> -	return ret;
> >> +	return dwc3_gadget_start_isoc_quirk(dep);
> >>  }
> >>
> >>  static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct
> >> dwc3_request *req)
> >>
> >>
> >> Would there be any obvious draw-back to going down this route? The thing
> >> is that, as it is, it seems like we will *always* have some corner case
> >> where we can't guarantee that we can even start a transfer since there's
> >> no upper-bound between XferNotReady and gadget driver finally queueing a
> >> request. Also, I can't simply read DSTS for the frame number because of
> >> top-most 2 bits.
> >>
> > For non-affected version of the IP, the xfernotready -> starttransfer time will
> have to be off by more than a couple seconds for the driver to produce an
> incorrect 16-bit frame number. If you're seeing errors here, maybe we just need
> to code review the relevant sections to make sure the 14/16-bit and rollover
> conditions are all handled correctly.
> 
> I think what Felipe may see is some delay in the system that causes the
> SW to not handle XferNotReady event in time. We already have the "retry"
> method handle that to a certain extend.
> 
> > But I can't think of any obvious drawbacks of the quirk, other than doing
> some unnecessary work, which shouldn't produce any bad side-effects. But we
> haven't really tested that.
> >
> 
> The workaround for the isoc_quirk requires 2 tries sending
> START_TRANSFER command. This means that you have to account the delay of
> that command completion plus potentially 1 more END_TRANSFER completion.
> That's why the quirk gives a buffer of at least 4 uframes of the
> scheduled isoc frame. So, it cannot schedule immediately on the next
> uframe, that's one of the drawbacks.
> 
> 

So the quirk is not ideal because it requires some extra time to retry start transfers. This might cause delays for getting the first request out.

The best way for non-affected IPs is probably to NOT use the quirk, and calculate the start transfer frame number based on the 16-bit xfernotready frame number and 14-bit DSTS frame number. As long as the delay between the xfernotready event and handling does not exceed 2 seconds, and you take into account rollover condition, this calculation should be correct.

The upper-bound between xfernotready and gadget's first request that you mention does not come into play, because if there is no request, the xfernotready event handler should do nothing. It should only do something when we have a queued request AND we get xfernotready.

John