[4/4] usb: dwc3: gadget: check if dep->frame_number is still valid

[Felipe Balbi <felipe.balbi@linux.intel.com>]

Hi,

Thinh Nguyen <thinh.nguyen@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.balbi@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.