[4/4] usb: dwc3: gadget: check if dep->frame_number is still valid

[Felipe Balbi <felipe.balbi@linux.intel.com>]

Hi,

Thinh Nguyen <thinh.nguyen@synopsys.com> writes:
>> Felipe Balbi <felipe.balbi@linux.intel.com> writes:
>>> In the case of interval of 1ms, it will start on the next interval.
>>>> frame_number + max(4, interval) will start at least 4 uframes in the future.
>>>>
>>>> In any case, what about immediately retry the START_TRANSFER command
>>>> with a new frame_number + (interval*retry) should it fail with
>>>> bus-expiry? You can set the number of retries to maybe 5 times. This
>>>> should remove the need to do time stamping.
>>> That seems like a good idea. Something like below? (on top of $subject)
>>>
>>> modified   drivers/usb/dwc3/core.h
>>> @@ -37,6 +37,7 @@
>>>  #define DWC3_EP0_SETUP_SIZE	512
>>>  #define DWC3_ENDPOINTS_NUM	32
>>>  #define DWC3_XHCI_RESOURCES_NUM	2
>>> +#define DWC3_ISOC_MAX_RETRIES	5
>>>  
>>>  #define DWC3_SCRATCHBUF_SIZE	4096	/* each buffer is assumed to be 4KiB */
>>>  #define DWC3_EVENT_BUFFERS_SIZE	4096
>>> modified   drivers/usb/dwc3/gadget.c
>>> @@ -1271,20 +1271,29 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
>>>  	u64 current_timestamp;
>>>  	u64 diff_timestamp;
>>>  	u32 elapsed_frames;
>>> +	int retries;
>>> +	int delta = 1
>>> +	int ret;
>>>  
>>>  	if (list_empty(&dep->pending_list)) {
>>>  		dep->flags |= DWC3_EP_PENDING_REQUEST;
>>>  		return -EAGAIN;
>>>  	}
>>>  
>>> -	current_timestamp = ktime_get_ns();
>>> -	diff_timestamp = current_timestamp - dep->frame_timestamp;
>>> -	elapsed_frames = DIV_ROUND_UP_ULL(diff_timestamp, 125000);
>>> +	for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
>>> +		current_timestamp = ktime_get_ns();
>>> +		diff_timestamp = current_timestamp - dep->frame_timestamp;
>>> +		elapsed_frames = DIV_ROUND_UP_ULL(diff_timestamp, 125000);
>>>  
>>> -	dep->frame_number += elapsed_frames;
>>> -	dep->frame_number = DWC3_ALIGN_FRAME(dep);
>>> +		dep->frame_number += elapsed_frames + (delta * i);
>> the other possibility is that we can call DWC3_ALIGN_FRAME() n times
>> since that will put the transfer on the following interval. That would
>> look like so:
>>
>> modified   drivers/usb/dwc3/core.h
>> @@ -37,6 +37,7 @@
>>  #define DWC3_EP0_SETUP_SIZE	512
>>  #define DWC3_ENDPOINTS_NUM	32
>>  #define DWC3_XHCI_RESOURCES_NUM	2
>> +#define DWC3_ISOC_MAX_RETRIES	5
>>  
>>  #define DWC3_SCRATCHBUF_SIZE	4096	/* each buffer is assumed to be 4KiB */
>>  #define DWC3_EVENT_BUFFERS_SIZE	4096
>> modified   drivers/usb/dwc3/gadget.c
>> @@ -27,7 +27,7 @@
>>  #include "gadget.h"
>>  #include "io.h"
>>  
>> -#define DWC3_ALIGN_FRAME(d)	(((d)->frame_number + (d)->interval) \
>> +#define DWC3_ALIGN_FRAME(d, n)	(((d)->frame_number + ((d)->interval * (n))) \
>>  					& ~((d)->interval - 1))
>>  
>>  /**
>> @@ -1271,20 +1271,28 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
>>  	u64 current_timestamp;
>>  	u64 diff_timestamp;
>>  	u32 elapsed_frames;
>> +	int retries;
>> +	int ret;
>>  
>>  	if (list_empty(&dep->pending_list)) {
>>  		dep->flags |= DWC3_EP_PENDING_REQUEST;
>>  		return -EAGAIN;
>>  	}
>>  
>> -	current_timestamp = ktime_get_ns();
>> -	diff_timestamp = current_timestamp - dep->frame_timestamp;
>> -	elapsed_frames = DIV_ROUND_UP_ULL(diff_timestamp, 125000);
>> +	for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
>> +		current_timestamp = ktime_get_ns();
>> +		diff_timestamp = current_timestamp - dep->frame_timestamp;
>> +		elapsed_frames = DIV_ROUND_UP_ULL(diff_timestamp, 125000);
>>  
>> -	dep->frame_number += elapsed_frames;
>> -	dep->frame_number = DWC3_ALIGN_FRAME(dep);
>> +		dep->frame_number += elapsed_frames;
>> +		dep->frame_number = DWC3_ALIGN_FRAME(dep, i);
>>  
>> -	return __dwc3_gadget_kick_transfer(dep);
>> +		ret = __dwc3_gadget_kick_transfer(dep);
>> +		if (ret != -EAGAIN)
>> +			break;
>> +	}
>> +
>> +	return ret;
>>  }
>>  
>>  static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
>>
>>
> I like the second method. But do we need to keep track of the
> frame_timestamp? I don't think it accurately reflects the timestamp of

no we don't. I've since removed it from my local tree.

> XferNotReady frame number.

> As for the number of retries, we should adjust it according to the
> service interval. For example, for larger service interval such as 16,
> then we don't need to try more than once. To calculate the max number of
> retries, we can do this check (where interval is from 1 to 16):
>
> if (interval >= (16 - (MAX_NUM_RETRIES >> 1))
>         num_retries = 1 << (16 - interval);
>
> Please check my math..
>
> If it fails for over 5 times in a row, then we should probably wait for
> the next XferNotReady to get the frame_number again. Also 5 is an
> arbitrary number I came up without any testing, we can probably decide
> on a better number. What do you think?

I have this now:


This means we will increment in full intervals, up to 5 intervals in the
future. If it still fails, then there's not much we can do.

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 131028501752..3390fa46ea30 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -37,6 +37,7 @@
 #define DWC3_EP0_SETUP_SIZE    512
 #define DWC3_ENDPOINTS_NUM     32
 #define DWC3_XHCI_RESOURCES_NUM        2
+#define DWC3_ISOC_MAX_RETRIES  5
 
 #define DWC3_SCRATCHBUF_SIZE   4096    /* each buffer is assumed to be 4KiB */
 #define DWC3_EVENT_BUFFERS_SIZE        4096
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index d8c7ad0c22e8..1590516735cb 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -27,7 +27,7 @@
 #include "gadget.h"
 #include "io.h"
 
-#define DWC3_ALIGN_FRAME(d)    (((d)->frame_number + (d)->interval) \
+#define DWC3_ALIGN_FRAME(d, n) (((d)->frame_number + ((d)->interval * (n))) \
                                        & ~((d)->interval - 1))
 
 /**
@@ -1268,13 +1268,24 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
 
 static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
 {
+       int retries;
+       int ret;
+       int i;
+
        if (list_empty(&dep->pending_list)) {
                dep->flags |= DWC3_EP_PENDING_REQUEST;
                return -EAGAIN;
        }
 
-       dep->frame_number = DWC3_ALIGN_FRAME(dep);
-       return __dwc3_gadget_kick_transfer(dep);
+       for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
+               dep->frame_number = DWC3_ALIGN_FRAME(dep, i);
+
+               ret = __dwc3_gadget_kick_transfer(dep);
+               if (ret != -EAGAIN)
+                       break;
+       }
+
+       return ret;
 }
 
 static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)