Hi Felipe,
On 11/8/2018 11:11 PM, Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <thinh.nguyen@xxxxxxxxxxxx> writes:
>> On 11/7/2018 10:58 PM, Felipe Balbi wrote:
>>> Gadget driver may take an unbounded amount of time to queue requests
>>> after XferNotReady. This is important for isochronous endpoints which
>>> need to be started for a specific (micro-)frame.
>>>
>>> Before kicking the transfer, let's check how much time has elapsed
>>> since dep->frame_number was updated and make sure we start the request
>>> to the next valid interval.
>>>
>>> Signed-off-by: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
>>> ---
>>> drivers/usb/dwc3/core.h | 5 +++++
>>> drivers/usb/dwc3/gadget.c | 11 +++++++++++
>>> 2 files changed, 16 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>> index 131028501752..306a2dd75ed5 100644
>>> --- a/drivers/usb/dwc3/core.h
>>> +++ b/drivers/usb/dwc3/core.h
>>> @@ -651,6 +651,7 @@ struct dwc3_event_buffer {
>>> * @number: endpoint number (1 - 15)
>>> * @type: set to bmAttributes & USB_ENDPOINT_XFERTYPE_MASK
>>> * @resource_index: Resource transfer index
>>> + * @frame_timestamp: timestamp of most recent frame number
>>> * @frame_number: set to the frame number we want this transfer to start (ISOC)
>>> * @interval: the interval on which the ISOC transfer is started
>>> * @name: a human readable name e.g. ep1out-bulk
>>> @@ -697,7 +698,11 @@ struct dwc3_ep {
>>> u8 number;
>>> u8 type;
>>> u8 resource_index;
>>> +
>>> + u64 frame_timestamp;
>>> u32 frame_number;
>>> +#define DWC3_EP_FRAME_NUMBER_MASK 0x3fff
>>> +
>>> u32 interval;
>>>
>>> char name[20];
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index d8c7ad0c22e8..00fe01a01977 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -1268,12 +1268,22 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
>>>
>>> static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
>>> {
>>> + u64 current_timestamp;
>>> + u64 diff_timestamp;
>>> + u32 elapsed_frames;
>>> +
>>> if (list_empty(&dep->pending_list)) {
>>> dep->flags |= DWC3_EP_PENDING_REQUEST;
>>> return -EAGAIN;
>>> }
>>>
>>> + current_timestamp = ktime_get_ns();
>>> + diff_timestamp = current_timestamp - dep->frame_timestamp;
>>> + elapsed_frames = DIV_ROUND_UP_ULL(diff_timestamp, 125000);
>>> +
>>> + dep->frame_number += elapsed_frames;
>>> dep->frame_number = DWC3_ALIGN_FRAME(dep);
>>> +
>>> return __dwc3_gadget_kick_transfer(dep);
>>> }
>>>
>>> @@ -2320,6 +2330,7 @@ static void dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep,
>>> const struct dwc3_event_depevt *event)
>>> {
>>> dep->frame_number = event->parameters;
>>> + dep->frame_timestamp = ktime_get_ns();
>>> }
>>>
>>> static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
>> This may not be enough. The dep->frame_timestamp may not correspond to
>> the frame_number from XferNotReady event. When there's system latency
>> (which is possible when this failure happens), the time the driver
>> handle the event may be a few uframes passed the time the controller's
>> XferInProgress uframe parameter.
>>
>> Rather than starting the isoc transfer immediately on the next interval.
>> How about starting the transfer with some minimum buffer uframes just
>> like before? (e.g. frame_number + max(4, interval))
> The problem with this is cases with interval of 1ms. This will result in
> a 4ms delay. I really want to start transfer as soon as possible and the
> timestamp trick seems to be the best idea so far, without resorting to
> 4 intervals delay. We do, however, have the possibility that this will
> start 2 intervals in the future because of the usage of
> DIV_ROUND_UP_ULL() and because of how DWC3_ALIGN_FRAME() is implemented.
>
> I understand what you're saying, though, but it seems like we don't
> have to avoid that case completely. We can only make it less likely.
>
In the case of interval of 1ms, it will start on the next interval.
frame_number + max(4, interval) will start at least 4 uframes in the future.
In any case, what about immediately retry the START_TRANSFER command
with a new frame_number + (interval*retry) should it fail with
bus-expiry? You can set the number of retries to maybe 5 times. This
should remove the need to do time stamping.
Thinh
