2017-04-27 18:35 GMT+03:00 Bin Liu <b-liu@xxxxxx>: > Hi Matwey, > > On Thu, Apr 27, 2017 at 01:20:33PM +0300, Matwey V. Kornilov wrote: >> This commit changes the order of actions undertaken in >> musb_advance_schedule() in order to overcome issue with broken >> isochronous transfer [1]. >> >> There is no harm to split musb_giveback into two pieces. The first >> unlinks finished urb, the second givebacks it. The issue here that >> givebacking may be quite time-consuming due to urb->complete() call. >> As it happens in case of pwc-driven web cameras. It may take about 0.5 >> ms to call __musb_giveback() that calls urb->callback() internally. >> Under specific circumstances setting MUSB_RXCSR_H_REQPKT in subsequent >> musb_start_urb() for the next urb will be too late to produce physical >> IN packet. Since auto req is not used by this module the exchange >> would be as the following: >> >> [ ] 7.220456 d= 0.000997 [182 + 3.667] [ 3] IN : 4.5 >> [ T ] 7.220459 d= 0.000003 [182 + 7.000] [800] DATA0: [skipped] >> [ ] 7.222456 d= 0.001997 [184 + 3.667] [ 3] IN : 4.5 >> [ ] 7.222459 d= 0.000003 [184 + 7.000] [ 3] DATA0: 00 00 >> >> It is known that missed IN in isochronous mode makes some >> perepherial broken. For instance, pwc-driven or uvc-driven >> web cameras. >> In order to workaround this issue we postpone calling >> urb->callback() after setting MUSB_RXCSR_H_REQPKT for the >> next urb if there is the next urb pending in queue. >> >> [1] https://www.spinics.net/lists/linux-usb/msg145747.html >> >> Fixes: f551e1352983 ("Revert "usb: musb: musb_host: Enable HCD_BH flag to handle urb return in bottom half"") >> Signed-off-by: Matwey V. Kornilov <matwey@xxxxxxxxxx> > > Thanks for the effort of working on this long standing issue, I know you > have spent alot of time on it, but what I am thinking is instead of > workaround the problem we need to understand the root cause - why > uvc-video takes longer to exec the urb callback, why only am335x > reported this issue. This is on my backlog, just seems never got time > for it... Have you tried other SoCs with Invetra MUSB? > > Ideally MUSB driver should be just using HCD_BH flag and let the core to > handle the urb callback, regardless the usb transfer types. I think the only reason why everything worked before with HCD_BH is that execution of urb->callback() was placed after musb_start(). The order of operations matters. However, you said that something was also wrong with HCD_BH and other peripherals. > > The MUSB drivers are already messy and complicated enough for > maintenance, I'd like to understand the root cause of the delay first > before decide how to solve the issue. > I feel from playing with OpenVizsla that REQPKT should be set well in advance. So, time window to set the flag is actually smaller than 1 ms. urb->callback() is never takes longer than 0.4 ms for pwc driver, but INs are skipped. At the same time musb_host doesn't utilize AutoReq here. I think many other USB host controllers (OHCI?) just rely on hardware to send IN packets in time. > Regards, > -Bin. > >> --- >> drivers/usb/musb/musb_host.c | 54 +++++++++++++++++++++++++++++++++++++------- >> 1 file changed, 46 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c >> index ac3a4952abb4..b590c2555dab 100644 >> --- a/drivers/usb/musb/musb_host.c >> +++ b/drivers/usb/musb/musb_host.c >> @@ -299,19 +299,24 @@ musb_start_urb(struct musb *musb, int is_in, struct musb_qh *qh) >> } >> } >> >> -/* Context: caller owns controller lock, IRQs are blocked */ >> -static void musb_giveback(struct musb *musb, struct urb *urb, int status) >> +static void __musb_giveback(struct musb *musb, struct urb *urb, int status) >> __releases(musb->lock) >> __acquires(musb->lock) >> { >> - trace_musb_urb_gb(musb, urb); >> - >> - usb_hcd_unlink_urb_from_ep(musb->hcd, urb); >> spin_unlock(&musb->lock); >> usb_hcd_giveback_urb(musb->hcd, urb, status); >> spin_lock(&musb->lock); >> } >> >> +/* Context: caller owns controller lock, IRQs are blocked */ >> +static void musb_giveback(struct musb *musb, struct urb *urb, int status) >> +{ >> + trace_musb_urb_gb(musb, urb); >> + >> + usb_hcd_unlink_urb_from_ep(musb->hcd, urb); >> + __musb_giveback(musb, urb, status); >> +} >> + >> /* For bulk/interrupt endpoints only */ >> static inline void musb_save_toggle(struct musb_qh *qh, int is_in, >> struct urb *urb) >> @@ -346,6 +351,7 @@ static void musb_advance_schedule(struct musb *musb, struct urb *urb, >> struct musb_hw_ep *ep = qh->hw_ep; >> int ready = qh->is_ready; >> int status; >> + int postponed_giveback = 0; >> >> status = (urb->status == -EINPROGRESS) ? 0 : urb->status; >> >> @@ -361,9 +367,35 @@ static void musb_advance_schedule(struct musb *musb, struct urb *urb, >> break; >> } >> >> - qh->is_ready = 0; >> - musb_giveback(musb, urb, status); >> - qh->is_ready = ready; >> + usb_hcd_unlink_urb_from_ep(musb->hcd, urb); >> + >> + /* It may take about 0.5 ms to call __musb_giveback() that >> + * calls urb->callback() internally. Under specific circumstances >> + * setting MUSB_RXCSR_H_REQPKT in subsequent musb_start_urb() for the >> + * next urb will be too late to produce physical IN packet. Since >> + * auto req is not used by this module the exchange would be as the >> + * following: >> + * >> + * [ ] 7.220456 d= 0.000997 [182 + 3.667] [ 3] IN : 4.5 >> + * [ T ] 7.220459 d= 0.000003 [182 + 7.000] [800] DATA0: [skipped] >> + * [ ] 7.222456 d= 0.001997 [184 + 3.667] [ 3] IN : 4.5 >> + * [ ] 7.222459 d= 0.000003 [184 + 7.000] [ 3] DATA0: 00 00 >> + * >> + * It is known that missed IN in isochronous mode makes some >> + * perepherial broken. For instance, pwc-driven or uvc-driven >> + * web cameras. >> + * In order to workaround this issue we postpone calling >> + * urb->callback() after setting MUSB_RXCSR_H_REQPKT for the >> + * next urb if there is the next urb pending in queue. >> + */ >> + if (is_in && qh->type == USB_ENDPOINT_XFER_ISOC >> + && !list_empty(&qh->hep->urb_list)) { >> + postponed_giveback = 1; >> + } else { >> + qh->is_ready = 0; >> + __musb_giveback(musb, urb, status); >> + qh->is_ready = ready; >> + } >> >> /* reclaim resources (and bandwidth) ASAP; deschedule it, and >> * invalidate qh as soon as list_empty(&hep->urb_list) >> @@ -428,6 +460,12 @@ static void musb_advance_schedule(struct musb *musb, struct urb *urb, >> hw_ep->epnum, is_in ? 'R' : 'T', next_urb(qh)); >> musb_start_urb(musb, is_in, qh); >> } >> + >> + if (postponed_giveback) { >> + qh->is_ready = 0; >> + __musb_giveback(musb, urb, status); >> + qh->is_ready = ready; >> + } >> } >> >> static u16 musb_h_flush_rxfifo(struct musb_hw_ep *hw_ep, u16 csr) >> -- >> 2.12.0 >> > -- With best regards, Matwey V. Kornilov. Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia 119234, Moscow, Universitetsky pr-k 13, +7 (495) 9392382 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
