* [PATCH v2 0/2] usb: dwc3: gadget: improve isoc handling @ 2020-06-24 14:49 Michael Grzeschik 2020-06-24 14:49 ` [PATCH v2 1/2] usb: dwc3: gadget: make starting isoc transfers more robust Michael Grzeschik 2020-06-24 14:49 ` [PATCH v2 2/2] usb: dwc3: gadget: when the started list is empty stop the active xfer Michael Grzeschik 0 siblings, 2 replies; 12+ messages in thread From: Michael Grzeschik @ 2020-06-24 14:49 UTC (permalink / raw) To: linux-usb; +Cc: Thinh.Nguyen, balbi, gregkh, kernel These two patches improve the isoc handling and make the dwc3 gadget driver somewhat usable with the UVC gadget for isochronous endpoints. The first patch makes starting isochronous transfers more reliable. I think it's more less, what Thinh suggested some time ago[1]. It's still not perfect because the first request must still be queued within 2 seconds but it's a lot better than the current situation. The second patch makes it possible to have gaps in the data stream. The UVC gadget relies on such behaviour. Without this, using the UVC gadget with a live stream stops after the first frame that needs more time to be scheduled. [1] https://marc.info/?l=linux-usb&m=156088170723824&w=4 Michael Grzeschik (1): usb: dwc3: gadget: when the started list is empty stop the active xfer Michael Olbrich (1): usb: dwc3: gadget: make starting isoc transfers more robust drivers/usb/dwc3/gadget.c | 42 +++++++++++++++++++++++++++------------ 1 file changed, 29 insertions(+), 13 deletions(-) -- 2.27.0 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 1/2] usb: dwc3: gadget: make starting isoc transfers more robust 2020-06-24 14:49 [PATCH v2 0/2] usb: dwc3: gadget: improve isoc handling Michael Grzeschik @ 2020-06-24 14:49 ` Michael Grzeschik 2020-06-24 19:26 ` Thinh Nguyen 2020-06-24 14:49 ` [PATCH v2 2/2] usb: dwc3: gadget: when the started list is empty stop the active xfer Michael Grzeschik 1 sibling, 1 reply; 12+ messages in thread From: Michael Grzeschik @ 2020-06-24 14:49 UTC (permalink / raw) To: linux-usb; +Cc: Thinh.Nguyen, balbi, gregkh, kernel, Michael Olbrich From: Michael Olbrich <m.olbrich@pengutronix.de> Currently __dwc3_gadget_start_isoc must be called very shortly after XferNotReady. Otherwise the frame number is outdated and start transfer will fail, even with several retries. DSTS provides the lower 14 bit of the frame number. Use it in combination with the frame number provided by XferNotReady to guess the current frame number. This will succeed unless more than one 14 rollover has happened since XferNotReady. Start transfer might still fail if the frame number is increased immediately after DSTS is read. So retries are still needed. Don't drop the current request if this happens. This way it is not lost and can be used immediately to try again with the next frame number. With this change, __dwc3_gadget_start_isoc is still not successfully in all cases bit it increases the acceptable delay after XferNotReady significantly. Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> --- v1 -> v2: - removed last_frame_number from struct - included rollover variable drivers/usb/dwc3/gadget.c | 37 +++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 421a0f73110a40b..0962ddd7fbf6ae6 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -1276,7 +1276,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep) static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *dep); -static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) +static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep, bool keep_req) { struct dwc3_gadget_ep_cmd_params params; struct dwc3_request *req; @@ -1314,12 +1314,9 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) } ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); - if (ret < 0) { + if (ret < 0 && (!keep_req || ret != -EAGAIN)) { struct dwc3_request *tmp; - if (ret == -EAGAIN) - return ret; - dwc3_stop_active_transfer(dep, true, true); list_for_each_entry_safe(req, tmp, &dep->started_list, list) @@ -1335,7 +1332,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) if (dep->stream_capable && req->request.is_last) dep->flags |= DWC3_EP_WAIT_TRANSFER_COMPLETE; - return 0; + return ret; } static int __dwc3_gadget_get_frame(struct dwc3 *dwc) @@ -1458,7 +1455,7 @@ static int dwc3_gadget_start_isoc_quirk(struct dwc3_ep *dep) dep->start_cmd_status = 0; dep->combo_num = 0; - return __dwc3_gadget_kick_transfer(dep); + return __dwc3_gadget_kick_transfer(dep, false); } static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep) @@ -1481,9 +1478,25 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep) } for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) { - dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1); + /* + * frame_number is set from XferNotReady and may be already + * out of date. DSTS only provides the lower 14 bit of the + * current frame number. So add the upper two bits of + * frame_number and handle a possible rollover. + * This will provide the correct frame_number unless more than + * rollover has happened since XferNotReady. + */ + u32 frame = __dwc3_gadget_get_frame(dwc); + bool rollover = frame < (dep->frame_number & 0x3fff); + + dep->frame_number = (dep->frame_number & ~0x3fff) | frame; + if (rollover) + dep->frame_number += BIT(14); + + dep->frame_number = DWC3_ALIGN_FRAME(dep, 1); - ret = __dwc3_gadget_kick_transfer(dep); + ret = __dwc3_gadget_kick_transfer(dep, + i + 1 < DWC3_ISOC_MAX_RETRIES); if (ret != -EAGAIN) break; } @@ -1567,7 +1580,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) } } - return __dwc3_gadget_kick_transfer(dep); + return __dwc3_gadget_kick_transfer(dep, false); } static int dwc3_gadget_ep_queue(struct usb_ep *ep, struct usb_request *request, @@ -2719,7 +2732,7 @@ static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep, if (status == -EXDEV && list_empty(&dep->started_list)) dwc3_stop_active_transfer(dep, true, true); else if (dwc3_gadget_ep_should_continue(dep)) - if (__dwc3_gadget_kick_transfer(dep) == 0) + if (__dwc3_gadget_kick_transfer(dep, false) == 0) no_started_trb = false; out: @@ -2904,7 +2917,7 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc, dwc3_gadget_ep_cleanup_cancelled_requests(dep); if ((dep->flags & DWC3_EP_DELAY_START) && !usb_endpoint_xfer_isoc(dep->endpoint.desc)) - __dwc3_gadget_kick_transfer(dep); + __dwc3_gadget_kick_transfer(dep, false); dep->flags &= ~DWC3_EP_DELAY_START; } -- 2.27.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc3: gadget: make starting isoc transfers more robust 2020-06-24 14:49 ` [PATCH v2 1/2] usb: dwc3: gadget: make starting isoc transfers more robust Michael Grzeschik @ 2020-06-24 19:26 ` Thinh Nguyen 2020-06-24 21:09 ` Thinh Nguyen 2020-06-25 11:44 ` Michael Grzeschik 0 siblings, 2 replies; 12+ messages in thread From: Thinh Nguyen @ 2020-06-24 19:26 UTC (permalink / raw) To: Michael Grzeschik, linux-usb Cc: Thinh Nguyen, balbi, gregkh, kernel, Michael Olbrich Hi, Michael Grzeschik wrote: > From: Michael Olbrich <m.olbrich@pengutronix.de> > > Currently __dwc3_gadget_start_isoc must be called very shortly after > XferNotReady. Otherwise the frame number is outdated and start transfer > will fail, even with several retries. Did you try with the recent update for isoc? (e.i., after 5 START_TRANSFER failures, the driver will issue END_TRANSFER to retry on the next XferNotReady event) Let me know if you still run into issues with that. > > DSTS provides the lower 14 bit of the frame number. Use it in combination > with the frame number provided by XferNotReady to guess the current frame > number. This will succeed unless more than one 14 rollover has happened > since XferNotReady. > > Start transfer might still fail if the frame number is increased > immediately after DSTS is read. So retries are still needed. > Don't drop the current request if this happens. This way it is not lost and > can be used immediately to try again with the next frame number. > > With this change, __dwc3_gadget_start_isoc is still not successfully in all > cases bit it increases the acceptable delay after XferNotReady > significantly. > > Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > > --- > v1 -> v2: - removed last_frame_number from struct > - included rollover variable > > drivers/usb/dwc3/gadget.c | 37 +++++++++++++++++++++++++------------ > 1 file changed, 25 insertions(+), 12 deletions(-) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 421a0f73110a40b..0962ddd7fbf6ae6 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -1276,7 +1276,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep) > > static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *dep); > > -static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) > +static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep, bool keep_req) Any reason to have keep_req? With the recent changes, if dwc3_send_gadget_ep_cmd() returns -EAGAIN, then the controller driver won't give back the request. > { > struct dwc3_gadget_ep_cmd_params params; > struct dwc3_request *req; > @@ -1314,12 +1314,9 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) > } > > ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); > - if (ret < 0) { > + if (ret < 0 && (!keep_req || ret != -EAGAIN)) { > struct dwc3_request *tmp; > > - if (ret == -EAGAIN) > - return ret; > - > dwc3_stop_active_transfer(dep, true, true); > > list_for_each_entry_safe(req, tmp, &dep->started_list, list) > @@ -1335,7 +1332,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) > if (dep->stream_capable && req->request.is_last) > dep->flags |= DWC3_EP_WAIT_TRANSFER_COMPLETE; > > - return 0; > + return ret; > } > > static int __dwc3_gadget_get_frame(struct dwc3 *dwc) > @@ -1458,7 +1455,7 @@ static int dwc3_gadget_start_isoc_quirk(struct dwc3_ep *dep) > dep->start_cmd_status = 0; > dep->combo_num = 0; > > - return __dwc3_gadget_kick_transfer(dep); > + return __dwc3_gadget_kick_transfer(dep, false); > } > > static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep) > @@ -1481,9 +1478,25 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep) > } > > for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) { > - dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1); > + /* > + * frame_number is set from XferNotReady and may be already > + * out of date. DSTS only provides the lower 14 bit of the > + * current frame number. So add the upper two bits of > + * frame_number and handle a possible rollover. > + * This will provide the correct frame_number unless more than > + * rollover has happened since XferNotReady. > + */ > + u32 frame = __dwc3_gadget_get_frame(dwc); > + bool rollover = frame < (dep->frame_number & 0x3fff); > + > + dep->frame_number = (dep->frame_number & ~0x3fff) | frame; > + if (rollover) > + dep->frame_number += BIT(14); > + > + dep->frame_number = DWC3_ALIGN_FRAME(dep, 1); > > - ret = __dwc3_gadget_kick_transfer(dep); > + ret = __dwc3_gadget_kick_transfer(dep, > + i + 1 < DWC3_ISOC_MAX_RETRIES); > if (ret != -EAGAIN) > break; > } > @@ -1567,7 +1580,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) > } > } > > - return __dwc3_gadget_kick_transfer(dep); > + return __dwc3_gadget_kick_transfer(dep, false); > } > > static int dwc3_gadget_ep_queue(struct usb_ep *ep, struct usb_request *request, > @@ -2719,7 +2732,7 @@ static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep, > if (status == -EXDEV && list_empty(&dep->started_list)) > dwc3_stop_active_transfer(dep, true, true); > else if (dwc3_gadget_ep_should_continue(dep)) > - if (__dwc3_gadget_kick_transfer(dep) == 0) > + if (__dwc3_gadget_kick_transfer(dep, false) == 0) > no_started_trb = false; > > out: > @@ -2904,7 +2917,7 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc, > dwc3_gadget_ep_cleanup_cancelled_requests(dep); > if ((dep->flags & DWC3_EP_DELAY_START) && > !usb_endpoint_xfer_isoc(dep->endpoint.desc)) > - __dwc3_gadget_kick_transfer(dep); > + __dwc3_gadget_kick_transfer(dep, false); > > dep->flags &= ~DWC3_EP_DELAY_START; > } BR, Thinh ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc3: gadget: make starting isoc transfers more robust 2020-06-24 19:26 ` Thinh Nguyen @ 2020-06-24 21:09 ` Thinh Nguyen 2020-06-25 11:44 ` Michael Grzeschik 1 sibling, 0 replies; 12+ messages in thread From: Thinh Nguyen @ 2020-06-24 21:09 UTC (permalink / raw) To: Michael Grzeschik, linux-usb; +Cc: balbi, gregkh, kernel, Michael Olbrich Thinh Nguyen wrote: > Hi, > > Michael Grzeschik wrote: >> From: Michael Olbrich <m.olbrich@pengutronix.de> >> >> Currently __dwc3_gadget_start_isoc must be called very shortly after >> XferNotReady. Otherwise the frame number is outdated and start transfer >> will fail, even with several retries. > Did you try with the recent update for isoc? (e.i., after 5 > START_TRANSFER failures, the driver will issue END_TRANSFER to retry on > the next XferNotReady event) > > Let me know if you still run into issues with that. Just want to clarify, I like this solution. I'm just curious how it behaves on your test setup with the current implementation. > >> DSTS provides the lower 14 bit of the frame number. Use it in combination >> with the frame number provided by XferNotReady to guess the current frame >> number. This will succeed unless more than one 14 rollover has happened >> since XferNotReady. >> >> Start transfer might still fail if the frame number is increased >> immediately after DSTS is read. So retries are still needed. >> Don't drop the current request if this happens. This way it is not lost and >> can be used immediately to try again with the next frame number. >> >> With this change, __dwc3_gadget_start_isoc is still not successfully in all >> cases bit it increases the acceptable delay after XferNotReady >> significantly. >> >> Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de> >> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> >> >> --- >> v1 -> v2: - removed last_frame_number from struct >> - included rollover variable >> >> drivers/usb/dwc3/gadget.c | 37 +++++++++++++++++++++++++------------ >> 1 file changed, 25 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 421a0f73110a40b..0962ddd7fbf6ae6 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -1276,7 +1276,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep) >> >> static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *dep); >> >> -static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) >> +static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep, bool keep_req) > Any reason to have keep_req? With the recent changes, if > dwc3_send_gadget_ep_cmd() returns -EAGAIN, then the controller driver > won't give back the request. > BR, Thinh ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc3: gadget: make starting isoc transfers more robust 2020-06-24 19:26 ` Thinh Nguyen 2020-06-24 21:09 ` Thinh Nguyen @ 2020-06-25 11:44 ` Michael Grzeschik 2020-06-25 19:35 ` Thinh Nguyen 1 sibling, 1 reply; 12+ messages in thread From: Michael Grzeschik @ 2020-06-25 11:44 UTC (permalink / raw) To: Thinh Nguyen; +Cc: linux-usb, balbi, gregkh, kernel, Michael Olbrich [-- Attachment #1: Type: text/plain, Size: 6812 bytes --] Hi! On Wed, Jun 24, 2020 at 07:26:42PM +0000, Thinh Nguyen wrote: >Michael Grzeschik wrote: >> From: Michael Olbrich <m.olbrich@pengutronix.de> >> >> Currently __dwc3_gadget_start_isoc must be called very shortly after >> XferNotReady. Otherwise the frame number is outdated and start transfer >> will fail, even with several retries. > >Did you try with the recent update for isoc? (e.i., after 5 >START_TRANSFER failures, the driver will issue END_TRANSFER to retry on >the next XferNotReady event) > >Let me know if you still run into issues with that. Yes. Without my patch I still see the issue. Event with the retry machanism. It is even worse. I even missed one additional patch, I had on top this one. See my note below. >> DSTS provides the lower 14 bit of the frame number. Use it in combination >> with the frame number provided by XferNotReady to guess the current frame >> number. This will succeed unless more than one 14 rollover has happened >> since XferNotReady. >> >> Start transfer might still fail if the frame number is increased >> immediately after DSTS is read. So retries are still needed. >> Don't drop the current request if this happens. This way it is not lost and >> can be used immediately to try again with the next frame number. >> >> With this change, __dwc3_gadget_start_isoc is still not successfully in all >> cases bit it increases the acceptable delay after XferNotReady >> significantly. >> >> Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de> >> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> >> >> --- >> v1 -> v2: - removed last_frame_number from struct >> - included rollover variable >> >> drivers/usb/dwc3/gadget.c | 37 +++++++++++++++++++++++++------------ >> 1 file changed, 25 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 421a0f73110a40b..0962ddd7fbf6ae6 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -1276,7 +1276,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep) >> >> static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *dep); >> >> -static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) >> +static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep, bool keep_req) > >Any reason to have keep_req? With the recent changes, if >dwc3_send_gadget_ep_cmd() returns -EAGAIN, then the controller driver >won't give back the request. Yes, we don't need the keep_req. I tried this and it worked without. I will remove the parameter in v3. >> { >> struct dwc3_gadget_ep_cmd_params params; >> struct dwc3_request *req; >> @@ -1314,12 +1314,9 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) >> } >> >> ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); >> - if (ret < 0) { >> + if (ret < 0 && (!keep_req || ret != -EAGAIN)) { >> struct dwc3_request *tmp; >> >> - if (ret == -EAGAIN) >> - return ret; >> - >> dwc3_stop_active_transfer(dep, true, true); >> >> list_for_each_entry_safe(req, tmp, &dep->started_list, list) >> @@ -1335,7 +1332,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) >> if (dep->stream_capable && req->request.is_last) >> dep->flags |= DWC3_EP_WAIT_TRANSFER_COMPLETE; >> >> - return 0; >> + return ret; >> } >> >> static int __dwc3_gadget_get_frame(struct dwc3 *dwc) >> @@ -1458,7 +1455,7 @@ static int dwc3_gadget_start_isoc_quirk(struct dwc3_ep *dep) >> dep->start_cmd_status = 0; >> dep->combo_num = 0; >> >> - return __dwc3_gadget_kick_transfer(dep); >> + return __dwc3_gadget_kick_transfer(dep, false); >> } >> >> static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep) >> @@ -1481,9 +1478,25 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep) >> } >> >> for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) { >> - dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1); >> + /* >> + * frame_number is set from XferNotReady and may be already >> + * out of date. DSTS only provides the lower 14 bit of the >> + * current frame number. So add the upper two bits of >> + * frame_number and handle a possible rollover. >> + * This will provide the correct frame_number unless more than >> + * rollover has happened since XferNotReady. >> + */ >> + u32 frame = __dwc3_gadget_get_frame(dwc); >> + bool rollover = frame < (dep->frame_number & 0x3fff); >> + >> + dep->frame_number = (dep->frame_number & ~0x3fff) | frame; >> + if (rollover) >> + dep->frame_number += BIT(14); >> + >> + dep->frame_number = DWC3_ALIGN_FRAME(dep, 1); This is not enough, we have to add i into the alignment to ensure that the stream is not failing: dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 2); I am even thiking to move the frame number calculation into the DWC3_DEPCMD_STARTTRANSFER code path of dwc3_send_gadget_ep_cmd. But this will break the dwc3_gadget_start_isoc_quirk function. What do you think? >> >> - ret = __dwc3_gadget_kick_transfer(dep); >> + ret = __dwc3_gadget_kick_transfer(dep, >> + i + 1 < DWC3_ISOC_MAX_RETRIES); >> if (ret != -EAGAIN) >> break; >> } >> @@ -1567,7 +1580,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) >> } >> } >> >> - return __dwc3_gadget_kick_transfer(dep); >> + return __dwc3_gadget_kick_transfer(dep, false); >> } >> >> static int dwc3_gadget_ep_queue(struct usb_ep *ep, struct usb_request *request, >> @@ -2719,7 +2732,7 @@ static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep, >> if (status == -EXDEV && list_empty(&dep->started_list)) >> dwc3_stop_active_transfer(dep, true, true); >> else if (dwc3_gadget_ep_should_continue(dep)) >> - if (__dwc3_gadget_kick_transfer(dep) == 0) >> + if (__dwc3_gadget_kick_transfer(dep, false) == 0) >> no_started_trb = false; >> >> out: >> @@ -2904,7 +2917,7 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc, >> dwc3_gadget_ep_cleanup_cancelled_requests(dep); >> if ((dep->flags & DWC3_EP_DELAY_START) && >> !usb_endpoint_xfer_isoc(dep->endpoint.desc)) >> - __dwc3_gadget_kick_transfer(dep); >> + __dwc3_gadget_kick_transfer(dep, false); >> >> dep->flags &= ~DWC3_EP_DELAY_START; >> } Thanks, Michael -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc3: gadget: make starting isoc transfers more robust 2020-06-25 11:44 ` Michael Grzeschik @ 2020-06-25 19:35 ` Thinh Nguyen 2020-06-25 20:14 ` Thinh Nguyen 0 siblings, 1 reply; 12+ messages in thread From: Thinh Nguyen @ 2020-06-25 19:35 UTC (permalink / raw) To: Michael Grzeschik, Thinh Nguyen Cc: linux-usb, balbi, gregkh, kernel, Michael Olbrich Hi, Michael Grzeschik wrote: > Hi! > > On Wed, Jun 24, 2020 at 07:26:42PM +0000, Thinh Nguyen wrote: >> Michael Grzeschik wrote: >>> From: Michael Olbrich <m.olbrich@pengutronix.de> >>> >>> Currently __dwc3_gadget_start_isoc must be called very shortly after >>> XferNotReady. Otherwise the frame number is outdated and start transfer >>> will fail, even with several retries. >> >> Did you try with the recent update for isoc? (e.i., after 5 >> START_TRANSFER failures, the driver will issue END_TRANSFER to retry on >> the next XferNotReady event) >> >> Let me know if you still run into issues with that. > > Yes. Without my patch I still see the issue. Event with the retry > machanism. It is even worse. I even missed one additional patch, > I had on top this one. See my note below. Ok. Can you clarify what issue you're seeing? > >>> DSTS provides the lower 14 bit of the frame number. Use it in >>> combination >>> with the frame number provided by XferNotReady to guess the current >>> frame >>> number. This will succeed unless more than one 14 rollover has happened >>> since XferNotReady. >>> >>> Start transfer might still fail if the frame number is increased >>> immediately after DSTS is read. So retries are still needed. >>> Don't drop the current request if this happens. This way it is not >>> lost and >>> can be used immediately to try again with the next frame number. >>> >>> With this change, __dwc3_gadget_start_isoc is still not successfully >>> in all >>> cases bit it increases the acceptable delay after XferNotReady >>> significantly. >>> >>> Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de> >>> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> >>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> >>> >>> --- >>> v1 -> v2: - removed last_frame_number from struct >>> - included rollover variable >>> >>> drivers/usb/dwc3/gadget.c | 37 +++++++++++++++++++++++++------------ >>> 1 file changed, 25 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>> index 421a0f73110a40b..0962ddd7fbf6ae6 100644 >>> --- a/drivers/usb/dwc3/gadget.c >>> +++ b/drivers/usb/dwc3/gadget.c >>> @@ -1276,7 +1276,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep >>> *dep) >>> >>> static void dwc3_gadget_ep_cleanup_cancelled_requests(struct >>> dwc3_ep *dep); >>> >>> -static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) >>> +static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep, bool >>> keep_req) >> >> Any reason to have keep_req? With the recent changes, if >> dwc3_send_gadget_ep_cmd() returns -EAGAIN, then the controller driver >> won't give back the request. > > Yes, we don't need the keep_req. I tried this and it worked without. > I will remove the parameter in v3. > >>> { >>> struct dwc3_gadget_ep_cmd_params params; >>> struct dwc3_request *req; >>> @@ -1314,12 +1314,9 @@ static int __dwc3_gadget_kick_transfer(struct >>> dwc3_ep *dep) >>> } >>> >>> ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); >>> - if (ret < 0) { >>> + if (ret < 0 && (!keep_req || ret != -EAGAIN)) { >>> struct dwc3_request *tmp; >>> >>> - if (ret == -EAGAIN) >>> - return ret; >>> - >>> dwc3_stop_active_transfer(dep, true, true); >>> >>> list_for_each_entry_safe(req, tmp, &dep->started_list, list) >>> @@ -1335,7 +1332,7 @@ static int __dwc3_gadget_kick_transfer(struct >>> dwc3_ep *dep) >>> if (dep->stream_capable && req->request.is_last) >>> dep->flags |= DWC3_EP_WAIT_TRANSFER_COMPLETE; >>> >>> - return 0; >>> + return ret; >>> } >>> >>> static int __dwc3_gadget_get_frame(struct dwc3 *dwc) >>> @@ -1458,7 +1455,7 @@ static int dwc3_gadget_start_isoc_quirk(struct >>> dwc3_ep *dep) >>> dep->start_cmd_status = 0; >>> dep->combo_num = 0; >>> >>> - return __dwc3_gadget_kick_transfer(dep); >>> + return __dwc3_gadget_kick_transfer(dep, false); >>> } >>> >>> static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep) >>> @@ -1481,9 +1478,25 @@ static int __dwc3_gadget_start_isoc(struct >>> dwc3_ep *dep) >>> } >>> >>> for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) { >>> - dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1); >>> + /* >>> + * frame_number is set from XferNotReady and may be already >>> + * out of date. DSTS only provides the lower 14 bit of the >>> + * current frame number. So add the upper two bits of >>> + * frame_number and handle a possible rollover. >>> + * This will provide the correct frame_number unless more than >>> + * rollover has happened since XferNotReady. >>> + */ >>> + u32 frame = __dwc3_gadget_get_frame(dwc); >>> + bool rollover = frame < (dep->frame_number & 0x3fff); >>> + >>> + dep->frame_number = (dep->frame_number & ~0x3fff) | frame; >>> + if (rollover) >>> + dep->frame_number += BIT(14); >>> + >>> + dep->frame_number = DWC3_ALIGN_FRAME(dep, 1); > > This is not enough, we have to add i into the alignment to ensure > that the stream is not failing: > > dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 2); > > I am even thiking to move the frame number calculation into the > DWC3_DEPCMD_STARTTRANSFER code path of dwc3_send_gadget_ep_cmd. But this > will break the dwc3_gadget_start_isoc_quirk function. What do you think? You shouldn't be keep calling __dwc3_gadget_get_frame() in a loop. You should do all these calculation to get the current frame_number only once before entering the retry loop. The issue here is we don't know when the XferNotReady event will be handled, which may be too late and multiple uframe had gone by. But once the event is being handled, it shouldn't take much time at all. That means __dwc3_gadget_get_frame() will return the same value. So, we need something like this: diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index f66ff7fd87a9..1cd73c2dbe71 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -1709,6 +1709,8 @@ 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; + u32 current_frame; + bool rollover; int ret; int i; @@ -1725,6 +1727,13 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep) return dwc3_gadget_start_isoc_quirk(dep); } + current_frame = __dwc3_gadget_get_frame(dwc); + rollover = current_frame < (dep->frame_number & 0x3fff); + + dep->frame_number = (dep->frame_number & ~0x3fff) | current_frame; + if (rollover) + dep->frame_number += BIT(14); + for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) { dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1); (Please create a macro for 0x3fff mask) Thanks, Thinh ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc3: gadget: make starting isoc transfers more robust 2020-06-25 19:35 ` Thinh Nguyen @ 2020-06-25 20:14 ` Thinh Nguyen 2020-06-25 20:20 ` Thinh Nguyen 0 siblings, 1 reply; 12+ messages in thread From: Thinh Nguyen @ 2020-06-25 20:14 UTC (permalink / raw) To: Michael Grzeschik; +Cc: linux-usb, balbi, gregkh, kernel, Michael Olbrich Thinh Nguyen wrote: > Hi, > > Michael Grzeschik wrote: >> Hi! >> >> On Wed, Jun 24, 2020 at 07:26:42PM +0000, Thinh Nguyen wrote: >>> Michael Grzeschik wrote: >>>> From: Michael Olbrich <m.olbrich@pengutronix.de> >>>> >>>> Currently __dwc3_gadget_start_isoc must be called very shortly after >>>> XferNotReady. Otherwise the frame number is outdated and start transfer >>>> will fail, even with several retries. >>> Did you try with the recent update for isoc? (e.i., after 5 >>> START_TRANSFER failures, the driver will issue END_TRANSFER to retry on >>> the next XferNotReady event) >>> >>> Let me know if you still run into issues with that. >> Yes. Without my patch I still see the issue. Event with the retry >> machanism. It is even worse. I even missed one additional patch, >> I had on top this one. See my note below. > Ok. Can you clarify what issue you're seeing? > >>>> DSTS provides the lower 14 bit of the frame number. Use it in >>>> combination >>>> with the frame number provided by XferNotReady to guess the current >>>> frame >>>> number. This will succeed unless more than one 14 rollover has happened >>>> since XferNotReady. >>>> >>>> Start transfer might still fail if the frame number is increased >>>> immediately after DSTS is read. So retries are still needed. >>>> Don't drop the current request if this happens. This way it is not >>>> lost and >>>> can be used immediately to try again with the next frame number. >>>> >>>> With this change, __dwc3_gadget_start_isoc is still not successfully >>>> in all >>>> cases bit it increases the acceptable delay after XferNotReady >>>> significantly. >>>> >>>> Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de> >>>> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> >>>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> >>>> >>>> --- >>>> v1 -> v2: - removed last_frame_number from struct >>>> - included rollover variable >>>> >>>> drivers/usb/dwc3/gadget.c | 37 +++++++++++++++++++++++++------------ >>>> 1 file changed, 25 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>>> index 421a0f73110a40b..0962ddd7fbf6ae6 100644 >>>> --- a/drivers/usb/dwc3/gadget.c >>>> +++ b/drivers/usb/dwc3/gadget.c >>>> @@ -1276,7 +1276,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep >>>> *dep) >>>> >>>> static void dwc3_gadget_ep_cleanup_cancelled_requests(struct >>>> dwc3_ep *dep); >>>> >>>> -static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) >>>> +static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep, bool >>>> keep_req) >>> Any reason to have keep_req? With the recent changes, if >>> dwc3_send_gadget_ep_cmd() returns -EAGAIN, then the controller driver >>> won't give back the request. >> Yes, we don't need the keep_req. I tried this and it worked without. >> I will remove the parameter in v3. >> >>>> { >>>> struct dwc3_gadget_ep_cmd_params params; >>>> struct dwc3_request *req; >>>> @@ -1314,12 +1314,9 @@ static int __dwc3_gadget_kick_transfer(struct >>>> dwc3_ep *dep) >>>> } >>>> >>>> ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); >>>> - if (ret < 0) { >>>> + if (ret < 0 && (!keep_req || ret != -EAGAIN)) { >>>> struct dwc3_request *tmp; >>>> >>>> - if (ret == -EAGAIN) >>>> - return ret; >>>> - >>>> dwc3_stop_active_transfer(dep, true, true); >>>> >>>> list_for_each_entry_safe(req, tmp, &dep->started_list, list) >>>> @@ -1335,7 +1332,7 @@ static int __dwc3_gadget_kick_transfer(struct >>>> dwc3_ep *dep) >>>> if (dep->stream_capable && req->request.is_last) >>>> dep->flags |= DWC3_EP_WAIT_TRANSFER_COMPLETE; >>>> >>>> - return 0; >>>> + return ret; >>>> } >>>> >>>> static int __dwc3_gadget_get_frame(struct dwc3 *dwc) >>>> @@ -1458,7 +1455,7 @@ static int dwc3_gadget_start_isoc_quirk(struct >>>> dwc3_ep *dep) >>>> dep->start_cmd_status = 0; >>>> dep->combo_num = 0; >>>> >>>> - return __dwc3_gadget_kick_transfer(dep); >>>> + return __dwc3_gadget_kick_transfer(dep, false); >>>> } >>>> >>>> static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep) >>>> @@ -1481,9 +1478,25 @@ static int __dwc3_gadget_start_isoc(struct >>>> dwc3_ep *dep) >>>> } >>>> >>>> for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) { >>>> - dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1); >>>> + /* >>>> + * frame_number is set from XferNotReady and may be already >>>> + * out of date. DSTS only provides the lower 14 bit of the >>>> + * current frame number. So add the upper two bits of >>>> + * frame_number and handle a possible rollover. >>>> + * This will provide the correct frame_number unless more than >>>> + * rollover has happened since XferNotReady. >>>> + */ >>>> + u32 frame = __dwc3_gadget_get_frame(dwc); >>>> + bool rollover = frame < (dep->frame_number & 0x3fff); >>>> + >>>> + dep->frame_number = (dep->frame_number & ~0x3fff) | frame; >>>> + if (rollover) >>>> + dep->frame_number += BIT(14); >>>> + >>>> + dep->frame_number = DWC3_ALIGN_FRAME(dep, 1); >> This is not enough, we have to add i into the alignment to ensure >> that the stream is not failing: >> >> dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 2); >> >> I am even thiking to move the frame number calculation into the >> DWC3_DEPCMD_STARTTRANSFER code path of dwc3_send_gadget_ep_cmd. But this >> will break the dwc3_gadget_start_isoc_quirk function. What do you think? > You shouldn't be keep calling __dwc3_gadget_get_frame() in a loop. You > should do all these calculation to get the current frame_number only > once before entering the retry loop. > > The issue here is we don't know when the XferNotReady event will be > handled, which may be too late and multiple uframe had gone by. But once > the event is being handled, it shouldn't take much time at all. That > means __dwc3_gadget_get_frame() will return the same value. > > So, we need something like this: > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index f66ff7fd87a9..1cd73c2dbe71 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -1709,6 +1709,8 @@ 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; > + u32 current_frame; > + bool rollover; > int ret; > int i; > > @@ -1725,6 +1727,13 @@ static int __dwc3_gadget_start_isoc(struct > dwc3_ep *dep) > return dwc3_gadget_start_isoc_quirk(dep); > } > > + current_frame = __dwc3_gadget_get_frame(dwc); > + rollover = current_frame < (dep->frame_number & 0x3fff); > + > + dep->frame_number = (dep->frame_number & ~0x3fff) | current_frame; > + if (rollover) > + dep->frame_number += BIT(14); > + > for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) { > dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1); > > > (Please create a macro for 0x3fff mask) > Forgot a couple of notes: 1) If bInterval is greater than 14, don't attempt to get current frame from DSTS and ignore this mechanism. 2) The rollover check needs to account for number of uframes per interval. If the difference is less than an interval length, then no need to update dep->frame_number. BR, Thinh ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc3: gadget: make starting isoc transfers more robust 2020-06-25 20:14 ` Thinh Nguyen @ 2020-06-25 20:20 ` Thinh Nguyen 2020-06-29 13:12 ` Michael Grzeschik 0 siblings, 1 reply; 12+ messages in thread From: Thinh Nguyen @ 2020-06-25 20:20 UTC (permalink / raw) To: Michael Grzeschik; +Cc: linux-usb, balbi, gregkh, kernel, Michael Olbrich Thinh Nguyen wrote: > Thinh Nguyen wrote: >> Hi, >> >> Michael Grzeschik wrote: >>> Hi! >>> >>> On Wed, Jun 24, 2020 at 07:26:42PM +0000, Thinh Nguyen wrote: >>>> Michael Grzeschik wrote: >>>>> From: Michael Olbrich <m.olbrich@pengutronix.de> >>>>> >>>>> Currently __dwc3_gadget_start_isoc must be called very shortly after >>>>> XferNotReady. Otherwise the frame number is outdated and start transfer >>>>> will fail, even with several retries. >>>> Did you try with the recent update for isoc? (e.i., after 5 >>>> START_TRANSFER failures, the driver will issue END_TRANSFER to retry on >>>> the next XferNotReady event) >>>> >>>> Let me know if you still run into issues with that. >>> Yes. Without my patch I still see the issue. Event with the retry >>> machanism. It is even worse. I even missed one additional patch, >>> I had on top this one. See my note below. >> Ok. Can you clarify what issue you're seeing? >> >>>>> DSTS provides the lower 14 bit of the frame number. Use it in >>>>> combination >>>>> with the frame number provided by XferNotReady to guess the current >>>>> frame >>>>> number. This will succeed unless more than one 14 rollover has happened >>>>> since XferNotReady. >>>>> >>>>> Start transfer might still fail if the frame number is increased >>>>> immediately after DSTS is read. So retries are still needed. >>>>> Don't drop the current request if this happens. This way it is not >>>>> lost and >>>>> can be used immediately to try again with the next frame number. >>>>> >>>>> With this change, __dwc3_gadget_start_isoc is still not successfully >>>>> in all >>>>> cases bit it increases the acceptable delay after XferNotReady >>>>> significantly. >>>>> >>>>> Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de> >>>>> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> >>>>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> >>>>> >>>>> --- >>>>> v1 -> v2: - removed last_frame_number from struct >>>>> - included rollover variable >>>>> >>>>> drivers/usb/dwc3/gadget.c | 37 +++++++++++++++++++++++++------------ >>>>> 1 file changed, 25 insertions(+), 12 deletions(-) >>>>> >>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>>>> index 421a0f73110a40b..0962ddd7fbf6ae6 100644 >>>>> --- a/drivers/usb/dwc3/gadget.c >>>>> +++ b/drivers/usb/dwc3/gadget.c >>>>> @@ -1276,7 +1276,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep >>>>> *dep) >>>>> >>>>> static void dwc3_gadget_ep_cleanup_cancelled_requests(struct >>>>> dwc3_ep *dep); >>>>> >>>>> -static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) >>>>> +static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep, bool >>>>> keep_req) >>>> Any reason to have keep_req? With the recent changes, if >>>> dwc3_send_gadget_ep_cmd() returns -EAGAIN, then the controller driver >>>> won't give back the request. >>> Yes, we don't need the keep_req. I tried this and it worked without. >>> I will remove the parameter in v3. >>> >>>>> { >>>>> struct dwc3_gadget_ep_cmd_params params; >>>>> struct dwc3_request *req; >>>>> @@ -1314,12 +1314,9 @@ static int __dwc3_gadget_kick_transfer(struct >>>>> dwc3_ep *dep) >>>>> } >>>>> >>>>> ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); >>>>> - if (ret < 0) { >>>>> + if (ret < 0 && (!keep_req || ret != -EAGAIN)) { >>>>> struct dwc3_request *tmp; >>>>> >>>>> - if (ret == -EAGAIN) >>>>> - return ret; >>>>> - >>>>> dwc3_stop_active_transfer(dep, true, true); >>>>> >>>>> list_for_each_entry_safe(req, tmp, &dep->started_list, list) >>>>> @@ -1335,7 +1332,7 @@ static int __dwc3_gadget_kick_transfer(struct >>>>> dwc3_ep *dep) >>>>> if (dep->stream_capable && req->request.is_last) >>>>> dep->flags |= DWC3_EP_WAIT_TRANSFER_COMPLETE; >>>>> >>>>> - return 0; >>>>> + return ret; >>>>> } >>>>> >>>>> static int __dwc3_gadget_get_frame(struct dwc3 *dwc) >>>>> @@ -1458,7 +1455,7 @@ static int dwc3_gadget_start_isoc_quirk(struct >>>>> dwc3_ep *dep) >>>>> dep->start_cmd_status = 0; >>>>> dep->combo_num = 0; >>>>> >>>>> - return __dwc3_gadget_kick_transfer(dep); >>>>> + return __dwc3_gadget_kick_transfer(dep, false); >>>>> } >>>>> >>>>> static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep) >>>>> @@ -1481,9 +1478,25 @@ static int __dwc3_gadget_start_isoc(struct >>>>> dwc3_ep *dep) >>>>> } >>>>> >>>>> for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) { >>>>> - dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1); >>>>> + /* >>>>> + * frame_number is set from XferNotReady and may be already >>>>> + * out of date. DSTS only provides the lower 14 bit of the >>>>> + * current frame number. So add the upper two bits of >>>>> + * frame_number and handle a possible rollover. >>>>> + * This will provide the correct frame_number unless more than >>>>> + * rollover has happened since XferNotReady. >>>>> + */ >>>>> + u32 frame = __dwc3_gadget_get_frame(dwc); >>>>> + bool rollover = frame < (dep->frame_number & 0x3fff); >>>>> + >>>>> + dep->frame_number = (dep->frame_number & ~0x3fff) | frame; >>>>> + if (rollover) >>>>> + dep->frame_number += BIT(14); >>>>> + >>>>> + dep->frame_number = DWC3_ALIGN_FRAME(dep, 1); >>> This is not enough, we have to add i into the alignment to ensure >>> that the stream is not failing: >>> >>> dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 2); >>> >>> I am even thiking to move the frame number calculation into the >>> DWC3_DEPCMD_STARTTRANSFER code path of dwc3_send_gadget_ep_cmd. But this >>> will break the dwc3_gadget_start_isoc_quirk function. What do you think? >> You shouldn't be keep calling __dwc3_gadget_get_frame() in a loop. You >> should do all these calculation to get the current frame_number only >> once before entering the retry loop. >> >> The issue here is we don't know when the XferNotReady event will be >> handled, which may be too late and multiple uframe had gone by. But once >> the event is being handled, it shouldn't take much time at all. That >> means __dwc3_gadget_get_frame() will return the same value. >> >> So, we need something like this: >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index f66ff7fd87a9..1cd73c2dbe71 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -1709,6 +1709,8 @@ 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; >> + u32 current_frame; >> + bool rollover; >> int ret; >> int i; >> >> @@ -1725,6 +1727,13 @@ static int __dwc3_gadget_start_isoc(struct >> dwc3_ep *dep) >> return dwc3_gadget_start_isoc_quirk(dep); >> } >> >> + current_frame = __dwc3_gadget_get_frame(dwc); >> + rollover = current_frame < (dep->frame_number & 0x3fff); >> + >> + dep->frame_number = (dep->frame_number & ~0x3fff) | current_frame; >> + if (rollover) >> + dep->frame_number += BIT(14); >> + >> for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) { >> dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1); >> >> >> (Please create a macro for 0x3fff mask) >> > Forgot a couple of notes: > > 1) If bInterval is greater than 14, don't attempt to get current frame > from DSTS and ignore this mechanism. > 2) The rollover check needs to account for number of uframes per > interval. If the difference is less than an interval length, then no > need to update dep->frame_number. > Ignore number 2), DWC3_ALIGN_FRAME() should take care of that... BR, Thinh ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc3: gadget: make starting isoc transfers more robust 2020-06-25 20:20 ` Thinh Nguyen @ 2020-06-29 13:12 ` Michael Grzeschik 0 siblings, 0 replies; 12+ messages in thread From: Michael Grzeschik @ 2020-06-29 13:12 UTC (permalink / raw) To: Thinh Nguyen; +Cc: linux-usb, balbi, gregkh, kernel, Michael Olbrich [-- Attachment #1: Type: text/plain, Size: 9217 bytes --] On Thu, Jun 25, 2020 at 08:20:26PM +0000, Thinh Nguyen wrote: >Thinh Nguyen wrote: >> Thinh Nguyen wrote: >>> Hi, >>> >>> Michael Grzeschik wrote: >>>> Hi! >>>> >>>> On Wed, Jun 24, 2020 at 07:26:42PM +0000, Thinh Nguyen wrote: >>>>> Michael Grzeschik wrote: >>>>>> From: Michael Olbrich <m.olbrich@pengutronix.de> >>>>>> >>>>>> Currently __dwc3_gadget_start_isoc must be called very shortly after >>>>>> XferNotReady. Otherwise the frame number is outdated and start transfer >>>>>> will fail, even with several retries. >>>>> Did you try with the recent update for isoc? (e.i., after 5 >>>>> START_TRANSFER failures, the driver will issue END_TRANSFER to retry on >>>>> the next XferNotReady event) >>>>> >>>>> Let me know if you still run into issues with that. >>>> Yes. Without my patch I still see the issue. Event with the retry >>>> machanism. It is even worse. I even missed one additional patch, >>>> I had on top this one. See my note below. >>> Ok. Can you clarify what issue you're seeing? Yes. There are many missed xfer events, flooding the uvc gadget function driver. >>>>>> DSTS provides the lower 14 bit of the frame number. Use it in >>>>>> combination >>>>>> with the frame number provided by XferNotReady to guess the current >>>>>> frame >>>>>> number. This will succeed unless more than one 14 rollover has happened >>>>>> since XferNotReady. >>>>>> >>>>>> Start transfer might still fail if the frame number is increased >>>>>> immediately after DSTS is read. So retries are still needed. >>>>>> Don't drop the current request if this happens. This way it is not >>>>>> lost and >>>>>> can be used immediately to try again with the next frame number. >>>>>> >>>>>> With this change, __dwc3_gadget_start_isoc is still not successfully >>>>>> in all >>>>>> cases bit it increases the acceptable delay after XferNotReady >>>>>> significantly. >>>>>> >>>>>> Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de> >>>>>> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> >>>>>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> >>>>>> >>>>>> --- >>>>>> v1 -> v2: - removed last_frame_number from struct >>>>>> - included rollover variable >>>>>> >>>>>> drivers/usb/dwc3/gadget.c | 37 +++++++++++++++++++++++++------------ >>>>>> 1 file changed, 25 insertions(+), 12 deletions(-) >>>>>> >>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>>>>> index 421a0f73110a40b..0962ddd7fbf6ae6 100644 >>>>>> --- a/drivers/usb/dwc3/gadget.c >>>>>> +++ b/drivers/usb/dwc3/gadget.c >>>>>> @@ -1276,7 +1276,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep >>>>>> *dep) >>>>>> >>>>>> static void dwc3_gadget_ep_cleanup_cancelled_requests(struct >>>>>> dwc3_ep *dep); >>>>>> >>>>>> -static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) >>>>>> +static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep, bool >>>>>> keep_req) >>>>> Any reason to have keep_req? With the recent changes, if >>>>> dwc3_send_gadget_ep_cmd() returns -EAGAIN, then the controller driver >>>>> won't give back the request. >>>> Yes, we don't need the keep_req. I tried this and it worked without. >>>> I will remove the parameter in v3. >>>> >>>>>> { >>>>>> struct dwc3_gadget_ep_cmd_params params; >>>>>> struct dwc3_request *req; >>>>>> @@ -1314,12 +1314,9 @@ static int __dwc3_gadget_kick_transfer(struct >>>>>> dwc3_ep *dep) >>>>>> } >>>>>> >>>>>> ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); >>>>>> - if (ret < 0) { >>>>>> + if (ret < 0 && (!keep_req || ret != -EAGAIN)) { >>>>>> struct dwc3_request *tmp; >>>>>> >>>>>> - if (ret == -EAGAIN) >>>>>> - return ret; >>>>>> - >>>>>> dwc3_stop_active_transfer(dep, true, true); >>>>>> >>>>>> list_for_each_entry_safe(req, tmp, &dep->started_list, list) >>>>>> @@ -1335,7 +1332,7 @@ static int __dwc3_gadget_kick_transfer(struct >>>>>> dwc3_ep *dep) >>>>>> if (dep->stream_capable && req->request.is_last) >>>>>> dep->flags |= DWC3_EP_WAIT_TRANSFER_COMPLETE; >>>>>> >>>>>> - return 0; >>>>>> + return ret; >>>>>> } >>>>>> >>>>>> static int __dwc3_gadget_get_frame(struct dwc3 *dwc) >>>>>> @@ -1458,7 +1455,7 @@ static int dwc3_gadget_start_isoc_quirk(struct >>>>>> dwc3_ep *dep) >>>>>> dep->start_cmd_status = 0; >>>>>> dep->combo_num = 0; >>>>>> >>>>>> - return __dwc3_gadget_kick_transfer(dep); >>>>>> + return __dwc3_gadget_kick_transfer(dep, false); >>>>>> } >>>>>> >>>>>> static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep) >>>>>> @@ -1481,9 +1478,25 @@ static int __dwc3_gadget_start_isoc(struct >>>>>> dwc3_ep *dep) >>>>>> } >>>>>> >>>>>> for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) { >>>>>> - dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1); >>>>>> + /* >>>>>> + * frame_number is set from XferNotReady and may be already >>>>>> + * out of date. DSTS only provides the lower 14 bit of the >>>>>> + * current frame number. So add the upper two bits of >>>>>> + * frame_number and handle a possible rollover. >>>>>> + * This will provide the correct frame_number unless more than >>>>>> + * rollover has happened since XferNotReady. >>>>>> + */ >>>>>> + u32 frame = __dwc3_gadget_get_frame(dwc); >>>>>> + bool rollover = frame < (dep->frame_number & 0x3fff); >>>>>> + >>>>>> + dep->frame_number = (dep->frame_number & ~0x3fff) | frame; >>>>>> + if (rollover) >>>>>> + dep->frame_number += BIT(14); >>>>>> + >>>>>> + dep->frame_number = DWC3_ALIGN_FRAME(dep, 1); >>>> This is not enough, we have to add i into the alignment to ensure >>>> that the stream is not failing: >>>> >>>> dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 2); >>>> >>>> I am even thiking to move the frame number calculation into the >>>> DWC3_DEPCMD_STARTTRANSFER code path of dwc3_send_gadget_ep_cmd. But this >>>> will break the dwc3_gadget_start_isoc_quirk function. What do you think? >>> You shouldn't be keep calling __dwc3_gadget_get_frame() in a loop. You >>> should do all these calculation to get the current frame_number only >>> once before entering the retry loop. >>> >>> The issue here is we don't know when the XferNotReady event will be >>> handled, which may be too late and multiple uframe had gone by. But once >>> the event is being handled, it shouldn't take much time at all. That >>> means __dwc3_gadget_get_frame() will return the same value. >>> >>> So, we need something like this: >>> >>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>> index f66ff7fd87a9..1cd73c2dbe71 100644 >>> --- a/drivers/usb/dwc3/gadget.c >>> +++ b/drivers/usb/dwc3/gadget.c >>> @@ -1709,6 +1709,8 @@ 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; >>> + u32 current_frame; >>> + bool rollover; >>> int ret; >>> int i; >>> >>> @@ -1725,6 +1727,13 @@ static int __dwc3_gadget_start_isoc(struct >>> dwc3_ep *dep) >>> return dwc3_gadget_start_isoc_quirk(dep); >>> } >>> >>> + current_frame = __dwc3_gadget_get_frame(dwc); >>> + rollover = current_frame < (dep->frame_number & 0x3fff); >>> + >>> + dep->frame_number = (dep->frame_number & ~0x3fff) | current_frame; >>> + if (rollover) >>> + dep->frame_number += BIT(14); >>> + >>> for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) { >>> dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1); >>> >>> >>> (Please create a macro for 0x3fff mask) >>> >> Forgot a couple of notes: >> >> 1) If bInterval is greater than 14, don't attempt to get current frame >> from DSTS and ignore this mechanism. >> 2) The rollover check needs to account for number of uframes per >> interval. If the difference is less than an interval length, then no >> need to update dep->frame_number. >> > >Ignore number 2), DWC3_ALIGN_FRAME() should take care of that... All right, I added the following changes to v3 before running the retry loop. if (desc->bInterval <= 14) { u32 current_frame = __dwc3_gadget_get_frame(dwc); bool rollover = current_frame < (dep->frame_number & 0x3fff); dep->frame_number = (dep->frame_number & ~0x3fff) | current_frame; if (rollover) dep->frame_number += BIT(14); dep->frame_number += BIT(14); } Regards, Michael -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 2/2] usb: dwc3: gadget: when the started list is empty stop the active xfer 2020-06-24 14:49 [PATCH v2 0/2] usb: dwc3: gadget: improve isoc handling Michael Grzeschik 2020-06-24 14:49 ` [PATCH v2 1/2] usb: dwc3: gadget: make starting isoc transfers more robust Michael Grzeschik @ 2020-06-24 14:49 ` Michael Grzeschik 2020-06-24 19:43 ` Thinh Nguyen 1 sibling, 1 reply; 12+ messages in thread From: Michael Grzeschik @ 2020-06-24 14:49 UTC (permalink / raw) To: linux-usb; +Cc: Thinh.Nguyen, balbi, gregkh, kernel When we have nothing left to be queued after handling the last trb we have to stop the current transfer. This way we can ensure that the next request will be queued with an new and valid timestamp and will not directly run into an missed xfer. Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> --- v1 -> v2: - This Patch replaces the following patch by Michael Olbrich: usb: dwc3: gadget: restart the transfer if a isoc request is queued too late drivers/usb/dwc3/gadget.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 0962ddd7fbf6ae6..b2b8b911ac79b39 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2729,7 +2729,10 @@ static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep, if (dep->flags & DWC3_EP_END_TRANSFER_PENDING) goto out; - if (status == -EXDEV && list_empty(&dep->started_list)) + if ((status == -EXDEV && list_empty(&dep->started_list)) || + (usb_endpoint_xfer_isoc(dep->endpoint.desc) && + list_empty(&dep->started_list) && + list_empty(&dep->pending_list))) dwc3_stop_active_transfer(dep, true, true); else if (dwc3_gadget_ep_should_continue(dep)) if (__dwc3_gadget_kick_transfer(dep, false) == 0) -- 2.27.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] usb: dwc3: gadget: when the started list is empty stop the active xfer 2020-06-24 14:49 ` [PATCH v2 2/2] usb: dwc3: gadget: when the started list is empty stop the active xfer Michael Grzeschik @ 2020-06-24 19:43 ` Thinh Nguyen 2020-06-25 12:04 ` Michael Grzeschik 0 siblings, 1 reply; 12+ messages in thread From: Thinh Nguyen @ 2020-06-24 19:43 UTC (permalink / raw) To: Michael Grzeschik, linux-usb; +Cc: Thinh Nguyen, balbi, gregkh, kernel Hi, Michael Grzeschik wrote: > When we have nothing left to be queued after handling the last trb > we have to stop the current transfer. This way we can ensure that > the next request will be queued with an new and valid timestamp > and will not directly run into an missed xfer. > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > > --- > v1 -> v2: - This Patch replaces the following patch by Michael Olbrich: > usb: dwc3: gadget: restart the transfer if a isoc request is queued too late > > drivers/usb/dwc3/gadget.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 0962ddd7fbf6ae6..b2b8b911ac79b39 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -2729,7 +2729,10 @@ static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep, > if (dep->flags & DWC3_EP_END_TRANSFER_PENDING) > goto out; > > - if (status == -EXDEV && list_empty(&dep->started_list)) > + if ((status == -EXDEV && list_empty(&dep->started_list)) || > + (usb_endpoint_xfer_isoc(dep->endpoint.desc) && > + list_empty(&dep->started_list) && > + list_empty(&dep->pending_list))) The -EXDEV check is also for isoc, maybe rearrange this for easier read: if (usb_endpoint_xfer_isoc(dep->endpoint.desc) && list_empty(&dep->started_list) && (list_empty(&dep->pending_list) || status == -EXDEV)) > dwc3_stop_active_transfer(dep, true, true); > else if (dwc3_gadget_ep_should_continue(dep)) > if (__dwc3_gadget_kick_transfer(dep, false) == 0) BR, Thinh ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] usb: dwc3: gadget: when the started list is empty stop the active xfer 2020-06-24 19:43 ` Thinh Nguyen @ 2020-06-25 12:04 ` Michael Grzeschik 0 siblings, 0 replies; 12+ messages in thread From: Michael Grzeschik @ 2020-06-25 12:04 UTC (permalink / raw) To: Thinh Nguyen; +Cc: linux-usb, gregkh, kernel, balbi [-- Attachment #1: Type: text/plain, Size: 2105 bytes --] On Wed, Jun 24, 2020 at 07:43:44PM +0000, Thinh Nguyen wrote: >Hi, > >Michael Grzeschik wrote: >> When we have nothing left to be queued after handling the last trb >> we have to stop the current transfer. This way we can ensure that >> the next request will be queued with an new and valid timestamp >> and will not directly run into an missed xfer. >> >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> >> >> --- >> v1 -> v2: - This Patch replaces the following patch by Michael Olbrich: >> usb: dwc3: gadget: restart the transfer if a isoc request is queued too late >> >> drivers/usb/dwc3/gadget.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 0962ddd7fbf6ae6..b2b8b911ac79b39 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -2729,7 +2729,10 @@ static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep, >> if (dep->flags & DWC3_EP_END_TRANSFER_PENDING) >> goto out; >> >> - if (status == -EXDEV && list_empty(&dep->started_list)) >> + if ((status == -EXDEV && list_empty(&dep->started_list)) || >> + (usb_endpoint_xfer_isoc(dep->endpoint.desc) && >> + list_empty(&dep->started_list) && >> + list_empty(&dep->pending_list))) > >The -EXDEV check is also for isoc, maybe rearrange this for easier read: >if (usb_endpoint_xfer_isoc(dep->endpoint.desc) && > list_empty(&dep->started_list) && > (list_empty(&dep->pending_list) || status == -EXDEV)) Right, I will change it in v3. >> dwc3_stop_active_transfer(dep, true, true); >> else if (dwc3_gadget_ep_should_continue(dep)) >> if (__dwc3_gadget_kick_transfer(dep, false) == 0) Thanks, Michael -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-06-29 19:28 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-24 14:49 [PATCH v2 0/2] usb: dwc3: gadget: improve isoc handling Michael Grzeschik 2020-06-24 14:49 ` [PATCH v2 1/2] usb: dwc3: gadget: make starting isoc transfers more robust Michael Grzeschik 2020-06-24 19:26 ` Thinh Nguyen 2020-06-24 21:09 ` Thinh Nguyen 2020-06-25 11:44 ` Michael Grzeschik 2020-06-25 19:35 ` Thinh Nguyen 2020-06-25 20:14 ` Thinh Nguyen 2020-06-25 20:20 ` Thinh Nguyen 2020-06-29 13:12 ` Michael Grzeschik 2020-06-24 14:49 ` [PATCH v2 2/2] usb: dwc3: gadget: when the started list is empty stop the active xfer Michael Grzeschik 2020-06-24 19:43 ` Thinh Nguyen 2020-06-25 12:04 ` Michael Grzeschik
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.