| Message ID | 1403282253300.13982@beth (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On 29/03/14 16:01, Mark Hills wrote: > I'm a bit late to this, but I think a more generic quirk is necessary. Thanks Mark, I don't know much about other devices, but I have provided some comments about my latest code: http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20140329/bab4ce5a/attachment-0001.bin > Or does some kind of spec define that these should be applied to the first > endpoint and that all will be affected? I used educated guesses to make the Mbox 1 functional: I set the altsetting of the interface based on the value provided in the first endpoint entry of the quirk, (since they must be all the same for a shared interface): + alts = &iface->altsetting[fp[0]->altset_idx]; This works for any device where the interface is shared across multiple endpoints, right? Otherwise you wouldn't use this new quirk type. After reading all the quirk data and adding streams one by one, I went on to configure the interface based on the quirk data for the first entry: + usb_set_interface(chip->dev, fp[0]->iface, 0); + snd_usb_init_pitch(chip, fp[0]->iface, alts, fp[0]); + snd_usb_init_sample_rate(chip, fp[0]->iface, alts, fp[0], fp[0]->rate_max); I assumed that the supported rates for all interfaces was the same and that they could be read from the first endpoint entry in the quirk. I know, most of the quirk data is ignored, but they share the same interface so most of it is redundant anyway, isn't it? I can't see what limitations this multi-endpoint quirk type has that might need to be adjusted for other devices. Can you provide an example of a device that uses multiple endpoints within a single interface whose supported rates differ between endpoints? If you can, then I also think we need a better model. If you can't, is it because it is impossible? Damien
On Sat, 29 Mar 2014, Damien Zammit wrote: > On 29/03/14 16:01, Mark Hills wrote: > > I'm a bit late to this, but I think a more generic quirk is necessary. > Thanks Mark, I don't know much about other devices, but I have provided > some comments about my latest code: > http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20140329/bab4ce5a/attachment-0001.bin Thanks. I took a look and I think I have a similar concern to Daniel for readability. The code should really be shared with the quirk on a single endpoint. The duplicate code in the patch already has different handling of maxpacksize and datainterval -- is that intentional? It's apparent because the new code ignores the values given in the quirk. > > Or does some kind of spec define that these should be applied to the first > > endpoint and that all will be affected? > I used educated guesses to make the Mbox 1 functional: > > I set the altsetting of the interface based on the value provided in the > first endpoint entry of the quirk, (since they must be all the same for > a shared interface): > + alts = &iface->altsetting[fp[0]->altset_idx]; > > This works for any device where the interface is shared across multiple > endpoints, right? Otherwise you wouldn't use this new quirk type. > > After reading all the quirk data and adding streams one by one, I went > on to configure the interface based on the quirk data for the first entry: > + usb_set_interface(chip->dev, fp[0]->iface, 0); > + snd_usb_init_pitch(chip, fp[0]->iface, alts, fp[0]); > + snd_usb_init_sample_rate(chip, fp[0]->iface, alts, fp[0], > fp[0]->rate_max); > > I assumed that the supported rates for all interfaces was the same and > that they could be read from the first endpoint entry in the quirk. > I know, most of the quirk data is ignored, but they share the same > interface so most of it is redundant anyway, isn't it? If the data in the quirk is truly redundant, then really it should not be present, or have the structure to use it -- it is misleading like this. The alternative is that the format is guaranteed to be identical between the endpoints on the same interface, which is what I attempted in my previous patch (however this assumption is incorrect, see below) > I can't see what limitations this multi-endpoint quirk type has that > might need to be adjusted for other devices. Can you provide an example > of a device that uses multiple endpoints within a single interface whose > supported rates differ between endpoints? Yes, I dug out a Novation Twitch here. It uses different number of channels (and hence buffer sizes) on the record and playback endpoints. At very least other Focusrite devices will have this, too. > If you can, then I also think we need a better model. If you can't, is > it because it is impossible? > > Damien > Ok so I did further experiments. One thing was to add more code to my patch that specifically sets sample_format etc. directly on the affected endpoint. It seems the assumption that an enpoint has sole ownership of the interface is quite deeply spread in the code. For example, set_format() called as part of snd_usb_pcm_prepare(). In my case the result is blocking on one audio stream when the other is opened, I didn't look into detail whether this was the driver or hardware. But I'm suprised your Mbox deals with the prepare step, perhaps it is suprisingly tolerant. Did you confirm that you could start and stop a recording during playback and that both streams were truly working? Specifically as snd_usb_pcm_close() shuts down the whole interface. Is your Mbox continuing smoothly when this happens on one of the streams? Thanks
On 30/03/14 09:57, Mark Hills wrote: > Thanks. I took a look and I think I have a similar concern to Daniel for > readability. The code should really be shared with the quirk on a single > endpoint. I guess a significant rewrite will be required. > The duplicate code in the patch already has different handling of > maxpacksize and datainterval -- is that intentional? It's apparent because > the new code ignores the values given in the quirk. Not intentional, maxpacksize and datainterval happened to be identical for both endpoints on the Mbox. > If the data in the quirk is truly redundant, then really it should not be > present, or have the structure to use it -- it is misleading like this. Yeah, I agree. I have an idea. The following is an excerpt from lsusb for the Mbox: Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber 1 bAlternateSetting 1 bNumEndpoints 2 bInterfaceClass 255 Vendor Specific Class bInterfaceSubClass 0 bInterfaceProtocol 0 iInterface 0 Endpoint Descriptor: bLength 9 bDescriptorType 5 bEndpointAddress 0x81 EP 1 IN bmAttributes 5 Transfer Type Isochronous Synch Type Asynchronous Usage Type Data wMaxPacketSize 0x0130 1x 304 bytes bInterval 1 bRefresh 0 bSynchAddress 0 Endpoint Descriptor: bLength 9 bDescriptorType 5 bEndpointAddress 0x02 EP 2 OUT bmAttributes 5 Transfer Type Isochronous Synch Type Asynchronous Usage Type Data wMaxPacketSize 0x0130 1x 304 bytes bInterval 1 bRefresh 0 bSynchAddress 0 Looking at the current struct for a snd_usb_audio_quirk: struct snd_usb_audio_quirk { const char *vendor_name; const char *product_name; int16_t ifnum; uint16_t type; const void *data; } It is quite versatile, we shouldn't need to alter the quirk struct, just the data it holds. Is an array of audioformats for this kind of interface the ideal choice? Also, maybe we can read bNumEndpoints to determine if it is a multi-endpoint interface and act accordingly, then we don't need an extra element in snd_usb_audio_quirk. How is a shared interface supposed to be initialised when it has different properties for each endpoint? > Yes, I dug out a Novation Twitch here. It uses different number of > channels (and hence buffer sizes) on the record and playback endpoints. At > very least other Focusrite devices will have this, too. Interesting. > It seems the assumption that an enpoint has sole ownership of the > interface is quite deeply spread in the code. For example, set_format() > called as part of snd_usb_pcm_prepare(). I am still trying to wrap my head around this one, but it sounds like the major stumbling block with the current code? > But I'm suprised your Mbox deals with the prepare step, perhaps it is > suprisingly tolerant. Did you confirm that you could start and stop a > recording during playback and that both streams were truly working? Yes, I can start and stop recording while headphones are blasting, and my microphone works. > Specifically as snd_usb_pcm_close() shuts down the whole interface. Is > your Mbox continuing smoothly when this happens on one of the streams? I use JACK so I'm not sure if the streams are ever closed until I shutdown the server. Damien
Hi Damien, On 03/30/2014 05:21 AM, Damien Zammit wrote: > On 30/03/14 09:57, Mark Hills wrote: >> Thanks. I took a look and I think I have a similar concern to Daniel for >> readability. The code should really be shared with the quirk on a single >> endpoint. > I guess a significant rewrite will be required. Yeah, now that you changed the code to handle an arbitrary amount of endpoints, you could as well change all user of QUIRK_AUDIO_FIXED_ENDPOINT over to the new one, and initialize .epmulti to 1. That should already work, right? (Though, I have to mention that I'm unhappy with the name of that variable :)). >> The duplicate code in the patch already has different handling of >> maxpacksize and datainterval -- is that intentional? It's apparent because >> the new code ignores the values given in the quirk. That souldn't be the case then, of course. > Not intentional, maxpacksize and datainterval happened to be identical > for both endpoints on the Mbox. So it's easy to change. > Looking at the current struct for a snd_usb_audio_quirk: > > struct snd_usb_audio_quirk { > const char *vendor_name; > const char *product_name; > int16_t ifnum; > uint16_t type; > const void *data; > } > > It is quite versatile, we shouldn't need to alter the quirk struct, just > the data it holds. A const void* can hold any kind of struct, and you need to cast it back to your struct eventually anyway. Thinking about it again, I don't like the idea of an extra member in struct snd_usb_audio_quirk either. What would be nicer is to introduce something like this: struct audioformats { unsigned int n_formats; const struct audioformat *format; }; ... and then define the quirk like this: .type = QUIRK_AUDIO_FIXED_MULTI_ENDPOINTS, .data = (const struct audioformats) { .n_formats = 2, .format = (const struct audioformat[]) { [0] = { .formats = SNDRV_PCM_FMTBIT_S24_3BE, ... }, [1] = { .formats = SNDRV_PCM_FMTBIT_S24_3BE, ... }, }, } IOW, encapsulate struct audioformat once more, so that the counter variable is specific to this kind of quirk. Also, you can write the actual quirk handler so that it loops over the array entries, so you can re-use the code for both QUIRK_AUDIO_FIXED_MULTI_ENDPOINTS and QUIRK_AUDIO_FIXED_ENDPOINT. Do you follow? :) > Is an array of audioformats for this kind of > interface the ideal choice? Not sure whether I understand what you mean. Would be best to share a patch that implements your idea. > Also, maybe we can read bNumEndpoints to determine if it is a > multi-endpoint interface and act accordingly, then we don't need an > extra element in snd_usb_audio_quirk. I think adding an array type is the best way to handle it. Thanks, Daniel
On 01/04/14 04:47, Daniel Mack wrote: > Yeah, now that you changed the code to handle an arbitrary amount of > endpoints, you could as well change all user of > QUIRK_AUDIO_FIXED_ENDPOINT over to the new one, and initialize .epmulti > to 1. That should already work, right? (Though, I have to mention that > I'm unhappy with the name of that variable :)). Given Mark's concerns, I'm not sure this will work for devices which share an interface with multiple endpoints where the endpoints have different properties. I think my code only works when the multiple endpoints have the same properties apart from the endpoint #. For example, the existing code which brings up a fixed interface assumes that there is only one endpoint and refers to it via an index which is usually hardcoded to 0. So most of the quirk would be ignored as only the data for the first mentioned endpoint would be used for certain functions. For example, functions such as get_endpoint(alts, 0) etc. This is the major stumbling block that Mark is referring to I think. What are your thoughts on this one Daniel? Have you seen Mark's untested code? It's part of this thread and seems to address this problem. > A const void* can hold any kind of struct, and you need to cast it back > to your struct eventually anyway. Thinking about it again, I don't like > the idea of an extra member in struct snd_usb_audio_quirk either. What > would be nicer is to introduce something like this: > > struct audioformats { > unsigned int n_formats; > const struct audioformat *format; > }; Yes, this looks good, I can use this idea. > Also, you can write the actual quirk handler so that it loops over the > array entries, so you can re-use the code for both > QUIRK_AUDIO_FIXED_MULTI_ENDPOINTS and QUIRK_AUDIO_FIXED_ENDPOINT. > > Do you follow? :) Yes, it makes sense.
diff --git a/sound/usb/card.h b/sound/usb/card.h index 9867ab8..3398708 100644 --- a/sound/usb/card.h +++ b/sound/usb/card.h @@ -20,6 +20,8 @@ struct audioformat { unsigned char attributes; /* corresponding attributes of cs endpoint */ unsigned char endpoint; /* endpoint */ unsigned char ep_attr; /* endpoint attributes */ + unsigned char nr_endpoints; /* number of endpoint table entries */ + unsigned char *endpoint_table; /* endpoint table */ unsigned char datainterval; /* log_2 of data packet interval */ unsigned char protocol; /* UAC_VERSION_1/2 */ unsigned int maxpacksize; /* max. packet size */ diff --git a/sound/usb/quirks-table.h b/sound/usb/quirks-table.h index f5f0595..ab64e50 100644 --- a/sound/usb/quirks-table.h +++ b/sound/usb/quirks-table.h @@ -2807,7 +2807,10 @@ YAMAHA_DEVICE(0x7010, "UB99"), .altsetting = 1, .altset_idx = 1, .attributes = UAC_EP_CS_ATTR_SAMPLE_RATE, - .endpoint = 0x02, + .nr_endpoints = 2, + .endpoint_table = (unsigned char[]) { + 0x02, 0x81 + }, .ep_attr = 0x01, .rates = SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000, diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c index 0df9ede..01c77e2 100644 --- a/sound/usb/quirks.c +++ b/sound/usb/quirks.c @@ -120,12 +120,12 @@ static int create_standard_audio_quirk(struct snd_usb_audio *chip, } /* - * create a stream for an endpoint/altsetting without proper descriptors + * create a stream using the given quirk and endpoint */ -static int create_fixed_stream_quirk(struct snd_usb_audio *chip, +static int create_stream_at_endpoint(struct snd_usb_audio *chip, struct usb_interface *iface, - struct usb_driver *driver, - const struct snd_usb_audio_quirk *quirk) + unsigned char endpoint, + const struct audioformat *quirk_data) { struct audioformat *fp; struct usb_host_interface *alts; @@ -133,7 +133,7 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip, int stream, err; unsigned *rate_table = NULL; - fp = kmemdup(quirk->data, sizeof(*fp), GFP_KERNEL); + fp = kmemdup(quirk_data, sizeof(*fp), GFP_KERNEL); if (!fp) { snd_printk(KERN_ERR "cannot memdup\n"); return -ENOMEM; @@ -152,20 +152,14 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip, fp->rate_table = rate_table; } - stream = (fp->endpoint & USB_DIR_IN) + stream = (endpoint & USB_DIR_IN) ? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK; - err = snd_usb_add_audio_stream(chip, stream, fp); + err = snd_usb_add_audio_stream(chip, stream, endpoint, fp); if (err < 0) { kfree(fp); kfree(rate_table); return err; } - if (fp->iface != get_iface_desc(&iface->altsetting[0])->bInterfaceNumber || - fp->altset_idx >= iface->num_altsetting) { - kfree(fp); - kfree(rate_table); - return -EINVAL; - } alts = &iface->altsetting[fp->altset_idx]; altsd = get_iface_desc(alts); fp->protocol = altsd->bInterfaceProtocol; @@ -174,9 +168,45 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip, fp->datainterval = snd_usb_parse_datainterval(chip, alts); if (fp->maxpacksize == 0) fp->maxpacksize = le16_to_cpu(get_endpoint(alts, 0)->wMaxPacketSize); + + return 0; +} + +/* + * create a stream for an endpoint/altsetting without proper descriptors + */ +static int create_fixed_stream_quirk(struct snd_usb_audio *chip, + struct usb_interface *iface, + struct usb_driver *driver, + struct snd_usb_audio_quirk *quirk) +{ + struct audioformat *fp = quirk->data; + struct usb_host_interface *alts; + int n, err; + + if (fp->iface != get_iface_desc(&iface->altsetting[0])->bInterfaceNumber || + fp->altset_idx >= iface->num_altsetting) { + return -EINVAL; + } + usb_set_interface(chip->dev, fp->iface, 0); + + if (fp->nr_endpoints == 0) { + err = create_stream_at_endpoint(chip, iface, fp->endpoint, fp); + if (err < 0) + return err; + } + for (n = 0; n < fp->nr_endpoints; n++) { + err = create_stream_at_endpoint(chip, iface, fp->endpoint_table[n], fp); + if (err < 0) + return err; + } + + /* FIXME: these functions fixed to the first endpoint */ + alts = &iface->altsetting[fp->altset_idx]; snd_usb_init_pitch(chip, fp->iface, alts, fp); snd_usb_init_sample_rate(chip, fp->iface, alts, fp, fp->rate_max); + return 0; } @@ -471,7 +501,7 @@ static int create_uaxx_quirk(struct snd_usb_audio *chip, stream = (fp->endpoint & USB_DIR_IN) ? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK; - err = snd_usb_add_audio_stream(chip, stream, fp); + err = snd_usb_add_audio_stream(chip, stream, fp->endpoint, fp); if (err < 0) { kfree(fp); return err; diff --git a/sound/usb/stream.c b/sound/usb/stream.c index 2fb71be..34947db 100644 --- a/sound/usb/stream.c +++ b/sound/usb/stream.c @@ -319,6 +319,7 @@ static struct snd_pcm_chmap_elem *convert_chmap(int channels, unsigned int bits, */ int snd_usb_add_audio_stream(struct snd_usb_audio *chip, int stream, + unsigned char endpoint, struct audioformat *fp) { struct snd_usb_stream *as; @@ -330,7 +331,7 @@ int snd_usb_add_audio_stream(struct snd_usb_audio *chip, if (as->fmt_type != fp->fmt_type) continue; subs = &as->substream[stream]; - if (subs->ep_num == fp->endpoint) { + if (subs->ep_num == endpoint) { list_add_tail(&fp->list, &subs->fmt_list); subs->num_formats++; subs->formats |= fp->formats; @@ -708,7 +709,7 @@ int snd_usb_parse_audio_interface(struct snd_usb_audio *chip, int iface_no) fp->chmap = convert_chmap(fp->channels, chconfig, protocol); snd_printdd(KERN_INFO "%d:%u:%d: add audio endpoint %#x\n", dev->devnum, iface_no, altno, fp->endpoint); - err = snd_usb_add_audio_stream(chip, stream, fp); + err = snd_usb_add_audio_stream(chip, stream, fp->endpoint, fp); if (err < 0) { kfree(fp->rate_table); kfree(fp->chmap); diff --git a/sound/usb/stream.h b/sound/usb/stream.h index c97f679..4c1b834 100644 --- a/sound/usb/stream.h +++ b/sound/usb/stream.h @@ -6,6 +6,7 @@ int snd_usb_parse_audio_interface(struct snd_usb_audio *chip, int snd_usb_add_audio_stream(struct snd_usb_audio *chip, int stream, + unsigned char endpoint, struct audioformat *fp); #endif /* __USBAUDIO_STREAM_H */