Hi,
Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> writes:
> Hi,
>
> On 5/30/2018 11:49 PM, Felipe Balbi wrote:
>> Paul Zimmerman <pauldzim@xxxxxxxxx> writes:
>>
>>> Hi Felipe,
>>>
>>> Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx> writes:
>>>
>>> < snip >
>>>
>>>> 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.
>>> < snip >
>>>
>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>> index 9d4dc8bed644..9cf89f3cf203 100644
>>>> --- a/drivers/usb/dwc3/gadget.c
>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>> @@ -1233,6 +1233,8 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
>>>> return DWC3_DSTS_SOFFN(reg);
>>>> }
>>>>
>>>> +#define DWC3_ALIGN_FRAME(d) (((d)->frame_number + (d)->interval) & ~((d)->interval - 1))
>>>> +
>>>> static void __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
>>>> {
>>>> if (list_empty(&dep->pending_list)) {
>>>> @@ -1242,11 +1244,7 @@ static void __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
>>>> return;
>>>> }
>>>>
>>>> - /*
>>>> - * Schedule the first trb for one interval in the future or at
>>>> - * least 4 microframes.
>>>> - */
>>>> - dep->frame_number += max_t(u32, 4, dep->interval);
>>>> + dep->frame_number = DWC3_ALIGN_FRAME(dep);
>>>> __dwc3_gadget_kick_transfer(dep);
>>>> }
>>> Pretty sure this could cause problems. Instead of starting at least 4 uframes
>>> in the future, this will now try to start at the next aligned uframe. If
>>> dep->interval is very small (say 1) and we are almost at the end of the
>>> current uframe, there might not be enough time?
>> perhaps, but I haven't seen that happen yet. Guess I'll get to this when
>> I see such a problem.
>>
> I did some quick tests against DWC_usb3 controller v3.10a in high-speed
> and super-speed. We'll start to run into issue when if DWC3 needs to
> prepare 64+ TRBs after XferNotReady for isoc transfers with service
> interval of 1 uframe (start_transfer command will fail with bus-expiry).
> It may be better to calculate for the starting future uframe in a proper
> way.
it is proper, actually. We don't have a single gadget driver preparing
that many isoc transfers in one go and, in any case, if this ever
happens, we will only miss a single interval.
After the first isoc transfer is started, we will be using update
transfer command to add more intervals, in that case we don't need to
deal with starting uFrame number anyway.
I'll just go with it and if we ever experience this problem, then we can
try to find a way to estimate how many uFrames in the future we need to
start the transfer. It's a pointless exercise until we have a real
falilure with a real gadget driver. Sorry
--
balbi
Attachment:
signature.asc
Description: PGP signature
