[alsa-devel] [RFC] ALSA: usb-audio: Add custom mixer status quirks for RME CC devices
This patch accessing vendor specific registers of RME Class Compliant devices works otherwise fine, but there is still one small problem. It is supposed to be a polled from the hardware, since the values indicate device status and are subject to change at all times. Behavior is correct if I keep running "amixer" all the time to retrieve values - sampling rate changes are visible and values get updated. However, when I poll the values from an application without reopening the mixer, they are being cached somewhere and the changes are not reflected. This problem doesn't exist for example with PCIe based RME HDSPe AIO and application gets updated values, but in this USB based device case they don't get updated. I have flagged all the mixer controls as read-only and volatile, but the volatile flag is not honored in this (assuming I've understood it correctly).
Any guidance on how to fix the caching problem is welcome!
Adds several vendor specific mixer quirks for RME's Class Compliant USB devices. These provide extra status information from the device otherwise not available.
These include AES/SPDIF rate and status information, current system sampling rate and measured frequency. This information is especially useful in cases where device's clock is slaved to external clock source.
Signed-off-by: Jussi Laako jussi@sonarnerd.net --- sound/usb/mixer_quirks.c | 345 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 345 insertions(+)
diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c index cbfb48b..1bdf5f0 100644 --- a/sound/usb/mixer_quirks.c +++ b/sound/usb/mixer_quirks.c @@ -1817,6 +1817,347 @@ static int dell_dock_mixer_init(struct usb_mixer_interface *mixer) return 0; }
+/* RME Class Compliant device quirks */ + +static const u32 snd_rme_rate_table[] = { + 32000, 44100, 48000, 50000, + 64000, 88200, 96000, 100000, + 128000, 176400, 192000, 200000, + 256000, 352800, 384000, 400000, + 512000, 705600, 768000, 800000 +}; + +static int snd_rme_get_status1(struct snd_kcontrol *kcontrol, + u32 *status1) +{ + struct usb_mixer_elem_list *list = snd_kcontrol_chip(kcontrol); + struct snd_usb_audio *chip = list->mixer->chip; + struct usb_device *dev = chip->dev; + int err; + + err = snd_usb_lock_shutdown(chip); + if (err < 0) + return err; + + err = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), + 23, /* GET_STATUS1 */ + USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE, + 0, 0, + status1, sizeof(*status1)); + if (err < 0) { + dev_err(&dev->dev, + "unable to issue vendor read request (ret = %d)", err); + goto end; + } + +end: + snd_usb_unlock_shutdown(chip); + return err; +} + +static int snd_rme_rate_get(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + u32 status1; + u32 rate = 0; + int idx; + int err; + + err = snd_rme_get_status1(kcontrol, &status1); + if (err < 0) + return err; + + switch (kcontrol->private_value) + { + case 0: /* System */ + idx = (status1 >> 16) & 0x1f; + if (idx < ARRAY_SIZE(snd_rme_rate_table)) + rate = snd_rme_rate_table[idx]; + break; + case 1: /* AES */ + idx = (status1 >> 8) & 0xf; + if (idx < 12) + rate = snd_rme_rate_table[idx]; + break; + case 2: /* SPDIF */ + idx = (status1 >> 12) & 0xf; + if (idx < 12) + rate = snd_rme_rate_table[idx]; + break; + default: + return -EINVAL; + } + + ucontrol->value.integer.value[0] = rate; + return 0; +} + +static int snd_rme_sync_state_get(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + u32 status1; + int idx = 0; + int err; + + err = snd_rme_get_status1(kcontrol, &status1); + if (err < 0) + return err; + + switch (kcontrol->private_value) + { + case 1: /* AES */ + if (status1 & 0x4) + idx = 2; + else if (status1 & 0x1) + idx = 1; + break; + case 2: /* SPDIF */ + if (status1 & 0x8) + idx = 2; + else if (status1 & 0x2) + idx = 1; + break; + default: + return -EINVAL; + } + + ucontrol->value.enumerated.item[0] = idx; + return 0; +} + +static int snd_rme_spdif_if_get(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + u32 status1; + int err; + + err = snd_rme_get_status1(kcontrol, &status1); + if (err < 0) + return err; + + ucontrol->value.enumerated.item[0] = (status1 >> 4) & 0x1; + return 0; +} + +static int snd_rme_spdif_format_get(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + u32 status1; + int err; + + err = snd_rme_get_status1(kcontrol, &status1); + if (err < 0) + return err; + + ucontrol->value.enumerated.item[0] = (status1 >> 5) & 0x1; + return 0; +} + +static int snd_rme_sync_source_get(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + u32 status1; + int err; + + err = snd_rme_get_status1(kcontrol, &status1); + if (err < 0) + return err; + + ucontrol->value.enumerated.item[0] = (status1 >> 6) & 0x3; + return 0; +} + +static int snd_rme_current_freq_get(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct usb_mixer_elem_list *list = snd_kcontrol_chip(kcontrol); + struct snd_usb_audio *chip = list->mixer->chip; + struct usb_device *dev = chip->dev; + u32 status1; + const u64 num = 104857600000000; + u32 den; + unsigned int freq; + int err; + + err = snd_usb_lock_shutdown(chip); + if (err < 0) + return err; + + err = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), + 23, /* GET_STATUS1 */ + USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE, + 0, 0, + &status1, sizeof(status1)); + if (err < 0) { + dev_err(&dev->dev, + "unable to issue vendor read request (ret = %d)", err); + goto end; + } + + err = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), + 17, /* GET_CURRENT_FREQ */ + USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE, + 0, 0, + &den, sizeof(den)); + if (err < 0) { + dev_err(&dev->dev, + "unable to issue vendor read request (ret = %d)", err); + goto end; + } + freq = (den == 0) ? 0 : num / den; + + freq <<= ((status1 >> 18) & 0x7); + ucontrol->value.integer.value[0] = freq; + +end: + snd_usb_unlock_shutdown(chip); + return err; +} + +static int snd_rme_rate_info(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_info *uinfo) +{ + uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER; + uinfo->count = 1; + uinfo->value.integer.min = + (kcontrol->private_value > 0) ? 0 : 32000; + uinfo->value.integer.max = + (kcontrol->private_value > 0) ? 200000 : 800000; + uinfo->value.integer.step = 0; + return 0; +} + +static int snd_rme_sync_state_info(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_info *uinfo) +{ + static const char *const sync_states[] = { + "No Lock", "Lock", "Sync" + }; + + return snd_ctl_enum_info(uinfo, 1, + ARRAY_SIZE(sync_states), sync_states); +} + +static int snd_rme_spdif_if_info(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_info *uinfo) +{ + static const char *const spdif_if[] = { + "Coaxial", "Optical" + }; + + return snd_ctl_enum_info(uinfo, 1, + ARRAY_SIZE(spdif_if), spdif_if); +} + +static int snd_rme_spdif_format_info(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_info *uinfo) +{ + static const char *const optical_type[] = { + "Consumer", "Professional" + }; + + return snd_ctl_enum_info(uinfo, 1, + ARRAY_SIZE(optical_type), optical_type); +} + +static int snd_rme_sync_source_info(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_info *uinfo) +{ + static const char *const sync_sources[] = { + "Internal", "AES", "SPDIF", "Internal" + }; + + return snd_ctl_enum_info(uinfo, 1, + ARRAY_SIZE(sync_sources), sync_sources); +} + +static struct snd_kcontrol_new snd_rme_controls[] = { + { + .iface = SNDRV_CTL_ELEM_IFACE_MIXER, + .name = "AES Rate", + .access = SNDRV_CTL_ELEM_ACCESS_READ | SNDRV_CTL_ELEM_ACCESS_VOLATILE, + .info = snd_rme_rate_info, + .get = snd_rme_rate_get, + .private_value = 1 + }, + { + .iface = SNDRV_CTL_ELEM_IFACE_MIXER, + .name = "AES Sync", + .access = SNDRV_CTL_ELEM_ACCESS_READ | SNDRV_CTL_ELEM_ACCESS_VOLATILE, + .info = snd_rme_sync_state_info, + .get = snd_rme_sync_state_get, + .private_value = 1 + }, + { + .iface = SNDRV_CTL_ELEM_IFACE_MIXER, + .name = "SPDIF Rate", + .access = SNDRV_CTL_ELEM_ACCESS_READ | SNDRV_CTL_ELEM_ACCESS_VOLATILE, + .info = snd_rme_rate_info, + .get = snd_rme_rate_get, + .private_value = 2 + }, + { + .iface = SNDRV_CTL_ELEM_IFACE_MIXER, + .name = "SPDIF Sync", + .access = SNDRV_CTL_ELEM_ACCESS_READ | SNDRV_CTL_ELEM_ACCESS_VOLATILE, + .info = snd_rme_sync_state_info, + .get = snd_rme_sync_state_get, + .private_value = 2 + }, + { + .iface = SNDRV_CTL_ELEM_IFACE_MIXER, + .name = "SPDIF Interface", + .access = SNDRV_CTL_ELEM_ACCESS_READ | SNDRV_CTL_ELEM_ACCESS_VOLATILE, + .info = snd_rme_spdif_if_info, + .get = snd_rme_spdif_if_get, + }, + { + .iface = SNDRV_CTL_ELEM_IFACE_MIXER, + .name = "SPDIF Format", + .access = SNDRV_CTL_ELEM_ACCESS_READ | SNDRV_CTL_ELEM_ACCESS_VOLATILE, + .info = snd_rme_spdif_format_info, + .get = snd_rme_spdif_format_get, + }, + { + .iface = SNDRV_CTL_ELEM_IFACE_MIXER, + .name = "Sync Source", + .access = SNDRV_CTL_ELEM_ACCESS_READ | SNDRV_CTL_ELEM_ACCESS_VOLATILE, + .info = snd_rme_sync_source_info, + .get = snd_rme_sync_source_get + }, + { + .iface = SNDRV_CTL_ELEM_IFACE_MIXER, + .name = "System Rate", + .access = SNDRV_CTL_ELEM_ACCESS_READ | SNDRV_CTL_ELEM_ACCESS_VOLATILE, + .info = snd_rme_rate_info, + .get = snd_rme_rate_get, + .private_value = 0 + }, + { + .iface = SNDRV_CTL_ELEM_IFACE_MIXER, + .name = "Current Frequency", + .access = SNDRV_CTL_ELEM_ACCESS_READ | SNDRV_CTL_ELEM_ACCESS_VOLATILE, + .info = snd_rme_rate_info, + .get = snd_rme_current_freq_get + } +}; + +static int snd_rme_controls_create(struct usb_mixer_interface *mixer) +{ + int err, i; + + for (i = 0; i < ARRAY_SIZE(snd_rme_controls); ++i) { + err = add_single_ctl_with_resume(mixer, 0, + NULL, + &snd_rme_controls[i], + NULL); + if (err < 0) + return err; + } + + return 0; +} + int snd_usb_mixer_apply_create_quirk(struct usb_mixer_interface *mixer) { int err = 0; @@ -1906,6 +2247,10 @@ int snd_usb_mixer_apply_create_quirk(struct usb_mixer_interface *mixer) break; }
+ /* RME Class Compliant devices */ + if (USB_ID_VENDOR(mixer->chip->usb_id) == 0x2a39) + err = snd_rme_controls_create(mixer); + return err; }
Hi Jussi,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on sound/for-next] [also build test ERROR on v4.19-rc4 next-20180921] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Jussi-Laako/ALSA-usb-audio-Add-cust... base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next config: mips-pistachio_defconfig (attached as .config) compiler: mipsel-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=7.2.0 make.cross ARCH=mips
All errors (new ones prefixed by >>):
ERROR: "__udivdi3" [sound/usb/snd-usb-audio.ko] undefined! ERROR: "__divdi3" [sound/usb/snd-usb-audio.ko] undefined!
--- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Jussi,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on sound/for-next] [also build test ERROR on v4.19-rc4 next-20180921] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Jussi-Laako/ALSA-usb-audio-Add-cust... base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next config: arm-omap2plus_defconfig (attached as .config) compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=7.2.0 make.cross ARCH=arm
All errors (new ones prefixed by >>):
ERROR: "__aeabi_ldivmod" [sound/usb/snd-usb-audio.ko] undefined! ERROR: "__aeabi_uldivmod" [sound/usb/snd-usb-audio.ko] undefined!
--- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Fri, 21 Sep 2018 00:07:19 +0200, Jussi Laako wrote:
This patch accessing vendor specific registers of RME Class Compliant devices works otherwise fine, but there is still one small problem. It is supposed to be a polled from the hardware, since the values indicate device status and are subject to change at all times. Behavior is correct if I keep running "amixer" all the time to retrieve values - sampling rate changes are visible and values get updated. However, when I poll the values from an application without reopening the mixer, they are being cached somewhere and the changes are not reflected. This problem doesn't exist for example with PCIe based RME HDSPe AIO and application gets updated values, but in this USB based device case they don't get updated. I have flagged all the mixer controls as read-only and volatile, but the volatile flag is not honored in this (assuming I've understood it correctly).
Any guidance on how to fix the caching problem is welcome!
The caching is currently enabled for all elements, but changing it should be trivial. The patch below adds is_volatile flag to the element, and you can set it to true in the quirk somehow for uncached controls.
thanks,
Takashi
--- --- a/sound/usb/mixer.c +++ b/sound/usb/mixer.c @@ -449,8 +449,10 @@ int snd_usb_get_cur_mix_value(struct usb_mixer_elem_info *cval, cval->control, channel, err); return err; } - cval->cached |= 1 << channel; - cval->cache_val[index] = *value; + if (!cval->is_volatile) { + cval->cached |= 1 << channel; + cval->cache_val[index] = *value; + } return 0; }
@@ -540,8 +542,10 @@ int snd_usb_set_cur_mix_value(struct usb_mixer_elem_info *cval, int channel, value); if (err < 0) return err; - cval->cached |= 1 << channel; - cval->cache_val[index] = value; + if (!cval->is_volatile) { + cval->cached |= 1 << channel; + cval->cache_val[index] = value; + } return 0; }
diff --git a/sound/usb/mixer.h b/sound/usb/mixer.h index 3d12af8bf191..cfbc274ce89f 100644 --- a/sound/usb/mixer.h +++ b/sound/usb/mixer.h @@ -70,6 +70,7 @@ struct usb_mixer_elem_info { int val_type; int min, max, res; int dBmin, dBmax; + bool is_volatile; int cached; int cache_val[MAX_CHANNELS]; u8 initialized;
The caching is currently enabled for all elements, but changing it should be trivial. The patch below adds is_volatile flag to the element, and you can set it to true in the quirk somehow for uncached controls.
This patch looks good and what I need! When trying to figure out where it is safe to set the volatile flag I thought that I could set it transparently in add_single_ctl_with_resume() in mixer_quirks.c based on the SNDRV_CTL_ELEM_ACCESS_VOLATILE flag in provided snd_kcontrol_new access-member. However, add_single_ctl_with_resume() is allocating just size of usb_mixer_elem_list unlike snd_create_std_mono_ctl_offset() which in turn is allocating full usb_mixer_elem_info size. However, the mixer code seems to be assuming that the item is always usb_mixer_elem_info instead of just list header item. Is this allocation behavior correct and is the item turned into usb_mixer_elem_info somewhere, or is this some kind of bug? So can I safely turn the allocation in add_single_ctl_with_resume() into zero initialized usb_mixer_elem_info instead and set the flag there while keeping correct behavior, or am I missing something?
Thanks,
- Jussi
On Fri, 21 Sep 2018 10:35:49 +0200, Jussi Laako wrote:
The caching is currently enabled for all elements, but changing it should be trivial. The patch below adds is_volatile flag to the element, and you can set it to true in the quirk somehow for uncached controls.
This patch looks good and what I need! When trying to figure out where it is safe to set the volatile flag I thought that I could set it transparently in add_single_ctl_with_resume() in mixer_quirks.c based on the SNDRV_CTL_ELEM_ACCESS_VOLATILE flag in provided snd_kcontrol_new access-member. However, add_single_ctl_with_resume() is allocating just size of usb_mixer_elem_list unlike snd_create_std_mono_ctl_offset() which in turn is allocating full usb_mixer_elem_info size. However, the mixer code seems to be assuming that the item is always usb_mixer_elem_info instead of just list header item. Is this allocation behavior correct and is the item turned into usb_mixer_elem_info somewhere, or is this some kind of bug? So can I safely turn the allocation in add_single_ctl_with_resume() into zero initialized usb_mixer_elem_info instead and set the flag there while keeping correct behavior, or am I missing something?
We can check SNDRV_CTL_ELEM_ACCESS_VOLATILE instead of introducing a new flag like my patch, yes. It's just a matter of easiness. Since there is no link from usb_mixer_elem_info to the assigned snd_kcontorl, we'd need to add an extra argument in the call path to inform about the volatileness, and it may become messy. We'll find the simplest way.
And, IMO, it'd be better to check the cache behavior at setting, since the cache may be accessed via multiple paths (at reading each element and at resuming the whole system). But it's no requirement, either.
Takashi
On 21.09.2018 11:43, Takashi Iwai wrote:
On Fri, 21 Sep 2018 10:35:49 +0200, Jussi Laako wrote:
The caching is currently enabled for all elements, but changing it should be trivial. The patch below adds is_volatile flag to the element, and you can set it to true in the quirk somehow for uncached controls.
This patch looks good and what I need! When trying to figure out where it is safe to set the volatile flag I thought that I could set it transparently in add_single_ctl_with_resume() in mixer_quirks.c based on the SNDRV_CTL_ELEM_ACCESS_VOLATILE flag in provided snd_kcontrol_new access-member. However, add_single_ctl_with_resume() is allocating just size of usb_mixer_elem_list unlike snd_create_std_mono_ctl_offset() which in turn is allocating full usb_mixer_elem_info size. However, the mixer code seems to be assuming that the item is always usb_mixer_elem_info instead of just list header item. Is this allocation behavior correct and is the item turned into usb_mixer_elem_info somewhere, or is this some kind of bug? So can I safely turn the allocation in add_single_ctl_with_resume() into zero initialized usb_mixer_elem_info instead and set the flag there while keeping correct behavior, or am I missing something?
We can check SNDRV_CTL_ELEM_ACCESS_VOLATILE instead of introducing a new flag like my patch, yes. It's just a matter of easiness. Since there is no link from usb_mixer_elem_info to the assigned snd_kcontorl, we'd need to add an extra argument in the call path to inform about the volatileness, and it may become messy. We'll find the simplest way.
And, IMO, it'd be better to check the cache behavior at setting, since the cache may be accessed via multiple paths (at reading each element and at resuming the whole system). But it's no requirement, either.
I have tried with attached patch, on top of your patch, but I'm still getting cached values from libasound2. It didn't break anything either... However, I don't see a link between this cache behavior and specific user space mixer descriptor. Because polling the mixer element with snd_mixer_selem_get_capture_volume() keeps always returning the same value. While in parallel checking the value by running "amixer" from command line gives updated correct value... So is there a second layer of cache somewhere in libasound2 to explain this discrepancy? Because one process sees updated value while other one doesn't. Difference being that the ALSA descriptors are closed and reopened every time "amixer" is run...
What is more perplexing is that from HDSPe driver same use case works, but it could be due to some event trigger from the HDSPe driver based on interrupt?
- Jussi
On Fri, 21 Sep 2018 14:53:20 +0200, Jussi Laako wrote:
On 21.09.2018 11:43, Takashi Iwai wrote:
On Fri, 21 Sep 2018 10:35:49 +0200, Jussi Laako wrote:
The caching is currently enabled for all elements, but changing it should be trivial. The patch below adds is_volatile flag to the element, and you can set it to true in the quirk somehow for uncached controls.
This patch looks good and what I need! When trying to figure out where it is safe to set the volatile flag I thought that I could set it transparently in add_single_ctl_with_resume() in mixer_quirks.c based on the SNDRV_CTL_ELEM_ACCESS_VOLATILE flag in provided snd_kcontrol_new access-member. However, add_single_ctl_with_resume() is allocating just size of usb_mixer_elem_list unlike snd_create_std_mono_ctl_offset() which in turn is allocating full usb_mixer_elem_info size. However, the mixer code seems to be assuming that the item is always usb_mixer_elem_info instead of just list header item. Is this allocation behavior correct and is the item turned into usb_mixer_elem_info somewhere, or is this some kind of bug? So can I safely turn the allocation in add_single_ctl_with_resume() into zero initialized usb_mixer_elem_info instead and set the flag there while keeping correct behavior, or am I missing something?
We can check SNDRV_CTL_ELEM_ACCESS_VOLATILE instead of introducing a new flag like my patch, yes. It's just a matter of easiness. Since there is no link from usb_mixer_elem_info to the assigned snd_kcontorl, we'd need to add an extra argument in the call path to inform about the volatileness, and it may become messy. We'll find the simplest way.
And, IMO, it'd be better to check the cache behavior at setting, since the cache may be accessed via multiple paths (at reading each element and at resuming the whole system). But it's no requirement, either.
I have tried with attached patch, on top of your patch, but I'm still getting cached values from libasound2. It didn't break anything either... However, I don't see a link between this cache behavior and specific user space mixer descriptor. Because polling the mixer element with snd_mixer_selem_get_capture_volume() keeps always returning the same value. While in parallel checking the value by running "amixer" from command line gives updated correct value... So is there a second layer of cache somewhere in libasound2 to explain this discrepancy? Because one process sees updated value while other one doesn't. Difference being that the ALSA descriptors are closed and reopened every time "amixer" is run...
Erm, I should have looked your *actual* patch before replying. Basically all your newly created elements are independent from the other USB standard things, and they are all read-only. That is, you don't need to use add_single_ctl_with_resume(), but just create each element via snd_ctl_new1() and add it via snd_ctl_add(). (Also you can pass the mixer object directly in private_value.)
If it still doesn't work as expected, it's rather something wrong in the alsa-lib side, or ACCESS_VOLATILE flag isn't passed / set properly.
Takashi
What is more perplexing is that from HDSPe driver same use case works, but it could be due to some event trigger from the HDSPe driver based on interrupt?
- Jussi
diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c index 762c190..cff31ce 100644 --- a/sound/usb/mixer_quirks.c +++ b/sound/usb/mixer_quirks.c @@ -152,20 +152,24 @@ static int add_single_ctl_with_resume(struct usb_mixer_interface *mixer, const struct snd_kcontrol_new *knew, struct usb_mixer_elem_list **listp) {
- struct usb_mixer_elem_info *cval; struct usb_mixer_elem_list *list; struct snd_kcontrol *kctl;
- list = kzalloc(sizeof(*list), GFP_KERNEL);
- if (!list)
- cval = kzalloc(sizeof(*cval), GFP_KERNEL);
- if (!cval) return -ENOMEM;
- list = &cval->head; if (listp) *listp = list;
- if (knew->access & SNDRV_CTL_ELEM_ACCESS_VOLATILE)
cval->is_volatile = 1;
- kctl = snd_ctl_new1(knew, list);
- kctl = snd_ctl_new1(knew, cval); if (!kctl) {
kfree(list);
kfree(cval);
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On 21.09.2018 16:06, Takashi Iwai wrote:
On Fri, 21 Sep 2018 14:53:20 +0200, Jussi Laako wrote:
On 21.09.2018 11:43, Takashi Iwai wrote:
On Fri, 21 Sep 2018 10:35:49 +0200, Jussi Laako wrote:
The caching is currently enabled for all elements, but changing it should be trivial. The patch below adds is_volatile flag to the element, and you can set it to true in the quirk somehow for uncached controls.
This patch looks good and what I need! When trying to figure out where it is safe to set the volatile flag I thought that I could set it transparently in add_single_ctl_with_resume() in mixer_quirks.c based on the SNDRV_CTL_ELEM_ACCESS_VOLATILE flag in provided snd_kcontrol_new access-member. However, add_single_ctl_with_resume() is allocating just size of usb_mixer_elem_list unlike snd_create_std_mono_ctl_offset() which in turn is allocating full usb_mixer_elem_info size. However, the mixer code seems to be assuming that the item is always usb_mixer_elem_info instead of just list header item. Is this allocation behavior correct and is the item turned into usb_mixer_elem_info somewhere, or is this some kind of bug? So can I safely turn the allocation in add_single_ctl_with_resume() into zero initialized usb_mixer_elem_info instead and set the flag there while keeping correct behavior, or am I missing something?
We can check SNDRV_CTL_ELEM_ACCESS_VOLATILE instead of introducing a new flag like my patch, yes. It's just a matter of easiness. Since there is no link from usb_mixer_elem_info to the assigned snd_kcontorl, we'd need to add an extra argument in the call path to inform about the volatileness, and it may become messy. We'll find the simplest way.
And, IMO, it'd be better to check the cache behavior at setting, since the cache may be accessed via multiple paths (at reading each element and at resuming the whole system). But it's no requirement, either.
I have tried with attached patch, on top of your patch, but I'm still getting cached values from libasound2. It didn't break anything either... However, I don't see a link between this cache behavior and specific user space mixer descriptor. Because polling the mixer element with snd_mixer_selem_get_capture_volume() keeps always returning the same value. While in parallel checking the value by running "amixer" from command line gives updated correct value... So is there a second layer of cache somewhere in libasound2 to explain this discrepancy? Because one process sees updated value while other one doesn't. Difference being that the ALSA descriptors are closed and reopened every time "amixer" is run...
Erm, I should have looked your *actual* patch before replying. Basically all your newly created elements are independent from the other USB standard things, and they are all read-only. That is, you don't need to use add_single_ctl_with_resume(), but just create each element via snd_ctl_new1() and add it via snd_ctl_add(). (Also you can pass the mixer object directly in private_value.)
If it still doesn't work as expected, it's rather something wrong in the alsa-lib side, or ACCESS_VOLATILE flag isn't passed / set properly.
By the way is the add_single_ctl_with_resume() correct in allocating just snd_mixer_elem_list instead of snd_mixer_elem_info? I got suspicious about this when reading the sources. Because the code is similar to snd_create_std_mono_ctl_offset() but they allocate different size which they go on to pass to the same functions. And in quite many places functions cast the snd_mixer_elem_list pointer to snd_mixer_elem_info pointer to access the entire structure which would of course be wrong if it's just snd_mixer_elem_list size item. The patch I posted in my previous mail that changes add_single_ctl_with_resume() to allocate snd_mixer_elem_info instead at least didn't seem to break things...
OK, I already started going through the alsa-lib code, I suspect something is going wrong there because different processes give different values depending on process lifetime and at the driver level there's no process descriptor context yet...
Thanks,
- Jussi
On 21.09.2018 16:26, Jussi Laako wrote:
On 21.09.2018 16:06, Takashi Iwai wrote:
On Fri, 21 Sep 2018 14:53:20 +0200, Jussi Laako wrote:
On 21.09.2018 11:43, Takashi Iwai wrote:
On Fri, 21 Sep 2018 10:35:49 +0200, Jussi Laako wrote:
The caching is currently enabled for all elements, but changing it should be trivial. The patch below adds is_volatile flag to the element, and you can set it to true in the quirk somehow for uncached controls.
This patch looks good and what I need! When trying to figure out where it is safe to set the volatile flag I thought that I could set it transparently in add_single_ctl_with_resume() in mixer_quirks.c based on the SNDRV_CTL_ELEM_ACCESS_VOLATILE flag in provided snd_kcontrol_new access-member. However, add_single_ctl_with_resume() is allocating just size of usb_mixer_elem_list unlike snd_create_std_mono_ctl_offset() which in turn is allocating full usb_mixer_elem_info size. However, the mixer code seems to be assuming that the item is always usb_mixer_elem_info instead of just list header item. Is this allocation behavior correct and is the item turned into usb_mixer_elem_info somewhere, or is this some kind of bug? So can I safely turn the allocation in add_single_ctl_with_resume() into zero initialized usb_mixer_elem_info instead and set the flag there while keeping correct behavior, or am I missing something?
We can check SNDRV_CTL_ELEM_ACCESS_VOLATILE instead of introducing a new flag like my patch, yes. It's just a matter of easiness. Since there is no link from usb_mixer_elem_info to the assigned snd_kcontorl, we'd need to add an extra argument in the call path to inform about the volatileness, and it may become messy. We'll find the simplest way.
And, IMO, it'd be better to check the cache behavior at setting, since the cache may be accessed via multiple paths (at reading each element and at resuming the whole system). But it's no requirement, either.
I have tried with attached patch, on top of your patch, but I'm still getting cached values from libasound2. It didn't break anything either... However, I don't see a link between this cache behavior and specific user space mixer descriptor. Because polling the mixer element with snd_mixer_selem_get_capture_volume() keeps always returning the same value. While in parallel checking the value by running "amixer" from command line gives updated correct value... So is there a second layer of cache somewhere in libasound2 to explain this discrepancy? Because one process sees updated value while other one doesn't. Difference being that the ALSA descriptors are closed and reopened every time "amixer" is run...
Erm, I should have looked your *actual* patch before replying. Basically all your newly created elements are independent from the other USB standard things, and they are all read-only. That is, you don't need to use add_single_ctl_with_resume(), but just create each element via snd_ctl_new1() and add it via snd_ctl_add(). (Also you can pass the mixer object directly in private_value.)
If it still doesn't work as expected, it's rather something wrong in the alsa-lib side, or ACCESS_VOLATILE flag isn't passed / set properly.
By the way is the add_single_ctl_with_resume() correct in allocating just snd_mixer_elem_list instead of snd_mixer_elem_info? I got suspicious about this when reading the sources. Because the code is similar to snd_create_std_mono_ctl_offset() but they allocate different size which they go on to pass to the same functions. And in quite many places functions cast the snd_mixer_elem_list pointer to snd_mixer_elem_info pointer to access the entire structure which would of course be wrong if it's just snd_mixer_elem_list size item. The patch I posted in my previous mail that changes add_single_ctl_with_resume() to allocate snd_mixer_elem_info instead at least didn't seem to break things...
OK, I already started going through the alsa-lib code, I suspect something is going wrong there because different processes give different values depending on process lifetime and at the driver level there's no process descriptor context yet...
OK, problem solved! My original patch for the RME mixer quirks is good, no changes needed to that one.
Problematic caching happens inside alsa-lib in simple mixer interface, where snd_mixer_selem_get_capture_volume() always returns cached value for mixer value read and doesn't trigger update from kernel. This is now fixed in my user space by using snd_hctl_elem_read() instead which triggers update from kernel, and as a side effect also seems to trigger update to the cached value so that later call to snd_mixer_selem_get_capture_volume() returns updated value too. I'm not sure if something should be done about snd_mixer_selem_get_capture_volume() behavior or not, but now at least I know how to work around this by using different API layer.
In addition, I've applied attached patch on top of your volatile patch, but I think that part is not strictly necessary but maybe good thing to do. This attached patch fixes (?) the allocation behavior of add_single_ctl_with_resume() I was talking about and adds volatile check on snd_usb_get_cur_mix_value() entry which should be superfluous to your change later in the function.
Thanks gain!
- Jussi
participants (3)
-
Jussi Laako -
kbuild test robot -
Takashi Iwai