Sorting out dwc3 ISOC endpoints once and for all

-----Original Message-----
From: Felipe Balbi <[email protected]>
Sent: Friday, June 7, 2019 2:50 AM
To: John Youn <[email protected]>
Cc: [email protected]
Subject: [RFC] Sorting out dwc3 ISOC endpoints once and for all

Now that we have dwc3_gadget_start_isoc_quirk() which figures out the correct combination for the top-most 2 bits in the frame number, why don't we just use that to start isochronous transfers and never, again, have Bus Expiry problems?

I mean something along the lines of below diff (completely untested):

modified drivers/usb/dwc3/gadget.c

(quoted diff) +1 -2

@@ -1369,9 +1369,8 @@ static int dwc3_gadget_start_isoc_quirk(struct
 dwc3_ep *dep)
 	else if (test0 && test1)
 		dep->combo_num = 0;
 
-	dep->frame_number &= 0x3fff;
 	dep->frame_number |= dep->combo_num << 14;
-	dep->frame_number += max_t(u32, 4, dep->interval);
+	dep->frame_number = DWC3_ALIGN_FRAME(dep, 1);
 
 	/* Reinitialize test variables */

(quoted diff) +3 -20

 	dep->start_cmd_status = 0;

@@ -1383,33 +1382,16 @@ static int dwc3_gadget_start_isoc_quirk(struct
 dwc3_ep *dep)
 static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
 {
 	struct dwc3 *dwc = dep->dwc;
-	int ret;
-	int i;
 
 	if (list_empty(&dep->pending_list)) {
 		dep->flags |= DWC3_EP_PENDING_REQUEST;
 		return -EAGAIN;
 	}
 
-	if (!dwc->dis_start_transfer_quirk && dwc3_is_usb31(dwc) &&
-	    (dwc->revision <= DWC3_USB31_REVISION_160A ||
-	     (dwc->revision == DWC3_USB31_REVISION_170A &&
-	      dwc->version_type >= DWC31_VERSIONTYPE_EA01 &&
-	      dwc->version_type <= DWC31_VERSIONTYPE_EA06))) {
-
-		if (dwc->gadget.speed <= USB_SPEED_HIGH && dep->direction)
-			return dwc3_gadget_start_isoc_quirk(dep);
-	}
-
-	for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
-		dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1);
+	dep->frame_number = __dwc3_gadget_get_frame(dwc);
+	dep->frame_number = DWC3_ALIGN_FRAME(dep, 1);
 
-		ret = __dwc3_gadget_kick_transfer(dep);
-		if (ret != -EAGAIN)
-			break;
-	}
-
-	return ret;
+	return dwc3_gadget_start_isoc_quirk(dep);
 }
 

static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct
dwc3_request *req)

Would there be any obvious draw-back to going down this route? The thing is that, as it is, it seems like we will *always* have some corner case where we can't guarantee that we can even start a transfer since there's no upper-bound between XferNotReady and gadget driver finally queueing a request. Also, I can't simply read DSTS for the frame number because of top-most 2 bits.

Quoted 7 lines

-----Original Message-----
From: Felipe Balbi <[email protected]>
Sent: Friday, June 7, 2019 2:50 AM
To: John Youn <[email protected]>
Cc: [email protected]
Subject: [RFC] Sorting out dwc3 ISOC endpoints once and for all

++ Thinh

Hi Felipe,

Sorry, missed this e-mail.

Quoted 4 lines

Now that we have dwc3_gadget_start_isoc_quirk() which figures out the correct combination for the top-most 2 bits in the frame number, why don't we just use that to start isochronous transfers and never, again, have Bus Expiry problems?

We should only see expiry problems on the affected versions with incorrect top-2 bits right?

Quoted 56 lines

I mean something along the lines of below diff (completely untested):

modified drivers/usb/dwc3/gadget.c

(quoted diff) +1 -2

@@ -1369,9 +1369,8 @@ static int dwc3_gadget_start_isoc_quirk(struct
 dwc3_ep *dep)
 	else if (test0 && test1)
 		dep->combo_num = 0;
 
