Hi,

On 4/12/2018 12:18 AM, Felipe Balbi wrote:
> 
> Hi,
> 
> Thinh Nguyen <thinh.ngu...@synopsys.com> writes:
>> Hi,
>>
>> On 4/11/2018 1:21 AM, Felipe Balbi wrote:
>>>
>>> Hi,
>>>
>>> Felipe Balbi <felipe.ba...@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?

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to