Hi,
Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> writes:
> Hi,
>
> On 4/11/2018 1:21 AM, Felipe Balbi wrote:
>>
>> Hi,
>>
>> Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx> 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
Attachment:
signature.asc
Description: PGP signature
