On Thu, Jun 16, 2011 at 03:57:22PM -0400, Alan Stern wrote: > On Thu, 16 Jun 2011, Sarah Sharp wrote: > > > > Adding a intr_target field to the URB? Then to set the field, usb core > > > not only need to know the number of interrupters enabled, but also needs > > > to find out the most suitable interrupter. Does every urb need to > > > calculate > > > the field before being submitted to the host driver? If so, I think it's > > > another performance lost. > > > > I think drivers probably know when they switch configurations or > > alternate interface settings what latency requirements they need. A > > webcam driver needs a latency less than the periodic rate of the > > isochronous endpoint. So we could have one call to allow the driver to > > set the interrupter once before it submits any URBs. > > > > The USB core could store that value in a per-driver structure, or the > > xHCI driver could store it in xhci_virt_ep. The xHCI driver could set > > up several different endpoint rings for periodic endpoints, and > > advertise their latencies (IMODI values) to the USB core. The USB core, > > based on the latency the driver needs, could set the driver's > > interrupter target once, or maybe once per alternate interface setting > > change. So that API design might avoid any per-URB performance hit. > > Drivers and the USB core should not have to care about interrupters. > That is an internal implementation detail for xhci-hcd. Yeah, you're probably right. I'd mostly like the option of allowing drivers to pick their interrupters for the UAS driver, but probably xHCI should have enough information to figure it out. I would still like to see the patchset revised to direct different endpoint types to different event rings, if it turns out that different endpoints do benefit from different latencies. > > If the drivers don't use this new API, the xHCI driver can try to make > > an informed choice (once per endpoint during the configure endpoint > > command) to try and decide which interrupter would be best. At the very > > least, we should probably have separate event rings for different device > > speeds, and maybe two dedicated event rings for HS and SS isochronous > > traffic? > > This is all guesswork. You shouldn't spent much time on it before > making some detailed measurements. We do need a way to experiment with the IMODI values in order to find the right solution. Maybe we just need a smaller patch to set the IMODI value for the main event ring, and experiment with different devices to see if different values help or hurt performance. I think I have one laying around somewhere that I pointed Andiry to. It adds a module parameter to xHCI to allow the IMODI value to be set. > > We probably want a separate event ring just for control transfers, with > > the IMODI field set to zero. There's no sense in making the hardware > > delay control transfers. Of course, control transfers are rarely > > (perhaps never?) performance critical. > > > > Bulk endpoints are a separate issue. I think we always want to set the > > IMODI value to zero for those, because the drivers really want to get > > notified immediately when the transfers are done. Especially the BoT > > driver, that waits on each bulk transfer before submitting the rest. So > > we can probably direct control and bulk endpoints to the same event ring > > with IMODI set to zero. > > It's true that latency rarely matters for control endpoints. For all > others, however, latency should be as small as possible. If we find that all types of devices just need to have the smallest latency possible, I don't think the multiple event rings patchset is going to be very useful. Maybe they would be helpful if some device driver is spending too much time in their completion handler? Andiry, can you think of any other reason to have multiple event rings? I wonder if maybe the multiple event rings were mostly a feature for virtualization, so that a host OS could just pass down an event ring to the xHCI host controller that belonged to the guest OS. Sarah Sharp -- 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
