Hi,
Thinh Nguyen <thinh.ngu...@synopsys.com> 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.ba...@linux.intel.com>
>> ---
>> 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. -- balbi
signature.asc
Description: PGP signature
