[RFC] Sorting out dwc3 ISOC endpoints once and for all

[RFC] Sorting out dwc3 ISOC endpoints once and for all

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2339 bytes --]


Hi John,

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
@@ -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 */
 	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.

best

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

RE: [RFC] Sorting out dwc3 ISOC endpoints once and for all

[John Youn]

> -----Original Message-----
> From: Felipe Balbi <felipe.balbi@linux.intel.com>
> Sent: Friday, June 7, 2019 2:50 AM
> To: John Youn <John.Youn@synopsys.com>
> Cc: linux-usb@vger.kernel.org
> Subject: [RFC] Sorting out dwc3 ISOC endpoints once and for all
> 

++ Thinh

Hi Felipe,

Sorry, missed this e-mail.

> 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?

> 
> I mean something along the lines of below diff (completely untested):
> 
> modified   drivers/usb/dwc3/gadget.c
> @@ -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 */
>  	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.
> 

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.

John

Re: [RFC] Sorting out dwc3 ISOC endpoints once and for all

[Thinh Nguyen]

Hi,

John Youn wrote:
>> -----Original Message-----
>> From: Felipe Balbi <felipe.balbi@linux.intel.com>
>> Sent: Friday, June 7, 2019 2:50 AM
>> To: John Youn <John.Youn@synopsys.com>
>> Cc: linux-usb@vger.kernel.org
>> Subject: [RFC] Sorting out dwc3 ISOC endpoints once and for all
>>
> ++ Thinh
>
> Hi Felipe,
>
> Sorry, missed this e-mail.
>
>> 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?
>
>> I mean something along the lines of below diff (completely untested):
>>
>> modified   drivers/usb/dwc3/gadget.c
>> @@ -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 */
>>  	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.
>>
> 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.

> 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?

BR,
Thinh

RE: [RFC] Sorting out dwc3 ISOC endpoints once and for all

[John Youn]

> -----Original Message-----
> From: Thinh Nguyen
> Sent: Monday, June 17, 2019 12:09 PM
> To: John Youn <johnyoun@synopsys.com>; Felipe Balbi
> <felipe.balbi@linux.intel.com>; John Youn <John.Youn@synopsys.com>
> Cc: linux-usb@vger.kernel.org
> Subject: Re: [RFC] Sorting out dwc3 ISOC endpoints once and for all
> 
> Hi,
> 
> John Youn wrote:
> >> -----Original Message-----
> >> From: Felipe Balbi <felipe.balbi@linux.intel.com>
> >> Sent: Friday, June 7, 2019 2:50 AM
> >> To: John Youn <John.Youn@synopsys.com>
> >> Cc: linux-usb@vger.kernel.org
> >> Subject: [RFC] Sorting out dwc3 ISOC endpoints once and for all
> >>
> > ++ Thinh
> >
> > Hi Felipe,
> >
> > Sorry, missed this e-mail.
> >
> >> 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?
> >
> >> I mean something along the lines of below diff (completely untested):
> >>
> >> modified   drivers/usb/dwc3/gadget.c
> >> @@ -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 */
> >>  	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.
> >>
> > 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.
> 
> > 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.
> 
> 

So the quirk is not ideal because it requires some extra time to retry start transfers. This might cause delays for getting the first request out.

The best way for non-affected IPs is probably to NOT use the quirk, and calculate the start transfer frame number based on the 16-bit xfernotready frame number and 14-bit DSTS frame number. As long as the delay between the xfernotready event and handling does not exceed 2 seconds, and you take into account rollover condition, this calculation should be correct.

The upper-bound between xfernotready and gadget's first request that you mention does not come into play, because if there is no request, the xfernotready event handler should do nothing. It should only do something when we have a queued request AND we get xfernotready.

John

Re: [RFC] Sorting out dwc3 ISOC endpoints once and for all

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2934 bytes --]


Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>
>>>  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.
>>>
>> 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.
>
>> 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?

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.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [RFC] Sorting out dwc3 ISOC endpoints once and for all

[Thinh Nguyen]

Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>>  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.
>>>>
>>> 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.
>>
>>> 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?
> 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.

> 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.

>
> 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.

> In any case, this is the biggest sort of issues in DWC3 right now :-)
>
> Anything else seems to behave nicely without any problems.
>

