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.
>
>> >> > But never mind. For a new API, how about
>> >> >
>> >> > void usb_new_iso_stream(struct usb_device *, struct usb_host_endpoint *);
>> >> >
>> >> > Does that look okay?
>> >>
>> >> Looks you missed one parameter of 'int on'.
>> >
>> > I don't understand. Why is another parameter needed?
>>
>> So do you only want to set streaming on and not streaming off?
>> If so, that looks a bit weird, could you describe its usage?
>
> 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. 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.
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.
Also with both streaming on and off information, the HCD might improve
bandwidth management in the future.
>
> In short, it would simply tell the HCD to act as though the next URB
> has URB_ISO_ASAP set. For HCDs that act as though URB_ISO_ASAP was
> always set, the new call wouldn't do anything -- they would not need to
> implement it.
>
>> > (with extra overhead for every URB!) to fix a problem that ought to be
>>
>> 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.
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.
Anyway this approach is only helpful if much lots of drivers need
changes for underrun case.
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