-	dep->frame_number &= 0x3fff;
 	dep->frame_number |= dep->combo_num << 14;
-	dep->frame_number += max_t(u32, 4, dep->interval);
+	dep->frame_number = DWC3_ALIGN_FRAME(dep, 1);
 
 	/* Reinitialize test variables */

(quoted diff) +3 -20

 	dep->start_cmd_status = 0;

@@ -1383,33 +1382,16 @@ static int dwc3_gadget_start_isoc_quirk(struct
 dwc3_ep *dep)
 static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
 {
 	struct dwc3 *dwc = dep->dwc;
-	int ret;
-	int i;
 
 	if (list_empty(&dep->pending_list)) {
 		dep->flags |= DWC3_EP_PENDING_REQUEST;
 		return -EAGAIN;
 	}
 
-	if (!dwc->dis_start_transfer_quirk && dwc3_is_usb31(dwc) &&
-	    (dwc->revision <= DWC3_USB31_REVISION_160A ||
-	     (dwc->revision == DWC3_USB31_REVISION_170A &&
-	      dwc->version_type >= DWC31_VERSIONTYPE_EA01 &&
-	      dwc->version_type <= DWC31_VERSIONTYPE_EA06))) {
-
-		if (dwc->gadget.speed <= USB_SPEED_HIGH && dep->direction)
-			return dwc3_gadget_start_isoc_quirk(dep);
-	}
-
-	for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
-		dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1);
+	dep->frame_number = __dwc3_gadget_get_frame(dwc);
+	dep->frame_number = DWC3_ALIGN_FRAME(dep, 1);
 
-		ret = __dwc3_gadget_kick_transfer(dep);
-		if (ret != -EAGAIN)
-			break;
-	}
-
-	return ret;
+	return dwc3_gadget_start_isoc_quirk(dep);
 }
 

static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct
dwc3_request *req)

Quoted 7 lines

Would there be any obvious draw-back to going down this route? The thing is that, as it is, it seems like we will *always* have some corner case where we can't guarantee that we can even start a transfer since there's no upper-bound between XferNotReady and gadget driver finally queueing a request. Also, I can't simply read DSTS for the frame number because of top-most 2 bits.

For non-affected version of the IP, the xfernotready -> starttransfer time will have to be off by more than a couple seconds for the driver to produce an incorrect 16-bit frame number. If you're seeing errors here, maybe we just need to code review the relevant sections to make sure the 14/16-bit and rollover conditions are all handled correctly.

But I can't think of any obvious drawbacks of the quirk, other than doing some unnecessary work, which shouldn't produce any bad side-effects. But we haven't really tested that.

-----Original Message-----
From: Thinh Nguyen
Sent: Monday, June 17, 2019 12:09 PM
To: John Youn <[email protected]>; Felipe Balbi <[email protected]>; John Youn <[email protected]>
Cc: [email protected]
Subject: Re: [RFC] Sorting out dwc3 ISOC endpoints once and for all

Hi,

Quoted 11 lines · John Youn

Quoted 7 lines

-----Original Message-----
From: Felipe Balbi <[email protected]>
Sent: Friday, June 7, 2019 2:50 AM
To: John Youn <[email protected]>
Cc: [email protected]
Subject: [RFC] Sorting out dwc3 ISOC endpoints once and for all

++ Thinh

Hi Felipe,

Quoted 7 lines

Sorry, missed this e-mail.

Quoted 4 lines

Now that we have dwc3_gadget_start_isoc_quirk() which figures out the correct combination for the top-most 2 bits in the frame number, why don't we just use that to start isochronous transfers and never, again, have Bus Expiry problems?

We should only see expiry problems on the affected versions with incorrect

top-2 bits right?

Quoted 57 lines

Quoted 57 lines

I mean something along the lines of below diff (completely untested):

modified drivers/usb/dwc3/gadget.c

(quoted diff) +1 -2

@@ -1369,9 +1369,8 @@ static int dwc3_gadget_start_isoc_quirk(struct
 dwc3_ep *dep)
 	else if (test0 && test1)
 		dep->combo_num = 0;
 