BR,
Thinh

Re: [RFC] Sorting out dwc3 ISOC endpoints once and for all

[Thinh Nguyen]

Thinh Nguyen wrote:
> Felipe Balbi wrote:
>> Hi,
>>
>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>>>  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.
>>>>>
>>>> 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.
>>>
>>>> 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?
>> 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.
>
>> 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.
>
>> 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.
>

Correction: these values are not internal. (somehow I was thinking the
schedule time is calculated internally with the 16-bit value led me to
say that they are internal).

Thinh

Re: [RFC] Sorting out dwc3 ISOC endpoints once and for all

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 4758 bytes --]


Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>>> 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.
>>>
>>>> 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?
>> 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.

>> 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.

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

> You will need to calculate this value along with BIT(15) and BIT(14) of
> XferNotReady for rollovers.
>
>>
>> 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.

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 :-)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

RE: [RFC] Sorting out dwc3 ISOC endpoints once and for all

[John Youn]

> -----Original Message-----
> From: linux-usb-owner@vger.kernel.org <linux-usb-owner@vger.kernel.org> On
> Behalf Of Felipe Balbi
> Sent: Tuesday, June 18, 2019 11:29 PM
> To: Thinh Nguyen <Thinh.Nguyen@synopsys.com>; Thinh Nguyen
> <Thinh.Nguyen@synopsys.com>; John Youn <John.Youn@synopsys.com>
> Cc: linux-usb@vger.kernel.org
> Subject: Re: [RFC] Sorting out dwc3 ISOC endpoints once and for all
> 
> 
> Hi,
> 
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
> >>>>> 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.
> >>>
> >>>> 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?
> >> 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.
> 
> >> 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.
> 
> well, that's better than nothing, but there's no upper-bound for the
> gadget driver, really.
> 

This will take care of the scenario you described above. Using xfernotready+DSTS to calculate the start transfer frame number should probably just be the default behavior.

For the case the gadget driver takes > 2 seconds to queue, you can go through the quirk. It's probably best to do this pre-emptively rather than rely on bus expiry. Because bus expiry only happens when your start frame is off by > 2 seconds. So you may get the top-2 bits wrong and start transfer will succeed, but you will have introduced a delay in the stream.

> 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 :-)

I believe the bits were added to allow for scheduling of large intervals, like 2 and 4 seconds. If anything DSTS should reveal the 16-bit frame number as well.

We can ask our hw engineers if they have any other suggestions for this case.

John

Re: [RFC] Sorting out dwc3 ISOC endpoints once and for all

[Thinh Nguyen]

Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>>>> 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.
>>>>
>>>>> 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?
>>> 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.

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.

> The best "workaround" I can think of would be to delay END_TRASFER until
> ep_queue time.
>
>>> 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.
> well, that's better than nothing, but there's no upper-bound for the
> gadget driver, really.
>
>> You will need to calculate this value along with BIT(15) and BIT(14) of
>> XferNotReady for rollovers.
>>
>>> 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.
> 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 :-)
>

Yeah... I made a correction in my previous email that it's not internal.
But like I said, we can create a quirk to use DSTS frame number to
workaround this issue with some limitations.

BR,
Thinh

Re: [RFC] Sorting out dwc3 ISOC endpoints once and for all

[Thinh Nguyen]

Hi,

Thinh Nguyen wrote:
> Felipe Balbi wrote:
>> Hi,
>>
>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>>>>> 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.
>>>>>
>>>>>> 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?
>>>> 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.
> 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.
>

I thought about this again. I think the best way to handle this is the
followings:
* For service interval less than 2 seconds, use DSTS and XferNotReady
frame number to calculate and schedule isoc.
* For service interval of 2 second or larger, just use XferNotReady
frame number.
* If START_TRANSFER command still fails with bus-expiry, then send
END_TRANSFER command to restart on a new XferNotReady. This should only
happen if the SW is too slow to handle XferNotReady event for over 2
seconds. This case should not happen often.

Note: bus-expiry happens when |current frame - scheduled frame| > 4 seconds.

This guarantees that the DWC3 will retry to schedule the isoc transfer
again. There's no need to create a quirk for the controller.

What do you think?

BR,
Thinh