[RFT/PATCH,18/38] usb: dwc3: gadget: check for Missed Isoc from event status

[Felipe Balbi <felipe.balbi@linux.intel.com>]

Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
> Hi,
>
> On 4/11/2018 1:21 AM, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Felipe Balbi <felipe.balbi@linux.intel.com> 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.