Skip to contentSkip to conversation

usb: dwc3: gadget: check if dep->frame_number is still valid

PATCH New Felipe Balbi 1 participant 1 message Nov 9, 2018 USB

lore
Share & tools
lore
#0 lore raw

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

Hi,

Quoted 81 lines · Thinh Nguyen
Quoted 66 lines · Felipe Balbi

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.

---
 drivers/usb/dwc3/core.h   |  5 +++++
 drivers/usb/dwc3/gadget.c | 11 +++++++++++
 2 files changed, 16 insertions(+)
drivers/usb/dwc3/core.h +5 -0
index 131028501752..306a2dd75ed5 100644
@@ -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];
drivers/usb/dwc3/gadget.c +10 -0
index d8c7ad0c22e8..00fe01a01977 100644
@@ -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);
 }
 

drivers/usb/dwc3/gadget.c static void dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep, · line 2336

Quoted 7 lines
drivers/usb/dwc3/gadget.c +1 -0
@@ -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.

Quoted 3 lines

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.