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

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

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.

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

Thinh
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html