On Sun, Jul 21, 2013 at 11:28 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > 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. I am wondering if -1 is special enough, since in theory -1 might be a valid time slot when HCD uses time slot ANDed with (mod - 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? If the API is defined as usb_iso_stream(udev, ep, start), drivers are required to call usb_iso_stream(udev, ep, 1) before starting stream, and call usb_iso_stream(udev, ep, 0) after stopping stream. > >> 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. Looks USB spec doesn't mention part of the bandwidth allocated by set_interface can't be freed. Also Clemens mentioned, there might be devices which exposes non-zero payload sizes in the default interface setting. > >> >> 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? For ehci example, one draft idea I thought about is to split the ehci->lock into three locks: ehci->periodic_lock which covers intr & isoc transfer, and ehci->async_lock which covers async schedule, and ehci->lock which manages some global thing but is held with short time. But the idea is far more mature, and might be wrong, and only for discussion because I haven't considered much details already. > >> 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? On each CPU, for one HCD, one urb->complete() won't be preempted by any other urb->complete() either running in tasklet or hardirq handler, so we can record the completing urb in one HCD percpu variable before calling urb->complete() and clear the variable after returning form urb->complete(). Then in submit path, we can find if the URB is scheduled in its own completion handler. Thanks, -- Ming Lei -- 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
