[FT C400,PATCH RFC,v4 00/10] M-Audio Fast Track C400

[FT C400,PATCH RFC,v4 00/10] M-Audio Fast Track C400

[Eldad Zack]

Hi,

The following patches adds support for the M-Audio Fast Track C400.
This is version 4 of the patch series.

* Incoroprated the feedback from Clemens Ladisch <clemens@ladisch.de>: 
Moved the quirks structure to the right place (also in other places, though
not strictly required) and removed the "set quirks" log messages.
Thanks, Clemens!

* Added a patch to address the regression introduced by commit
947d299686aa9cc8aecf749d54e8475c6e498956 , "ALSA: snd-usb: properly initialize
the sync endpoint". I believe there are no other devices that affected by this
issue. Opening only the playback stream without first opening the capture
stream now works with the C400.

v3:
http://mailman.alsa-project.org/pipermail/alsa-devel/2012-November/057477.html

v2:
http://mailman.alsa-project.org/pipermail/alsa-devel/2012-November/056750.html

This series was tested with the latest mainline tree, 3.7-rc7
(HEAD e23739b4ade80a3a7f87198f008f6c44a7cbc9fd )

Also applies against current sound-unstable (not tested).
(HEAD 900b0690ee7b2a227c5db5f1642ceb67d90c49fc )

* Everything works: mixer controls, effects control, playback and capture
(with the correct sampling frequency set). 
Note that the device doesn't really have a master control or left/right
pan. It's just "emulated" in the software.

* To deal with the mixer channel IDs, limited by cmask bit length,
I introduced an offset value. This allows to get rid of the shorehorning
of the IDs into cmask and device-referencing in the low-level functions.
Is this a good approach?

* Audio in/out seems to work good in this series, mainly because the
playback is an implicit feedback endpoint, and it is now configured
with the capture endpoint as its sync-buddy. Also needed to correct
the calculation, because of the different channel numbers between
the two endpoints on the C400 (6 playback - implicit feedback, 4 capture).

* The clock sources are still named "Unit 129" (0x81, internal) and
"Unit 130" (0x82, SPDIF). I'll fix this later. I also plan to fix the
channel names (i.e., instead of "AIn3", use "SPDIF In L").

* I've touched one FTU mixer creation function. I'd appreciate if someone
can test this with the FTU to make sure I didn't break something.
(I haven't gotten any feedback on that one yet)

Cheers,
Eldad

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>

Eldad Zack (10):
  usb-audio: replace hardcoded value with const
  usb-audio: correct sync endpoint initialization
  usb-audio: use sender stride for implicit feedback
  usb-audio: add control index offset
  usb-audio: skip UAC2 EFFECT_UNIT
  usb-audio: parameterize FTU effect unit control
  usb-audio: M-Audio Fast Track C400 quirks table
  usb-audio: Fast Track C400 mixer ranges
  usb-audio: Fast Track C400 mixer controls
  usb-audio: FT C400 sync playback EP to capture EP

 sound/usb/endpoint.c     |    9 ++-
 sound/usb/mixer.c        |   44 +++++++++-
 sound/usb/mixer.h        |    1 +
 sound/usb/mixer_quirks.c |  216 ++++++++++++++++++++++++++++++++++++++++++++--
 sound/usb/pcm.c          |   94 ++++++++++++++++++--
 sound/usb/quirks-table.h |   71 +++++++++++++++
 6 files changed, 414 insertions(+), 21 deletions(-)

-- 
1.7.8.6

[FT C400, PATCH RFC, v4 01/10] usb-audio: replace hardcoded value with const

[Eldad Zack]

In this context, 0x01 is USB_ENDPOINT_XFER_ISOC.

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/pcm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index ef6fa24..ff8cbbf 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -381,7 +381,7 @@ static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt)
 		/* ... and check descriptor size before accessing bSynchAddress
 		   because there is a version of the SB Audigy 2 NX firmware lacking
 		   the audio fields in the endpoint descriptors */
