Thinh Nguyen wrote:
> Thinh Nguyen wrote:
>> Hi,
>>
>> Michael Grzeschik wrote:
>>> Hi!
>>>
>>> On Wed, Jun 24, 2020 at 07:26:42PM +0000, Thinh Nguyen wrote:
>>>> Michael Grzeschik wrote:
>>>>> From: Michael Olbrich <m.olbrich@xxxxxxxxxxxxxx>
>>>>>
>>>>> Currently __dwc3_gadget_start_isoc must be called very shortly after
>>>>> XferNotReady. Otherwise the frame number is outdated and start transfer
>>>>> will fail, even with several retries.
>>>> Did you try with the recent update for isoc? (e.i., after 5
>>>> START_TRANSFER failures, the driver will issue END_TRANSFER to retry on
>>>> the next XferNotReady event)
>>>>
>>>> Let me know if you still run into issues with that.
>>> Yes. Without my patch I still see the issue. Event with the retry
>>> machanism. It is even worse. I even missed one additional patch,
>>> I had on top this one. See my note below.
>> Ok. Can you clarify what issue you're seeing?
>>
>>>>> DSTS provides the lower 14 bit of the frame number. Use it in
>>>>> combination
>>>>> with the frame number provided by XferNotReady to guess the current
>>>>> frame
>>>>> number. This will succeed unless more than one 14 rollover has happened
>>>>> since XferNotReady.
>>>>>
>>>>> Start transfer might still fail if the frame number is increased
>>>>> immediately after DSTS is read. So retries are still needed.
>>>>> Don't drop the current request if this happens. This way it is not
>>>>> lost and
>>>>> can be used immediately to try again with the next frame number.
>>>>>
>>>>> With this change, __dwc3_gadget_start_isoc is still not successfully
>>>>> in all
>>>>> cases bit it increases the acceptable delay after XferNotReady
>>>>> significantly.
>>>>>
>>>>> Signed-off-by: Michael Olbrich <m.olbrich@xxxxxxxxxxxxxx>
>>>>> Signed-off-by: Michael Tretter <m.tretter@xxxxxxxxxxxxxx>
>>>>> Signed-off-by: Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx>
>>>>>
>>>>> ---
>>>>> v1 -> v2: - removed last_frame_number from struct
>>>>> - included rollover variable
>>>>>
>>>>> drivers/usb/dwc3/gadget.c | 37 +++++++++++++++++++++++++------------
>>>>> 1 file changed, 25 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>>> index 421a0f73110a40b..0962ddd7fbf6ae6 100644
>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>> @@ -1276,7 +1276,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep
>>>>> *dep)
>>>>>
>>>>> static void dwc3_gadget_ep_cleanup_cancelled_requests(struct
>>>>> dwc3_ep *dep);
>>>>>
>>>>> -static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
>>>>> +static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep, bool
>>>>> keep_req)
>>>> Any reason to have keep_req? With the recent changes, if
>>>> dwc3_send_gadget_ep_cmd() returns -EAGAIN, then the controller driver
>>>> won't give back the request.
>>> Yes, we don't need the keep_req. I tried this and it worked without.
>>> I will remove the parameter in v3.
>>>
>>>>> {
>>>>> struct dwc3_gadget_ep_cmd_params params;
>>>>> struct dwc3_request *req;
>>>>> @@ -1314,12 +1314,9 @@ static int __dwc3_gadget_kick_transfer(struct
>>>>> dwc3_ep *dep)
>>>>> }
>>>>>
>>>>> ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms);
>>>>> - if (ret < 0) {
>>>>> + if (ret < 0 && (!keep_req || ret != -EAGAIN)) {
>>>>> struct dwc3_request *tmp;
>>>>>
>>>>> - if (ret == -EAGAIN)
>>>>> - return ret;
>>>>> -
>>>>> dwc3_stop_active_transfer(dep, true, true);
>>>>>
>>>>> list_for_each_entry_safe(req, tmp, &dep->started_list, list)
>>>>> @@ -1335,7 +1332,7 @@ static int __dwc3_gadget_kick_transfer(struct
>>>>> dwc3_ep *dep)
>>>>> if (dep->stream_capable && req->request.is_last)
>>>>> dep->flags |= DWC3_EP_WAIT_TRANSFER_COMPLETE;
>>>>>
>>>>> - return 0;
>>>>> + return ret;
>>>>> }
>>>>>
>>>>> static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
>>>>> @@ -1458,7 +1455,7 @@ static int dwc3_gadget_start_isoc_quirk(struct
>>>>> dwc3_ep *dep)
>>>>> dep->start_cmd_status = 0;
>>>>> dep->combo_num = 0;
>>>>>
>>>>> - return __dwc3_gadget_kick_transfer(dep);
>>>>> + return __dwc3_gadget_kick_transfer(dep, false);
>>>>> }
>>>>>
>>>>> static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
>>>>> @@ -1481,9 +1478,25 @@ static int __dwc3_gadget_start_isoc(struct
>>>>> dwc3_ep *dep)
>>>>> }
>>>>>
>>>>> for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
>>>>> - dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1);
>>>>> + /*
>>>>> + * frame_number is set from XferNotReady and may be already
>>>>> + * out of date. DSTS only provides the lower 14 bit of the
>>>>> + * current frame number. So add the upper two bits of
>>>>> + * frame_number and handle a possible rollover.
>>>>> + * This will provide the correct frame_number unless more than
>>>>> + * rollover has happened since XferNotReady.
>>>>> + */
>>>>> + u32 frame = __dwc3_gadget_get_frame(dwc);
>>>>> + bool rollover = frame < (dep->frame_number & 0x3fff);
>>>>> +
>>>>> + dep->frame_number = (dep->frame_number & ~0x3fff) | frame;
>>>>> + if (rollover)
>>>>> + dep->frame_number += BIT(14);
>>>>> +
>>>>> + dep->frame_number = DWC3_ALIGN_FRAME(dep, 1);
>>> This is not enough, we have to add i into the alignment to ensure
>>> that the stream is not failing:
>>>
>>> dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 2);
>>>
>>> I am even thiking to move the frame number calculation into the
>>> DWC3_DEPCMD_STARTTRANSFER code path of dwc3_send_gadget_ep_cmd. But this
>>> will break the dwc3_gadget_start_isoc_quirk function. What do you think?
>> You shouldn't be keep calling __dwc3_gadget_get_frame() in a loop. You
>> should do all these calculation to get the current frame_number only
>> once before entering the retry loop.
>>
>> The issue here is we don't know when the XferNotReady event will be
>> handled, which may be too late and multiple uframe had gone by. But once
>> the event is being handled, it shouldn't take much time at all. That
>> means __dwc3_gadget_get_frame() will return the same value.
>>
>> So, we need something like this:
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index f66ff7fd87a9..1cd73c2dbe71 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -1709,6 +1709,8 @@ static int dwc3_gadget_start_isoc_quirk(struct
>> dwc3_ep *dep)
>> static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
>> {
>> struct dwc3 *dwc = dep->dwc;
>> + u32 current_frame;
>> + bool rollover;
>> int ret;
>> int i;
>>
>> @@ -1725,6 +1727,13 @@ static int __dwc3_gadget_start_isoc(struct
>> dwc3_ep *dep)
>> return dwc3_gadget_start_isoc_quirk(dep);
>> }
>>
>> + current_frame = __dwc3_gadget_get_frame(dwc);
>> + rollover = current_frame < (dep->frame_number & 0x3fff);
>> +
>> + dep->frame_number = (dep->frame_number & ~0x3fff) | current_frame;
>> + if (rollover)
>> + dep->frame_number += BIT(14);
>> +
>> for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
>> dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1);
>>
>>
>> (Please create a macro for 0x3fff mask)
>>
> Forgot a couple of notes:
>
> 1) If bInterval is greater than 14, don't attempt to get current frame
> from DSTS and ignore this mechanism.
> 2) The rollover check needs to account for number of uframes per
> interval. If the difference is less than an interval length, then no
> need to update dep->frame_number.
>
Ignore number 2), DWC3_ALIGN_FRAME() should take care of that...
BR,
Thinh