-	dep->frame_number &= 0x3fff;
 	dep->frame_number |= dep->combo_num << 14;
-	dep->frame_number += max_t(u32, 4, dep->interval);
+	dep->frame_number = DWC3_ALIGN_FRAME(dep, 1);
 
 	/* Reinitialize test variables */

(quoted diff) +3 -20

 	dep->start_cmd_status = 0;

@@ -1383,33 +1382,16 @@ static int dwc3_gadget_start_isoc_quirk(struct
 dwc3_ep *dep)
 static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
 {
 	struct dwc3 *dwc = dep->dwc;
-	int ret;
-	int i;
 
 	if (list_empty(&dep->pending_list)) {
 		dep->flags |= DWC3_EP_PENDING_REQUEST;
 		return -EAGAIN;
 	}
 
-	if (!dwc->dis_start_transfer_quirk && dwc3_is_usb31(dwc) &&
-	    (dwc->revision <= DWC3_USB31_REVISION_160A ||
-	     (dwc->revision == DWC3_USB31_REVISION_170A &&
-	      dwc->version_type >= DWC31_VERSIONTYPE_EA01 &&
-	      dwc->version_type <= DWC31_VERSIONTYPE_EA06))) {
-
-		if (dwc->gadget.speed <= USB_SPEED_HIGH && dep->direction)
-			return dwc3_gadget_start_isoc_quirk(dep);
-	}
-
-	for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
-		dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1);
+	dep->frame_number = __dwc3_gadget_get_frame(dwc);
+	dep->frame_number = DWC3_ALIGN_FRAME(dep, 1);
 
-		ret = __dwc3_gadget_kick_transfer(dep);
-		if (ret != -EAGAIN)
-			break;
-	}
-
-	return ret;
+	return dwc3_gadget_start_isoc_quirk(dep);
 }
 

static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct
dwc3_request *req)

Quoted 8 lines

Quoted 7 lines

Would there be any obvious draw-back to going down this route? The thing is that, as it is, it seems like we will *always* have some corner case where we can't guarantee that we can even start a transfer since there's no upper-bound between XferNotReady and gadget driver finally queueing a request. Also, I can't simply read DSTS for the frame number because of top-most 2 bits.

For non-affected version of the IP, the xfernotready -> starttransfer time will

have to be off by more than a couple seconds for the driver to produce an incorrect 16-bit frame number. If you're seeing errors here, maybe we just need to code review the relevant sections to make sure the 14/16-bit and rollover conditions are all handled correctly.

I think what Felipe may see is some delay in the system that causes the SW to not handle XferNotReady event in time. We already have the "retry" method handle that to a certain extend.

Quoted 1 line

But I can't think of any obvious drawbacks of the quirk, other than doing

some unnecessary work, which shouldn't produce any bad side-effects. But we haven't really tested that.

Quoted 1 line

The workaround for the isoc_quirk requires 2 tries sending START_TRANSFER command. This means that you have to account the delay of that command completion plus potentially 1 more END_TRANSFER completion. That's why the quirk gives a buffer of at least 4 uframes of the scheduled isoc frame. So, it cannot schedule immediately on the next uframe, that's one of the drawbacks.

Quoted 12 lines

Quoted 5 lines

static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct
dwc3_request *req)

Quoted 7 lines

Would there be any obvious draw-back to going down this route? The thing is that, as it is, it seems like we will *always* have some corner case where we can't guarantee that we can even start a transfer since there's no upper-bound between XferNotReady and gadget driver finally queueing a request. Also, I can't simply read DSTS for the frame number because of top-most 2 bits.

Quoted 5 lines

For non-affected version of the IP, the xfernotready -> starttransfer time will have to be off by more than a couple seconds for the driver to produce an incorrect 16-bit frame number. If you're seeing errors here, maybe we just need to code review the relevant sections to make sure the 14/16-bit and rollover conditions are all handled correctly.

I think what Felipe may see is some delay in the system that causes the SW to not handle XferNotReady event in time. We already have the "retry" method handle that to a certain extend.

