Hi,
On 4/12/2018 12:18 AM, Felipe Balbi wrote:
>
> 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?
Yes, this will work.
>
> @@ -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);
>
BR,
Thinh
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
