[alsa-devel] [PATCH v5] ALSA: usb: Work around CM6631 sample rate change bug
The C-Media CM6631 USB receiver doesn't respond to changes in sample rate while the interface is active. The same behavior is observed in other UAC2 hardware like the VIA VT1731.
Reset the interface after setting the sampling frequency on sample rate changes, to ensure that the sample rate set by snd_usb_init_sample_rate() is used. Otherwise, the device will try to use the sample rate of the previous stream, causing distorted sound on sample rate changes.
The reset is performed for all UAC2 devices, as it should not affect a standards compliant device, but it is only necessary for C-Media CM6631, VIA VT1731 and possibly others.
Failure to read sample rate from the device is not handled as an error in set_sample_rate_v2(), as (permanent or intermittent) failure to read sample rate isn't essential for a successful sample rate set.
Signed-off-by: Torstein Hegge hegge@resisty.net --- Changes since v4: - Sample rate read failure is no longer fatal.
Failure to read sample rate from the device is degraded from a fatal error to a warning message. The Schiit Bifrost has shown intermittent sample rate read failures, and this shouldn't be a reason to abort the initialization.
- The reset is performed for all UAC2 devices.
The number of devices affected by this issue seems to be larger than just the C-Media based devices, issues with the VIA VT1731 has been reported by Davor Herga to alsa-user [1].
The additional reset should not affect standards compliant devices and will hopefully improve the compatibility with future devices tested with the OS X UAC2 driver.
This can cause problems for a theoretical device where sample rate read always fails, causing the reset to be performed every time the sample rate is set, *and* where repeated resets cause resume after pause to not resume properly, like the issues seen with CM6631 in the first iteration of this patch.
[1] http://thread.gmane.org/gmane.linux.alsa.user/37312
Changes since v3: - Use given target rate instead of the new rate read from device when checking if a reset is needed. Some devices (at least Schiit Bifrost) doesn't report a reliable sample rate back right after setting a new rate. This appears to fix the issues with v3 of this patch with the Schiit, as reported by Chris Hermansen in http://thread.gmane.org/gmane.linux.alsa.user/36935/focus=37284
sound/usb/clock.c | 45 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 35 insertions(+), 10 deletions(-)
diff --git a/sound/usb/clock.c b/sound/usb/clock.c index 5e634a2..9e2703a 100644 --- a/sound/usb/clock.c +++ b/sound/usb/clock.c @@ -253,7 +253,7 @@ static int set_sample_rate_v2(struct snd_usb_audio *chip, int iface, { struct usb_device *dev = chip->dev; unsigned char data[4]; - int err, crate; + int err, cur_rate, prev_rate; int clock = snd_usb_clock_find_source(chip, fmt->clock);
if (clock < 0) @@ -266,6 +266,19 @@ static int set_sample_rate_v2(struct snd_usb_audio *chip, int iface, return -ENXIO; }
+ err = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), UAC2_CS_CUR, + USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN, + UAC2_CS_CONTROL_SAM_FREQ << 8, + snd_usb_ctrl_intf(chip) | (clock << 8), + data, sizeof(data)); + if (err < 0) { + snd_printk(KERN_WARNING "%d:%d:%d: cannot get freq (v2)\n", + dev->devnum, iface, fmt->altsetting); + prev_rate = 0; + } else { + prev_rate = data[0] | (data[1] << 8) | (data[2] << 16) | (data[3] << 24); + } + data[0] = rate; data[1] = rate >> 8; data[2] = rate >> 16; @@ -280,19 +293,31 @@ static int set_sample_rate_v2(struct snd_usb_audio *chip, int iface, return err; }
- if ((err = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), UAC2_CS_CUR, - USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN, - UAC2_CS_CONTROL_SAM_FREQ << 8, - snd_usb_ctrl_intf(chip) | (clock << 8), - data, sizeof(data))) < 0) { + err = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), UAC2_CS_CUR, + USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN, + UAC2_CS_CONTROL_SAM_FREQ << 8, + snd_usb_ctrl_intf(chip) | (clock << 8), + data, sizeof(data)); + if (err < 0) { snd_printk(KERN_WARNING "%d:%d:%d: cannot get freq (v2)\n", dev->devnum, iface, fmt->altsetting); - return err; + cur_rate = 0; + } else { + cur_rate = data[0] | (data[1] << 8) | (data[2] << 16) | (data[3] << 24); }
- crate = data[0] | (data[1] << 8) | (data[2] << 16) | (data[3] << 24); - if (crate != rate) - snd_printd(KERN_WARNING "current rate %d is different from the runtime rate %d\n", crate, rate); + if (cur_rate != rate) { + snd_printd(KERN_WARNING + "current rate %d is different from the runtime rate %d\n", + cur_rate, rate); + } + + /* Some devices doesn't respond to sample rate changes while the + * interface is active. */ + if (rate != prev_rate) { + usb_set_interface(dev, iface, 0); + usb_set_interface(dev, iface, fmt->altsetting); + }
return 0; }
Torstein Hegge wrote:
The C-Media CM6631 USB receiver doesn't respond to changes in sample rate while the interface is active. The same behavior is observed in other UAC2 hardware like the VIA VT1731.
Might it be possible that these devices have reset the interface (see UAC2 section 3.16.3), and that the driver hasn't noticed this because it doesn't handle Active Alternate Setting Control interrupts?
Regards, Clemens
On Wed, Mar 27, 2013 at 11:09:01 +0100, Clemens Ladisch wrote:
Torstein Hegge wrote:
The C-Media CM6631 USB receiver doesn't respond to changes in sample rate while the interface is active. The same behavior is observed in other UAC2 hardware like the VIA VT1731.
Might it be possible that these devices have reset the interface (see UAC2 section 3.16.3), and that the driver hasn't noticed this because it doesn't handle Active Alternate Setting Control interrupts?
If that was the case, wouldn't the device be in altsetting zero after the sample rate set and not be able to output sound at all? And as long as the current altsetting supports the target sample rate as reported in /proc/asound/card1/stream0, Active Alternate Setting Control shouldn't have to be invoked.
Torstein
On 27.03.2013 12:47, Torstein Hegge wrote:
On Wed, Mar 27, 2013 at 11:09:01 +0100, Clemens Ladisch wrote:
Torstein Hegge wrote:
The C-Media CM6631 USB receiver doesn't respond to changes in sample rate while the interface is active. The same behavior is observed in other UAC2 hardware like the VIA VT1731.
Might it be possible that these devices have reset the interface (see UAC2 section 3.16.3), and that the driver hasn't noticed this because it doesn't handle Active Alternate Setting Control interrupts?
If that was the case, wouldn't the device be in altsetting zero after the sample rate set and not be able to output sound at all? And as long as the current altsetting supports the target sample rate as reported in /proc/asound/card1/stream0, Active Alternate Setting Control shouldn't have to be invoked.
You could just try and catch that currently unhandled interrupt. We should support that anyway at some point ...
Daniel
On Wed, Mar 27, 2013 at 12:52:14 +0100, Daniel Mack wrote:
On 27.03.2013 12:47, Torstein Hegge wrote:
On Wed, Mar 27, 2013 at 11:09:01 +0100, Clemens Ladisch wrote:
Torstein Hegge wrote:
The C-Media CM6631 USB receiver doesn't respond to changes in sample rate while the interface is active. The same behavior is observed in other UAC2 hardware like the VIA VT1731.
Might it be possible that these devices have reset the interface (see UAC2 section 3.16.3), and that the driver hasn't noticed this because it doesn't handle Active Alternate Setting Control interrupts?
If that was the case, wouldn't the device be in altsetting zero after the sample rate set and not be able to output sound at all? And as long as the current altsetting supports the target sample rate as reported in /proc/asound/card1/stream0, Active Alternate Setting Control shouldn't have to be invoked.
You could just try and catch that currently unhandled interrupt. We should support that anyway at some point ...
It doesn't look like snd_usb_mixer_interrupt_v2() is invoked on sample rate changes. As far as I can tell, that should be invoked on any interrupts, but it was never triggered, so I might be wrong.
Torstein
At Tue, 26 Mar 2013 22:10:05 +0100, Torstein Hegge wrote:
The C-Media CM6631 USB receiver doesn't respond to changes in sample rate while the interface is active. The same behavior is observed in other UAC2 hardware like the VIA VT1731.
Reset the interface after setting the sampling frequency on sample rate changes, to ensure that the sample rate set by snd_usb_init_sample_rate() is used. Otherwise, the device will try to use the sample rate of the previous stream, causing distorted sound on sample rate changes.
The reset is performed for all UAC2 devices, as it should not affect a standards compliant device, but it is only necessary for C-Media CM6631, VIA VT1731 and possibly others.
Failure to read sample rate from the device is not handled as an error in set_sample_rate_v2(), as (permanent or intermittent) failure to read sample rate isn't essential for a successful sample rate set.
Signed-off-by: Torstein Hegge hegge@resisty.net
Changes since v4:
- Sample rate read failure is no longer fatal.
Failure to read sample rate from the device is degraded from a fatal error to a warning message. The Schiit Bifrost has shown intermittent sample rate read failures, and this shouldn't be a reason to abort the initialization.
- The reset is performed for all UAC2 devices.
The number of devices affected by this issue seems to be larger than just the C-Media based devices, issues with the VIA VT1731 has been reported by Davor Herga to alsa-user [1].
The additional reset should not affect standards compliant devices and will hopefully improve the compatibility with future devices tested with the OS X UAC2 driver.
This can cause problems for a theoretical device where sample rate read always fails, causing the reset to be performed every time the sample rate is set, *and* where repeated resets cause resume after pause to not resume properly, like the issues seen with CM6631 in the first iteration of this patch.
[1] http://thread.gmane.org/gmane.linux.alsa.user/37312
Changes since v3:
- Use given target rate instead of the new rate read from device when checking if a reset is needed. Some devices (at least Schiit Bifrost) doesn't report a reliable sample rate back right after setting a new rate. This appears to fix the issues with v3 of this patch with the Schiit, as reported by Chris Hermansen in http://thread.gmane.org/gmane.linux.alsa.user/36935/focus=37284
What about the latest status of the patch?
If both Clemens and Daniel are happy with it, I can apply it for the next 3.9-rc.
Though, it would be nicer if two identical calls of snd_usb_ctl_msg can be put into a single function, such as,
static int get_cur_sample_rate_v2(struct snd_usb_audio *chip, int iface, int altsetting, int clock) { struct usb_device *dev = chip->dev; unsigned char data[4]; int err;
err = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), UAC2_CS_CUR, USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN, UAC2_CS_CONTROL_SAM_FREQ << 8, snd_usb_ctrl_intf(chip) | (clock << 8), data, sizeof(data)); if (err < 0) { snd_printk(KERN_WARNING "%d:%d:%d: cannot get freq (v2)\n", dev->devnum, iface, altsetting); return 0; }
return data[0] | (data[1] << 8) | (data[2] << 16) | (data[3] << 24); }
thanks,
Takashi
sound/usb/clock.c | 45 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 35 insertions(+), 10 deletions(-)
diff --git a/sound/usb/clock.c b/sound/usb/clock.c index 5e634a2..9e2703a 100644 --- a/sound/usb/clock.c +++ b/sound/usb/clock.c @@ -253,7 +253,7 @@ static int set_sample_rate_v2(struct snd_usb_audio *chip, int iface, { struct usb_device *dev = chip->dev; unsigned char data[4];
- int err, crate;
int err, cur_rate, prev_rate; int clock = snd_usb_clock_find_source(chip, fmt->clock);
if (clock < 0)
@@ -266,6 +266,19 @@ static int set_sample_rate_v2(struct snd_usb_audio *chip, int iface, return -ENXIO; }
- err = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), UAC2_CS_CUR,
USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN,UAC2_CS_CONTROL_SAM_FREQ << 8,snd_usb_ctrl_intf(chip) | (clock << 8),data, sizeof(data));- if (err < 0) {
snd_printk(KERN_WARNING "%d:%d:%d: cannot get freq (v2)\n",dev->devnum, iface, fmt->altsetting);prev_rate = 0;- } else {
prev_rate = data[0] | (data[1] << 8) | (data[2] << 16) | (data[3] << 24);- }
- data[0] = rate; data[1] = rate >> 8; data[2] = rate >> 16;
@@ -280,19 +293,31 @@ static int set_sample_rate_v2(struct snd_usb_audio *chip, int iface, return err; }
- if ((err = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), UAC2_CS_CUR,
USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN,UAC2_CS_CONTROL_SAM_FREQ << 8,snd_usb_ctrl_intf(chip) | (clock << 8),data, sizeof(data))) < 0) {
- err = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), UAC2_CS_CUR,
USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN,UAC2_CS_CONTROL_SAM_FREQ << 8,snd_usb_ctrl_intf(chip) | (clock << 8),data, sizeof(data));- if (err < 0) { snd_printk(KERN_WARNING "%d:%d:%d: cannot get freq (v2)\n", dev->devnum, iface, fmt->altsetting);
return err;
cur_rate = 0;- } else {
cur_rate = data[0] | (data[1] << 8) | (data[2] << 16) | (data[3] << 24);
- crate = data[0] | (data[1] << 8) | (data[2] << 16) | (data[3] << 24);
- if (crate != rate)
snd_printd(KERN_WARNING "current rate %d is different from the runtime rate %d\n", crate, rate);
if (cur_rate != rate) {
snd_printd(KERN_WARNING"current rate %d is different from the runtime rate %d\n",cur_rate, rate);}
/* Some devices doesn't respond to sample rate changes while the
* interface is active. */if (rate != prev_rate) {
usb_set_interface(dev, iface, 0);usb_set_interface(dev, iface, fmt->altsetting);}
return 0;
}
1.7.10.4
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Takashi Iwai wrote:
Torstein Hegge wrote:
The C-Media CM6631 USB receiver doesn't respond to changes in sample rate while the interface is active. The same behavior is observed in other UAC2 hardware like the VIA VT1731.
Reset the interface after setting the sampling frequency on sample rate changes, to ensure that the sample rate set by snd_usb_init_sample_rate() is used. Otherwise, the device will try to use the sample rate of the previous stream, causing distorted sound on sample rate changes.
The reset is performed for all UAC2 devices, as it should not affect a standards compliant device, but it is only necessary for C-Media CM6631, VIA VT1731 and possibly others.
Failure to read sample rate from the device is not handled as an error in set_sample_rate_v2(), as (permanent or intermittent) failure to read sample rate isn't essential for a successful sample rate set.
Signed-off-by: Torstein Hegge hegge@resisty.net
What about the latest status of the patch?
If both Clemens and Daniel are happy with it, I can apply it for the next 3.9-rc.
Acked-by: Clemens Ladisch clemens@ladisch.de with or without this change:
Though, it would be nicer if two identical calls of snd_usb_ctl_msg can be put into a single function,
Regards, Clemens
At Wed, 03 Apr 2013 12:15:49 +0200, Clemens Ladisch wrote:
Takashi Iwai wrote:
Torstein Hegge wrote:
The C-Media CM6631 USB receiver doesn't respond to changes in sample rate while the interface is active. The same behavior is observed in other UAC2 hardware like the VIA VT1731.
Reset the interface after setting the sampling frequency on sample rate changes, to ensure that the sample rate set by snd_usb_init_sample_rate() is used. Otherwise, the device will try to use the sample rate of the previous stream, causing distorted sound on sample rate changes.
The reset is performed for all UAC2 devices, as it should not affect a standards compliant device, but it is only necessary for C-Media CM6631, VIA VT1731 and possibly others.
Failure to read sample rate from the device is not handled as an error in set_sample_rate_v2(), as (permanent or intermittent) failure to read sample rate isn't essential for a successful sample rate set.
Signed-off-by: Torstein Hegge hegge@resisty.net
What about the latest status of the patch?
If both Clemens and Daniel are happy with it, I can apply it for the next 3.9-rc.
Acked-by: Clemens Ladisch clemens@ladisch.de with or without this change:
OK, I applied the patch now without modification for for-linus branch. Then back-merged to for-next branch with the additional clean-up patch below.
Eldad, please rebase your patches to for-next branch of sound git tree.
thanks,
Takashi
--- From: Takashi Iwai tiwai@suse.de Subject: [PATCH] ALSA: usb-audio: Clean up the code in set_sample_rate_v2()
Just for cleaning up, introduce a new function get_sample_rate_v2() for replacing two identical calls in set_sample_rate_v2().
No functional change.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/clock.c | 47 +++++++++++++++++++++++------------------------ 1 file changed, 23 insertions(+), 24 deletions(-)
diff --git a/sound/usb/clock.c b/sound/usb/clock.c index 9e2703a..e3ccf3d 100644 --- a/sound/usb/clock.c +++ b/sound/usb/clock.c @@ -247,6 +247,27 @@ static int set_sample_rate_v1(struct snd_usb_audio *chip, int iface, return 0; }
+static int get_sample_rate_v2(struct snd_usb_audio *chip, int iface, + int altsetting, int clock) +{ + struct usb_device *dev = chip->dev; + unsigned char data[4]; + int err; + + err = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), UAC2_CS_CUR, + USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN, + UAC2_CS_CONTROL_SAM_FREQ << 8, + snd_usb_ctrl_intf(chip) | (clock << 8), + data, sizeof(data)); + if (err < 0) { + snd_printk(KERN_WARNING "%d:%d:%d: cannot get freq (v2)\n", + dev->devnum, iface, altsetting); + return 0; + } + + return data[0] | (data[1] << 8) | (data[2] << 16) | (data[3] << 24); +} + static int set_sample_rate_v2(struct snd_usb_audio *chip, int iface, struct usb_host_interface *alts, struct audioformat *fmt, int rate) @@ -266,18 +287,7 @@ static int set_sample_rate_v2(struct snd_usb_audio *chip, int iface, return -ENXIO; }
- err = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), UAC2_CS_CUR, - USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN, - UAC2_CS_CONTROL_SAM_FREQ << 8, - snd_usb_ctrl_intf(chip) | (clock << 8), - data, sizeof(data)); - if (err < 0) { - snd_printk(KERN_WARNING "%d:%d:%d: cannot get freq (v2)\n", - dev->devnum, iface, fmt->altsetting); - prev_rate = 0; - } else { - prev_rate = data[0] | (data[1] << 8) | (data[2] << 16) | (data[3] << 24); - } + prev_rate = get_sample_rate_v2(chip, iface, fmt->altsetting, clock);
data[0] = rate; data[1] = rate >> 8; @@ -293,18 +303,7 @@ static int set_sample_rate_v2(struct snd_usb_audio *chip, int iface, return err; }
- err = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), UAC2_CS_CUR, - USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN, - UAC2_CS_CONTROL_SAM_FREQ << 8, - snd_usb_ctrl_intf(chip) | (clock << 8), - data, sizeof(data)); - if (err < 0) { - snd_printk(KERN_WARNING "%d:%d:%d: cannot get freq (v2)\n", - dev->devnum, iface, fmt->altsetting); - cur_rate = 0; - } else { - cur_rate = data[0] | (data[1] << 8) | (data[2] << 16) | (data[3] << 24); - } + cur_rate = get_sample_rate_v2(chip, iface, fmt->altsetting, clock);
if (cur_rate != rate) { snd_printd(KERN_WARNING
On Wed, 3 Apr 2013, Takashi Iwai wrote:
At Wed, 03 Apr 2013 12:15:49 +0200, Clemens Ladisch wrote:
Takashi Iwai wrote:
Torstein Hegge wrote:
What about the latest status of the patch?
If both Clemens and Daniel are happy with it, I can apply it for the next 3.9-rc.
Acked-by: Clemens Ladisch clemens@ladisch.de with or without this change:
OK, I applied the patch now without modification for for-linus branch. Then back-merged to for-next branch with the additional clean-up patch below.
Eldad, please rebase your patches to for-next branch of sound git tree.
Thanks for the heads up, Takashi! I've sent it out with all the changes moments ago.
Cheers, Eldad
participants (5)
-
Clemens Ladisch -
Daniel Mack -
Eldad Zack -
Takashi Iwai -
Torstein Hegge