Thinh Nguyen wrote: > Felipe Balbi wrote: >> Hi, >> >> Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> 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. > Correction: these values are not internal. (somehow I was thinking the schedule time is calculated internally with the 16-bit value led me to say that they are internal). Thinh
