On 11/14/2018 12:42 AM, Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <thinh.nguyen@xxxxxxxxxxxx> writes:
>>> 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
> 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:
>
> 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);
Shouldn't this be like this?
dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1);
> +
> + 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)
>
> 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.
>
I agree. Also, depending on the application requirement, we may want to
giveback/cleanup request(s) every time we fail so that the data won't be
pushed back/delayed for too long.
We could experiment and decide on the maximum delay time (base on the
service interval length and number of retries) to be considered
acceptable to start giveback stale requests.
Thinh
