Hi,
Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx> writes:
>>> 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.
>
>> 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:
>
> @@ -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;
> }
>
> 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 <felipe.balbi@xxxxxxxxxxxxxxx>
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.
Signed-off-by: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
---
drivers/usb/dwc3/debug.h | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/dwc3/debug.h b/drivers/usb/dwc3/debug.h
index 0be6a554be57..c66d216dcc30 100644
--- a/drivers/usb/dwc3/debug.h
+++ b/drivers/usb/dwc3/debug.h
@@ -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 */
--
2.16.1
8<------------------------------------------------------------------------
From 77f9d84d785c2d088e82c90b7d3ad844ce59d668 Mon Sep 17 00:00:00 2001
From: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
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 <felipe.balbi@xxxxxxxxxxxxxxx>
---
drivers/usb/dwc3/gadget.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index e0618afa14cb..9d4dc8bed644 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -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;
}
--
2.16.1
8<------------------------------------------------------------------------
From 09ded2962aaf0ebbb40e3e7147d5ad4486c04601 Mon Sep 17 00:00:00 2001
From: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
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 <felipe.balbi@xxxxxxxxxxxxxxx>
---
drivers/usb/dwc3/gadget.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 9d4dc8bed644..9cf89f3cf203 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -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,
--
2.16.1
--
balbi
Attachment:
signature.asc
Description: PGP signature
