[RFC] ALSA: usb-audio: Add custom mixer status quirks for RME CC devices

[RFC] ALSA: usb-audio: Add custom mixer status quirks for RME CC devices

[Jussi Laako]

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!

[PATCH] ALSA: usb-audio: Add custom mixer status quirks for RME CC devices

[Jussi Laako]

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;
 }
 
-- 
2.7.4

Re: [RFC] ALSA: usb-audio: Add custom mixer status quirks for RME CC devices

[Takashi Iwai]

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;

Re: [RFC] ALSA: usb-audio: Add custom mixer status quirks for RME CC devices

[Jussi Laako]

> 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

Re: [RFC] ALSA: usb-audio: Add custom mixer status quirks for RME CC devices

[Takashi Iwai]

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

Re: [RFC] ALSA: usb-audio: Add custom mixer status quirks for RME CC devices

[Jussi Laako]

[-- Attachment #1: Type: text/plain, Size: 2781 bytes --]

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

[-- Attachment #2: tmp.patch --]
[-- Type: text/x-patch, Size: 973 bytes --]

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;
 	list->mixer = mixer;
 	list->id = id;
 	list->resume = resume;
-	kctl = snd_ctl_new1(knew, list);
+	kctl = snd_ctl_new1(knew, cval);
 	if (!kctl) {
-		kfree(list);
+		kfree(cval);
 		return -ENOMEM;
 	}
 	kctl->private_free = snd_usb_mixer_elem_free;

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [RFC] ALSA: usb-audio: Add custom mixer status quirks for RME CC devices

[Takashi Iwai]

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;
>  	list->mixer = mixer;
>  	list->id = id;
>  	list->resume = resume;
> -	kctl = snd_ctl_new1(knew, list);
> +	kctl = snd_ctl_new1(knew, cval);
>  	if (!kctl) {
> -		kfree(list);
> +		kfree(cval);
>  		return -ENOMEM;
>  	}
>  	kctl->private_free = snd_usb_mixer_elem_free;
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [RFC] ALSA: usb-audio: Add custom mixer status quirks for RME CC devices

[Jussi Laako]

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

Re: [PATCH] ALSA: usb-audio: Add custom mixer status quirks for RME CC devices

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1168 bytes --]

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-custom-mixer-status-quirks-for-RME-CC-devices/20180922-102141
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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 25004 bytes --]

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [RFC] ALSA: usb-audio: Add custom mixer status quirks for RME CC devices

[Jussi Laako]

[-- Attachment #1: Type: text/plain, Size: 5631 bytes --]

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

[-- Attachment #2: volatile-fix-2.patch --]
[-- Type: text/x-patch, Size: 1357 bytes --]

diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 6d360fa..21c053e 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -438,7 +438,7 @@ int snd_usb_get_cur_mix_value(struct usb_mixer_elem_info *cval,
 {
 	int err;
 
-	if (cval->cached & (1 << channel)) {
+	if ((cval->cached & (1 << channel)) && !cval->is_volatile) {
 		*value = cval->cache_val[index];
 		return 0;
 	}
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;
 	list->mixer = mixer;
 	list->id = id;
 	list->resume = resume;
-	kctl = snd_ctl_new1(knew, list);
+	kctl = snd_ctl_new1(knew, cval);
 	if (!kctl) {
-		kfree(list);
+		kfree(cval);
 		return -ENOMEM;
 	}
 	kctl->private_free = snd_usb_mixer_elem_free;

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ALSA: usb-audio: Add custom mixer status quirks for RME CC devices

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1181 bytes --]

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-custom-mixer-status-quirks-for-RME-CC-devices/20180922-102141
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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34349 bytes --]

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]