Re: [PATCH] usb: musb: musb_host: Introduce postponed URB giveback

[Bin Liu]

On Thu, Apr 27, 2017 at 07:26:31PM +0300, Matwey V. Kornilov wrote:
> 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?

That is the plan, I got an A20 board, but haven't bring it up yet.

> 
> >
> > 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.

HCD_BH flag cause some issues which are docummented in the commit log of
f551e1352983.
But even with HCD_BH flag, it didn't work for uvc webcams, it still misses
IN tokens. It might helps pwc webcams though.

> > 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.

Setting REQPKT in advance might be the solution, but I'd like to
understand why only Isoch transfer shows such issue, and why only am335x
reports this issue. The later concerns me more if this would be a
system issue not only in usb module.

> 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.

For Isoch transfer, MUSB has to be programmed for every transaction, but
urb callback takes too long with spinlock held, which cause MUSB misses
issuing IN tokens.

Regards,
-Bin.