Hi, Thinh Nguyen <[email protected]> writes: > Hi, > > On 4/11/2018 1:21 AM, Felipe Balbi wrote: >> >> Hi, >> >> Felipe Balbi <[email protected]> writes: >>>>> Without XferNotReady, we won't have a reliable way to know the uFrame >>>>> number. Read the Isochronous programming sequence from your databook. >>>> >>>> Right. We need XferNotReady to know when to start isoc transfer. But if >>>> there are still queued requests, DWC3 can just wait to see if any of >>>> them will succeed to continue with the transfer just as how DWC3 is >>>> handling it now. >>> >>> That's not what the databook says though. And that's also not intention >>> of how the code is written as of now either. The way the code is written >>> is the following: >>> >>> queue() -> XferNotReady -> start_isoc() -> if (missed) do_nothing() -> >>> queue() -> end_transfer. >>> >>> That's not really waiting for the queue to be consumed, it's just >>> delaying end transfer until we get another queue(). IOW, it just >>> *happens* to give the controller time to go through the list of started >>> requests. >>> >>>> If we end and restart the transfer right away, then we may lose more >>>> isoc data than necessary (due to isoc scheduling at least 4 uFrame >>>> ahead of time). Is there something you see that doesn't work with the >>>> current implementation? >>> >>> Not _really_, I'm just trying to make the code easier to read and, I >>> think, I've achieved that. Now, if we need to delay end transfer in the >>> case where we have more requests in the controller's queue, that's easy >>> enough to implement: >>> >>> @@ -2371,7 +2371,8 @@ static void >>> dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep, >>> if (event->status & DEPEVT_STATUS_BUSERR) >>> status = -ECONNRESET; >>> >>> - if (event->status & DEPEVT_STATUS_MISSED_ISOC) { >>> + if (event->status & DEPEVT_STATUS_MISSED_ISOC && >>> + list_empty(&dep->started_list) { >>> status = -EXDEV; > > Maybe we should return the -EXDEV status every time there's a missed isoc.
you mean like this?
@@ -2358,10 +2358,11 @@ static void
dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
if (event->status & DEPEVT_STATUS_BUSERR)
status = -ECONNRESET;
- if (event->status & DEPEVT_STATUS_MISSED_ISOC &&
- list_empty(&dep->started_list)) {
+ if (event->status & DEPEVT_STATUS_MISSED_ISOC) {
status = -EXDEV;
- stop = true;
+
+ if (list_empty(&dep->started_list))
+ stop = true;
}
dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);
>>> stop = true;
>>> }
>>>
>>> I'm not sure this is a good idea though. Once we miss an interval, don't
>>> we need to know the next frame when transfer needs to be scheduled?
>>>
>>> Meaning we would need XferNotReady to properly schedule the new
>>> transfer?
>>
>> thinking about this a little more. This extra list_empty() check is not
>> wrong at all :-) I've amended this series with the 3 patches below. I'll
>> resend the series once I've given more time for people to test. Patches
>> have been updated to the branch on kernel.org as well, btw.
>
> Great! :)
> Thanks for all the new updates. I'll test it out when I have a chance.
sure, thanks a lot.
--
balbi
signature.asc
Description: PGP signature
