[alsa-devel] [PATCH] ALSA: usb-audio - Capture and duplex support for Digidesign Mbox 1 sound card.
This patch completes the support for Digidesign Mbox 1 sound card by adding capture.
The issue with this card was that it has two endpoints on the same interface. This patch creates a dual endpoint quirk. The quirk interface needs a second audioformat struct for this to work which I called ".data2".
Signed-off-by: Damien Zammit damien@zamaudio.com
This second patch removes a few extra lines I added by mistake when I merged my private repo to the one for submission.
On 27/01/14 03:52, Clemens Ladisch wrote:
This patch creates a dual endpoint quirk.
The quirk interface needs a second audioformat struct for this to work which I called ".data2".
Couldn't you just let .data point to an array of two structs?
Thanks Clemens, I have created a new patch using this suggestion.
Damien
Hi Clemens,
I wish to bring to your attention that the patch I submitted previously (0001-Cleaned-up-Digidesign-Mbox-1-patch.patch) has been working for the last two months on my device and other people have also reported complete success with it. (see http://www.zamaudio.com/?p=953 for details).
We discovered and fixed an issue with outdated firmware causing sporadic white noise on the device; Avid (the manufacturer) provided an update which fixed this issue entirely (v0.22). Unfortunately, the firmware update can only be applied using a Mac computer since it is bundled in a .dmg installer.
If users do not apply the firmware update, the device will still work with linux, but every second or third time they open a playback stream there is a loud fuzzing noise which could possibly damage external equipment. I suppose it is caused by a bug in the firmware.
Therefore, with the official updated firmware and my usb-audio quirk provided in the previous post, I am pleased to say that Digidesign Mbox 1 (0dba:1000) works well in duplex mode, in both 44100 and 48000Hz modes (SPDIF untested).
I am keen to get my patch into the kernel with your recommendations for cleaning up the code.
Although I have not inspected the inside of the unit, in my opinion it has higher quality converters and preamps than the second generation models. Also, even though it is powered by dirty USB power, it is surprisingly quiet on the main outputs when silence is playing.
Regards, Damien
On 27/01/14 14:02, Damien Zammit wrote:
On 27/01/14 03:52, Clemens Ladisch wrote:
This patch creates a dual endpoint quirk.
The quirk interface needs a second audioformat struct for this to work which I called ".data2".
Couldn't you just let .data point to an array of two structs?
Thanks Clemens, I have created a new patch using this suggestion.
Damien
Hi Damien,
Thanks for your patch! See my two comments below.
On 01/27/2014 04:02 AM, Damien Zammit wrote:
On 27/01/14 03:52, Clemens Ladisch wrote:
This patch creates a dual endpoint quirk.
>The quirk interface needs a second audioformat struct for this to work >which I called ".data2".
Couldn't you just let .data point to an array of two structs?
Thanks Clemens, I have created a new patch using this suggestion.
[...]
- fp2 = kmemdup((const struct audioformat*)(quirk->data + sizeof(const struct audioformat)), sizeof(*fp2), GFP_KERNEL);
- if (!fp2) {
snd_printk(KERN_ERR "cannot memdup 2\n");return -ENOMEM;- }
- if (fp1->nr_rates > MAX_NR_RATES) {
kfree(fp1);kfree(fp2);
Please do proper error unwinding here with jump labels rather than open-coding the kfree() calls from multiple places.
Also, I wonder whether a more generic quirk type to set up a dynamic number of fixed interfaces wouldn't be nicer. IOW, add a field to struct snd_usb_audio_quirk to denote the number of array members in quirk->data and call the quirk type QUIRK_AUDIO_FIXED_MULTI_ENDPOINT or something. Then, rewrite the logic to iterate over the interfaces in a loop. That might also make the code more readable.
Thanks, Daniel
On Mon, 3 Mar 2014, Daniel Mack wrote:
Hi Damien,
Thanks for your patch! See my two comments below.
On 01/27/2014 04:02 AM, Damien Zammit wrote:
On 27/01/14 03:52, Clemens Ladisch wrote:
This patch creates a dual endpoint quirk. > >The quirk interface needs a second audioformat struct for this to work > >which I called ".data2".
Couldn't you just let .data point to an array of two structs?
Thanks Clemens, I have created a new patch using this suggestion.
[...]
- fp2 = kmemdup((const struct audioformat*)(quirk->data + sizeof(const struct audioformat)), sizeof(*fp2), GFP_KERNEL);
- if (!fp2) {
snd_printk(KERN_ERR "cannot memdup 2\n");return -ENOMEM;- }
- if (fp1->nr_rates > MAX_NR_RATES) {
kfree(fp1);kfree(fp2);Please do proper error unwinding here with jump labels rather than open-coding the kfree() calls from multiple places.
Also, I wonder whether a more generic quirk type to set up a dynamic number of fixed interfaces wouldn't be nicer. IOW, add a field to struct snd_usb_audio_quirk to denote the number of array members in quirk->data and call the quirk type QUIRK_AUDIO_FIXED_MULTI_ENDPOINT or something. Then, rewrite the logic to iterate over the interfaces in a loop. That might also make the code more readable.
I'm a bit late to this, but I think a more generic quirk is necessary.
The problem with the patch is that the new code still calls functions hard-wired to the first endpoint on the interface, eg. snd_usb_init_pitch(). Also parameters such as altsetting apply to the interface, not endpoint.
So whilst it may work in the given case, much of the quirk is ignored.
This is worth fixing though, as there is the same limitation with the Focusrite/Novation interfaces, and I suspect others too.
A cleaner implementation is probably an array of endpoints in the quirk, something like the patch below (untested)
I found the problem is functions using an endpoint index (eg. 0) whereas mainly the function uses the ID (eg. 0x81). What is the relationship between the two?
It's to replace calls such as:
usb_sndctrlpipe(dev, 0) get_endpoint(alts, 0)
Or does some kind of spec define that these should be applied to the first endpoint and that all will be affected?
Thanks
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/ba...
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/ba...
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.
On 04/03/14 08:01, Daniel Mack wrote:
Hi Damien,
Thanks for your patch! See my two comments below.
On 01/27/2014 04:02 AM, Damien Zammit wrote:
On 27/01/14 03:52, Clemens Ladisch wrote:
This patch creates a dual endpoint quirk. >> The quirk interface needs a second audioformat struct for this to work >> which I called ".data2".
Couldn't you just let .data point to an array of two structs?
Thanks Clemens, I have created a new patch using this suggestion.
[...]
- fp2 = kmemdup((const struct audioformat*)(quirk->data + sizeof(const struct audioformat)), sizeof(*fp2), GFP_KERNEL);
- if (!fp2) {
snd_printk(KERN_ERR "cannot memdup 2\n");return -ENOMEM;- }
- if (fp1->nr_rates > MAX_NR_RATES) {
kfree(fp1);kfree(fp2);Please do proper error unwinding here with jump labels rather than open-coding the kfree() calls from multiple places.
Also, I wonder whether a more generic quirk type to set up a dynamic number of fixed interfaces wouldn't be nicer. IOW, add a field to struct snd_usb_audio_quirk to denote the number of array members in quirk->data and call the quirk type QUIRK_AUDIO_FIXED_MULTI_ENDPOINT or something. Then, rewrite the logic to iterate over the interfaces in a loop. That might also make the code more readable.
Hi Daniel, Clemens, I have addressed the above issues, please find attached my new patch. I did a proper git log message that describes my changes. I am keen to get this reviewed and hopefully accepted soon. I did a clean clone of Takashi's 'sound' (for-next) and applied my changes on top of that.
Damien
Hi Damien,
On 03/29/2014 06:35 AM, Damien Zammit wrote:
A few revisions of this patch have been made. This patch provides support for usb cards which have their streams on multiple endpoints within the same interface. In particular, it provides duplex mode support for Digidesign Mbox 1.
I followed ALSA dev team's suggestion of making it work for N endpoints rather than hardcoding it to two, and improved error handling has been provided. An extra parameter has been added as requested to the usbaudio quirk struct 'epmulti' to give the number of endpoints within a multi interface.
A few style nits below, which you can fix for the next version. Most important, the error unwinder should be more in-line with other code in the kernel.
diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c index 7c57f22..affbced 100644 --- a/sound/usb/quirks.c +++ b/sound/usb/quirks.c @@ -180,6 +180,84 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip, return 0; }
+/*
- create N streams for an interface without proper descriptors but with multiple endpoints
- */
+static int create_fixed_multi_stream_quirk(struct snd_usb_audio *chip,
struct usb_interface *iface,struct usb_driver *driver,const struct snd_usb_audio_quirk *quirk)+{
- struct audioformat **fp;
- struct usb_host_interface *alts = NULL;
- int *stream;
- int err, i;
- unsigned *rate_table = NULL;
- int rates_found;
- rates_found = 0;
- stream = (int*) kmalloc(sizeof(int) * quirk->epmulti, GFP_KERNEL);
No need to cast the result of k[mzc]alloc. Same for other locations below.
- if (!stream)
goto err_mem;
err = -ENOMEM; goto exit_err;
- fp = (struct audioformat**) kmalloc(sizeof((const struct audioformat*)quirk->data) * quirk->epmulti, GFP_KERNEL);
- if (!(fp))
Extra parenthesis not needed.
goto err_mem;- for (i = 0; i < quirk->epmulti; ++i) {
i++ is prefered if the operation order doesn't matter. It's not important but a little more common in the kernel.
fp[i] = kmemdup((const struct audioformat*)(quirk->data + i*sizeof(const struct audioformat)), sizeof(*fp[i]), GFP_KERNEL);
You need to do something about these overlong lines, as they're also quite unparsable for humans. One solution is to pre-calculate the size into a temporary variable.
if (!(fp[i])) {
Extra parenthesis not needed.
usb_audio_err(chip, "cannot memdup %d\n", i);
err = -ENOMEM; goto exit_err;
goto err_mem;}if (fp[i]->nr_rates > MAX_NR_RATES) {err = -EINVAL;goto err_rates;}if (fp[i]->nr_rates > 0 && !rates_found) {rate_table = kmemdup(fp[i]->rate_table,sizeof(int) * fp[i]->nr_rates, GFP_KERNEL);rates_found = 1;
Not sure if I overlook anything, but isn't the extra variable rates_found redundant here, as can derive the same information by checking for (rate_table != NULL)?
}if (!rate_table) {err = -ENOMEM;goto err_rates;}
This check makes more sense directly underneath the kmemdup().
fp[i]->rate_table = rate_table;stream[i] = (fp[i]->endpoint & USB_DIR_IN)? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;err = snd_usb_add_audio_stream(chip, stream[i], fp[i]);if (err < 0)goto err_ratetable;if (fp[i]->iface != get_iface_desc(&iface->altsetting[0])->bInterfaceNumber ||fp[i]->altset_idx >= iface->num_altsetting) {err = -EINVAL;goto err_ratetable;}alts = &iface->altsetting[fp[0]->altset_idx];fp[i]->datainterval = snd_usb_parse_datainterval(chip, alts);fp[i]->maxpacksize = le16_to_cpu(get_endpoint(alts, 0)->wMaxPacketSize);- }
- 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);
- return 0;
+err_mem:
- return -ENOMEM;
You can drop this ...
+err_ratetable:
- kfree(rate_table);
+err_rates:
- for (i = 0; i < quirk->epmulti; ++i)
if(fp[i])kfree(fp[i]);- kfree(fp);
- kfree(stream);
... and place another label here:
exit_err:
- return err;
+}
Thanks, Daniel
participants (4)
-
Clemens Ladisch -
Damien Zammit -
Daniel Mack -
Mark Hills