Quoted 4 lines

But I can't think of any obvious drawbacks of the quirk, other than doing some unnecessary work, which shouldn't produce any bad side-effects. But we haven't really tested that.

The workaround for the isoc_quirk requires 2 tries sending START_TRANSFER command. This means that you have to account the delay of that command completion plus potentially 1 more END_TRANSFER completion. That's why the quirk gives a buffer of at least 4 uframes of the scheduled isoc frame. So, it cannot schedule immediately on the next uframe, that's one of the drawbacks.

Hi Felipe,

Since you're asking this, it means you're still seeing issue with your setup despite retrying to send START_TRANSFER command 5 times. What's the worse delay responding to XferNotReady you're seeing in your setup?

Hi,

Quoted 32 lines · Thinh Nguyen

Quoted 11 lines

Quoted 11 lines

static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct
dwc3_request *req)

Would there be any obvious draw-back to going down this route? The thing is that, as it is, it seems like we will *always* have some corner case where we can't guarantee that we can even start a transfer since there's no upper-bound between XferNotReady and gadget driver finally queueing a request. Also, I can't simply read DSTS for the frame number because of top-most 2 bits.

Quoted 5 lines

For non-affected version of the IP, the xfernotready -> starttransfer time will have to be off by more than a couple seconds for the driver to produce an incorrect 16-bit frame number. If you're seeing errors here, maybe we just need to code review the relevant sections to make sure the 14/16-bit and rollover conditions are all handled correctly.

I think what Felipe may see is some delay in the system that causes the SW to not handle XferNotReady event in time. We already have the "retry" method handle that to a certain extend.

Quoted 4 lines

But I can't think of any obvious drawbacks of the quirk, other than doing some unnecessary work, which shouldn't produce any bad side-effects. But we haven't really tested that.

The workaround for the isoc_quirk requires 2 tries sending START_TRANSFER command. This means that you have to account the delay of that command completion plus potentially 1 more END_TRANSFER completion. That's why the quirk gives a buffer of at least 4 uframes of the scheduled isoc frame. So, it cannot schedule immediately on the next uframe, that's one of the drawbacks.

Quoted 5 lines

Hi Felipe,

Since you're asking this, it means you're still seeing issue with your setup despite retrying to send START_TRANSFER command 5 times. What's the worse delay responding to XferNotReady you're seeing in your setup?

There's no upper-bound on how long the gadget will take to enqueue a request. We see problems with UVC gadget all the time. It can take a lot of time to decide to enqueue data.

Usually I hear this from folks using UVC gadget with a real sensor on the background.

I've seen gadget enqueueing as far as 20 intervals in the future. But remember, there's no upper-bound. And that's the problem. If we could just read the frame number from DSTS and use that, we wouldn't have any issues. But since DSTS only contains 14 our of the 16 bits the controller needs, then we can't really use that.

To me, it seems like this part of the controller wasn't well thought-out. These extra two bits, perhaps, should be internal to the controller and SW should have no knowledge that they exist.

In any case, this is the biggest sort of issues in DWC3 right now :-)

Anything else seems to behave nicely without any problems.

Quoted 35 lines · Felipe Balbi

Hi,

Quoted 32 lines · Thinh Nguyen

Quoted 11 lines

Quoted 11 lines

static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct
dwc3_request *req)

Would there be any obvious draw-back to going down this route? The thing is that, as it is, it seems like we will *always* have some corner case where we can't guarantee that we can even start a transfer since there's no upper-bound between XferNotReady and gadget driver finally queueing a request. Also, I can't simply read DSTS for the frame number because of top-most 2 bits.

Quoted 5 lines

For non-affected version of the IP, the xfernotready -> starttransfer time will have to be off by more than a couple seconds for the driver to produce an incorrect 16-bit frame number. If you're seeing errors here, maybe we just need to code review the relevant sections to make sure the 14/16-bit and rollover conditions are all handled correctly.

I think what Felipe may see is some delay in the system that causes the SW to not handle XferNotReady event in time. We already have the "retry" method handle that to a certain extend.

