Re: [RFT/PATCH 18/38] usb: dwc3: gadget: check for Missed Isoc from event status
Hi,
Thinh Nguyen writes:
> Hi,
>
> On 5/30/2018 11:49 PM, Felipe Balbi wrote:
>> Paul Zimmerman writes:
>>
>>> Hi Felipe,
>>>
>>> Felipe Balbi writes:
>>>
>>> < snip >
>>>
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.
>>> < snip >
>>>
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);
}
>>> Pretty sure this could cause problems. Instead of starting at least 4
>>> uframes
>>> in the future, this will now try to start at the next aligned uframe. If
>>> dep->interval is very small (say 1) and we are almost at the end of the
>>> current uframe, there might not be enough time?
>> perhaps, but I haven't seen that happen yet. Guess I'll get to this when
>> I see such a problem.
>>
> I did some quick tests against DWC_usb3 controller v3.10a in high-speed
> and super-speed. We'll start to run into issue when if DWC3 needs to
> prepare 64+ TRBs after XferNotReady for isoc transfers with service
> interval of 1 uframe (start_transfer command will fail with bus-expiry).
> It may be better to calculate for the starting future uframe in a proper
> way.
it is proper, actually. We don't have a single gadget driver preparing
that many isoc transfers in one go and, in any case, if this ever
happens, we will only miss a single interval.
After the first isoc transfer is started, we will be using update
transfer command to add more intervals, in that case we don't need to
deal with starting uFrame number anyway.
I'll just go with it and if we ever experience this problem, then we can
try to find a way to estimate how many uFrames in the future we need to
start the transfer. It's a pointless exercise until we have a real
falilure with a real gadget driver. Sorry
--
balbi
signature.asc
Description: PGP signature
Re: [RFT/PATCH 18/38] usb: dwc3: gadget: check for Missed Isoc from event status
Hi,
On 5/30/2018 11:49 PM, Felipe Balbi wrote:
> Paul Zimmerman writes:
>
>> Hi Felipe,
>>
>> Felipe Balbi writes:
>>
>> < snip >
>>
>>> 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.
>> < snip >
>>
>>> 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);
>>> }
>> Pretty sure this could cause problems. Instead of starting at least 4 uframes
>> in the future, this will now try to start at the next aligned uframe. If
>> dep->interval is very small (say 1) and we are almost at the end of the
>> current uframe, there might not be enough time?
> perhaps, but I haven't seen that happen yet. Guess I'll get to this when
> I see such a problem.
>
I did some quick tests against DWC_usb3 controller v3.10a in high-speed
and super-speed. We'll start to run into issue when if DWC3 needs to
prepare 64+ TRBs after XferNotReady for isoc transfers with service
interval of 1 uframe (start_transfer command will fail with bus-expiry).
It may be better to calculate for the starting future uframe in a proper
way.
BR,
Thinh
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFT/PATCH 18/38] usb: dwc3: gadget: check for Missed Isoc from event status
Hi,
Paul Zimmerman writes:
> Hi Felipe,
>
> Felipe Balbi writes:
>
> < snip >
>
>> 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.
>
> < snip >
>
>> 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);
>> }
>
> Pretty sure this could cause problems. Instead of starting at least 4 uframes
> in the future, this will now try to start at the next aligned uframe. If
> dep->interval is very small (say 1) and we are almost at the end of the
> current uframe, there might not be enough time?
perhaps, but I haven't seen that happen yet. Guess I'll get to this when
I see such a problem.
--
balbi
signature.asc
Description: PGP signature
Re: [RFT/PATCH 18/38] usb: dwc3: gadget: check for Missed Isoc from event status
Hi Felipe,
Felipe Balbi writes:
< snip >
> 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.
< snip >
> 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);
> }
Pretty sure this could cause problems. Instead of starting at least 4 uframes
in the future, this will now try to start at the next aligned uframe. If
dep->interval is very small (say 1) and we are almost at the end of the
current uframe, there might not be enough time?
--
Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFT/PATCH 18/38] usb: dwc3: gadget: check for Missed Isoc from event status
Hi, Thinh Nguyen writes: >>> Maybe we should return the -EXDEV status every time there's a missed isoc. >> >> you mean like this? > > Yes, this will work. updated to branch -- balbi signature.asc Description: PGP signature
Re: [RFT/PATCH 18/38] usb: dwc3: gadget: check for Missed Isoc from event status
Hi,
On 4/12/2018 12:18 AM, Felipe Balbi wrote:
>
> Hi,
>
> Thinh Nguyen writes:
>> Hi,
>>
>> On 4/11/2018 1:21 AM, Felipe Balbi wrote:
>>>
>>> Hi,
>>>
>>> Felipe Balbi 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;
>>
>> Maybe we should return the -EXDEV status every time there's a missed isoc.
>
> you mean like this?
Yes, this will work.
>
> @@ -2358,10 +2358,11 @@ 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 &&
> - list_empty(&dep->started_list)) {
> + if (event->status & DEPEVT_STATUS_MISSED_ISOC) {
> status = -EXDEV;
> - stop = true;
> +
> + if (list_empty(&dep->started_list))
> + stop = true;
> }
>
> dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);
>
BR,
Thinh
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFT/PATCH 18/38] usb: dwc3: gadget: check for Missed Isoc from event status
Hi,
Thinh Nguyen writes:
> Hi,
>
> On 4/11/2018 1:21 AM, Felipe Balbi wrote:
>>
>> Hi,
>>
>> Felipe Balbi 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;
>
> Maybe we should return the -EXDEV status every time there's a missed isoc.
you mean like this?
@@ -2358,10 +2358,11 @@ 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 &&
- list_empty(&dep->started_list)) {
+ if (event->status & DEPEVT_STATUS_MISSED_ISOC) {
status = -EXDEV;
- stop = true;
+
+ if (list_empty(&dep->started_list))
+ stop = true;
}
dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);
>>> 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.
>
> Great! :)
> Thanks for all the new updates. I'll test it out when I have a chance.
sure, thanks a lot.
--
balbi
signature.asc
Description: PGP signature
Re: [RFT/PATCH 18/38] usb: dwc3: gadget: check for Missed Isoc from event status
Hi,
On 4/11/2018 1:21 AM, Felipe Balbi wrote:
>
> Hi,
>
> Felipe Balbi 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;
Maybe we should return the -EXDEV status every time there's a missed isoc.
>> 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.
Great! :)
Thanks for all the new updates. I'll test it out when I have a chance.
Thinh
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFT/PATCH 18/38] usb: dwc3: gadget: check for Missed Isoc from event status
Hi,
Felipe Balbi 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
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
---
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
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
---
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/g
Re: [RFT/PATCH 18/38] usb: dwc3: gadget: check for Missed Isoc from event status
Hi,
Thinh Nguyen writes:
>>> On 4/9/2018 4:28 AM, Felipe Balbi wrote:
In case we get an event with status set to Missed Isoc, this means we
have missed an isochronous interval and should issue End Transfer
command and wait for the following XferNotReady.
>>>
>>> Why does DWC3 need to issue End Transfer if there are still queued requests?
>>
>> 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?
--
balbi
signature.asc
Description: PGP signature
Re: [RFT/PATCH 18/38] usb: dwc3: gadget: check for Missed Isoc from event status
Hi,
On 4/10/2018 12:36 AM, Felipe Balbi wrote:
>
> Hi,
>
> Thinh Nguyen writes:
>> Hi Felipe,
>>
>> On 4/9/2018 4:28 AM, Felipe Balbi wrote:
>>> In case we get an event with status set to Missed Isoc, this means we
>>> have missed an isochronous interval and should issue End Transfer
>>> command and wait for the following XferNotReady.
>>
>> Why does DWC3 need to issue End Transfer if there are still queued requests?
>
> 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. 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?
>
>>> @@ -2383,14 +2380,25 @@ static void
>>> dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
>>>{
>>> struct dwc3 *dwc = dep->dwc;
>>> unsignedstatus = 0;
>>> + boolstop = false;
>>>
>>> dwc3_gadget_endpoint_frame_from_event(dep, event);
>>>
>>> if (event->status & DEPEVT_STATUS_BUSERR)
>>> status = -ECONNRESET;
>>>
>>> + if (event->status & DEPEVT_STATUS_MISSED_ISOC) {
>>> + status = -ECONNRESET;
>>
>> Missed isoc shouldn't cause this error status or if it should return an
>> error status at all. Maybe the status can be -EXDEV, similar to the host
>> side (/Documentation/driver-api/usb/error-codes.rst).
>
> fair enough. I'll change to EXDEV
>
BR,
Thinh
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFT/PATCH 18/38] usb: dwc3: gadget: check for Missed Isoc from event status
Hi,
Thinh Nguyen writes:
> Hi Felipe,
>
> On 4/9/2018 4:28 AM, Felipe Balbi wrote:
>> In case we get an event with status set to Missed Isoc, this means we
>> have missed an isochronous interval and should issue End Transfer
>> command and wait for the following XferNotReady.
>
> Why does DWC3 need to issue End Transfer if there are still queued requests?
Without XferNotReady, we won't have a reliable way to know the uFrame
number. Read the Isochronous programming sequence from your databook.
>> @@ -2383,14 +2380,25 @@ static void
>> dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
>> {
>> struct dwc3 *dwc = dep->dwc;
>> unsignedstatus = 0;
>> +boolstop = false;
>>
>> dwc3_gadget_endpoint_frame_from_event(dep, event);
>>
>> if (event->status & DEPEVT_STATUS_BUSERR)
>> status = -ECONNRESET;
>>
>> +if (event->status & DEPEVT_STATUS_MISSED_ISOC) {
>> +status = -ECONNRESET;
>
> Missed isoc shouldn't cause this error status or if it should return an
> error status at all. Maybe the status can be -EXDEV, similar to the host
> side (/Documentation/driver-api/usb/error-codes.rst).
fair enough. I'll change to EXDEV
--
balbi
signature.asc
Description: PGP signature
Re: [RFT/PATCH 18/38] usb: dwc3: gadget: check for Missed Isoc from event status
Hi Felipe,
On 4/9/2018 4:28 AM, Felipe Balbi wrote:
> In case we get an event with status set to Missed Isoc, this means we
> have missed an isochronous interval and should issue End Transfer
> command and wait for the following XferNotReady.
Why does DWC3 need to issue End Transfer if there are still queued requests?
>
> Let's do that early, rather than late.
>
> Signed-off-by: Felipe Balbi
> ---
> drivers/usb/dwc3/core.h | 5 +++--
> drivers/usb/dwc3/gadget.c | 14 +++---
> 2 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 5ee895113906..8862118c3b79 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1192,11 +1192,12 @@ struct dwc3_event_depevt {
> /* Within XferNotReady */
> #define DEPEVT_STATUS_TRANSFER_ACTIVE BIT(3)
>
> -/* Within XferComplete */
> +/* Within XferComplete or XferInProgress */
> #define DEPEVT_STATUS_BUSERRBIT(0)
> #define DEPEVT_STATUS_SHORT BIT(1)
> #define DEPEVT_STATUS_IOC BIT(2)
> -#define DEPEVT_STATUS_LSTBIT(3)
> +#define DEPEVT_STATUS_LSTBIT(3) /* XferComplete */
> +#define DEPEVT_STATUS_MISSED_ISOC BIT(3) /* XferInProgress */
>
> /* Stream event only */
> #define DEPEVT_STREAMEVT_FOUND 1
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 0b003367cc7c..ed44b85e59dc 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2361,9 +2361,6 @@ static void
> dwc3_gadget_ep_cleanup_completed_requests(struct dwc3_ep *dep,
>* entry is added into request list.
>*/
> dep->flags = DWC3_EP_PENDING_REQUEST;
> - } else {
> - dwc3_stop_active_transfer(dep, true);
> - dep->flags = DWC3_EP_ENABLED;
> }
> }
> }
> @@ -2383,14 +2380,25 @@ static void
> dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
> {
> struct dwc3 *dwc = dep->dwc;
> unsignedstatus = 0;
> + boolstop = false;
>
> dwc3_gadget_endpoint_frame_from_event(dep, event);
>
> if (event->status & DEPEVT_STATUS_BUSERR)
> status = -ECONNRESET;
>
> + if (event->status & DEPEVT_STATUS_MISSED_ISOC) {
> + status = -ECONNRESET;
Missed isoc shouldn't cause this error status or if it should return an
error status at all. Maybe the status can be -EXDEV, similar to the host
side (/Documentation/driver-api/usb/error-codes.rst).
> + stop = true;
> + }
> +
> dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);
>
> + if (stop) {
> + dwc3_stop_active_transfer(dep, true);
> + dep->flags = DWC3_EP_ENABLED;
> + }
> +
> /*
>* WORKAROUND: This is the 2nd half of U1/U2 -> U0 workaround.
>* See dwc3_gadget_linksts_change_interrupt() for 1st half.
>
BR,
Thinh
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFT/PATCH 18/38] usb: dwc3: gadget: check for Missed Isoc from event status
In case we get an event with status set to Missed Isoc, this means we
have missed an isochronous interval and should issue End Transfer
command and wait for the following XferNotReady.
Let's do that early, rather than late.
Signed-off-by: Felipe Balbi
---
drivers/usb/dwc3/core.h | 5 +++--
drivers/usb/dwc3/gadget.c | 14 +++---
2 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 5ee895113906..8862118c3b79 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1192,11 +1192,12 @@ struct dwc3_event_depevt {
/* Within XferNotReady */
#define DEPEVT_STATUS_TRANSFER_ACTIVE BIT(3)
-/* Within XferComplete */
+/* Within XferComplete or XferInProgress */
#define DEPEVT_STATUS_BUSERR BIT(0)
#define DEPEVT_STATUS_SHORTBIT(1)
#define DEPEVT_STATUS_IOC BIT(2)
-#define DEPEVT_STATUS_LST BIT(3)
+#define DEPEVT_STATUS_LST BIT(3) /* XferComplete */
+#define DEPEVT_STATUS_MISSED_ISOC BIT(3) /* XferInProgress */
/* Stream event only */
#define DEPEVT_STREAMEVT_FOUND 1
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 0b003367cc7c..ed44b85e59dc 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2361,9 +2361,6 @@ static void
dwc3_gadget_ep_cleanup_completed_requests(struct dwc3_ep *dep,
* entry is added into request list.
*/
dep->flags = DWC3_EP_PENDING_REQUEST;
- } else {
- dwc3_stop_active_transfer(dep, true);
- dep->flags = DWC3_EP_ENABLED;
}
}
}
@@ -2383,14 +2380,25 @@ static void
dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
{
struct dwc3 *dwc = dep->dwc;
unsignedstatus = 0;
+ boolstop = false;
dwc3_gadget_endpoint_frame_from_event(dep, event);
if (event->status & DEPEVT_STATUS_BUSERR)
status = -ECONNRESET;
+ if (event->status & DEPEVT_STATUS_MISSED_ISOC) {
+ status = -ECONNRESET;
+ stop = true;
+ }
+
dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);
+ if (stop) {
+ dwc3_stop_active_transfer(dep, true);
+ dep->flags = DWC3_EP_ENABLED;
+ }
+
/*
* WORKAROUND: This is the 2nd half of U1/U2 -> U0 workaround.
* See dwc3_gadget_linksts_change_interrupt() for 1st half.
--
2.16.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
