On Wed, 10 Jul 2013, Devin Heitmueller wrote: > > I bet the problem is related to the usage of the URB_ISO_ASAP flag. > > em28xx_alloc_urbs() sets URB_ISO_ASAP in urb->transfer_flags, and the > > value never gets cleared. In fact, that flag bit is supposed to be set > > only in the first URB of a stream, not in the following URBs. (The > > same mistake is present for the URBs in the audio stream.) > > Wow, REALLY? Ok, if that's the case then I will fix that and see if > it makes any difference. That really should be documented somewhere > because I've seen it done that way in a bunch of different drivers > (and in fact done it myself that way in several drivers I wrote in the > media tree). It _is_ documented, in a couple of places. It is discussed in the kerneldoc preceding the definition of struct urb in include/linux/usb.h and in the kerneldoc for usb_submit_urb() in drivers/usb/core/urb.c. However, the meaning now is different from what it used to be in the past, and it is liable to undergo a slight change in the future. Stay tuned. Basically, URB_ISO_ASAP now means that the driver wants the URB to be scheduled for the next definitely available time slot, and is willing to skip some time slots if necessary to insure this. Not setting URB_ISO_ASAP means the driver wants the URB to be scheduled for the time slot following the end of the preceding URB, even if that slot may already be expired. There is a certain ambiguity about what to do when a stream is started or restarted, because then the notion of "preceding URB" doesn't make sense. Even worse, the host controller driver can't always tell when a stream is being restarted -- that's why I suggest avoiding the whole issue by always setting URB_ISO_ASAP on the first URB of a new or restarting stream. The "definitely available" phrase above is probably the source of your trouble. A time slot that is already in the past certainly isn't available. However, a time slot that is still in the future may not be definitely available either, because the hardware requires that URBs be submitted somewhat in advance of when they will be used. Depending on the hardware, a time slot may not be "definitely" available unless it is over 1 ms in the future. I recommend setting up isochronous pipelines to be at least 2 ms long. You can use less if 2 ms is more latency than you want, but don't go under 1 ms. > Just so I'm understanding what is supposed to be the expected behavior > - so it should be set in the first URB, but what about when we > resubmit the URBs? Should I be clearing the flag prior to resubmitting > the first URB (since it will be unchanged)? Or is the expected > behavior that I set it on the first URB, and then nothing is supposed > to ever touch transfer flags on any URB from that point forward? Given that so many drivers do this wrong, it might be worthwhile for the USB core to clear the URB_ISO_ASAP flag before calling the URB's completion handler. That way drivers won't have to worry about clearing it themselves. (On the other hand, there is at least one driver that probably does want to have the flag set during resubmissions, so then it would have to be updated.) Alternatively, and for now at least, you can try clearing that flag yourself before resubmitting the URBs. > While on that topic, I'm clearing the status and actual_length fields > in all of the iso_frame_desc[] fields of the URB prior to resubmitting > - should I be doing that? You don't have to; the USB core does that for you. There's nothing _wrong_ with it; it's just a waste of time. > Are there other fields I should be > resetting? For an isochronous stream, all you really need to do for resubmission is make sure the transfer_buffer (and possibly transfer_dma), transfer_buffer_length, number_of_packets, and iso_frame_desc[i].offset and .length values are set properly. Everything else can be left as is. > Thank you so much for your help! You're welcome. 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