Quoted 4 lines

But I can't think of any obvious drawbacks of the quirk, other than doing some unnecessary work, which shouldn't produce any bad side-effects. But we haven't really tested that.

The workaround for the isoc_quirk requires 2 tries sending START_TRANSFER command. This means that you have to account the delay of that command completion plus potentially 1 more END_TRANSFER completion. That's why the quirk gives a buffer of at least 4 uframes of the scheduled isoc frame. So, it cannot schedule immediately on the next uframe, that's one of the drawbacks.

Quoted 8 lines

Quoted 5 lines

Hi Felipe,

Since you're asking this, it means you're still seeing issue with your setup despite retrying to send START_TRANSFER command 5 times. What's the worse delay responding to XferNotReady you're seeing in your setup?

There's no upper-bound on how long the gadget will take to enqueue a request. We see problems with UVC gadget all the time. It can take a lot of time to decide to enqueue data.

That's why there's a mechanism in the controller to return bus-expiry status to let the SW know if it had scheduled isoc too late. SW can do 2
things: 1) re-schedule at a later timer or 2) send END_TRANSFER command to wait for the next XferNotReady to try again.

Quoted 8 lines

Usually I hear this from folks using UVC gadget with a real sensor on the background.

I've seen gadget enqueueing as far as 20 intervals in the future. But remember, there's no upper-bound. And that's the problem. If we could just read the frame number from DSTS and use that, we wouldn't have any issues. But since DSTS only contains 14 our of the 16 bits the controller needs, then we can't really use that.

You can create another quirk for devices that have this behavior to use frame number in DSTS instead. As John had pointed out and mentioned, this will only work if the service interval and the delay in the scheduling of isoc is within 2 seconds.

You will need to calculate this value along with BIT(15) and BIT(14) of XferNotReady for rollovers.

Quoted 3 lines

To me, it seems like this part of the controller wasn't well thought-out. These extra two bits, perhaps, should be internal to the controller and SW should have no knowledge that they exist.

These values are internal. SW should not have knowledge of it. This implementation will not follow the programming guide and should be used as a quirk for devices that are too slow to handle the XferNotReady event but want to schedule isoc immediately after handling the event.

Quoted 33 lines

Quoted 33 lines

Quoted 7 lines

Quoted 7 lines

Would there be any obvious draw-back to going down this route? The thing is that, as it is, it seems like we will *always* have some corner case where we can't guarantee that we can even start a transfer since there's no upper-bound between XferNotReady and gadget driver finally queueing a request. Also, I can't simply read DSTS for the frame number because of top-most 2 bits.

Quoted 5 lines

For non-affected version of the IP, the xfernotready -> starttransfer time will have to be off by more than a couple seconds for the driver to produce an incorrect 16-bit frame number. If you're seeing errors here, maybe we just need to code review the relevant sections to make sure the 14/16-bit and rollover conditions are all handled correctly.

I think what Felipe may see is some delay in the system that causes the SW to not handle XferNotReady event in time. We already have the "retry" method handle that to a certain extend.

Quoted 4 lines

But I can't think of any obvious drawbacks of the quirk, other than doing some unnecessary work, which shouldn't produce any bad side-effects. But we haven't really tested that.

The workaround for the isoc_quirk requires 2 tries sending START_TRANSFER command. This means that you have to account the delay of that command completion plus potentially 1 more END_TRANSFER completion. That's why the quirk gives a buffer of at least 4 uframes of the scheduled isoc frame. So, it cannot schedule immediately on the next uframe, that's one of the drawbacks.

Hi Felipe,

Since you're asking this, it means you're still seeing issue with your setup despite retrying to send START_TRANSFER command 5 times. What's the worse delay responding to XferNotReady you're seeing in your setup?

Quoted 3 lines

There's no upper-bound on how long the gadget will take to enqueue a request. We see problems with UVC gadget all the time. It can take a lot of time to decide to enqueue data.