-		if ((get_endpoint(alts, 1)->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) != 0x01 ||
+		if ((get_endpoint(alts, 1)->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) != USB_ENDPOINT_XFER_ISOC ||
 		    (get_endpoint(alts, 1)->bLength >= USB_DT_ENDPOINT_AUDIO_SIZE &&
 		     get_endpoint(alts, 1)->bSynchAddress != 0 &&
 		     !implicit_fb)) {
-- 
1.7.8.6

[FT C400, PATCH RFC, v4 02/10] usb-audio: correct sync ep init

[Eldad Zack]

Commit 947d299686aa9cc8aecf749d54e8475c6e498956
"ALSA: snd-usb: properly initialize the sync endpoint" introduced a
regression for devices that has a sync endpoint with a channel
number different than the data endpoint channel.
Due to a different channel and period bytes count, attempting to
initialize the sync endpoint will fail. With xhci it yields:

cannot submit urb 0, error -90: internal error

With this patch, if a sync endpoint has multiple audioformats, an
audioformat with a matching number of channels is preferred. Otherwise,
an audioformat will be picked randomly (i.e., first audioformat with a
non-zero channel count) and the period bytes count is adjusted
accordingly.

Cc: Jeffrey Barish <jeff_barish@earthlink.net>
Cc: Daniel Mack <zonque@gmail.com>
Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/pcm.c |   79 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 72 insertions(+), 7 deletions(-)

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index ff8cbbf..2158943 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -438,6 +438,76 @@ add_sync_ep:
 }
 
 /*
+ * Configure the sync ep using the rate and pcm format of the data ep.
+ */
+static int configure_sync_endpoint(struct snd_usb_substream *subs)
+{
+	int ret;
+	struct list_head *p;
+	struct audioformat *sync_fp;
+	int sync_channels = 0;
+	int sync_period_bytes = subs->period_bytes;
+	int direction = subs->direction ^ 1;
+	struct snd_usb_substream *sync_subs =
+		&subs->stream->substream[direction];
+
+	if (sync_subs->ep_num != subs->sync_endpoint->ep_num) {
+		snd_printk(KERN_ERR "%s: cannot find the sync ep (direction %d, found %x, wanted %x).\n",
+			__func__, direction,
+				subs->stream->substream[direction].ep_num,
+				subs->sync_endpoint->ep_num);
+		return -EINVAL;
+	}
+
+	/*
+	 * Try to find a format with a number of channels matching
+	 * the data substream. If no such format was found, pick one
+	 * with non zero channel number at random.
+	 */
+	list_for_each(p, &sync_subs->fmt_list) {
+		struct audioformat *fp;
+		fp = list_entry(p, struct audioformat, list);
+
+		if (sync_channels == 0) {
+			sync_fp = fp;
+			sync_channels = fp->channels;
+		} else {
+			if (fp->channels == subs->channels) {
+				sync_channels = fp->channels;
+				sync_fp = fp;
+			}
+		}
+	}
+
+	if (sync_channels < 1) {
+		snd_printk(KERN_ERR "%s: no valid audioformat for sync ep %x found\n",
+			__func__, sync_subs->ep_num);
+		return -EINVAL;
+	}
+
+	/*
+	 * Recalculated the period bytes if channel number differ between
+	 * data and sync ep.
+	 */
+	if (sync_channels != subs->channels) {
+		sync_period_bytes = (subs->period_bytes / subs->channels) *
+			sync_channels;
+		snd_printdd(KERN_INFO "%s: sync ep period bytes recalc %d -> %d\n",
+			__func__, subs->period_bytes, sync_period_bytes);
+	}
+
+	ret = snd_usb_endpoint_set_params(subs->sync_endpoint,
+					  subs->pcm_format,
+					  sync_channels,
+					  sync_period_bytes,
+					  subs->cur_rate,
+					  sync_fp,
+					  NULL);
+
+	return ret;
+}
+
+/*
  * configure endpoint params
  *
  * called  during initial setup and upon resume
@@ -459,13 +529,8 @@ static int configure_endpoint(struct snd_usb_substream *subs)
 		return ret;
 
 	if (subs->sync_endpoint)
-		ret = snd_usb_endpoint_set_params(subs->sync_endpoint,
-						  subs->pcm_format,
-						  subs->channels,
-						  subs->period_bytes,
-						  subs->cur_rate,
-						  subs->cur_audiofmt,
-						  NULL);
+		ret = configure_sync_endpoint(subs);
+
 	return ret;
 }
 
-- 
1.7.8.6

[FT C400, PATCH RFC, v4 03/10] usb-audio: use sender stride for implicit feedback

[Eldad Zack]

For implicit feedback endpoints, the number of bytes for each packet
is matched by the corresponding synchronizing endpoint.
The size is calculated by taking the actual size and dividing it by
the stride - currently by the endpoint's stride, but we should use the
synchronization source's stride.
This is evident when the number of channels differ between the
synchronization source and the implicitly fed-back endpoint, as with
M-Audio Fast Track C400 - the synchronization source (capture)
has 4 channels, while the implicit feedback mode endpoint has 6.

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/endpoint.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index 34de6f2..72e711c 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -1034,15 +1034,18 @@ void snd_usb_handle_sync_urb(struct snd_usb_endpoint *ep,
 		/*
 		 * Iterate through the inbound packet and prepare the lengths
 		 * for the output packet. The OUT packet we are about to send
-		 * will have the same amount of payload bytes than the IN
-		 * packet we just received.
+		 * will have the same amount of payload bytes per stride as the
+		 * IN packet we just received. Since the actual size is scaled
+		 * by the stride, use the sender stride to calculate the length
+		 * in case the number of channels differ between the implicitly
+		 * fed-back endpoint and the synchronizing endpoint.
 		 */
 
 		out_packet->packets = in_ctx->packets;
 		for (i = 0; i < in_ctx->packets; i++) {
 			if (urb->iso_frame_desc[i].status == 0)
 				out_packet->packet_size[i] =
-					urb->iso_frame_desc[i].actual_length / ep->stride;
+					urb->iso_frame_desc[i].actual_length / sender->stride;
 			else
 				out_packet->packet_size[i] = 0;
 		}
-- 
1.7.8.6

[FT C400, PATCH RFC, v4 04/10] usb-audio: add control index offset

[Eldad Zack]

Currently, channel IDs exceeding 31 (0x1f) cannot be used.
The channel ID is derived from the cmask. Extending cmask
to a 64-bit type would only allow it to go up to 63 (0x3f).
Some devices have channel IDs exceeding that as well.
To address that, add an offset to the mixer element which
is then accounted for in the UAC set/get functions.

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/mixer.c        |    4 ++++
 sound/usb/mixer.h        |    1 +
 sound/usb/mixer_quirks.c |   16 +++++++++++++++-
 3 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 298070e..b0fc6ae 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -382,6 +382,8 @@ error:
 
 static int get_ctl_value(struct usb_mixer_elem_info *cval, int request, int validx, int *value_ret)
 {
+	validx += cval->idx_off;
+
 	return (cval->mixer->protocol == UAC_VERSION_1) ?
 		get_ctl_value_v1(cval, request, validx, value_ret) :
 		get_ctl_value_v2(cval, request, validx, value_ret);
@@ -432,6 +434,8 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval,
 	unsigned char buf[2];
 	int idx = 0, val_len, err, timeout = 10;
 
+	validx += cval->idx_off;
+
 	if (cval->mixer->protocol == UAC_VERSION_1) {
 		val_len = cval->val_type >= USB_MIXER_S16 ? 2 : 1;
 	} else { /* UAC_VERSION_2 */
diff --git a/sound/usb/mixer.h b/sound/usb/mixer.h
index a7f3d45..aab80df 100644
--- a/sound/usb/mixer.h
+++ b/sound/usb/mixer.h
@@ -43,6 +43,7 @@ struct usb_mixer_elem_info {
 	unsigned int id;
 	unsigned int control;	/* CS or ICN (high byte) */
 	unsigned int cmask; /* channel mask bitmap: 0 = master */
+	unsigned int idx_off; /* Control index offset */
 	unsigned int ch_readonly;
 	unsigned int master_readonly;
 	int channels;
diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c
index ae2b714..4199b97 100644
--- a/sound/usb/mixer_quirks.c
+++ b/sound/usb/mixer_quirks.c
@@ -63,11 +63,12 @@ static void usb_mixer_elem_free(struct snd_kcontrol *kctl)
  * Since there doesn't seem to be a devices that needs a multichannel
  * version, we keep it mono for simplicity.
  */
-static int snd_create_std_mono_ctl(struct usb_mixer_interface *mixer,
+static int snd_create_std_mono_ctl_offset(struct usb_mixer_interface *mixer,
 				unsigned int unitid,
 				unsigned int control,
 				unsigned int cmask,
 				int val_type,
+				unsigned int idx_off,
 				const char *name,
 				snd_kcontrol_tlv_rw_t *tlv_callback)
 {
@@ -85,6 +86,7 @@ static int snd_create_std_mono_ctl(struct usb_mixer_interface *mixer,
 	cval->channels = 1;
 	cval->control = control;
 	cval->cmask = cmask;
+	cval->idx_off = idx_off;
 
 	/* get_min_max() is called only for integer volumes later,
 	 * so provide a short-cut for booleans */
@@ -120,6 +122,18 @@ static int snd_create_std_mono_ctl(struct usb_mixer_interface *mixer,
 	return 0;
 }
 
+static int snd_create_std_mono_ctl(struct usb_mixer_interface *mixer,
+				unsigned int unitid,
+				unsigned int control,
+				unsigned int cmask,
+				int val_type,
+				const char *name,
+				snd_kcontrol_tlv_rw_t *tlv_callback)
+{
+	return snd_create_std_mono_ctl_offset(mixer, unitid, control, cmask,
+		val_type, 0 /* Offset */, name, tlv_callback);
+}
+
 /*
  * Create a set of standard UAC controls from a table
  */
-- 
1.7.8.6

[FT C400, PATCH RFC, v4 05/10] usb-audio: skip UAC2 EFFECT_UNIT

[Eldad Zack]

Current code mishandles the case where the device is a UAC2
and the bDescriptorSubtype is a UAC2 Effect Unit (0x07).
It tries to parse it as a Processing Unit (which is similar to two
other UAC1 units with overlapping subtypes), but since the structure
is different (See: 4.7.2.10, 4.7.2.11 in UAC2 standard), the parsing
is done incorrectly and prevents the device from initializing.
For now, just ignore the unit.

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/mixer.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index b0fc6ae..4eacbe2 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -723,8 +723,19 @@ static int check_input_term(struct mixer_build *state, int id, struct usb_audio_
 			return 0;
 		}
 		case UAC1_PROCESSING_UNIT:
-		case UAC1_EXTENSION_UNIT: {
+		case UAC1_EXTENSION_UNIT:
+		/* UAC2_PROCESSING_UNIT_V2 */
+		/* UAC2_EFFECT_UNIT */ {
 			struct uac_processing_unit_descriptor *d = p1;
+
+			if (state->mixer->protocol == UAC_VERSION_2 &&
+				hdr[2] == UAC2_EFFECT_UNIT) {
+				/* UAC2/UAC1 unit IDs overlap here in an
+				 * uncompatible way. Ignore this unit for now.
+				 */
+				return 0;
+			}
+
 			if (d->bNrInPins) {
 				id = d->baSourceID[0];
 				break; /* continue to parse */
-- 
1.7.8.6

[FT C400, PATCH RFC, v4 06/10] usb-audio: parameterize FTU effect unit control

[Eldad Zack]

Adds the unit ID and the control as parameters to the creation of the
effect unit control for the M-Audio Fast Track Ultra. This allows the
code to be shared with other devices that use different unit ID and
control, such as the M-Audio Fast Track C400.

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/mixer_quirks.c |   24 ++++++++++++++++--------
 1 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c
index 4199b97..a614dab 100644
--- a/sound/usb/mixer_quirks.c
+++ b/sound/usb/mixer_quirks.c
@@ -635,11 +635,13 @@ static int snd_nativeinstruments_create_mixer(struct usb_mixer_interface *mixer,
 }
 
 /* M-Audio FastTrack Ultra quirks */
-/* FTU Effect switch */
+/* FTU Effect switch (also used by C400) */
 struct snd_ftu_eff_switch_priv_val {
 	struct usb_mixer_interface *mixer;
 	int cached_value;
 	int is_cached;
+	int bUnitID;
+	int validx;
 };
 
 static int snd_ftu_eff_switch_info(struct snd_kcontrol *kcontrol,
@@ -674,9 +676,8 @@ static int snd_ftu_eff_switch_get(struct snd_kcontrol *kctl,
 	struct snd_ftu_eff_switch_priv_val *pval;
 	int err;
 	unsigned char value[2];
+	int id, validx;
 
-	const int id = 6;
-	const int validx = 1;
 	const int val_len = 2;
 
 	value[0] = 0x00;
@@ -698,6 +699,8 @@ static int snd_ftu_eff_switch_get(struct snd_kcontrol *kctl,
 	if (snd_BUG_ON(!chip))
 		return -EINVAL;
 
+	id = pval->bUnitID;
+	validx = pval->validx;
 
 	down_read(&mixer->chip->shutdown_rwsem);
 	if (mixer->chip->shutdown)
@@ -728,10 +731,8 @@ static int snd_ftu_eff_switch_put(struct snd_kcontrol *kctl,
 	struct usb_mixer_interface *mixer;
 	int changed, cur_val, err, new_val;
 	unsigned char value[2];
+	int id, validx;
 
-
-	const int id = 6;
-	const int validx = 1;
 	const int val_len = 2;
 
 	changed = 0;
@@ -749,6 +750,9 @@ static int snd_ftu_eff_switch_put(struct snd_kcontrol *kctl,
 	if (snd_BUG_ON(!chip))
 		return -EINVAL;
 
+	id = pval->bUnitID;
+	validx = pval->validx;
+
 	if (!pval->is_cached) {
 		/* Read current value */
 		down_read(&mixer->chip->shutdown_rwsem);
@@ -793,7 +797,8 @@ static int snd_ftu_eff_switch_put(struct snd_kcontrol *kctl,
 	return changed;
 }
 
-static int snd_ftu_create_effect_switch(struct usb_mixer_interface *mixer)
+static int snd_ftu_create_effect_switch(struct usb_mixer_interface *mixer,
+	int validx, int bUnitID)
 {
 	static struct snd_kcontrol_new template = {
 		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
@@ -816,6 +821,8 @@ static int snd_ftu_create_effect_switch(struct usb_mixer_interface *mixer)
 	pval->cached_value = 0;
 	pval->is_cached = 0;
 	pval->mixer = mixer;
+	pval->bUnitID = bUnitID;
+	pval->validx = validx;
 
 	template.private_value = (unsigned long) pval;
 	kctl = snd_ctl_new1(&template, mixer->chip);
@@ -974,9 +981,10 @@ static int snd_ftu_create_mixer(struct usb_mixer_interface *mixer)
 	if (err < 0)
 		return err;
 
-	err = snd_ftu_create_effect_switch(mixer);
+	err = snd_ftu_create_effect_switch(mixer, 1, 6);
 	if (err < 0)
 		return err;
+
 	err = snd_ftu_create_effect_volume_ctl(mixer);
 	if (err < 0)
 		return err;
-- 
1.7.8.6

[FT C400, PATCH RFC, v4 07/10] usb-audio: M-Audio Fast Track C400 quirks table

[Eldad Zack]

Adds a quirks table for the M-Audio Fast Track C400.
Thanks to Clemens Ladisch <clemens@ladisch.de> for pointing out that
the table must be sorted.

Based on the following patch from the alsa-devel list:
http://mailman.alsa-project.org/pipermail/alsa-devel/2012-May/051676.html

See also:
http://mailman.alsa-project.org/pipermail/alsa-devel/2012-April/051219.html

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/quirks-table.h |   71 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 71 insertions(+), 0 deletions(-)

diff --git a/sound/usb/quirks-table.h b/sound/usb/quirks-table.h
index 88d8ceb..0635801 100644
--- a/sound/usb/quirks-table.h
+++ b/sound/usb/quirks-table.h
@@ -2163,6 +2163,77 @@ YAMAHA_DEVICE(0x7010, "UB99"),
 	}
 },
 {
+	USB_DEVICE_VENDOR_SPEC(0x0763, 0x2030),
+	.driver_info = (unsigned long) &(const struct snd_usb_audio_quirk) {
+		/* .vendor_name = "M-Audio", */
+		/* .product_name = "Fast Track C400", */
+		.ifnum = QUIRK_ANY_INTERFACE,
+		.type = QUIRK_COMPOSITE,
+		.data = &(const struct snd_usb_audio_quirk[]) {
+			{
+				.ifnum = 1,
+				.type = QUIRK_AUDIO_STANDARD_MIXER,
+			},
+			/* Playback */
+			{
+				.ifnum = 2,
+				.type = QUIRK_AUDIO_FIXED_ENDPOINT,
+				.data = &(const struct audioformat) {
+					.formats = SNDRV_PCM_FMTBIT_S24_3LE,
+					.channels = 6,
+					.iface = 2,
+					.altsetting = 1,
+					.altset_idx = 1,
+					.attributes = UAC_EP_CS_ATTR_SAMPLE_RATE,
+					.endpoint = 0x01,
+					.ep_attr = 0x09,
+					.rates = SNDRV_PCM_RATE_44100 |
+						 SNDRV_PCM_RATE_48000 |
+						 SNDRV_PCM_RATE_88200 |
+						 SNDRV_PCM_RATE_96000,
+					.rate_min = 44100,
+					.rate_max = 96000,
+					.nr_rates = 4,
+					.rate_table = (unsigned int[]) {
+							44100, 48000, 88200, 96000
+					},
+					.clock = 0x81,
+				}
+			},
+			/* Capture */
+			{
+				.ifnum = 3,
+				.type = QUIRK_AUDIO_FIXED_ENDPOINT,
+				.data = &(const struct audioformat) {
+					.formats = SNDRV_PCM_FMTBIT_S24_3LE,
+					.channels = 4,
+					.iface = 3,
+					.altsetting = 1,
+					.altset_idx = 1,
+					.attributes = UAC_EP_CS_ATTR_SAMPLE_RATE,
+					.endpoint = 0x81,
+					.ep_attr = 0x05,
+					.rates = SNDRV_PCM_RATE_44100 |
+						 SNDRV_PCM_RATE_48000 |
+						 SNDRV_PCM_RATE_88200 |
+						 SNDRV_PCM_RATE_96000,
+					.rate_min = 44100,
+					.rate_max = 96000,
+					.nr_rates = 4,
+					.rate_table = (unsigned int[]) {
+						44100, 48000, 88200, 96000
+					},
+					.clock = 0x81,
+				}
+			},
+			/* MIDI */
+			{
+				.ifnum = -1 /* Interface = 4 */
+			}
+		}
+	}
+},
+{
 	USB_DEVICE_VENDOR_SPEC(0x0763, 0x2080),
 	.driver_info = (unsigned long) & (const struct snd_usb_audio_quirk) {
 		/* .vendor_name = "M-Audio", */
-- 
1.7.8.6

[FT C400, PATCH RFC, v4 08/10] usb-audio: Fast Track C400 mixer ranges

[Eldad Zack]

Add ranges for various Fast Track C400 controls, as observed
while using the vendor's mixer control software (res values
are an estimation).

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/mixer.c |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 4eacbe2..feed500 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -806,6 +806,33 @@ static void volume_control_quirks(struct usb_mixer_elem_info *cval,
 				  struct snd_kcontrol *kctl)
 {
 	switch (cval->mixer->chip->usb_id) {
+	case USB_ID(0x0763, 0x2030): /* M-Audio Fast Track C400 */
+		if (strcmp(kctl->id.name, "Effect Duration") == 0) {
+			cval->min = 0x0000;
+			cval->max = 0xffff;
+			cval->res = 0x00e6;
+			break;
+		}
+		if (strcmp(kctl->id.name, "Effect Volume") == 0 ||
+		    strcmp(kctl->id.name, "Effect Feedback Volume") == 0) {
+			cval->min = 0x00;
+			cval->max = 0xff;
+			break;
+		}
+		if (strstr(kctl->id.name, "Effect Return") != NULL) {
+			cval->min = 0xb706;
+			cval->max = 0xff7b;
+			cval->res = 0x0073;
+			break;
+		}
+		if ((strstr(kctl->id.name, "Playback Volume") != NULL) ||
+			(strstr(kctl->id.name, "Effect Send") != NULL)) {
+			cval->min = 0xb5fb; /* -73 dB = 0xb6ff */
+			cval->max = 0xfcfe;
+			cval->res = 0x0073;
+		}
+		break;
+
 	case USB_ID(0x0763, 0x2081): /* M-Audio Fast Track Ultra 8R */
 	case USB_ID(0x0763, 0x2080): /* M-Audio Fast Track Ultra */
 		if (strcmp(kctl->id.name, "Effect Duration") == 0) {
-- 
1.7.8.6

[FT C400, PATCH RFC, v4 09/10] usb-audio: Fast Track C400 mixer controls

[Eldad Zack]

Add a mixer quirks for the M-Audio Fast Track C400
and create the following:

* Volume controls
* Effect Type (reusing FTU controls)
* Effect Volume
* Effect Send/Return
* Effect Program
* Effect Feedback

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/mixer_quirks.c |  176 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 176 insertions(+), 0 deletions(-)

diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c
index a614dab..bf28a1b 100644
--- a/sound/usb/mixer_quirks.c
+++ b/sound/usb/mixer_quirks.c
@@ -1027,6 +1027,178 @@ void snd_emuusb_set_samplerate(struct snd_usb_audio *chip,
 	}
 }
 
+/* M-Audio Fast Track C400 */
+/* C400 volume controls, this control needs a volume quirk, see mixer.c */
+static int snd_c400_create_vol_ctls(struct usb_mixer_interface *mixer)
+{
+	char name[64];
+	unsigned int cmask, offset;
+	int out, chan, err;
+
+	const unsigned int id = 0x40;
+	const int val_type = USB_MIXER_S16;
+	const int control = 1;
+
+	for (chan = 0; chan < 10; chan++) {
+		for (out = 0; out < 6; out++) {
+			if (chan < 6) {
+				snprintf(name, sizeof(name),
+					"PCM%d-Out%d Playback Volume",
+					chan + 1, out + 1);
+			} else {
+				snprintf(name, sizeof(name),
+					"In%d-Out%d Playback Volume",
+					chan - 5, out + 1);
+			}
+
+			cmask = (out == 0) ? 0 : 1 << (out - 1);
+			offset = chan * 6;
+			err = snd_create_std_mono_ctl_offset(mixer, id, control,
+						cmask, val_type, offset, name,
+						&snd_usb_mixer_vol_tlv);
+			if (err < 0)
+				return err;
+		}
+	}
+
+	return 0;
+}
+
+/* This control needs a volume quirk, see mixer.c */
+static int snd_c400_create_effect_volume_ctl(struct usb_mixer_interface *mixer)
+{
+	static const char name[] = "Effect Volume";
+	const unsigned int id = 0x43;
+	const int val_type = USB_MIXER_U8;
+	const unsigned int control = 3;
+	const unsigned int cmask = 0;
+
+	return snd_create_std_mono_ctl(mixer, id, control, cmask, val_type,
+					name, snd_usb_mixer_vol_tlv);
+}
+
+/* This control needs a volume quirk, see mixer.c */
+static int snd_c400_create_effect_duration_ctl(struct usb_mixer_interface *mixer)
+{
+	static const char name[] = "Effect Duration";
+	const unsigned int id = 0x43;
+	const int val_type = USB_MIXER_S16;
+	const unsigned int control = 4;
+	const unsigned int cmask = 0;
+
+	return snd_create_std_mono_ctl(mixer, id, control, cmask, val_type,
+					name, snd_usb_mixer_vol_tlv);
+}
+
+/* This control needs a volume quirk, see mixer.c */
+static int snd_c400_create_effect_feedback_ctl(struct usb_mixer_interface *mixer)
+{
+	static const char name[] = "Effect Feedback Volume";
+	const unsigned int id = 0x43;
+	const int val_type = USB_MIXER_U8;
+	const unsigned int control = 5;
+	const unsigned int cmask = 0;
+
+	return snd_create_std_mono_ctl(mixer, id, control, cmask, val_type,
+					name, NULL);
+}
+
+static int snd_c400_create_effect_vol_ctls(struct usb_mixer_interface *mixer)
+{
+	char name[64];
+	unsigned int cmask;
+	int chan, err;
+
+	const unsigned int id = 0x42;
+	const int val_type = USB_MIXER_S16;
+	const int control = 1;
+
+	for (chan = 0; chan < 10; chan++) {
+		if (chan < 6) {
+			snprintf(name, sizeof(name),
+				"Effect Send DOut%d",
+				chan + 1);
+		} else {
+			snprintf(name, sizeof(name),
+				"Effect Send AIn%d",
+				chan - 5);
+		}
+
+		cmask = (chan == 0) ? 0 : 1 << (chan - 1);
+		err = snd_create_std_mono_ctl(mixer, id, control,
+						cmask, val_type, name,
+						&snd_usb_mixer_vol_tlv);
+		if (err < 0)
+			return err;
+	}
+
+	return 0;
+}
+
+static int snd_c400_create_effect_ret_vol_ctls(struct usb_mixer_interface *mixer)
+{
+	char name[64];
+	unsigned int cmask;
+	int chan, err;
+
+	const unsigned int id = 0x40;
+	const int val_type = USB_MIXER_S16;
+	const int control = 1;
+	const int chan_id[6] = { 0, 7, 2, 9, 4, 0xb };
+	const unsigned int offset = 0x3c;
+				/* { 0x3c, 0x43, 0x3e, 0x45, 0x40, 0x47 } */
+
+	for (chan = 0; chan < 6; chan++) {
+		snprintf(name, sizeof(name),
+			"Effect Return %d",
+			chan + 1);
+
+		cmask = (chan_id[chan] == 0) ? 0 : 1 << (chan_id[chan] - 1);
+		err = snd_create_std_mono_ctl_offset(mixer, id, control,
+						cmask, val_type, offset, name,
+						&snd_usb_mixer_vol_tlv);
+		if (err < 0)
+			return err;
+	}
+
+	return 0;
+}
+
+static int snd_c400_create_mixer(struct usb_mixer_interface *mixer)
+{
+	int err;
+
+	err = snd_c400_create_vol_ctls(mixer);
+	if (err < 0)
+		return err;
+
+	err = snd_c400_create_effect_vol_ctls(mixer);
+	if (err < 0)
+		return err;
+
+	err = snd_c400_create_effect_ret_vol_ctls(mixer);
+	if (err < 0)
+		return err;
+
+	err = snd_ftu_create_effect_switch(mixer, 2, 0x43);
+	if (err < 0)
+		return err;
+
+	err = snd_c400_create_effect_volume_ctl(mixer);
+	if (err < 0)
+		return err;
+
+	err = snd_c400_create_effect_duration_ctl(mixer);
+	if (err < 0)
+		return err;
+
+	err = snd_c400_create_effect_feedback_ctl(mixer);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
 /*
  * The mixer units for Ebox-44 are corrupt, and even where they
  * are valid they presents mono controls as L and R channels of
@@ -1124,6 +1296,10 @@ int snd_usb_mixer_apply_create_quirk(struct usb_mixer_interface *mixer)
 					      snd_audigy2nx_proc_read);
 		break;
 
+	case USB_ID(0x0763, 0x2030): /* M-Audio Fast Track C400 */
+		err = snd_c400_create_mixer(mixer);
+		break;
+
 	case USB_ID(0x0763, 0x2080): /* M-Audio Fast Track Ultra */
 	case USB_ID(0x0763, 0x2081): /* M-Audio Fast Track Ultra 8R */
 		err = snd_ftu_create_mixer(mixer);
-- 
1.7.8.6

[FT C400, PATCH RFC, v4 10/10] usb-audio: FT C400 sync playback EP to capture EP

[Eldad Zack]

The playback endpoint uses implicit feedback mode, similar
to the M-Audio FTU. Like with the FTU, we need to associate
the sync pipe ourselves.

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/pcm.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 2158943..f30f46ca 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -359,6 +359,19 @@ static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt)
 	attr = fmt->ep_attr & USB_ENDPOINT_SYNCTYPE;
 
 	switch (subs->stream->chip->usb_id) {
+	case USB_ID(0x0763, 0x2030): /* M-Audio Fast Track C400 */
+		if (is_playback) {
+			implicit_fb = 1;
+			ep = 0x81;
+			iface = usb_ifnum_to_if(dev, 3);
+
+			if (!iface || iface->num_altsetting == 0)
+				return -EINVAL;
+
+			alts = &iface->altsetting[1];
+			goto add_sync_ep;
+		}
+		break;
 	case USB_ID(0x0763, 0x2080): /* M-Audio FastTrack Ultra */
 	case USB_ID(0x0763, 0x2081):
 		if (is_playback) {
-- 
1.7.8.6

Re: [FT C400, PATCH RFC, v4 00/10] M-Audio Fast Track C400

[Takashi Iwai]

At Wed, 28 Nov 2012 23:55:31 +0100,
Eldad Zack wrote:
> 
> Hi,
> 
> The following patches adds support for the M-Audio Fast Track C400.
> This is version 4 of the patch series.
> 
> * Incoroprated the feedback from Clemens Ladisch <clemens@ladisch.de>: 
> Moved the quirks structure to the right place (also in other places, though
> not strictly required) and removed the "set quirks" log messages.
> Thanks, Clemens!
> 
> * Added a patch to address the regression introduced by commit
> 947d299686aa9cc8aecf749d54e8475c6e498956 , "ALSA: snd-usb: properly initialize
> the sync endpoint". I believe there are no other devices that affected by this
> issue. Opening only the playback stream without first opening the capture
> stream now works with the C400.

Thanks.  I applied all patches except for the new one (2/10 usb-audio:
correct sync ep init).  This patch needs a bit more care, as it
seems.

I know it must be fixed for FT C400 but the fix itself is independent
from others, so I merged others first to the rest work easier.
 
(snip)
> * To deal with the mixer channel IDs, limited by cmask bit length,
> I introduced an offset value. This allows to get rid of the shorehorning
> of the IDs into cmask and device-referencing in the low-level functions.
> Is this a good approach?

Looks good enough to me.  It has a minimum impact, so safe to apply.


Takashi

Re: [FT C400, PATCH RFC, v4 02/10] usb-audio: correct sync ep init

[Takashi Iwai]

At Wed, 28 Nov 2012 23:55:33 +0100,
Eldad Zack wrote:
> 
> Commit 947d299686aa9cc8aecf749d54e8475c6e498956
> "ALSA: snd-usb: properly initialize the sync endpoint" introduced a
> regression for devices that has a sync endpoint with a channel
> number different than the data endpoint channel.
> Due to a different channel and period bytes count, attempting to
> initialize the sync endpoint will fail. With xhci it yields:
> 
> cannot submit urb 0, error -90: internal error
> 
> With this patch, if a sync endpoint has multiple audioformats, an
> audioformat with a matching number of channels is preferred. Otherwise,
> an audioformat will be picked randomly (i.e., first audioformat with a
> non-zero channel count) and the period bytes count is adjusted
> accordingly.
> 
> Cc: Jeffrey Barish <jeff_barish@earthlink.net>
> Cc: Daniel Mack <zonque@gmail.com>
> Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
> ---
>  sound/usb/pcm.c |   79 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 files changed, 72 insertions(+), 7 deletions(-)
> 
> diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
> index ff8cbbf..2158943 100644
> --- a/sound/usb/pcm.c
> +++ b/sound/usb/pcm.c
> @@ -438,6 +438,76 @@ add_sync_ep:
>  }
>  
>  /*
> + * Configure the sync ep using the rate and pcm format of the data ep.
> + */
> +static int configure_sync_endpoint(struct snd_usb_substream *subs)
> +{
> +	int ret;
> +	struct list_head *p;
> +	struct audioformat *sync_fp;
> +	int sync_channels = 0;
> +	int sync_period_bytes = subs->period_bytes;
> +	int direction = subs->direction ^ 1;

Better to rename for avoiding misunderstanding.
"direction" sounds like the direction of this substream.

> +	struct snd_usb_substream *sync_subs =
> +		&subs->stream->substream[direction];
> +
> +	if (sync_subs->ep_num != subs->sync_endpoint->ep_num) {
> +		snd_printk(KERN_ERR "%s: cannot find the sync ep (direction %d, found %x, wanted %x).\n",
> +			__func__, direction,
> +				subs->stream->substream[direction].ep_num,
> +				subs->sync_endpoint->ep_num);
> +		return -EINVAL;
> +	}

This sanity check would suit better where the implicit_fb is checked.
It doesn't have to be performed at each time.

And, the whole restriction you introduced is only for implicit fb
case, no?

> +
> +	/*
> +	 * Try to find a format with a number of channels matching
> +	 * the data substream. If no such format was found, pick one
> +	 * with non zero channel number at random.
> +	 */
> +	list_for_each(p, &sync_subs->fmt_list) {

Cleaner to use list_for_each_entry().
(Yes, there are other codes using list_for_each() there, but remember
 how old the usb-audio driver code is :)

> +		struct audioformat *fp;
> +		fp = list_entry(p, struct audioformat, list);
> +
> +		if (sync_channels == 0) {
> +			sync_fp = fp;
> +			sync_channels = fp->channels;
> +		} else {
> +			if (fp->channels == subs->channels) {
> +				sync_channels = fp->channels;
> +				sync_fp = fp;
> +			}

Can be simplified like

		if (!sync_channels || fp->channels == sub->channels) {
			sync_channels = fp->channels;
			sync_fp = fp;
		}

But, I wonder whether you don't have to check other parameters such as
rate, format, etc?


thanks,

Takashi

> +		}
> +	}
> +
> +	if (sync_channels < 1) {
> +		snd_printk(KERN_ERR "%s: no valid audioformat for sync ep %x found\n",
> +			__func__, sync_subs->ep_num);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Recalculated the period bytes if channel number differ between
> +	 * data and sync ep.
> +	 */
> +	if (sync_channels != subs->channels) {
> +		sync_period_bytes = (subs->period_bytes / subs->channels) *
> +			sync_channels;
> +		snd_printdd(KERN_INFO "%s: sync ep period bytes recalc %d -> %d\n",
> +			__func__, subs->period_bytes, sync_period_bytes);
> +	}
> +
> +	ret = snd_usb_endpoint_set_params(subs->sync_endpoint,
> +					  subs->pcm_format,
> +					  sync_channels,
> +					  sync_period_bytes,
> +					  subs->cur_rate,
> +					  sync_fp,
> +					  NULL);
> +
> +	return ret;
> +}
> +
> +/*
>   * configure endpoint params
>   *
>   * called  during initial setup and upon resume
> @@ -459,13 +529,8 @@ static int configure_endpoint(struct snd_usb_substream *subs)
>  		return ret;
>  
>  	if (subs->sync_endpoint)
> -		ret = snd_usb_endpoint_set_params(subs->sync_endpoint,
> -						  subs->pcm_format,
> -						  subs->channels,
> -						  subs->period_bytes,
> -						  subs->cur_rate,
> -						  subs->cur_audiofmt,
> -						  NULL);
> +		ret = configure_sync_endpoint(subs);
> +
>  	return ret;
>  }
>  
> -- 
> 1.7.8.6
> 

Re: [FT C400, PATCH RFC, v4 00/10] M-Audio Fast Track C400

[Eldad Zack]

Hi Takashi,

On Thu, 29 Nov 2012, Takashi Iwai wrote:
> At Wed, 28 Nov 2012 23:55:31 +0100,
> Eldad Zack wrote:
> > 
> > * Incoroprated the feedback from Clemens Ladisch <clemens@ladisch.de>: 
> > Moved the quirks structure to the right place (also in other places, though
> > not strictly required) and removed the "set quirks" log messages.
> > Thanks, Clemens!
> > 
> > * Added a patch to address the regression introduced by commit
> > 947d299686aa9cc8aecf749d54e8475c6e498956 , "ALSA: snd-usb: properly initialize
> > the sync endpoint". I believe there are no other devices that affected by this
> > issue. Opening only the playback stream without first opening the capture
> > stream now works with the C400.
> 
> Thanks.  I applied all patches except for the new one (2/10 usb-audio:
> correct sync ep init).  This patch needs a bit more care, as it
> seems.
> 
> I know it must be fixed for FT C400 but the fix itself is independent
> from others, so I merged others first to the rest work easier.

Thanks! That's great, now I only need to concentrate on this one
issue. I went through your helpful comments and I'll try to work out a
better patch soon and send it for review.
Should I wait with it until the 3.8 merge window closes?

BTW - given that the code is old, isn't it time to slightly change 
these? :)

card.c: *   (Tentative) USB Audio Driver for ALSA
mixer.c: *   (Tentative) USB Audio Driver for ALSA
usbaudio.h: *   (Tentative) USB Audio Driver for ALSA

Cheers,
Eldad

Re: [FT C400, PATCH RFC, v4 00/10] M-Audio Fast Track C400

[Takashi Iwai]

At Fri, 30 Nov 2012 15:06:00 +0100 (CET),
Eldad Zack wrote:
> 
> 
> Hi Takashi,
> 
> On Thu, 29 Nov 2012, Takashi Iwai wrote:
> > At Wed, 28 Nov 2012 23:55:31 +0100,
> > Eldad Zack wrote:
> > > 
> > > * Incoroprated the feedback from Clemens Ladisch <clemens@ladisch.de>: 
> > > Moved the quirks structure to the right place (also in other places, though
> > > not strictly required) and removed the "set quirks" log messages.
> > > Thanks, Clemens!
> > > 
> > > * Added a patch to address the regression introduced by commit
> > > 947d299686aa9cc8aecf749d54e8475c6e498956 , "ALSA: snd-usb: properly initialize
> > > the sync endpoint". I believe there are no other devices that affected by this
> > > issue. Opening only the playback stream without first opening the capture
> > > stream now works with the C400.
> > 
> > Thanks.  I applied all patches except for the new one (2/10 usb-audio:
> > correct sync ep init).  This patch needs a bit more care, as it
> > seems.
> > 
> > I know it must be fixed for FT C400 but the fix itself is independent
> > from others, so I merged others first to the rest work easier.
> 
> Thanks! That's great, now I only need to concentrate on this one
> issue. I went through your helpful comments and I'll try to work out a
> better patch soon and send it for review.
> Should I wait with it until the 3.8 merge window closes?

Your other patches have been already merged, so better be quick, so
that the last piece can be put in to 3.8, too :)

> BTW - given that the code is old, isn't it time to slightly change 
> these? :)
> 
> card.c: *   (Tentative) USB Audio Driver for ALSA
> mixer.c: *   (Tentative) USB Audio Driver for ALSA
> usbaudio.h: *   (Tentative) USB Audio Driver for ALSA

Yes, clean up patches are welcome.  But these are in a lower priority,
so will be queued for 3.9 later.


thanks,

Takashi

Re: [FT C400, PATCH RFC, v4 02/10] usb-audio: correct sync ep init

[Eldad Zack]

Hi Takashi,

On Thu, 29 Nov 2012, Takashi Iwai wrote:
> At Wed, 28 Nov 2012 23:55:33 +0100,
> Eldad Zack wrote:
> >  /*
> > + * Configure the sync ep using the rate and pcm format of the data ep.
> > + */
> > +static int configure_sync_endpoint(struct snd_usb_substream *subs)
> > +{
> > +	int ret;
> > +	struct list_head *p;
> > +	struct audioformat *sync_fp;
> > +	int sync_channels = 0;
> > +	int sync_period_bytes = subs->period_bytes;
> > +	int direction = subs->direction ^ 1;
> 
> Better to rename for avoiding misunderstanding.
> "direction" sounds like the direction of this substream.

Thanks for the pointer. I ended up getting rid rid of it 
altogether, since the following is redundant:

> > +	struct snd_usb_substream *sync_subs =
> > +		&subs->stream->substream[direction];
> > +
> > +	if (sync_subs->ep_num != subs->sync_endpoint->ep_num) {
> > +		snd_printk(KERN_ERR "%s: cannot find the sync ep (direction %d, found %x, wanted %x).\n",
> > +			__func__, direction,
> > +				subs->stream->substream[direction].ep_num,
> > +				subs->sync_endpoint->ep_num);
> > +		return -EINVAL;
> > +	}
> 
> This sanity check would suit better where the implicit_fb is checked.
> It doesn't have to be performed at each time.
> 
> And, the whole restriction you introduced is only for implicit fb
> case, no?

Yes, I took a closer look and saw that the only way for the 
sync_endpoint member to be at all initialized is when it is 1) implicit 
fb, 2) in the same stream as the data and in the opposite direction,
so there's no point in checking that at all.

> > +	/*
> > +	 * Try to find a format with a number of channels matching
> > +	 * the data substream. If no such format was found, pick one
> > +	 * with non zero channel number at random.
> > +	 */
> > +	list_for_each(p, &sync_subs->fmt_list) {
> Cleaner to use list_for_each_entry().
> (Yes, there are other codes using list_for_each() there, but remember
>  how old the usb-audio driver code is :)

Thanks again, it looks more concise that way. I'll see if it is possible 
to clean up other spots the same way and post later.

> Can be simplified like
> 
> 		if (!sync_channels || fp->channels == sub->channels) {
> 			sync_channels = fp->channels;
> 			sync_fp = fp;
> 		}
> 
> But, I wonder whether you don't have to check other parameters such as
> rate, format, etc?

Yes, especially since both rate and format will influence the period 
bytes count and then the usb host driver will error out. It is unlikely
that such a configuration exist, which doesn't support the same rate
and PCM format in both direction, but it is worth checking.

Patch follows.

Cheers,
Eldad

[PATCH sound-unstable] usb-audio: sync ep init fix for audioformat mistmatch

[Eldad Zack]

Commit 947d299686aa9cc8aecf749d54e8475c6e498956 , "ALSA: snd-usb:
properly initialize the sync endpoint", while correcting the
initialization of the sync endpoint when opening just the data
endpoint, prevents devices that has a sync endpoint with a channel
number different than the data endpoint channel from functioning.
Due to a different channel and period bytes count. attempting to
initialize the sync endpoint will fail at the usb host driver:
(example, when using xhci:)

 cannot submit urb 0, error -90: internal error

With this patch, if a sync endpoint has multiple audioformats, a
matching audioformat is perferred. An audioformat must be found
with at least one channel and support the requested sample rate
and PCM format, otherwise the stream will not be opened.

If the number of channels differ between the selected audioformat
and the requested format, adjust the period bytes count accordingly.
It is safe to perform the calculation on the basis of the channel
count, since the PCM audio format and the rate must be supported by
the selected audioformat.

Cc: Jeffrey Barish <jeff_barish@earthlink.net>
Cc: Daniel Mack <zonque@gmail.com>
Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/pcm.c |  106 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 99 insertions(+), 7 deletions(-)

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 5d3cf85..e539a5a 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -451,6 +451,103 @@ add_sync_ep:
 }
 
 /*
+ * Return the score of matching two audioformats.
+ * Veto the audioformat if:
+ * - It has no channels for some reason.
+ * - Requested PCM format is not supported.
+ * - Requested sample rate is not supported.
+ */
+static int match_endpoint_audioformats(struct audioformat *fp,
+	struct audioformat *match, int rate,
+	snd_pcm_format_t pcm_format)
+{
+	int i;
+	int score = 0;
+
+	if (fp->channels < 1) {
+		snd_printdd("%s: (fmt @%p) no channels\n", __func__, fp);
+		return 0;
+	}
+
+	if (!(fp->formats && pcm_format)) {
+		snd_printdd("%s: (fmt @%p) no match for format %d\n", __func__,
+			fp, pcm_format);
+		return 0;
+	}
+
+	for (i = 0; i < 4; i++) {
+		if (fp->rate_table[i] == rate) {
+			score++;
+			break;
+		}
+	}
+	if (!score) {
+		snd_printdd("%s: (fmt @%p) no match for rate %d\n", __func__,
+			fp, rate);
+		return 0;
+	}
+
+	if (fp->channels == match->channels)
+		score++;
+
+	snd_printdd("%s: (fmt @%p) score %d\n", __func__, fp, score);
+
+	return score;
+}
+
+/*
+ * Configure the sync ep using the rate and pcm format of the data ep.
+ */
+static int configure_sync_endpoint(struct snd_usb_substream *subs)
+{
+	int ret;
+	struct audioformat *fp;
+	struct audioformat *sync_fp = NULL;
+	int cur_score = 0;
+	int sync_period_bytes = subs->period_bytes;
+	struct snd_usb_substream *sync_subs =
+		&subs->stream->substream[subs->direction ^ 1];
+
+	/* Try to find the best matching audioformat. */
+	list_for_each_entry(fp, &sync_subs->fmt_list, list) {
+		int score = match_endpoint_audioformats(fp, subs->cur_audiofmt,
+			subs->cur_rate, subs->pcm_format);
+
+		if (score > cur_score) {
+			sync_fp = fp;
+			cur_score = score;
+		}
+	}
+
+	if (unlikely(sync_fp == NULL)) {
+		snd_printk(KERN_ERR "%s: no valid audioformat for sync ep %x found\n",
+			__func__, sync_subs->ep_num);
+		return -EINVAL;
+	}
+
+	/*
+	 * Recalculated the period bytes if channel number differ between
+	 * data and sync ep audioformat.
+	 */
+	if (sync_fp->channels != subs->channels) {
+		sync_period_bytes = (subs->period_bytes / subs->channels) *
+			sync_fp->channels;
+		snd_printdd("%s: adjusted sync ep period bytes (%d -> %d)\n",
+			__func__, subs->period_bytes, sync_period_bytes);
+	}
+
+	ret = snd_usb_endpoint_set_params(subs->sync_endpoint,
+					  subs->pcm_format,
+					  sync_fp->channels,
+					  sync_period_bytes,
+					  subs->cur_rate,
+					  sync_fp,
+					  NULL);
+
+	return ret;
+}
+
+/*
  * configure endpoint params
  *
  * called  during initial setup and upon resume
@@ -472,13 +569,8 @@ static int configure_endpoint(struct snd_usb_substream *subs)
 		return ret;
 
 	if (subs->sync_endpoint)
-		ret = snd_usb_endpoint_set_params(subs->sync_endpoint,
-						  subs->pcm_format,
-						  subs->channels,
-						  subs->period_bytes,
-						  subs->cur_rate,
-						  subs->cur_audiofmt,
-						  NULL);
+		ret = configure_sync_endpoint(subs);
+
 	return ret;
 }
 
-- 
1.7.8.6

Re: [PATCH sound-unstable] usb-audio: sync ep init fix for audioformat mistmatch

[Takashi Iwai]

At Sat,  1 Dec 2012 23:50:26 +0100,
Eldad Zack wrote:
> 
> Commit 947d299686aa9cc8aecf749d54e8475c6e498956 , "ALSA: snd-usb:
> properly initialize the sync endpoint", while correcting the
> initialization of the sync endpoint when opening just the data
> endpoint, prevents devices that has a sync endpoint with a channel
> number different than the data endpoint channel from functioning.
> Due to a different channel and period bytes count. attempting to
> initialize the sync endpoint will fail at the usb host driver:
> (example, when using xhci:)
> 
>  cannot submit urb 0, error -90: internal error
> 
> With this patch, if a sync endpoint has multiple audioformats, a
> matching audioformat is perferred. An audioformat must be found
> with at least one channel and support the requested sample rate
> and PCM format, otherwise the stream will not be opened.
> 
> If the number of channels differ between the selected audioformat
> and the requested format, adjust the period bytes count accordingly.
> It is safe to perform the calculation on the basis of the channel
> count, since the PCM audio format and the rate must be supported by
> the selected audioformat.
> 
> Cc: Jeffrey Barish <jeff_barish@earthlink.net>
> Cc: Daniel Mack <zonque@gmail.com>
> Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
> ---
>  sound/usb/pcm.c |  106 +++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 files changed, 99 insertions(+), 7 deletions(-)
> 
> diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
> index 5d3cf85..e539a5a 100644
> --- a/sound/usb/pcm.c
> +++ b/sound/usb/pcm.c
> @@ -451,6 +451,103 @@ add_sync_ep:
>  }
>  
>  /*
> + * Return the score of matching two audioformats.
> + * Veto the audioformat if:
> + * - It has no channels for some reason.
> + * - Requested PCM format is not supported.
> + * - Requested sample rate is not supported.
> + */
> +static int match_endpoint_audioformats(struct audioformat *fp,
> +	struct audioformat *match, int rate,
> +	snd_pcm_format_t pcm_format)
> +{
> +	int i;
> +	int score = 0;
> +
> +	if (fp->channels < 1) {
> +		snd_printdd("%s: (fmt @%p) no channels\n", __func__, fp);
> +		return 0;
> +	}
> +
> +	if (!(fp->formats && pcm_format)) {
> +		snd_printdd("%s: (fmt @%p) no match for format %d\n", __func__,
> +			fp, pcm_format);
> +		return 0;
> +	}
> +
> +	for (i = 0; i < 4; i++) {

Use fp->nr_rates instead of 4.
Otherwise looks good to me.


thanks,

Takashi


> +		if (fp->rate_table[i] == rate) {
> +			score++;
> +			break;
> +		}
> +	}
> +	if (!score) {
> +		snd_printdd("%s: (fmt @%p) no match for rate %d\n", __func__,
> +			fp, rate);
> +		return 0;
> +	}
> +
> +	if (fp->channels == match->channels)
> +		score++;
> +
> +	snd_printdd("%s: (fmt @%p) score %d\n", __func__, fp, score);
> +
> +	return score;
> +}
> +
> +/*
> + * Configure the sync ep using the rate and pcm format of the data ep.
> + */
> +static int configure_sync_endpoint(struct snd_usb_substream *subs)
> +{
> +	int ret;
> +	struct audioformat *fp;
> +	struct audioformat *sync_fp = NULL;
> +	int cur_score = 0;
> +	int sync_period_bytes = subs->period_bytes;
> +	struct snd_usb_substream *sync_subs =
> +		&subs->stream->substream[subs->direction ^ 1];
> +
> +	/* Try to find the best matching audioformat. */
> +	list_for_each_entry(fp, &sync_subs->fmt_list, list) {
> +		int score = match_endpoint_audioformats(fp, subs->cur_audiofmt,
> +			subs->cur_rate, subs->pcm_format);
> +
> +		if (score > cur_score) {
> +			sync_fp = fp;
> +			cur_score = score;
> +		}
> +	}
> +
> +	if (unlikely(sync_fp == NULL)) {
> +		snd_printk(KERN_ERR "%s: no valid audioformat for sync ep %x found\n",
> +			__func__, sync_subs->ep_num);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Recalculated the period bytes if channel number differ between
> +	 * data and sync ep audioformat.
> +	 */
> +	if (sync_fp->channels != subs->channels) {
> +		sync_period_bytes = (subs->period_bytes / subs->channels) *
> +			sync_fp->channels;
> +		snd_printdd("%s: adjusted sync ep period bytes (%d -> %d)\n",
> +			__func__, subs->period_bytes, sync_period_bytes);
> +	}
> +
> +	ret = snd_usb_endpoint_set_params(subs->sync_endpoint,
> +					  subs->pcm_format,
> +					  sync_fp->channels,
> +					  sync_period_bytes,
> +					  subs->cur_rate,
> +					  sync_fp,
> +					  NULL);
> +
> +	return ret;
> +}
> +
> +/*
>   * configure endpoint params
>   *
>   * called  during initial setup and upon resume
> @@ -472,13 +569,8 @@ static int configure_endpoint(struct snd_usb_substream *subs)
>  		return ret;
>  
>  	if (subs->sync_endpoint)
> -		ret = snd_usb_endpoint_set_params(subs->sync_endpoint,
> -						  subs->pcm_format,
> -						  subs->channels,
> -						  subs->period_bytes,
> -						  subs->cur_rate,
> -						  subs->cur_audiofmt,
> -						  NULL);
> +		ret = configure_sync_endpoint(subs);
> +
>  	return ret;
>  }
>  
> -- 
> 1.7.8.6
> 

Re: [PATCH sound-unstable] usb-audio: sync ep init fix for audioformat mistmatch

[Daniel Mack]

On 01.12.2012 23:50, Eldad Zack wrote:
> Commit 947d299686aa9cc8aecf749d54e8475c6e498956 , "ALSA: snd-usb:
> properly initialize the sync endpoint", while correcting the
> initialization of the sync endpoint when opening just the data
> endpoint, prevents devices that has a sync endpoint with a channel
> number different than the data endpoint channel from functioning.
> Due to a different channel and period bytes count. attempting to
> initialize the sync endpoint will fail at the usb host driver:
> (example, when using xhci:)
> 
>  cannot submit urb 0, error -90: internal error
> 
> With this patch, if a sync endpoint has multiple audioformats, a
> matching audioformat is perferred. An audioformat must be found
> with at least one channel and support the requested sample rate
> and PCM format, otherwise the stream will not be opened.
> 
> If the number of channels differ between the selected audioformat
> and the requested format, adjust the period bytes count accordingly.
> It is safe to perform the calculation on the basis of the channel
> count, since the PCM audio format and the rate must be supported by
> the selected audioformat.

Thanks for fixing this up, and sorry for the late response on this.

> 
> Cc: Jeffrey Barish <jeff_barish@earthlink.net>
> Cc: Daniel Mack <zonque@gmail.com>
> Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
> ---
>  sound/usb/pcm.c |  106 +++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 files changed, 99 insertions(+), 7 deletions(-)
> 
> diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
> index 5d3cf85..e539a5a 100644
> --- a/sound/usb/pcm.c
> +++ b/sound/usb/pcm.c
> @@ -451,6 +451,103 @@ add_sync_ep:
>  }
>  
>  /*
> + * Return the score of matching two audioformats.
> + * Veto the audioformat if:
> + * - It has no channels for some reason.
> + * - Requested PCM format is not supported.
> + * - Requested sample rate is not supported.
> + */
> +static int match_endpoint_audioformats(struct audioformat *fp,
> +	struct audioformat *match, int rate,
> +	snd_pcm_format_t pcm_format)
> +{
> +	int i;
> +	int score = 0;
> +
> +	if (fp->channels < 1) {
> +		snd_printdd("%s: (fmt @%p) no channels\n", __func__, fp);
> +		return 0;
> +	}
> +
> +	if (!(fp->formats && pcm_format)) {

That should be a binary &, no?

Otherwise, together with the fix Takashi suggested, looks good to me as
well.


Thanks,
Daniel


> +		snd_printdd("%s: (fmt @%p) no match for format %d\n", __func__,
> +			fp, pcm_format);
> +		return 0;
> +	}
> +
> +	for (i = 0; i < 4; i++) {
> +		if (fp->rate_table[i] == rate) {
> +			score++;
> +			break;
> +		}
> +	}
> +	if (!score) {
> +		snd_printdd("%s: (fmt @%p) no match for rate %d\n", __func__,
> +			fp, rate);
> +		return 0;
> +	}
> +
> +	if (fp->channels == match->channels)
> +		score++;
> +
> +	snd_printdd("%s: (fmt @%p) score %d\n", __func__, fp, score);
> +
> +	return score;
> +}
> +
> +/*
> + * Configure the sync ep using the rate and pcm format of the data ep.
> + */
> +static int configure_sync_endpoint(struct snd_usb_substream *subs)
> +{
> +	int ret;
> +	struct audioformat *fp;
> +	struct audioformat *sync_fp = NULL;
> +	int cur_score = 0;
> +	int sync_period_bytes = subs->period_bytes;
> +	struct snd_usb_substream *sync_subs =
> +		&subs->stream->substream[subs->direction ^ 1];
> +
> +	/* Try to find the best matching audioformat. */
> +	list_for_each_entry(fp, &sync_subs->fmt_list, list) {
> +		int score = match_endpoint_audioformats(fp, subs->cur_audiofmt,
> +			subs->cur_rate, subs->pcm_format);
> +
> +		if (score > cur_score) {
> +			sync_fp = fp;
> +			cur_score = score;
> +		}
> +	}
> +
> +	if (unlikely(sync_fp == NULL)) {
> +		snd_printk(KERN_ERR "%s: no valid audioformat for sync ep %x found\n",
> +			__func__, sync_subs->ep_num);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Recalculated the period bytes if channel number differ between
> +	 * data and sync ep audioformat.
> +	 */
> +	if (sync_fp->channels != subs->channels) {
> +		sync_period_bytes = (subs->period_bytes / subs->channels) *
> +			sync_fp->channels;
> +		snd_printdd("%s: adjusted sync ep period bytes (%d -> %d)\n",
> +			__func__, subs->period_bytes, sync_period_bytes);
> +	}
> +
> +	ret = snd_usb_endpoint_set_params(subs->sync_endpoint,
> +					  subs->pcm_format,
> +					  sync_fp->channels,
> +					  sync_period_bytes,
> +					  subs->cur_rate,
> +					  sync_fp,
> +					  NULL);
> +
> +	return ret;
> +}
> +
> +/*
>   * configure endpoint params
>   *
>   * called  during initial setup and upon resume
> @@ -472,13 +569,8 @@ static int configure_endpoint(struct snd_usb_substream *subs)
>  		return ret;
>  
>  	if (subs->sync_endpoint)
> -		ret = snd_usb_endpoint_set_params(subs->sync_endpoint,
> -						  subs->pcm_format,
> -						  subs->channels,
> -						  subs->period_bytes,
> -						  subs->cur_rate,
> -						  subs->cur_audiofmt,
> -						  NULL);
> +		ret = configure_sync_endpoint(subs);
> +
>  	return ret;
>  }
>  
> 

Re: [PATCH sound-unstable] usb-audio: sync ep init fix for audioformat mistmatch

[Takashi Iwai]

At Mon, 03 Dec 2012 10:34:29 +0100,
Daniel Mack wrote:
> 
> On 01.12.2012 23:50, Eldad Zack wrote:
> > Commit 947d299686aa9cc8aecf749d54e8475c6e498956 , "ALSA: snd-usb:
> > properly initialize the sync endpoint", while correcting the
> > initialization of the sync endpoint when opening just the data
> > endpoint, prevents devices that has a sync endpoint with a channel
> > number different than the data endpoint channel from functioning.
> > Due to a different channel and period bytes count. attempting to
> > initialize the sync endpoint will fail at the usb host driver:
> > (example, when using xhci:)
> > 
> >  cannot submit urb 0, error -90: internal error
> > 
> > With this patch, if a sync endpoint has multiple audioformats, a
> > matching audioformat is perferred. An audioformat must be found
> > with at least one channel and support the requested sample rate
> > and PCM format, otherwise the stream will not be opened.
> > 
> > If the number of channels differ between the selected audioformat
> > and the requested format, adjust the period bytes count accordingly.
> > It is safe to perform the calculation on the basis of the channel
> > count, since the PCM audio format and the rate must be supported by
> > the selected audioformat.
> 
> Thanks for fixing this up, and sorry for the late response on this.
> 
> > 
> > Cc: Jeffrey Barish <jeff_barish@earthlink.net>
> > Cc: Daniel Mack <zonque@gmail.com>
> > Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
> > ---
> >  sound/usb/pcm.c |  106 +++++++++++++++++++++++++++++++++++++++++++++++++++----
> >  1 files changed, 99 insertions(+), 7 deletions(-)
> > 
> > diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
> > index 5d3cf85..e539a5a 100644
> > --- a/sound/usb/pcm.c
> > +++ b/sound/usb/pcm.c
> > @@ -451,6 +451,103 @@ add_sync_ep:
> >  }
> >  
> >  /*
> > + * Return the score of matching two audioformats.
> > + * Veto the audioformat if:
> > + * - It has no channels for some reason.
> > + * - Requested PCM format is not supported.
> > + * - Requested sample rate is not supported.
> > + */
> > +static int match_endpoint_audioformats(struct audioformat *fp,
> > +	struct audioformat *match, int rate,
> > +	snd_pcm_format_t pcm_format)
> > +{
> > +	int i;
> > +	int score = 0;
> > +
> > +	if (fp->channels < 1) {
> > +		snd_printdd("%s: (fmt @%p) no channels\n", __func__, fp);
> > +		return 0;
> > +	}
> > +
> > +	if (!(fp->formats && pcm_format)) {
> 
> That should be a binary &, no?

Oh yeah, this must be

	if (!(fp->formats && (1ULL << pcm_format)))


Takashi

> 
> Otherwise, together with the fix Takashi suggested, looks good to me as
> well.
> 
> 
> Thanks,
> Daniel
> 
> 
> > +		snd_printdd("%s: (fmt @%p) no match for format %d\n", __func__,
> > +			fp, pcm_format);
> > +		return 0;
> > +	}
> > +
> > +	for (i = 0; i < 4; i++) {
> > +		if (fp->rate_table[i] == rate) {
> > +			score++;
> > +			break;
> > +		}
> > +	}
> > +	if (!score) {
> > +		snd_printdd("%s: (fmt @%p) no match for rate %d\n", __func__,
> > +			fp, rate);
> > +		return 0;
> > +	}
> > +
> > +	if (fp->channels == match->channels)
> > +		score++;
> > +
> > +	snd_printdd("%s: (fmt @%p) score %d\n", __func__, fp, score);
> > +
> > +	return score;
> > +}
> > +
> > +/*
> > + * Configure the sync ep using the rate and pcm format of the data ep.
> > + */
> > +static int configure_sync_endpoint(struct snd_usb_substream *subs)
> > +{
> > +	int ret;
> > +	struct audioformat *fp;
> > +	struct audioformat *sync_fp = NULL;
> > +	int cur_score = 0;
> > +	int sync_period_bytes = subs->period_bytes;
> > +	struct snd_usb_substream *sync_subs =
> > +		&subs->stream->substream[subs->direction ^ 1];
> > +
> > +	/* Try to find the best matching audioformat. */
> > +	list_for_each_entry(fp, &sync_subs->fmt_list, list) {
> > +		int score = match_endpoint_audioformats(fp, subs->cur_audiofmt,
> > +			subs->cur_rate, subs->pcm_format);
> > +
> > +		if (score > cur_score) {
> > +			sync_fp = fp;
> > +			cur_score = score;
> > +		}
> > +	}
> > +
> > +	if (unlikely(sync_fp == NULL)) {
> > +		snd_printk(KERN_ERR "%s: no valid audioformat for sync ep %x found\n",
> > +			__func__, sync_subs->ep_num);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/*
> > +	 * Recalculated the period bytes if channel number differ between
> > +	 * data and sync ep audioformat.
> > +	 */
> > +	if (sync_fp->channels != subs->channels) {
> > +		sync_period_bytes = (subs->period_bytes / subs->channels) *
> > +			sync_fp->channels;
> > +		snd_printdd("%s: adjusted sync ep period bytes (%d -> %d)\n",
> > +			__func__, subs->period_bytes, sync_period_bytes);
> > +	}
> > +
> > +	ret = snd_usb_endpoint_set_params(subs->sync_endpoint,
> > +					  subs->pcm_format,
> > +					  sync_fp->channels,
> > +					  sync_period_bytes,
> > +					  subs->cur_rate,
> > +					  sync_fp,
> > +					  NULL);
> > +
> > +	return ret;
> > +}
> > +
> > +/*
> >   * configure endpoint params
> >   *
> >   * called  during initial setup and upon resume
> > @@ -472,13 +569,8 @@ static int configure_endpoint(struct snd_usb_substream *subs)
> >  		return ret;
> >  
> >  	if (subs->sync_endpoint)
> > -		ret = snd_usb_endpoint_set_params(subs->sync_endpoint,
> > -						  subs->pcm_format,
> > -						  subs->channels,
> > -						  subs->period_bytes,
> > -						  subs->cur_rate,
> > -						  subs->cur_audiofmt,
> > -						  NULL);
> > +		ret = configure_sync_endpoint(subs);
> > +
> >  	return ret;
> >  }
> >  
> > 
> 

Re: [PATCH sound-unstable] usb-audio: sync ep init fix for audioformat mistmatch

[Eldad Zack]

Hi Daniel, Takashi,

On Mon, 3 Dec 2012, Takashi Iwai wrote:
> At Mon, 03 Dec 2012 10:34:29 +0100,
> Daniel Mack wrote:
> > On 01.12.2012 23:50, Eldad Zack wrote:
> > 
> > Thanks for fixing this up, and sorry for the late response on this.

It was fun to write, so I'm glad you were busy with other things :)

> > > +	if (!(fp->formats && pcm_format)) {
> > 
> > That should be a binary &, no?
> 
> Oh yeah, this must be
> 
> 	if (!(fp->formats && (1ULL << pcm_format)))

Oh, I see now I managed to somehow concoct two bugs in one line.
At least it worked because of the logical & ...

Fixed now, and of course the nr_rates.
I'll test and submit soon.

Many thanks to both of you, I appreciate it!

Cheers,
Eldad

[PATCH sound-unstable] usb-audio: sync ep init fix for audioformat mismatch

[Eldad Zack]

Commit 947d299686aa9cc8aecf749d54e8475c6e498956 , "ALSA: snd-usb:
properly initialize the sync endpoint", while correcting the
initialization of the sync endpoint when opening just the data
endpoint, prevents devices that has a sync endpoint, with a channel
number different than that of the data endpoint, from functioning.
Due to a different channel and period bytes count, attempting to
initialize the sync endpoint will fail at the usb host driver.
For example, when using xhci:

 cannot submit urb 0, error -90: internal error

With this patch, if a sync endpoint has multiple audioformats, a
matching audioformat is preferred. An audioformat must be found
with at least one channel and support the requested sample rate
and PCM format, otherwise the stream will not be opened.

If the number of channels differ between the selected audioformat
and the requested format, adjust the period bytes count accordingly.
It is safe to perform the calculation on the basis of the channel
count, since the requested PCM audio format and the rate must be
supported by the selected audioformat.

Cc: Jeffrey Barish <jeff_barish@earthlink.net>
Cc: Daniel Mack <zonque@gmail.com>
Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 sound/usb/pcm.c |  106 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 99 insertions(+), 7 deletions(-)

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 5d3cf85..ec0318c 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -451,6 +451,103 @@ add_sync_ep:
 }
 
 /*
+ * Return the score of matching two audioformats.
+ * Veto the audioformat if:
+ * - It has no channels for some reason.
+ * - Requested PCM format is not supported.
+ * - Requested sample rate is not supported.
+ */
+static int match_endpoint_audioformats(struct audioformat *fp,
+	struct audioformat *match, int rate,
+	snd_pcm_format_t pcm_format)
+{
+	int i;
+	int score = 0;
+
+	if (fp->channels < 1) {
+		snd_printdd("%s: (fmt @%p) no channels\n", __func__, fp);
+		return 0;
+	}
+
+	if (!(fp->formats & (1ULL << pcm_format))) {
+		snd_printdd("%s: (fmt @%p) no match for format %d\n", __func__,
+			fp, pcm_format);
+		return 0;
+	}
+
+	for (i = 0; i < fp->nr_rates; i++) {
+		if (fp->rate_table[i] == rate) {
+			score++;
+			break;
+		}
+	}
+	if (!score) {
+		snd_printdd("%s: (fmt @%p) no match for rate %d\n", __func__,
+			fp, rate);
+		return 0;
+	}
+
+	if (fp->channels == match->channels)
+		score++;
+
+	snd_printdd("%s: (fmt @%p) score %d\n", __func__, fp, score);
+
+	return score;
+}
+
+/*
+ * Configure the sync ep using the rate and pcm format of the data ep.
+ */
+static int configure_sync_endpoint(struct snd_usb_substream *subs)
+{
+	int ret;
+	struct audioformat *fp;
+	struct audioformat *sync_fp = NULL;
+	int cur_score = 0;
+	int sync_period_bytes = subs->period_bytes;
+	struct snd_usb_substream *sync_subs =
+		&subs->stream->substream[subs->direction ^ 1];
+
+	/* Try to find the best matching audioformat. */
+	list_for_each_entry(fp, &sync_subs->fmt_list, list) {
+		int score = match_endpoint_audioformats(fp, subs->cur_audiofmt,
+			subs->cur_rate, subs->pcm_format);
+
+		if (score > cur_score) {
+			sync_fp = fp;
+			cur_score = score;
+		}
+	}
+
+	if (unlikely(sync_fp == NULL)) {
+		snd_printk(KERN_ERR "%s: no valid audioformat for sync ep %x found\n",
+			__func__, sync_subs->ep_num);
+		return -EINVAL;
+	}
+
+	/*
+	 * Recalculate the period bytes if channel number differ between
+	 * data and sync ep audioformat.
+	 */
+	if (sync_fp->channels != subs->channels) {
+		sync_period_bytes = (subs->period_bytes / subs->channels) *
+			sync_fp->channels;
+		snd_printdd("%s: adjusted sync ep period bytes (%d -> %d)\n",
+			__func__, subs->period_bytes, sync_period_bytes);
+	}
+
+	ret = snd_usb_endpoint_set_params(subs->sync_endpoint,
+					  subs->pcm_format,
+					  sync_fp->channels,
+					  sync_period_bytes,
+					  subs->cur_rate,
+					  sync_fp,
+					  NULL);
+
+	return ret;
+}
+
+/*
  * configure endpoint params
  *
  * called  during initial setup and upon resume
@@ -472,13 +569,8 @@ static int configure_endpoint(struct snd_usb_substream *subs)
 		return ret;
 
 	if (subs->sync_endpoint)
-		ret = snd_usb_endpoint_set_params(subs->sync_endpoint,
-						  subs->pcm_format,
-						  subs->channels,
-						  subs->period_bytes,
-						  subs->cur_rate,
-						  subs->cur_audiofmt,
-						  NULL);
+		ret = configure_sync_endpoint(subs);
+
 	return ret;
 }
 
-- 
1.7.8.6

Re: [PATCH sound-unstable] usb-audio: sync ep init fix for audioformat mismatch

[Takashi Iwai]

At Mon,  3 Dec 2012 20:30:09 +0100,
Eldad Zack wrote:
> 
> Commit 947d299686aa9cc8aecf749d54e8475c6e498956 , "ALSA: snd-usb:
> properly initialize the sync endpoint", while correcting the
> initialization of the sync endpoint when opening just the data
> endpoint, prevents devices that has a sync endpoint, with a channel
> number different than that of the data endpoint, from functioning.
> Due to a different channel and period bytes count, attempting to
> initialize the sync endpoint will fail at the usb host driver.
> For example, when using xhci:
> 
>  cannot submit urb 0, error -90: internal error
> 
> With this patch, if a sync endpoint has multiple audioformats, a
> matching audioformat is preferred. An audioformat must be found
> with at least one channel and support the requested sample rate
> and PCM format, otherwise the stream will not be opened.
> 
> If the number of channels differ between the selected audioformat
> and the requested format, adjust the period bytes count accordingly.
> It is safe to perform the calculation on the basis of the channel
> count, since the requested PCM audio format and the rate must be
> supported by the selected audioformat.
> 
> Cc: Jeffrey Barish <jeff_barish@earthlink.net>
> Cc: Daniel Mack <zonque@gmail.com>
> Signed-off-by: Eldad Zack <eldad@fogrefinery.com>

Thanks, applied.

At the next time, however, create a patch based on either for-next or
for-linus branch of sound git tree.  sound-unstable tree is a place to
play with experimental patches.  The place to put the real patches
that will be fed to the upstream is sound git tree.


Takashi

> ---
>  sound/usb/pcm.c |  106 +++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 files changed, 99 insertions(+), 7 deletions(-)
> 
> diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
> index 5d3cf85..ec0318c 100644
> --- a/sound/usb/pcm.c
> +++ b/sound/usb/pcm.c
> @@ -451,6 +451,103 @@ add_sync_ep:
>  }
>  
>  /*
> + * Return the score of matching two audioformats.
> + * Veto the audioformat if:
> + * - It has no channels for some reason.
> + * - Requested PCM format is not supported.
> + * - Requested sample rate is not supported.
> + */
> +static int match_endpoint_audioformats(struct audioformat *fp,
> +	struct audioformat *match, int rate,
> +	snd_pcm_format_t pcm_format)
> +{
> +	int i;
> +	int score = 0;
> +
> +	if (fp->channels < 1) {
> +		snd_printdd("%s: (fmt @%p) no channels\n", __func__, fp);
> +		return 0;
> +	}
> +
> +	if (!(fp->formats & (1ULL << pcm_format))) {
> +		snd_printdd("%s: (fmt @%p) no match for format %d\n", __func__,
> +			fp, pcm_format);
> +		return 0;
> +	}
> +
> +	for (i = 0; i < fp->nr_rates; i++) {
> +		if (fp->rate_table[i] == rate) {
> +			score++;
> +			break;
> +		}
> +	}
> +	if (!score) {
> +		snd_printdd("%s: (fmt @%p) no match for rate %d\n", __func__,
> +			fp, rate);
> +		return 0;
> +	}
> +
> +	if (fp->channels == match->channels)
> +		score++;
> +
> +	snd_printdd("%s: (fmt @%p) score %d\n", __func__, fp, score);
> +
> +	return score;
> +}
> +
> +/*
> + * Configure the sync ep using the rate and pcm format of the data ep.
> + */
> +static int configure_sync_endpoint(struct snd_usb_substream *subs)
> +{
> +	int ret;
> +	struct audioformat *fp;
> +	struct audioformat *sync_fp = NULL;
> +	int cur_score = 0;
> +	int sync_period_bytes = subs->period_bytes;
> +	struct snd_usb_substream *sync_subs =
> +		&subs->stream->substream[subs->direction ^ 1];
> +
> +	/* Try to find the best matching audioformat. */
> +	list_for_each_entry(fp, &sync_subs->fmt_list, list) {
> +		int score = match_endpoint_audioformats(fp, subs->cur_audiofmt,
> +			subs->cur_rate, subs->pcm_format);
> +
> +		if (score > cur_score) {
> +			sync_fp = fp;
> +			cur_score = score;
> +		}
> +	}
> +
> +	if (unlikely(sync_fp == NULL)) {
> +		snd_printk(KERN_ERR "%s: no valid audioformat for sync ep %x found\n",
> +			__func__, sync_subs->ep_num);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Recalculate the period bytes if channel number differ between
> +	 * data and sync ep audioformat.
> +	 */
> +	if (sync_fp->channels != subs->channels) {
> +		sync_period_bytes = (subs->period_bytes / subs->channels) *
> +			sync_fp->channels;
> +		snd_printdd("%s: adjusted sync ep period bytes (%d -> %d)\n",
> +			__func__, subs->period_bytes, sync_period_bytes);
> +	}
> +
> +	ret = snd_usb_endpoint_set_params(subs->sync_endpoint,
> +					  subs->pcm_format,
> +					  sync_fp->channels,
> +					  sync_period_bytes,
> +					  subs->cur_rate,
> +					  sync_fp,
> +					  NULL);
> +
> +	return ret;
> +}
> +
> +/*
>   * configure endpoint params
>   *
>   * called  during initial setup and upon resume
> @@ -472,13 +569,8 @@ static int configure_endpoint(struct snd_usb_substream *subs)
>  		return ret;
>  
>  	if (subs->sync_endpoint)
> -		ret = snd_usb_endpoint_set_params(subs->sync_endpoint,
> -						  subs->pcm_format,
> -						  subs->channels,
> -						  subs->period_bytes,
> -						  subs->cur_rate,
> -						  subs->cur_audiofmt,
> -						  NULL);
> +		ret = configure_sync_endpoint(subs);
> +
>  	return ret;
>  }
>  
> -- 
> 1.7.8.6
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [PATCH sound-unstable] usb-audio: sync ep init fix for audioformat mismatch

[Eldad Zack]

On Tue, 4 Dec 2012, Takashi Iwai wrote:
> At Mon,  3 Dec 2012 20:30:09 +0100,
> Eldad Zack wrote:
> > 
> > Commit 947d299686aa9cc8aecf749d54e8475c6e498956 , "ALSA: snd-usb:
> > properly initialize the sync endpoint", while correcting the
> > initialization of the sync endpoint when opening just the data
> > endpoint, prevents devices that has a sync endpoint, with a channel
> > number different than that of the data endpoint, from functioning.
> > Due to a different channel and period bytes count, attempting to
> > initialize the sync endpoint will fail at the usb host driver.
> > For example, when using xhci:
> > 
> >  cannot submit urb 0, error -90: internal error
> > 
> > With this patch, if a sync endpoint has multiple audioformats, a
> > matching audioformat is preferred. An audioformat must be found
> > with at least one channel and support the requested sample rate
> > and PCM format, otherwise the stream will not be opened.
> > 
> > If the number of channels differ between the selected audioformat
> > and the requested format, adjust the period bytes count accordingly.
> > It is safe to perform the calculation on the basis of the channel
> > count, since the requested PCM audio format and the rate must be
> > supported by the selected audioformat.
> > 
> > Cc: Jeffrey Barish <jeff_barish@earthlink.net>
> > Cc: Daniel Mack <zonque@gmail.com>
> > Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
> 
> Thanks, applied.

Great!

> At the next time, however, create a patch based on either for-next or
> for-linus branch of sound git tree.  sound-unstable tree is a place to
> play with experimental patches.  The place to put the real patches
> that will be fed to the upstream is sound git tree.

Ah, sorry, I mixed up unstable with to-next then.
Thanks for the heads up! 

Cheers,
Eldad

Re: [FT C400, PATCH RFC, v4 00/10] M-Audio Fast Track C400

[Felix Homann]

Sorry for being late on this. I've just tested Takashi's kernel with your
patches included. Everything seems fine for the FTU 8R.


Kind regards,
Felix


2012/11/30 Takashi Iwai <tiwai@suse.de>

> At Fri, 30 Nov 2012 15:06:00 +0100 (CET),
> Eldad Zack wrote:
> >
> >
> > Hi Takashi,
> >
> > On Thu, 29 Nov 2012, Takashi Iwai wrote:
> > > At Wed, 28 Nov 2012 23:55:31 +0100,
> > > Eldad Zack wrote:
> > > >
> > > > * Incoroprated the feedback from Clemens Ladisch <clemens@ladisch.de
> >:
> > > > Moved the quirks structure to the right place (also in other places,
> though
> > > > not strictly required) and removed the "set quirks" log messages.
> > > > Thanks, Clemens!
> > > >
> > > > * Added a patch to address the regression introduced by commit
> > > > 947d299686aa9cc8aecf749d54e8475c6e498956 , "ALSA: snd-usb: properly
> initialize
> > > > the sync endpoint". I believe there are no other devices that
> affected by this
> > > > issue. Opening only the playback stream without first opening the
> capture
> > > > stream now works with the C400.
> > >
> > > Thanks.  I applied all patches except for the new one (2/10 usb-audio:
> > > correct sync ep init).  This patch needs a bit more care, as it
> > > seems.
> > >
> > > I know it must be fixed for FT C400 but the fix itself is independent
> > > from others, so I merged others first to the rest work easier.
> >
> > Thanks! That's great, now I only need to concentrate on this one
> > issue. I went through your helpful comments and I'll try to work out a
> > better patch soon and send it for review.
> > Should I wait with it until the 3.8 merge window closes?
>
> Your other patches have been already merged, so better be quick, so
> that the last piece can be put in to 3.8, too :)
>
> > BTW - given that the code is old, isn't it time to slightly change
> > these? :)
> >
> > card.c: *   (Tentative) USB Audio Driver for ALSA
> > mixer.c: *   (Tentative) USB Audio Driver for ALSA
> > usbaudio.h: *   (Tentative) USB Audio Driver for ALSA
>
> Yes, clean up patches are welcome.  But these are in a lower priority,
> so will be queued for 3.9 later.
>
>
> thanks,
>
> Takashi
>

Re: [FT C400, PATCH RFC, v4 00/10] M-Audio Fast Track C400

[Eldad Zack]

On Fri, 7 Dec 2012, Felix Homann wrote:

> Sorry for being late on this. I've just tested Takashi's kernel with your patches included. Everything seems fine for the FTU 8R.
> 
> Kind regards,
> Felix

That's good news, thanks for testing!

Cheers,
Eldad