[alsa-devel] [PATCH 0/5] RFC for snd-usb: rework usb endpoint logic
tiwai at suse.de
Tue Nov 1 17:19:27 CET 2011
At Mon, 31 Oct 2011 13:38:20 +0100,
Daniel Mack wrote:
> On 10/31/2011 01:10 PM, Daniel Mack wrote:
> > I didn't sign-off the patches on purpose, as I would really like to
> > get them reviewed before they go in. Can people have a look and state
> > whether the whole idea is at all sane?
> To briefly state the idea as such: the new implementation defines a
> model (snd_usb_endpoint) that handles everything that is related to an
> USB endpoint and its streaming. There are functions to activate and
> deactivate an endpoint (which call usb_set_interface()), and to start
> and stop its URBs. It also has function pointers to be called when data
> was received or is about to be sent, and pointer to a sync slave
> (another snd_usb_endpoint) that is informed when data has been received.
> A snd_usb_endpoint knows about its state and implements a refcounting,
> so only the first user will actually start the URBs and only the last
> one to stop it will tear them down again.
> With this sort of abstraction, the actual streaming is decoupled from
> the pcm handling, which makes the "implicit feedback" mechanisms easy to
> implement. All the code that actually handles the payload of a stream's
> packets is now implemented in pcm.c, which is were it belongs to.
> But I'm sure there are some unresolved corner cases which need attention.
The overall design looks good to me. Nice work!
A few nitpicking:
- No need for check of activated flags at starting streams?
- Better to clear subs->data_endpoint & co at closing.
- There might be unbalances when deactivate_endpoints() isn't called
in prepare callback before activate_endpoints(). The apps may call
open -> hw_params -> prepare -> hw_params -> prepare -> trigger
trigger (stop) -> prepare -> trigger (start)
So, be careful about refcounting and active flags.
And, yes, the protection for snd_usb_add_endpoint() would be really
needed for concurrent access. The card-global mutex would be enough.
More information about the Alsa-devel