That's why there's a mechanism in the controller to return bus-expiry status to let the SW know if it had scheduled isoc too late. SW can do 2
things: 1) re-schedule at a later timer or 2) send END_TRANSFER command to wait for the next XferNotReady to try again.

Quoted 8 lines

Usually I hear this from folks using UVC gadget with a real sensor on the background.

I've seen gadget enqueueing as far as 20 intervals in the future. But remember, there's no upper-bound. And that's the problem. If we could just read the frame number from DSTS and use that, we wouldn't have any issues. But since DSTS only contains 14 our of the 16 bits the controller needs, then we can't really use that.

You can create another quirk for devices that have this behavior to use frame number in DSTS instead. As John had pointed out and mentioned, this will only work if the service interval and the delay in the scheduling of isoc is within 2 seconds.

You will need to calculate this value along with BIT(15) and BIT(14) of XferNotReady for rollovers.

Quoted 1 line

Quoted 3 lines

To me, it seems like this part of the controller wasn't well thought-out. These extra two bits, perhaps, should be internal to the controller and SW should have no knowledge that they exist.

These values are internal. SW should not have knowledge of it. This implementation will not follow the programming guide and should be used as a quirk for devices that are too slow to handle the XferNotReady event but want to schedule isoc immediately after handling the event.

-----Original Message-----
From: [email protected] <[email protected]> On Behalf Of Felipe Balbi
Sent: Tuesday, June 18, 2019 11:29 PM
To: Thinh Nguyen <[email protected]>; Thinh Nguyen <[email protected]>; John Youn <[email protected]>
Cc: [email protected]
Subject: Re: [RFC] Sorting out dwc3 ISOC endpoints once and for all

Hi,

Quoted 1 line · Thinh Nguyen

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Would there be any obvious draw-back to going down this route? The

thing

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

is that, as it is, it seems like we will *always* have some corner case where we can't guarantee that we can even start a transfer since there's no upper-bound between XferNotReady and gadget driver finally

queueing a

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 8 lines

Quoted 3 lines

request. Also, I can't simply read DSTS for the frame number because of top-most 2 bits.

For non-affected version of the IP, the xfernotready -> starttransfer time will have to be off by more than a couple seconds for the driver to produce an incorrect 16-bit frame number. If you're seeing errors here, maybe we just need to code review the relevant sections to make sure the 14/16-bit and rollover conditions are all handled correctly.

I think what Felipe may see is some delay in the system that causes the SW to not handle XferNotReady event in time. We already have the "retry" method handle that to a certain extend.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

But I can't think of any obvious drawbacks of the quirk, other than doing some unnecessary work, which shouldn't produce any bad side-effects. But we haven't really tested that.

The workaround for the isoc_quirk requires 2 tries sending START_TRANSFER command. This means that you have to account the

delay of

Quoted 1 line

Quoted 1 line

Quoted 1 line

that command completion plus potentially 1 more END_TRANSFER

completion.

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

That's why the quirk gives a buffer of at least 4 uframes of the scheduled isoc frame. So, it cannot schedule immediately on the next uframe, that's one of the drawbacks.

Hi Felipe,

Since you're asking this, it means you're still seeing issue with your setup despite retrying to send START_TRANSFER command 5 times. What's the worse delay responding to XferNotReady you're seeing in your setup?

Quoted 8 lines

Quoted 3 lines

There's no upper-bound on how long the gadget will take to enqueue a request. We see problems with UVC gadget all the time. It can take a lot of time to decide to enqueue data.

That's why there's a mechanism in the controller to return bus-expiry status to let the SW know if it had scheduled isoc too late. SW can do 2
things: 1) re-schedule at a later timer or 2) send END_TRANSFER command to wait for the next XferNotReady to try again.

All of this is still rather flakey. Can I send consecutive END_TRANSFER commands and get new XferNotReady at any moment? Consider this situation:

. transfer started
. transfer completes with status Missed ISOC . driver issues END_TRANSFER (as required by docs) . XferNotReady fires
. driver updates current frame number
. several mS of nothing
. finally gadget enqueues a transfer
. Start Transfer command
. completes with Bus Expiry

