On Tue, Oct 20, 2009 at 10:55:41PM -0400, Alan Stern wrote: > On Tue, 20 Oct 2009, Sarah Sharp wrote: > > > > I agree, this needs to be fixed. But it is a separate issue, so it > > > should be fixed in a separate patch. Sarah, I can work out a patch > > > that handles this first; then your IST can go on top of it. How does > > > that sound? > > > > Yes, that sounds fine. At least this code is getting looked at. :) > > There's still a problem. What should happen if an entire URB without > ISO_ASAP falls within the IST? We can't accept it -- since no siTDs > would be added to the schedule, we would have no way to recognize when > the URB had completed and to give it back. But if we fail the > submission then the stream's ending frame won't advance. Is your plan to give errors for some of the packets within an URB that fall within the IST, and schedule the others in the URB? And by "stream's ending frame", do you mean stream->next_uframe? In the situation you describe, there's another issue. If the whole URB falls within the IST, and we're not under a heavy load, the driver may be submitting one URB with one packet each time. In that case, the driver will continually see an error with each submission. That will produce the same effect as the bug would (blank video, no audio, etc). The driver should probably just be fixed in that case. > I suppose we could treat the URB as though the ISO_ASAP flag had been > set. But this doesn't seem like a very good idea. Really, we don't > have any sensible way for drivers to handle this sort of error. Maybe the completion handlers should, when they encounter an URB with all packets that have errors, read the microframe register and attempt to schedule further in the future. Or just set ISO_ASAP. > P.S.: I noticed some other bugs today. The new code for checking > whether an URB extends too far into the future should be invoked for > new streams as well as continuing ones. > > And in usb_submit_urb(), the code that checks whether urb->interval is > set correctly will reduce the value if it too high. This is fine for > interrupt URBs but it's a very bad idea for isochronous URBs. We > should fail those submissions. > > Finally, one of the things we discussed was allowing drivers to specify > urb->start_frame for new streams (to get precise synchronization of two > devices, for instance). But with xHCI, the hardware does all the > scheduling as soon as an altsetting is installed. How can you tell it, > later on when the driver submits the first URB, what the start frame > should be? The xHC reserves bandwidth for the endpoint when it is added to its internal structures (currently at device configuration; later I'll add alternate interface setting support which will modify the endpoints' information). It doesn't schedule any transactions until you ring the doorbell for an endpoint when you enqueue buffers from an URB. The xHCI driver can specify which frame an isoc transfer goes out in. It has a "Start Isoch ASAP" flag, and a frame ID field for each isoc TRB. If the ASAP flag is not set, and a frame ID is specified, then the transfer is sent out when that field matches bits 13:3 of the microframe index register. So xHCI has a 2047 frame list length internally. Sarah -- 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
