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

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

> -----Original Message-----
> From: linux-usb-owner@vger.kernel.org <linux-usb-owner@vger.kernel.org> On
> Behalf Of Felipe Balbi
> Sent: Tuesday, June 18, 2019 11:29 PM
> To: Thinh Nguyen <Thinh.Nguyen@synopsys.com>; Thinh Nguyen
> <Thinh.Nguyen@synopsys.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,
> 
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
> >>>>> 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.
> >>>
> >>>
> >>> Hi Felipe,
> >>>
> >>> Since you're asking this, it means you're still seeing issue with your
> >>> setup despite retrying to send START_TRANSFER command 5 times. What's
> >>> the worse delay responding to XferNotReady you're seeing in your setup?
> >> There's no upper-bound on how long the gadget will take to enqueue a
> >> request. We see problems with UVC gadget all the time. It can take a lot
> >> of time to decide to enqueue data.
> >
> > That's why there's a mechanism in the controller to return bus-expiry
> > status to let the SW know if it had scheduled isoc too late. SW can do 2
> > things: 1) re-schedule at a later timer or 2) send END_TRANSFER command
> > to wait for the next XferNotReady to try again.
> 
> All of this is still rather flakey. Can I send consecutive END_TRANSFER
> commands and get new XferNotReady at any moment? Consider this
> situation:
> 
> . transfer started
> . transfer completes with status Missed ISOC
> . driver issues END_TRANSFER (as required by docs)
> . XferNotReady fires
> . driver updates current frame number
> . several mS of nothing
> . finally gadget enqueues a transfer
> . Start Transfer command
> . completes with Bus Expiry
> 
> Can I issue *another* END_TRANSFER at this point? I don't even have a
> valid transfer resource since transfer wasn't started.
> 
> The best "workaround" I can think of would be to delay END_TRASFER until
> ep_queue time.
> 
> >> Usually I hear this from folks using UVC gadget with a real sensor on
> >> the background.
> >>
> >> I've seen gadget enqueueing as far as 20 intervals in the future. But
> >> remember, there's no upper-bound. And that's the problem. If we could
> >> just read the frame number from DSTS and use that, we wouldn't have any
> >> issues. But since DSTS only contains 14 our of the 16 bits the
> >> controller needs, then we can't really use that.
> >
> > You can create another quirk for devices that have this behavior to use
> > frame number in DSTS instead.  As John had pointed out and mentioned,
> > this will only work if the service interval and the delay in the
> > scheduling of isoc is within 2 seconds.
> 
> well, that's better than nothing, but there's no upper-bound for the
> gadget driver, really.
> 

This will take care of the scenario you described above. Using xfernotready+DSTS to calculate the start transfer frame number should probably just be the default behavior.

For the case the gadget driver takes > 2 seconds to queue, you can go through the quirk. It's probably best to do this pre-emptively rather than rely on bus expiry. Because bus expiry only happens when your start frame is off by > 2 seconds. So you may get the top-2 bits wrong and start transfer will succeed, but you will have introduced a delay in the stream.

> They are *not* internal if SW needs to know that to start a transfer
> properly it needs these extra two bits :-) What I meant to say was that
> we should never have a 16-bit frame number. Only 14 bits. But in any
> case, we can't change the HW now :-)

I believe the bits were added to allow for scheduling of large intervals, like 2 and 4 seconds. If anything DSTS should reveal the 16-bit frame number as well.

We can ask our hw engineers if they have any other suggestions for this case.

John