Can I issue *another* END_TRANSFER at this point? I don't even have a valid transfer resource since transfer wasn't started.

The best "workaround" I can think of would be to delay END_TRASFER until ep_queue time.

Quoted 9 lines

Quoted 8 lines

Usually I hear this from folks using UVC gadget with a real sensor on the background.

I've seen gadget enqueueing as far as 20 intervals in the future. But remember, there's no upper-bound. And that's the problem. If we could just read the frame number from DSTS and use that, we wouldn't have any issues. But since DSTS only contains 14 our of the 16 bits the controller needs, then we can't really use that.

Quoted 4 lines

You can create another quirk for devices that have this behavior to use frame number in DSTS instead. As John had pointed out and mentioned, this will only work if the service interval and the delay in the scheduling of isoc is within 2 seconds.

well, that's better than nothing, but there's no upper-bound for the gadget driver, really.

They are *not* internal if SW needs to know that to start a transfer properly it needs these extra two bits :-) What I meant to say was that we should never have a 16-bit frame number. Only 14 bits. But in any case, we can't change the HW now :-)

Hi,

Quoted 33 lines · Thinh Nguyen

Quoted 33 lines

Quoted 33 lines

Quoted 7 lines

Quoted 7 lines

Would there be any obvious draw-back to going down this route? The thing is that, as it is, it seems like we will *always* have some corner case where we can't guarantee that we can even start a transfer since there's no upper-bound between XferNotReady and gadget driver finally queueing a request. Also, I can't simply read DSTS for the frame number because of top-most 2 bits.

Quoted 5 lines

For non-affected version of the IP, the xfernotready -> starttransfer time will have to be off by more than a couple seconds for the driver to produce an incorrect 16-bit frame number. If you're seeing errors here, maybe we just need to code review the relevant sections to make sure the 14/16-bit and rollover conditions are all handled correctly.

I think what Felipe may see is some delay in the system that causes the SW to not handle XferNotReady event in time. We already have the "retry" method handle that to a certain extend.

Quoted 4 lines

But I can't think of any obvious drawbacks of the quirk, other than doing some unnecessary work, which shouldn't produce any bad side-effects. But we haven't really tested that.

The workaround for the isoc_quirk requires 2 tries sending START_TRANSFER command. This means that you have to account the delay of that command completion plus potentially 1 more END_TRANSFER completion. That's why the quirk gives a buffer of at least 4 uframes of the scheduled isoc frame. So, it cannot schedule immediately on the next uframe, that's one of the drawbacks.

Hi Felipe,

Since you're asking this, it means you're still seeing issue with your setup despite retrying to send START_TRANSFER command 5 times. What's the worse delay responding to XferNotReady you're seeing in your setup?

Quoted 7 lines

Quoted 3 lines

There's no upper-bound on how long the gadget will take to enqueue a request. We see problems with UVC gadget all the time. It can take a lot of time to decide to enqueue data.

That's why there's a mechanism in the controller to return bus-expiry status to let the SW know if it had scheduled isoc too late. SW can do 2
things: 1) re-schedule at a later timer or 2) send END_TRANSFER command to wait for the next XferNotReady to try again.

All of this is still rather flakey. Can I send consecutive END_TRANSFER commands and get new XferNotReady at any moment? Consider this situation:

. transfer started
. transfer completes with status Missed ISOC . driver issues END_TRANSFER (as required by docs) . XferNotReady fires
. driver updates current frame number
. several mS of nothing
. finally gadget enqueues a transfer
. Start Transfer command
. completes with Bus Expiry

Can I issue *another* END_TRANSFER at this point? I don't even have a valid transfer resource since transfer wasn't started.

The best "workaround" I can think of would be to delay END_TRASFER until ep_queue time.

Quoted 8 lines

Quoted 8 lines

Usually I hear this from folks using UVC gadget with a real sensor on the background.

I've seen gadget enqueueing as far as 20 intervals in the future. But remember, there's no upper-bound. And that's the problem. If we could just read the frame number from DSTS and use that, we wouldn't have any issues. But since DSTS only contains 14 our of the 16 bits the controller needs, then we can't really use that.

