[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:
>>>> 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.

That seems like a good idea. Something like below? (on top of $subject)

modified   drivers/usb/dwc3/core.h
@@ -37,6 +37,7 @@
 #define DWC3_EP0_SETUP_SIZE	512
 #define DWC3_ENDPOINTS_NUM	32
 #define DWC3_XHCI_RESOURCES_NUM	2
+#define DWC3_ISOC_MAX_RETRIES	5
 
 #define DWC3_SCRATCHBUF_SIZE	4096	/* each buffer is assumed to be 4KiB */
 #define DWC3_EVENT_BUFFERS_SIZE	4096
modified   drivers/usb/dwc3/gadget.c
@@ -1271,20 +1271,29 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
 	u64 current_timestamp;
 	u64 diff_timestamp;
 	u32 elapsed_frames;
+	int retries;
+	int delta = 1
+	int ret;
 
 	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);
+	for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
+		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);
+		dep->frame_number += elapsed_frames + (delta * i);
+		dep->frame_number = DWC3_ALIGN_FRAME(dep);
 
-	return __dwc3_gadget_kick_transfer(dep);
+		ret = __dwc3_gadget_kick_transfer(dep);
+		if (ret != -EAGAIN)
+			break;
+	}
+
+	return ret;
 }
 
 static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)