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

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

Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>>  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.
>>
>>
>> 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.

> 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.

You will need to calculate this value along with BIT(15) and BIT(14) of
XferNotReady for rollovers.

>
> To me, it seems like this part of the controller wasn't well
> thought-out. These extra two bits, perhaps, should be internal to the
> controller and SW should have no knowledge that they exist.

These values are internal. SW should not have knowledge of it. This
implementation will not follow the programming guide and should be used
as a quirk for devices that are too slow to handle the XferNotReady
event but want to schedule isoc immediately after handling the event.

> In any case, this is the biggest sort of issues in DWC3 right now :-)
>
> Anything else seems to behave nicely without any problems.
>

BR,
Thinh