Quoted 4 lines

You can create another quirk for devices that have this behavior to use frame number in DSTS instead. As John had pointed out and mentioned, this will only work if the service interval and the delay in the scheduling of isoc is within 2 seconds.

well, that's better than nothing, but there's no upper-bound for the gadget driver, really.

Quoted 6 lines

You will need to calculate this value along with BIT(15) and BIT(14) of XferNotReady for rollovers.

Quoted 3 lines

To me, it seems like this part of the controller wasn't well thought-out. These extra two bits, perhaps, should be internal to the controller and SW should have no knowledge that they exist.

Quoted 4 lines

These values are internal. SW should not have knowledge of it. This implementation will not follow the programming guide and should be used as a quirk for devices that are too slow to handle the XferNotReady event but want to schedule isoc immediately after handling the event.

They are *not* internal if SW needs to know that to start a transfer properly it needs these extra two bits :-) What I meant to say was that we should never have a 16-bit frame number. Only 14 bits. But in any case, we can't change the HW now :-)

Quoted 57 lines · Felipe Balbi

Hi,

Quoted 33 lines · Thinh Nguyen

Quoted 33 lines

Quoted 33 lines

Quoted 7 lines

Quoted 7 lines

Would there be any obvious draw-back to going down this route? The thing is that, as it is, it seems like we will *always* have some corner case where we can't guarantee that we can even start a transfer since there's no upper-bound between XferNotReady and gadget driver finally queueing a request. Also, I can't simply read DSTS for the frame number because of top-most 2 bits.

Quoted 5 lines

For non-affected version of the IP, the xfernotready -> starttransfer time will have to be off by more than a couple seconds for the driver to produce an incorrect 16-bit frame number. If you're seeing errors here, maybe we just need to code review the relevant sections to make sure the 14/16-bit and rollover conditions are all handled correctly.

I think what Felipe may see is some delay in the system that causes the SW to not handle XferNotReady event in time. We already have the "retry" method handle that to a certain extend.

Quoted 4 lines

But I can't think of any obvious drawbacks of the quirk, other than doing some unnecessary work, which shouldn't produce any bad side-effects. But we haven't really tested that.

The workaround for the isoc_quirk requires 2 tries sending START_TRANSFER command. This means that you have to account the delay of that command completion plus potentially 1 more END_TRANSFER completion. That's why the quirk gives a buffer of at least 4 uframes of the scheduled isoc frame. So, it cannot schedule immediately on the next uframe, that's one of the drawbacks.

Hi Felipe,

Since you're asking this, it means you're still seeing issue with your setup despite retrying to send START_TRANSFER command 5 times. What's the worse delay responding to XferNotReady you're seeing in your setup?

Quoted 7 lines

Quoted 3 lines

There's no upper-bound on how long the gadget will take to enqueue a request. We see problems with UVC gadget all the time. It can take a lot of time to decide to enqueue data.

That's why there's a mechanism in the controller to return bus-expiry status to let the SW know if it had scheduled isoc too late. SW can do 2
things: 1) re-schedule at a later timer or 2) send END_TRANSFER command to wait for the next XferNotReady to try again.

All of this is still rather flakey. Can I send consecutive END_TRANSFER commands and get new XferNotReady at any moment? Consider this situation:

. transfer started
. transfer completes with status Missed ISOC . driver issues END_TRANSFER (as required by docs) . XferNotReady fires
. driver updates current frame number
. several mS of nothing
. finally gadget enqueues a transfer
. Start Transfer command
. completes with Bus Expiry

Quoted 2 lines

Can I issue *another* END_TRANSFER at this point? I don't even have a valid transfer resource since transfer wasn't started.

Yes you can. If the START_TRANSFER command completes, you will get the transfer resource index to use for END_TRANSFER command (regardless of bus-expiry status).

However, there's a chance if the SW somehow keeps handling the XferNotReady event late over and over and the isoc transfer never get started. That's where you can create a quirk and use frame number from DSTS instead.