On Sun, 21 Jul 2013, Ming Lei wrote: > On Sun, Jul 21, 2013 at 2:00 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > > On Sat, 20 Jul 2013, Ming Lei wrote: > > > >> > No, we don't have to change every driver using isochronous URBs. Many > >> > of them set URB_ISO_ASAP on every URB -- in fact, AFAIK snd-usb-audio > >> > is the only one that has been fixed not to do this. > >> > >> OK, if you are sure just snd-usb-audio and very less drivers need the change, > >> using the API only is correct way. > > > > I haven't checked in detail, but this seems very likely. > > For ehci example, URB_ISO_ASAP is not checked when isoc queue is > empty, so will cause a new schedule when underrun happens. ehci-hcd is indeed one of the drivers that will need to be updated. So will ohci-hcd and uhci-hcd, when they start using tasklets. > > The new call doesn't turn streaming on or off. The driver does that > > by submitting URBs or not submitting them. > > > > The new call merely separates an old stream from a new one. It tells > > the HCD that the old stream (if any) is finished, so that any URBs > > submitted in the future will belong to a new stream. In particular, it > > tells the HCD that the next URB for this endpoint should be assigned to > > a time slot that will be visible to the hardware, instead of being > > assigned to the next time slot after the last packet of the previous > > URB (which has probably expired already). > > OK, I got it, looks usb_new_iso_stream(udev, ep) need to be called > before starting a new stream. Or after ending an old stream. Either way would work. > From view of HCD, the hcd only > knows a stream has been started, and doesn't know if it has been stopped. > And the most possible usage for hcd is that: > > if (test_and_clear_bit(ISOC_NEW_STREAM, ep->stream_flag) > ; /* new stream */ > > So looks hcd has to write the flag. Not exactly. The HCD has to keep track of the next available time slot for the endpoint. Some special value, like -1, will indicate that no stream is running yet. So the new function will merely have to set the slot value to -1. > If we define the API as usb_iso_stream(udev, ep, start), hcd only > need to read the flag, so it should be simper to use and implement. It wouldn't be simpler for drivers. And what happens if the HCD gets an URB submitted for an endpoint where the stream_flag is off? > Also with both streaming on and off information, the HCD might improve > bandwidth management in the future. I doubt it very much. The USB spec states that bandwidth for periodic endpoints gets allocated whenever a new altsetting is installed, and that is the approach the HCDs will try to take. > >> In a normal system, the URB count won't be very much(<< 1000), and if > >> you mean the overhead is from memory, I guess it won't cause real memory > >> increase per URB for slab's sake. If you mean performance loss with > >> set/get the single lockless/common variable, maybe we should focus on > >> the big HCD lock, which might be the biggest bottle of USB protocol, :-) > > > > If you're talking about the hcd_urb_list_lock, note that it could > > easily be made hcd-specific. I haven't done that because the regions > > it protects are all very short, so there probably isn't very much > > contention. > > I mean the HCD private lock, and there are heavy contention > on the lock. I don't have any ideas how to reduce that contention. Do you? > Also I thought of one improvement on the idea of 'urb->completing': > > - define one percpu 'struct urb *' variable inside 'struct hcd' to record > the completing urb, so HCD can see easily if the urb is being resubmitted > inside its completion handler. Even on a UP system, what happens if multiple URBs get completed before the tasklet runs and one of them is resubmitted? > Anyway this approach is only helpful if much lots of drivers need > changes for underrun case. Alan Stern -- 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
