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

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

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