Hi Felipe,
On 11/9/2018 3:04 AM, Felipe Balbi wrote:
> Hi again,
>
> Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx> 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
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?
BR,
Thinh
