usb: dwc3: gadget: check for Missed Isoc from event status
Subsystems (2)
PATCH 18/38 usb: dwc3: gadget: check for Missed Isoc from event status
Hi,
Quoted 41 lines · Felipe Balbi
Quoted 7 lines
Quoted 2 lines
Without XferNotReady, we won't have a reliable way to know the uFrame number. Read the Isochronous programming sequence from your databook.
Right. We need XferNotReady to know when to start isoc transfer. But if there are still queued requests, DWC3 can just wait to see if any of them will succeed to continue with the transfer just as how DWC3 is handling it now.
That's not what the databook says though. And that's also not intention of how the code is written as of now either. The way the code is written is the following:
queue()-> XferNotReady ->start_isoc()-> if (missed)do_nothing()->queue()->end_transfer.That's not really waiting for the queue to be consumed, it's just delaying end transfer until we get another
queue(). IOW, it just *happens* to give the controller time to go through the list of started requests.Quoted 4 lines
If we end and restart the transfer right away, then we may lose more isoc data than necessary (due to isoc scheduling at least 4 uFrame ahead of time). Is there something you see that doesn't work with the current implementation?
Not _really_, I'm just trying to make the code easier to read and, I think, I've achieved that. Now, if we need to delay end transfer in the case where we have more requests in the controller's queue, that's easy enough to implement:
(quoted diff) +2
-1@@ -2371,7 +2371,8 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep, if (event->status & DEPEVT_STATUS_BUSERR) status = -ECONNRESET; - if (event->status & DEPEVT_STATUS_MISSED_ISOC) { + if (event->status & DEPEVT_STATUS_MISSED_ISOC && + list_empty(&dep->started_list) { status = -EXDEV; stop = true; }
Quoted 5 lines
I'm not sure this is a good idea though. Once we miss an interval, don't we need to know the next frame when transfer needs to be scheduled?
Meaning we would need XferNotReady to properly schedule the new transfer?
thinking about this a little more. This extra list_empty() check is not wrong at all :-) I've amended this series with the 3 patches below. I'll resend the series once I've given more time for people to test. Patches have been updated to the branch on kernel.org as well, btw.
8<------------------------------------------------------------------------
From 811476d1799c606b3af2b40022fe333cef586387 Mon Sep 17 00:00:00 2001
From: Felipe Balbi <[email protected]>
Date: Wed, 11 Apr 2018 10:31:53 +0300
Subject: [PATCH 1/3] usb: dwc3: debug: decode uFrame from event too
XferNotReady and XferInProgress give us the uFrame number we're currently in. Printing that out on tracepoints may help us find bugs in transfer scheduling.
--- drivers/usb/dwc3/debug.h | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
drivers/usb/dwc3/debug.h +7 -3
@@ -493,14 +493,18 @@ dwc3_ep_event_string(char *str, const struct dwc3_event_depevt *event,case DWC3_DEPEVT_XFERINPROGRESS : len =strlen (str); -sprintf (str + len,"Transfer In Progress (%c%c%c)" , +sprintf (str + len,"Transfer In Progress [%d] (%c%c%c)" , + event->parameters, status &DEPEVT_STATUS_SHORT ?'S' :'s' , status &DEPEVT_STATUS_IOC ?'I' :'i' , status &DEPEVT_STATUS_LST ?'M' :'m' );break ;case DWC3_DEPEVT_XFERNOTREADY : -strcat (str,"Transfer Not Ready" ); -strcat (str, status &DEPEVT_STATUS_TRANSFER_ACTIVE ? + len =strlen (str); + +sprintf (str + len,"Transfer Not Ready [%d]%s" , + event->parameters, + status &DEPEVT_STATUS_TRANSFER_ACTIVE ?" (Active)" :" (Not Active)" );/* Control Endpoints */
Signature · 22 lines
2.16.1 8<------------------------------------------------------------------------ From 77f9d84d785c2d088e82c90b7d3ad844ce59d668 Mon Sep 17 00:00:00 2001 From: Felipe Balbi <[email protected]> Date: Wed, 11 Apr 2018 10:32:52 +0300 Subject: [PATCH 2/3] usb: dwc3: gadget: don't issue End Transfer if we have started reqs In case we have many started requests and one of them in the middle is completed with Missed Isoc, let's not End Transfer as that would result in us loosing (possibly) many more intervals. Instead, let's allow the controller to go through its list of started requests. Signed-off-by: Felipe Balbi <[email protected]> --- drivers/usb/dwc3/gadget.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/usb/dwc3/gadget.c +2 -1
@@ -2371,7 +2371,8 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,if (event->status &DEPEVT_STATUS_BUSERR ) status = -ECONNRESET ; -if (event->status &DEPEVT_STATUS_MISSED_ISOC ) { +if (event->status &DEPEVT_STATUS_MISSED_ISOC && +list_empty (&dep->started_list)) { status = -EXDEV ; stop = true; }
Signature · 17 lines
2.16.1 8<------------------------------------------------------------------------ From 09ded2962aaf0ebbb40e3e7147d5ad4486c04601 Mon Sep 17 00:00:00 2001 From: Felipe Balbi <[email protected]> Date: Wed, 11 Apr 2018 10:34:34 +0300 Subject: [PATCH 3/3] usb: dwc3: gadget: always start isochronous aligned to dep->interval We will *always* start transfer to the next uFrame number aligned to dep->interval. Signed-off-by: Felipe Balbi <[email protected]> --- drivers/usb/dwc3/gadget.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)
drivers/usb/dwc3/gadget.c +4 -10
@@ -1233,6 +1233,8 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)return DWC3_DSTS_SOFFN (reg); } +#define DWC3_ALIGN_FRAME (d) (((d)->frame_number + (d)->interval) & ~((d)->interval - 1)) +static void __dwc3_gadget_start_isoc (struct dwc3_ep *dep) {if (list_empty (&dep->pending_list)) {
@@ -1242,11 +1244,7 @@ static void __dwc3_gadget_start_isoc(struct dwc3_ep *dep)return ; } -/* -* Schedule the first trb for one interval in the future or at -* least 4 microframes. -*/ - dep->frame_number +=max_t (u32,4 , dep->interval); + dep->frame_number =DWC3_ALIGN_FRAME (dep);__dwc3_gadget_kick_transfer (dep); }
@@ -2352,11 +2350,7 @@ static void dwc3_gadget_ep_cleanup_completed_requests(struct dwc3_ep *dep,static void dwc3_gadget_endpoint_frame_from_event (struct dwc3_ep *dep,const struct dwc3_event_depevt *event) { -u32 cur_uf, mask; - - mask = ~(dep->interval -1 ); - cur_uf = event->parameters & mask; - dep->frame_number = cur_uf; + dep->frame_number = event->parameters; } static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,