[PATCH] ALSA: core - add more card sysfs entries
There are several strings which are describing the card. As time goes, we have new universal drivers which probe components in a way, which is disassociated from the card structure (ASoC). Also, some drivers may require to select another firmware depending on the specific platform using udev. The new firmware may change the sound card behaviour.
This patch allows flexible modifications of the card description from the user space to handle the specific boot / plug-in settings.
Cc: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Cc: Mark Brown broonie@kernel.org Signed-off-by: Jaroslav Kysela perex@perex.cz --- sound/core/init.c | 166 +++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 155 insertions(+), 11 deletions(-)
diff --git a/sound/core/init.c b/sound/core/init.c index ef41f5b3a240..01b26912a4d0 100644 --- a/sound/core/init.c +++ b/sound/core/init.c @@ -662,6 +662,33 @@ void snd_card_set_id(struct snd_card *card, const char *nid) } EXPORT_SYMBOL(snd_card_set_id);
+#define EXTRA_ID_CHARS "_-" +#define EXTRA_NAME_CHARS " _-.:" + +static bool safe_attr_strcpy(char *dst, size_t dst_count, + const char *src, size_t src_count, + const char *extra_characters) +{ + size_t idx, copy; + int c; + + copy = src_count >= dst_count ? dst_count - 1 : src_count; + for (idx = 0; idx < copy; idx++) { + c = src[idx]; + if (c < ' ') { + copy = idx; + break; + } + if (!isalnum(c) && !strchr(extra_characters, c)) + return false; + } + if (copy < 3) + return false; + memcpy(dst, src, copy); + dst[copy] = '\0'; + return true; +} + static ssize_t card_id_show_attr(struct device *dev, struct device_attribute *attr, char *buf) @@ -676,18 +703,10 @@ card_id_store_attr(struct device *dev, struct device_attribute *attr, { struct snd_card *card = container_of(dev, struct snd_card, card_dev); char buf1[sizeof(card->id)]; - size_t copy = count > sizeof(card->id) - 1 ? - sizeof(card->id) - 1 : count; - size_t idx; - int c;
- for (idx = 0; idx < copy; idx++) { - c = buf[idx]; - if (!isalnum(c) && c != '_' && c != '-') - return -EINVAL; - } - memcpy(buf1, buf, copy); - buf1[copy] = '\0'; + if (!safe_attr_strcpy(buf1, sizeof(buf1), buf, count, EXTRA_ID_CHARS)) + return -EINVAL; + mutex_lock(&snd_card_mutex); if (!card_id_ok(NULL, buf1)) { mutex_unlock(&snd_card_mutex); @@ -712,9 +731,134 @@ card_number_show_attr(struct device *dev,
static DEVICE_ATTR(number, 0444, card_number_show_attr, NULL);
+static ssize_t +card_driver_show_attr(struct device *dev, struct device_attribute *attr, char *buf) +{ + struct snd_card *card = container_of(dev, struct snd_card, card_dev); + return scnprintf(buf, PAGE_SIZE, "%s\n", card->driver); +} + +static ssize_t +card_driver_store_attr(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct snd_card *card = container_of(dev, struct snd_card, card_dev); + char driver1[sizeof(card->driver)]; + + if (!safe_attr_strcpy(driver1, sizeof(driver1), buf, count, EXTRA_NAME_CHARS)) + return -EINVAL; + mutex_lock(&snd_card_mutex); + strcpy(card->driver, driver1); + mutex_unlock(&snd_card_mutex); + return count; +} + +static DEVICE_ATTR(driver, 0644, card_driver_show_attr, card_driver_store_attr); + +static ssize_t +card_name_show_attr(struct device *dev, struct device_attribute *attr, char *buf) +{ + struct snd_card *card = container_of(dev, struct snd_card, card_dev); + return scnprintf(buf, PAGE_SIZE, "%s\n", card->shortname); +} + +static ssize_t +card_name_store_attr(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct snd_card *card = container_of(dev, struct snd_card, card_dev); + char name1[sizeof(card->shortname)]; + + if (!safe_attr_strcpy(name1, sizeof(name1), buf, count, EXTRA_NAME_CHARS)) + return -EINVAL; + mutex_lock(&snd_card_mutex); + strcpy(card->shortname, name1); + mutex_unlock(&snd_card_mutex); + return count; +} + +static DEVICE_ATTR(name, 0644, card_name_show_attr, card_name_store_attr); + +static ssize_t +card_longname_show_attr(struct device *dev, struct device_attribute *attr, char *buf) +{ + struct snd_card *card = container_of(dev, struct snd_card, card_dev); + return scnprintf(buf, PAGE_SIZE, "%s\n", card->longname); +} + +static ssize_t +card_longname_store_attr(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct snd_card *card = container_of(dev, struct snd_card, card_dev); + char longname1[sizeof(card->longname)]; + + if (!safe_attr_strcpy(longname1, sizeof(longname1), buf, count, EXTRA_NAME_CHARS)) + return -EINVAL; + mutex_lock(&snd_card_mutex); + strcpy(card->longname, longname1); + mutex_unlock(&snd_card_mutex); + return count; +} + +static DEVICE_ATTR(longname, 0644, card_longname_show_attr, card_longname_store_attr); + +static ssize_t +card_mixername_show_attr(struct device *dev, struct device_attribute *attr, char *buf) +{ + struct snd_card *card = container_of(dev, struct snd_card, card_dev); + return scnprintf(buf, PAGE_SIZE, "%s\n", card->mixername); +} + +static ssize_t +card_mixername_store_attr(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct snd_card *card = container_of(dev, struct snd_card, card_dev); + char mixername1[sizeof(card->mixername)]; + + if (!safe_attr_strcpy(mixername1, sizeof(mixername1), buf, count, EXTRA_NAME_CHARS)) + return -EINVAL; + mutex_lock(&snd_card_mutex); + strcpy(card->mixername, mixername1); + mutex_unlock(&snd_card_mutex); + return count; +} + +static DEVICE_ATTR(mixername, 0644, card_mixername_show_attr, card_mixername_store_attr); + +static ssize_t +card_components_show_attr(struct device *dev, struct device_attribute *attr, char *buf) +{ + struct snd_card *card = container_of(dev, struct snd_card, card_dev); + return scnprintf(buf, PAGE_SIZE, "%s\n", card->components); +} + +static ssize_t +card_components_store_attr(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct snd_card *card = container_of(dev, struct snd_card, card_dev); + char components1[sizeof(card->components)]; + + if (!safe_attr_strcpy(components1, sizeof(components1), buf, count, EXTRA_NAME_CHARS)) + return -EINVAL; + mutex_lock(&snd_card_mutex); + strcpy(card->components, components1); + mutex_unlock(&snd_card_mutex); + return count; +} + +static DEVICE_ATTR(components, 0644, card_components_show_attr, card_components_store_attr); + static struct attribute *card_dev_attrs[] = { &dev_attr_id.attr, &dev_attr_number.attr, + &dev_attr_driver.attr, + &dev_attr_name.attr, + &dev_attr_longname.attr, + &dev_attr_mixername.attr, + &dev_attr_components.attr, NULL };
On Thu, Apr 08, 2021 at 11:43:14AM +0200, Jaroslav Kysela wrote:
There are several strings which are describing the card. As time goes, we have new universal drivers which probe components in a way, which is disassociated from the card structure (ASoC). Also, some drivers may require to select another firmware depending on the specific platform using udev. The new firmware may change the sound card behaviour.
This patch allows flexible modifications of the card description from the user space to handle the specific boot / plug-in settings.
Cc: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Cc: Mark Brown broonie@kernel.org Signed-off-by: Jaroslav Kysela perex@perex.cz
sound/core/init.c | 166 +++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 155 insertions(+), 11 deletions(-)
diff --git a/sound/core/init.c b/sound/core/init.c index ef41f5b3a240..01b26912a4d0 100644 --- a/sound/core/init.c +++ b/sound/core/init.c @@ -662,6 +662,33 @@ void snd_card_set_id(struct snd_card *card, const char *nid) } EXPORT_SYMBOL(snd_card_set_id);
+#define EXTRA_ID_CHARS "_-" +#define EXTRA_NAME_CHARS " _-.:"
+static bool safe_attr_strcpy(char *dst, size_t dst_count,
const char *src, size_t src_count,const char *extra_characters)+{
- size_t idx, copy;
- int c;
- copy = src_count >= dst_count ? dst_count - 1 : src_count;
- for (idx = 0; idx < copy; idx++) {
c = src[idx];if (c < ' ') {copy = idx;break;}if (!isalnum(c) && !strchr(extra_characters, c))return false;- }
- if (copy < 3)
return false;- memcpy(dst, src, copy);
- dst[copy] = '\0';
- return true;
+}
static ssize_t card_id_show_attr(struct device *dev, struct device_attribute *attr, char *buf) @@ -676,18 +703,10 @@ card_id_store_attr(struct device *dev, struct device_attribute *attr, { struct snd_card *card = container_of(dev, struct snd_card, card_dev); char buf1[sizeof(card->id)];
size_t copy = count > sizeof(card->id) - 1 ?
sizeof(card->id) - 1 : count;size_t idx;
int c;
for (idx = 0; idx < copy; idx++) {
c = buf[idx];if (!isalnum(c) && c != '_' && c != '-')return -EINVAL;}
memcpy(buf1, buf, copy);
buf1[copy] = '\0';
- if (!safe_attr_strcpy(buf1, sizeof(buf1), buf, count, EXTRA_ID_CHARS))
return -EINVAL;- mutex_lock(&snd_card_mutex); if (!card_id_ok(NULL, buf1)) { mutex_unlock(&snd_card_mutex);
@@ -712,9 +731,134 @@ card_number_show_attr(struct device *dev,
static DEVICE_ATTR(number, 0444, card_number_show_attr, NULL);
Use DEVICE_ATTR_RO() instead.
+static ssize_t +card_driver_show_attr(struct device *dev, struct device_attribute *attr, char *buf) +{
- struct snd_card *card = container_of(dev, struct snd_card, card_dev);
- return scnprintf(buf, PAGE_SIZE, "%s\n", card->driver);
+}
+static ssize_t +card_driver_store_attr(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)+{
- struct snd_card *card = container_of(dev, struct snd_card, card_dev);
- char driver1[sizeof(card->driver)];
- if (!safe_attr_strcpy(driver1, sizeof(driver1), buf, count, EXTRA_NAME_CHARS))
return -EINVAL;- mutex_lock(&snd_card_mutex);
- strcpy(card->driver, driver1);
- mutex_unlock(&snd_card_mutex);
- return count;
+}
+static DEVICE_ATTR(driver, 0644, card_driver_show_attr, card_driver_store_attr);
Use DEVICE_ATTR_RW() instead.
+static ssize_t +card_name_show_attr(struct device *dev, struct device_attribute *attr, char *buf) +{
- struct snd_card *card = container_of(dev, struct snd_card, card_dev);
- return scnprintf(buf, PAGE_SIZE, "%s\n", card->shortname);
+}
+static ssize_t +card_name_store_attr(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)+{
- struct snd_card *card = container_of(dev, struct snd_card, card_dev);
- char name1[sizeof(card->shortname)];
- if (!safe_attr_strcpy(name1, sizeof(name1), buf, count, EXTRA_NAME_CHARS))
return -EINVAL;- mutex_lock(&snd_card_mutex);
- strcpy(card->shortname, name1);
- mutex_unlock(&snd_card_mutex);
- return count;
+}
+static DEVICE_ATTR(name, 0644, card_name_show_attr, card_name_store_attr);
Use DEVICE_ATTR_RW() instead.
+static ssize_t +card_longname_show_attr(struct device *dev, struct device_attribute *attr, char *buf) +{
- struct snd_card *card = container_of(dev, struct snd_card, card_dev);
- return scnprintf(buf, PAGE_SIZE, "%s\n", card->longname);
+}
+static ssize_t +card_longname_store_attr(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)+{
- struct snd_card *card = container_of(dev, struct snd_card, card_dev);
- char longname1[sizeof(card->longname)];
- if (!safe_attr_strcpy(longname1, sizeof(longname1), buf, count, EXTRA_NAME_CHARS))
return -EINVAL;- mutex_lock(&snd_card_mutex);
- strcpy(card->longname, longname1);
- mutex_unlock(&snd_card_mutex);
- return count;
+}
+static DEVICE_ATTR(longname, 0644, card_longname_show_attr, card_longname_store_attr);
Use DEVICE_ATTR_RW() instead.
+static ssize_t +card_mixername_show_attr(struct device *dev, struct device_attribute *attr, char *buf) +{
- struct snd_card *card = container_of(dev, struct snd_card, card_dev);
- return scnprintf(buf, PAGE_SIZE, "%s\n", card->mixername);
+}
+static ssize_t +card_mixername_store_attr(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)+{
- struct snd_card *card = container_of(dev, struct snd_card, card_dev);
- char mixername1[sizeof(card->mixername)];
- if (!safe_attr_strcpy(mixername1, sizeof(mixername1), buf, count, EXTRA_NAME_CHARS))
return -EINVAL;- mutex_lock(&snd_card_mutex);
- strcpy(card->mixername, mixername1);
- mutex_unlock(&snd_card_mutex);
- return count;
+}
+static DEVICE_ATTR(mixername, 0644, card_mixername_show_attr, card_mixername_store_attr);
Use DEVICE_ATTR_RW() instead.
+static ssize_t +card_components_show_attr(struct device *dev, struct device_attribute *attr, char *buf) +{
- struct snd_card *card = container_of(dev, struct snd_card, card_dev);
- return scnprintf(buf, PAGE_SIZE, "%s\n", card->components);
+}
+static ssize_t +card_components_store_attr(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)+{
- struct snd_card *card = container_of(dev, struct snd_card, card_dev);
- char components1[sizeof(card->components)];
- if (!safe_attr_strcpy(components1, sizeof(components1), buf, count, EXTRA_NAME_CHARS))
return -EINVAL;- mutex_lock(&snd_card_mutex);
- strcpy(card->components, components1);
- mutex_unlock(&snd_card_mutex);
- return count;
+}
+static DEVICE_ATTR(components, 0644, card_components_show_attr, card_components_store_attr);
Use DEVICE_ATTR_RW() instead.
static struct attribute *card_dev_attrs[] = { &dev_attr_id.attr, &dev_attr_number.attr,
- &dev_attr_driver.attr,
- &dev_attr_name.attr,
- &dev_attr_longname.attr,
- &dev_attr_mixername.attr,
- &dev_attr_components.attr, NULL
};
-- 2.30.2
It should be done to emit snd_ctl_event when changing card parameters. Silent change is the worst since many userspace applications can refer to them in advance.
Regards
Takashi Sakamoto
On Thu, 08 Apr 2021 12:38:19 +0200, Takashi Sakamoto wrote:
It should be done to emit snd_ctl_event when changing card parameters. Silent change is the worst since many userspace applications can refer to them in advance.
Agreed. The changes of those names during operation is fairly intrusive, and can bring many side-effects.
Any reason that mixername and co have to be changeable inevitably? If that's about UCM profile and its selection of the hardware configuration, can it be another additional information instead of overwriting the existing strings set by the driver?
thanks,
Takashi
Dne 08. 04. 21 v 13:05 Takashi Iwai napsal(a):
On Thu, 08 Apr 2021 12:38:19 +0200, Takashi Sakamoto wrote:
It should be done to emit snd_ctl_event when changing card parameters. Silent change is the worst since many userspace applications can refer to them in advance.
Agreed. The changes of those names during operation is fairly intrusive, and can bring many side-effects.
The event may be nice, indeed. The locking is also not so safe. But it should be implemented separately.
Any reason that mixername and co have to be changeable inevitably? If that's about UCM profile and its selection of the hardware configuration, can it be another additional information instead of overwriting the existing strings set by the driver?
Thanks for the discussion. I expected it. The ASoC drivers set all those parameters in an inconsistent way (nobody verifies that) and I just propose to fix things at boot in the user space. The goal is not to allow the random user changes, but to modify this settings via udev in a persistent way. Also, another firmware can really make the card different from the user space view.
Yes, it's for UCM, but even if we don't consider this purpose, the kernel API should return some reasonable information rather than very generic (or empty) strings which are used in the native ALSA utilities for example. So, I think that we should allow to "fix" this info also from the user space rather than to extend the existing API.
Jaroslav
On Thu, Apr 08, 2021 at 01:21:52PM +0200, Jaroslav Kysela wrote:
Yes, it's for UCM, but even if we don't consider this purpose, the kernel API should return some reasonable information rather than very generic (or empty) strings which are used in the native ALSA utilities for example. So, I think that we should allow to "fix" this info also from the user space rather than to extend the existing API.
Half the point with UCM was supposed to be to rewrite the control names we get from the devices into standard things that are useful for userspace, having to remap things for UCM doesn't sound right.
On Thu, 08 Apr 2021 14:05:02 +0200, Mark Brown wrote:
On Thu, Apr 08, 2021 at 01:21:52PM +0200, Jaroslav Kysela wrote:
Yes, it's for UCM, but even if we don't consider this purpose, the kernel API should return some reasonable information rather than very generic (or empty) strings which are used in the native ALSA utilities for example. So, I think that we should allow to "fix" this info also from the user space rather than to extend the existing API.
Half the point with UCM was supposed to be to rewrite the control names we get from the devices into standard things that are useful for userspace, having to remap things for UCM doesn't sound right.
I guess the question here is how to identify the proper profile for a certain platform and how to get it passed over the whole system. Theoretically, the rename of the card name or mixer name strings could be done in user-space side, too (e.g. mapping in alsa-lib or whatever), so I don't think it mandatory to make them variable via sysfs, if it's meant only for the consistency reason.
Didn't we discuss in the past about the possibility to store the profile name in the card component string?
thanks,
Takashi
On 4/8/21 8:18 AM, Takashi Iwai wrote:
On Thu, 08 Apr 2021 14:05:02 +0200, Mark Brown wrote:
On Thu, Apr 08, 2021 at 01:21:52PM +0200, Jaroslav Kysela wrote:
Yes, it's for UCM, but even if we don't consider this purpose, the kernel API should return some reasonable information rather than very generic (or empty) strings which are used in the native ALSA utilities for example. So, I think that we should allow to "fix" this info also from the user space rather than to extend the existing API.
Half the point with UCM was supposed to be to rewrite the control names we get from the devices into standard things that are useful for userspace, having to remap things for UCM doesn't sound right.
I guess the question here is how to identify the proper profile for a certain platform and how to get it passed over the whole system. Theoretically, the rename of the card name or mixer name strings could be done in user-space side, too (e.g. mapping in alsa-lib or whatever), so I don't think it mandatory to make them variable via sysfs, if it's meant only for the consistency reason.
Didn't we discuss in the past about the possibility to store the profile name in the card component string?
Here's a summary of an earlier discussion with Jaroslav, based on an initial ask from Curtis https://github.com/thesofproject/linux/issues/2766:
When a specific PCI ID is detected, we probe the SOF driver and will load a default firmware binary and topology.
Because of OEM or user customization, we will have multiple versions of firmware and topology that will have to be enabled in specific setting. The last thing we want is hard-coded rules in the kernel on which firmware customization to use for which platform.
The suggestion was made to use udev rules to modify the default path for the firmware and topology, so that e.g. on a specific Chromebook you could load firmware from /lib/firmware/google/<device name>/sof-tplg. The same can happen for other OEMs that support Linux distributions such as Dell and Lenovo.
If the users wipes the OEM image and installs a standard distribution on the same device, they would by default use the firmware generated from the SOF main branch, without any differentiation and 3rd party IP.
So the point is: how do we expose this information to UCM? In the machine driver where the card is created? There is zero information on what the firmware/topology does. The information can only be extracted when the topology is loaded when probing the SOF component driver.
I don't think the point was to rewrite the controls but make sure that UCM is aware that the card definition was changed by a different selection of firmware.
Jaroslav, please correct me if I misunderstood the intent of this patch!
On Thu, Apr 08, 2021 at 09:12:57AM -0500, Pierre-Louis Bossart wrote:
On 4/8/21 8:18 AM, Takashi Iwai wrote:
I guess the question here is how to identify the proper profile for a certain platform and how to get it passed over the whole system. Theoretically, the rename of the card name or mixer name strings could be done in user-space side, too (e.g. mapping in alsa-lib or whatever), so I don't think it mandatory to make them variable via sysfs, if it's meant only for the consistency reason.
Didn't we discuss in the past about the possibility to store the profile name in the card component string?
Because of OEM or user customization, we will have multiple versions of firmware and topology that will have to be enabled in specific setting. The last thing we want is hard-coded rules in the kernel on which firmware customization to use for which platform.
...
If the users wipes the OEM image and installs a standard distribution on the same device, they would by default use the firmware generated from the SOF main branch, without any differentiation and 3rd party IP.
So the point is: how do we expose this information to UCM? In the machine driver where the card is created? There is zero information on what the firmware/topology does. The information can only be extracted when the topology is loaded when probing the SOF component driver.
So what we're looking for here is a mechanism to tell userspace what firmware has been loaded?
I don't think the point was to rewrite the controls but make sure that UCM is aware that the card definition was changed by a different selection of firmware.
Jaroslav, please correct me if I misunderstood the intent of this patch!
Dne 08. 04. 21 v 16:47 Mark Brown napsal(a):
On Thu, Apr 08, 2021 at 09:12:57AM -0500, Pierre-Louis Bossart wrote:
On 4/8/21 8:18 AM, Takashi Iwai wrote:
I guess the question here is how to identify the proper profile for a certain platform and how to get it passed over the whole system. Theoretically, the rename of the card name or mixer name strings could be done in user-space side, too (e.g. mapping in alsa-lib or whatever), so I don't think it mandatory to make them variable via sysfs, if it's meant only for the consistency reason.
Didn't we discuss in the past about the possibility to store the profile name in the card component string?
Because of OEM or user customization, we will have multiple versions of firmware and topology that will have to be enabled in specific setting. The last thing we want is hard-coded rules in the kernel on which firmware customization to use for which platform.
...
If the users wipes the OEM image and installs a standard distribution on the same device, they would by default use the firmware generated from the SOF main branch, without any differentiation and 3rd party IP.
So the point is: how do we expose this information to UCM? In the machine driver where the card is created? There is zero information on what the firmware/topology does. The information can only be extracted when the topology is loaded when probing the SOF component driver.
So what we're looking for here is a mechanism to tell userspace what firmware has been loaded?
It's just a part of the issue with the proper runtime sound card identification. But yes, the firmware can really change the created sound card (controls, PCM devices, paramters for those devices). In this case, UCM should pick another configuration.
I'm looking for a straight solution here.
Jaroslav
On 4/8/2021 5:04 PM, Jaroslav Kysela wrote:
Dne 08. 04. 21 v 16:47 Mark Brown napsal(a):
On Thu, Apr 08, 2021 at 09:12:57AM -0500, Pierre-Louis Bossart wrote:
On 4/8/21 8:18 AM, Takashi Iwai wrote:
I guess the question here is how to identify the proper profile for a certain platform and how to get it passed over the whole system. Theoretically, the rename of the card name or mixer name strings could be done in user-space side, too (e.g. mapping in alsa-lib or whatever), so I don't think it mandatory to make them variable via sysfs, if it's meant only for the consistency reason.
Didn't we discuss in the past about the possibility to store the profile name in the card component string?
Because of OEM or user customization, we will have multiple versions of firmware and topology that will have to be enabled in specific setting. The last thing we want is hard-coded rules in the kernel on which firmware customization to use for which platform.
...
If the users wipes the OEM image and installs a standard distribution on the same device, they would by default use the firmware generated from the SOF main branch, without any differentiation and 3rd party IP.
So the point is: how do we expose this information to UCM? In the machine driver where the card is created? There is zero information on what the firmware/topology does. The information can only be extracted when the topology is loaded when probing the SOF component driver.
So what we're looking for here is a mechanism to tell userspace what firmware has been loaded?
It's just a part of the issue with the proper runtime sound card identification. But yes, the firmware can really change the created sound card (controls, PCM devices, paramters for those devices). In this case, UCM should pick another configuration.
I'm looking for a straight solution here.
Jaroslav
Wouldn't dsp_status from snd_hwdep already work here? The only thing which would need to be done is to connect it to ASoC card?
And in userspace ask using hwdep interface for what is the loaded FW?
Amadeusz
Dne 08. 04. 21 v 15:18 Takashi Iwai napsal(a):
On Thu, 08 Apr 2021 14:05:02 +0200, Mark Brown wrote:
On Thu, Apr 08, 2021 at 01:21:52PM +0200, Jaroslav Kysela wrote:
Yes, it's for UCM, but even if we don't consider this purpose, the kernel API should return some reasonable information rather than very generic (or empty) strings which are used in the native ALSA utilities for example. So, I think that we should allow to "fix" this info also from the user space rather than to extend the existing API.
Half the point with UCM was supposed to be to rewrite the control names we get from the devices into standard things that are useful for userspace, having to remap things for UCM doesn't sound right.
I guess the question here is how to identify the proper profile for a certain platform and how to get it passed over the whole system. Theoretically, the rename of the card name or mixer name strings could be done in user-space side, too (e.g. mapping in alsa-lib or whatever), so I don't think it mandatory to make them variable via sysfs, if it's meant only for the consistency reason.
There are two things to consider (please, don't concentrate to UCM here):
1) the card identification 2) the user experience
Actually, the generic ASoC drivers are too much generic and they didn't provide a solid information about the hardware.
Two examples:
rpi with PCM5100A DAC (not hifiberry):
# cat /proc/asound/cards 0 [sndrpihifiberry]: RPi-simple - snd_rpi_hifiberry_dac snd_rpi_hifiberry_dac
SOF SoundWire driver:
# cat /proc/asound/cards 0 [sofsoundwire ]: sof-soundwire - sof-soundwire Intel Soundwire SOF # amixer -c 0 info Card hw:0 'sofsoundwire'/'Intel Soundwire SOF' Mixer name : 'Intel Kabylake HDMI' Components : 'HDA:8086280b,80860101,00100000 cfg-spk:4 cfg-amp:2 hs:rt711 spk:rt1308 mic:rt715'
As you can see, the information for both drivers is quite inaccurate and users (including me) may want some flexibility to change those strings to something else. It may resolve some UCM problems, but it's just one small piece of the issue.
Another issue is when the udev driver loader can change some parameters which modifies the driver behaviour and sound device structure for the given card (as discussed in another thread).
When we have a common standard layer for the plug-and-play handling (udev), we should concentrate to allow changing / refining of this information there. Those strings are not used for anything else than the user space. So from my view, there's no reason to create another mechanism to handle the overrides. It should be a safe, fast, flexible and _optional_ solution. The udev can alter the sysfs attributes directly without any hassle with the file modifications or looking for another way to pass / store this information somewhere.
I evaluated the other possibilities like store the overwrites to a file, udev environment (per device) and they are not so easy or create extra dependencies (alsa-lib -> libudev).
Didn't we discuss in the past about the possibility to store the profile name in the card component string?
It's already possible to extract any information from the components string. The current UCM is very flexible, but it does not allow to use a missing information.
The main questions is: Can we make those strings writable or not? What prevents us to not allow that?
Jaroslav
On 4/8/21 10:01 AM, Jaroslav Kysela wrote:
When we have a common standard layer for the plug-and-play handling (udev), we should concentrate to allow changing / refining of this information there. Those strings are not used for anything else than the user space. So from my view, there's no reason to create another mechanism to handle the overrides. It should be a safe, fast, flexible and_optional_ solution. The udev can alter the sysfs attributes directly without any hassle with the file modifications or looking for another way to pass / store this information somewhere.
There's one part where I am lost.
The initial idea of udev what to modify kernel parameters to pick a different path for firmware/topology before probing the PCI driver. At that point there is no card and no sysfs attributes just yet, they will be added at a later point during the probe itself.
So are we talking about a second set of rules that would be applied when the card is created?
Dne 08. 04. 21 v 17:32 Pierre-Louis Bossart napsal(a):
On 4/8/21 10:01 AM, Jaroslav Kysela wrote:
When we have a common standard layer for the plug-and-play handling (udev), we should concentrate to allow changing / refining of this information there. Those strings are not used for anything else than the user space. So from my view, there's no reason to create another mechanism to handle the overrides. It should be a safe, fast, flexible and_optional_ solution. The udev can alter the sysfs attributes directly without any hassle with the file modifications or looking for another way to pass / store this information somewhere.
There's one part where I am lost.
The initial idea of udev what to modify kernel parameters to pick a different path for firmware/topology before probing the PCI driver. At
This may be a problematic point. The kernel cmdline cannot be modified from udev (as far as I know). The module parameters can be set using modprobe's config files or when loaded with sysfs attributes (/sys/module/*/parameters). Eventually, you can call the modprobe command with custom module parameters when the appropriate MODALIAS is probed.
Perhaps, I'm missing something here, too. Some example udev rules may help.
that point there is no card and no sysfs attributes just yet, they will be added at a later point during the probe itself.
So are we talking about a second set of rules that would be applied when the card is created?
Yes, I'm talking about rules which depends on the sound driver specific sysfs attributes (you can match the modified /sys/module/*/parameters here).
Jaroslav
When we have a common standard layer for the plug-and-play handling (udev), we should concentrate to allow changing / refining of this information there. Those strings are not used for anything else than the user space. So from my view, there's no reason to create another mechanism to handle the overrides. It should be a safe, fast, flexible and_optional_ solution. The udev can alter the sysfs attributes directly without any hassle with the file modifications or looking for another way to pass / store this information somewhere.
There's one part where I am lost.
The initial idea of udev what to modify kernel parameters to pick a different path for firmware/topology before probing the PCI driver. At
This may be a problematic point. The kernel cmdline cannot be modified from udev (as far as I know). The module parameters can be set using modprobe's config files or when loaded with sysfs attributes (/sys/module/*/parameters). Eventually, you can call the modprobe command with custom module parameters when the appropriate MODALIAS is probed.
Perhaps, I'm missing something here, too. Some example udev rules may help.
see the example shared by Curtis
SUBSYSTEM=="pci", ATTR{vendor}=="0x8086", ATTR{device}=="0xa0c8", ATTR{class}=="0x040100", ATTRS{[dmi/id]board_name}=="Eldrid", RUN+="/sbin/modprobe snd_sof_pci tplg_path=intel/sof-tplg/pdm1"
Those 'path' parameters would have to be set prior to creating the card, making them writable via sysfs would not work, the firmware and topology are already loaded and changing the paths would have no effect.
that point there is no card and no sysfs attributes just yet, they will be added at a later point during the probe itself.
So are we talking about a second set of rules that would be applied when the card is created?
Yes, I'm talking about rules which depends on the sound driver specific sysfs attributes (you can match the modified /sys/module/*/parameters here).
you lost me with 'match the modified parameters' wording. who matches and who modifies those parameters?
Dne 08. 04. 21 v 20:51 Pierre-Louis Bossart napsal(a):
When we have a common standard layer for the plug-and-play handling (udev), we should concentrate to allow changing / refining of this information there. Those strings are not used for anything else than the user space. So from my view, there's no reason to create another mechanism to handle the overrides. It should be a safe, fast, flexible and_optional_ solution. The udev can alter the sysfs attributes directly without any hassle with the file modifications or looking for another way to pass / store this information somewhere.
There's one part where I am lost.
The initial idea of udev what to modify kernel parameters to pick a different path for firmware/topology before probing the PCI driver. At
This may be a problematic point. The kernel cmdline cannot be modified from udev (as far as I know). The module parameters can be set using modprobe's config files or when loaded with sysfs attributes (/sys/module/*/parameters). Eventually, you can call the modprobe command with custom module parameters when the appropriate MODALIAS is probed.
Perhaps, I'm missing something here, too. Some example udev rules may help.
see the example shared by Curtis
SUBSYSTEM=="pci", ATTR{vendor}=="0x8086", ATTR{device}=="0xa0c8", ATTR{class}=="0x040100", ATTRS{[dmi/id]board_name}=="Eldrid", RUN+="/sbin/modprobe snd_sof_pci tplg_path=intel/sof-tplg/pdm1"
Those 'path' parameters would have to be set prior to creating the card, making them writable via sysfs would not work, the firmware and topology are already loaded and changing the paths would have no effect.
Yes, it's something what I suggested. It's own modprobe call which modifies the module parameters.
that point there is no card and no sysfs attributes just yet, they will be added at a later point during the probe itself.
So are we talking about a second set of rules that would be applied when the card is created?
Yes, I'm talking about rules which depends on the sound driver specific sysfs attributes (you can match the modified /sys/module/*/parameters here).
you lost me with 'match the modified parameters' wording. who matches and who modifies those parameters?
You can probably add something like this to the sound-card.rules:
SUBSYSTEMS=="pci",ATTR{device/driver/module}=="snd_soc_sof_sdw", ATTR{device/driver/module/../snd_sof_pci/parameters/tplg_path}=="intel/sof-tplg/pdm1", DO_SOMETHING_HERE
DO_SOMETHING_HERE may be ATTR{longname}="My Long Name" for example when my change is accepted.
Jaroslav
Yes, I'm talking about rules which depends on the sound driver specific sysfs attributes (you can match the modified /sys/module/*/parameters here).
you lost me with 'match the modified parameters' wording. who matches and who modifies those parameters?
You can probably add something like this to the sound-card.rules:
SUBSYSTEMS=="pci",ATTR{device/driver/module}=="snd_soc_sof_sdw", ATTR{device/driver/module/../snd_sof_pci/parameters/tplg_path}=="intel/sof-tplg/pdm1", DO_SOMETHING_HERE
DO_SOMETHING_HERE may be ATTR{longname}="My Long Name" for example when my change is accepted.
Humm, not sure this can work due to dependencies.
The machine device is neither an ACPI nor PCI one. It's a platform device.
When the PCI device is detected, the PCI core will call the SOF driver probe, which will first try and boot the firmware, and then create the platform device. That results in the probe of the machine driver which creates the card, but that happens *after* booting the firmware.
the DSP firmware is setup starting here:
https://elixir.bootlin.com/linux/latest/source/sound/soc/sof/core.c#L138
and the machine device is created almost last, after registering the ASoC components.
https://elixir.bootlin.com/linux/latest/source/sound/soc/sof/core.c#L234
when the card is created, it's too late to change the firmware path or any firmware-related parameters.
On Thu, Apr 8, 2021 at 12:43 PM Pierre-Louis Bossart < pierre-louis.bossart@linux.intel.com> wrote:
Yes, I'm talking about rules which depends on the sound driver
specific sysfs
attributes (you can match the modified /sys/module/*/parameters here).
you lost me with 'match the modified parameters' wording. who matches and who modifies those parameters?
You can probably add something like this to the sound-card.rules:
SUBSYSTEMS=="pci",ATTR{device/driver/module}=="snd_soc_sof_sdw",
ATTR{device/driver/module/../snd_sof_pci/parameters/tplg_path}=="intel/sof-tplg/pdm1",
DO_SOMETHING_HERE
DO_SOMETHING_HERE may be ATTR{longname}="My Long Name" for example when
my change is accepted.
Humm, not sure this can work due to dependencies.
The machine device is neither an ACPI nor PCI one. It's a platform device.
When the PCI device is detected, the PCI core will call the SOF driver probe, which will first try and boot the firmware, and then create the platform device. That results in the probe of the machine driver which creates the card, but that happens *after* booting the firmware.
the DSP firmware is setup starting here:
https://elixir.bootlin.com/linux/latest/source/sound/soc/sof/core.c#L138
and the machine device is created almost last, after registering the ASoC components.
https://elixir.bootlin.com/linux/latest/source/sound/soc/sof/core.c#L234
when the card is created, it's too late to change the firmware path or any firmware-related parameters.
Thanks for raising this to my attention Pierre, yes we are also experiencing a similar issue since chromium builds will have default sof tplg and fw, but chrome builds will not. Since our UCMs are in a single location right now they will contain commands that will not execute on the defaults and we have no way of indicating to our audio server how to handle this. I am definitely interested in seeing a feature where we can pass indicators to the UCM about what we have available.
Dne 08. 04. 21 v 21:41 Pierre-Louis Bossart napsal(a):
Yes, I'm talking about rules which depends on the sound driver specific sysfs attributes (you can match the modified /sys/module/*/parameters here).
you lost me with 'match the modified parameters' wording. who matches and who modifies those parameters?
You can probably add something like this to the sound-card.rules:
SUBSYSTEMS=="pci",ATTR{device/driver/module}=="snd_soc_sof_sdw", ATTR{device/driver/module/../snd_sof_pci/parameters/tplg_path}=="intel/sof-tplg/pdm1", DO_SOMETHING_HERE
DO_SOMETHING_HERE may be ATTR{longname}="My Long Name" for example when my change is accepted.
Humm, not sure this can work due to dependencies.
The machine device is neither an ACPI nor PCI one. It's a platform device.
When the PCI device is detected, the PCI core will call the SOF driver probe, which will first try and boot the firmware, and then create the platform device. That results in the probe of the machine driver which creates the card, but that happens *after* booting the firmware.
the DSP firmware is setup starting here:
https://elixir.bootlin.com/linux/latest/source/sound/soc/sof/core.c#L138
and the machine device is created almost last, after registering the ASoC components.
https://elixir.bootlin.com/linux/latest/source/sound/soc/sof/core.c#L234
when the card is created, it's too late to change the firmware path or any firmware-related parameters.
I just tried to describe the possible 2nd stage - modify the sysfs attributes when the card with the modified firmware is created (all modules are loaded and initialized). The 1st stage like from Curtis must be retained. It ensures to load the right fw.
SYSTEMS=="pci" checks also parents and card0 links to pci device: card0 -> ../../devices/pci0000:00/0000:00:1f.3/sof_sdw/sound/card0 . You can modify this matching anyway - the goal is to run commands for the specific driver and module parameters when the card is loaded (avoid to change the card attributes for other hw).
I'm not an udev expert, so there may be a bug in my suggestion. I also think that the filter may be specified more elegantly (probably using the DRIVER match or so).
Jaroslav
Dne 08. 04. 21 v 22:01 Jaroslav Kysela napsal(a):
Dne 08. 04. 21 v 21:41 Pierre-Louis Bossart napsal(a):
Yes, I'm talking about rules which depends on the sound driver specific sysfs attributes (you can match the modified /sys/module/*/parameters here).
you lost me with 'match the modified parameters' wording. who matches and who modifies those parameters?
You can probably add something like this to the sound-card.rules:
SUBSYSTEMS=="pci",ATTR{device/driver/module}=="snd_soc_sof_sdw", ATTR{device/driver/module/../snd_sof_pci/parameters/tplg_path}=="intel/sof-tplg/pdm1", DO_SOMETHING_HERE
DO_SOMETHING_HERE may be ATTR{longname}="My Long Name" for example when my change is accepted.
Humm, not sure this can work due to dependencies.
The machine device is neither an ACPI nor PCI one. It's a platform device.
When the PCI device is detected, the PCI core will call the SOF driver probe, which will first try and boot the firmware, and then create the platform device. That results in the probe of the machine driver which creates the card, but that happens *after* booting the firmware.
the DSP firmware is setup starting here:
https://elixir.bootlin.com/linux/latest/source/sound/soc/sof/core.c#L138
and the machine device is created almost last, after registering the ASoC components.
https://elixir.bootlin.com/linux/latest/source/sound/soc/sof/core.c#L234
when the card is created, it's too late to change the firmware path or any firmware-related parameters.
I just tried to describe the possible 2nd stage - modify the sysfs attributes when the card with the modified firmware is created (all modules are loaded and initialized). The 1st stage like from Curtis must be retained. It ensures to load the right fw.
SYSTEMS=="pci" checks also parents and card0 links to pci device: card0 -> ../../devices/pci0000:00/0000:00:1f.3/sof_sdw/sound/card0 . You can modify this matching anyway - the goal is to run commands for the specific driver and module parameters when the card is loaded (avoid to change the card attributes for other hw).
I'm not an udev expert, so there may be a bug in my suggestion. I also think that the filter may be specified more elegantly (probably using the DRIVER match or so).
Another way to use two rules - use internal udev device environment variable (it seems more straight):
PCI detection level:
SUBSYSTEM=="pci", ATTR{vendor}=="0x8086", ATTR{device}=="0xa0c8", ATTR{class}=="0x040100",ATTRS{[dmi/id]board_name}=="Eldrid", ENV{SOUND_SOF_PROFILE}="MyProfile",...module stuff...
Card instance level (sound-card.rules):
SUBSYSTEMS=="pci",ENV{SOUND_SOF_PROFILE}=="MyProfile",...attr setup...
Jaroslav
On Thu, 08 Apr 2021 20:51:41 +0200, Pierre-Louis Bossart wrote:
When we have a common standard layer for the plug-and-play handling (udev), we should concentrate to allow changing / refining of this information there. Those strings are not used for anything else than the user space. So from my view, there's no reason to create another mechanism to handle the overrides. It should be a safe, fast, flexible and_optional_ solution. The udev can alter the sysfs attributes directly without any hassle with the file modifications or looking for another way to pass / store this information somewhere.
There's one part where I am lost.
The initial idea of udev what to modify kernel parameters to pick a different path for firmware/topology before probing the PCI driver. At
This may be a problematic point. The kernel cmdline cannot be modified from udev (as far as I know). The module parameters can be set using modprobe's config files or when loaded with sysfs attributes (/sys/module/*/parameters). Eventually, you can call the modprobe command with custom module parameters when the appropriate MODALIAS is probed.
Perhaps, I'm missing something here, too. Some example udev rules may help.
see the example shared by Curtis
SUBSYSTEM=="pci", ATTR{vendor}=="0x8086", ATTR{device}=="0xa0c8", ATTR{class}=="0x040100", ATTRS{[dmi/id]board_name}=="Eldrid", RUN+="/sbin/modprobe snd_sof_pci tplg_path=intel/sof-tplg/pdm1"
Those 'path' parameters would have to be set prior to creating the card, making them writable via sysfs would not work, the firmware and topology are already loaded and changing the paths would have no effect.
Couldn't the driver probe the firmware files with some device-specific string suffix at first? e.g. the driver can issue request_firmware() with $base_file-$dmi_board at first, then falls back to the generic $base_file. A similar method was already used in Broadcom WiFi driver.
Also, the driver may do request_firmware() with a fixed path for the custom firmware at first (e.g. "intel/sof-tplg-custom"); then a system integrator may set up a specific configuration even that doesn't match with DMI or whatever identifier.
Takashi
Dne 09. 04. 21 v 9:39 Takashi Iwai napsal(a):
On Thu, 08 Apr 2021 20:51:41 +0200, Pierre-Louis Bossart wrote:
When we have a common standard layer for the plug-and-play handling (udev), we should concentrate to allow changing / refining of this information there. Those strings are not used for anything else than the user space. So from my view, there's no reason to create another mechanism to handle the overrides. It should be a safe, fast, flexible and_optional_ solution. The udev can alter the sysfs attributes directly without any hassle with the file modifications or looking for another way to pass / store this information somewhere.
There's one part where I am lost.
The initial idea of udev what to modify kernel parameters to pick a different path for firmware/topology before probing the PCI driver. At
This may be a problematic point. The kernel cmdline cannot be modified from udev (as far as I know). The module parameters can be set using modprobe's config files or when loaded with sysfs attributes (/sys/module/*/parameters). Eventually, you can call the modprobe command with custom module parameters when the appropriate MODALIAS is probed.
Perhaps, I'm missing something here, too. Some example udev rules may help.
see the example shared by Curtis
SUBSYSTEM=="pci", ATTR{vendor}=="0x8086", ATTR{device}=="0xa0c8", ATTR{class}=="0x040100", ATTRS{[dmi/id]board_name}=="Eldrid", RUN+="/sbin/modprobe snd_sof_pci tplg_path=intel/sof-tplg/pdm1"
Those 'path' parameters would have to be set prior to creating the card, making them writable via sysfs would not work, the firmware and topology are already loaded and changing the paths would have no effect.
Couldn't the driver probe the firmware files with some device-specific string suffix at first? e.g. the driver can issue request_firmware() with $base_file-$dmi_board at first, then falls back to the generic $base_file. A similar method was already used in Broadcom WiFi driver.
Also, the driver may do request_firmware() with a fixed path for the custom firmware at first (e.g. "intel/sof-tplg-custom"); then a system integrator may set up a specific configuration even that doesn't match with DMI or whatever identifier.
And when we have two firmware files which differs just by functionality requested by user? Although your method will work, I won't close the possibility to configure everything in udev rather using a hard coded fw load scheme only.
Jaroslav
On Fri, 09 Apr 2021 10:34:03 +0200, Jaroslav Kysela wrote:
Dne 09. 04. 21 v 9:39 Takashi Iwai napsal(a):
On Thu, 08 Apr 2021 20:51:41 +0200, Pierre-Louis Bossart wrote:
When we have a common standard layer for the plug-and-play handling (udev), we should concentrate to allow changing / refining of this information there. Those strings are not used for anything else than the user space. So from my view, there's no reason to create another mechanism to handle the overrides. It should be a safe, fast, flexible and_optional_ solution. The udev can alter the sysfs attributes directly without any hassle with the file modifications or looking for another way to pass / store this information somewhere.
There's one part where I am lost.
The initial idea of udev what to modify kernel parameters to pick a different path for firmware/topology before probing the PCI driver. At
This may be a problematic point. The kernel cmdline cannot be modified from udev (as far as I know). The module parameters can be set using modprobe's config files or when loaded with sysfs attributes (/sys/module/*/parameters). Eventually, you can call the modprobe command with custom module parameters when the appropriate MODALIAS is probed.
Perhaps, I'm missing something here, too. Some example udev rules may help.
see the example shared by Curtis
SUBSYSTEM=="pci", ATTR{vendor}=="0x8086", ATTR{device}=="0xa0c8", ATTR{class}=="0x040100", ATTRS{[dmi/id]board_name}=="Eldrid", RUN+="/sbin/modprobe snd_sof_pci tplg_path=intel/sof-tplg/pdm1"
Those 'path' parameters would have to be set prior to creating the card, making them writable via sysfs would not work, the firmware and topology are already loaded and changing the paths would have no effect.
Couldn't the driver probe the firmware files with some device-specific string suffix at first? e.g. the driver can issue request_firmware() with $base_file-$dmi_board at first, then falls back to the generic $base_file. A similar method was already used in Broadcom WiFi driver.
Also, the driver may do request_firmware() with a fixed path for the custom firmware at first (e.g. "intel/sof-tplg-custom"); then a system integrator may set up a specific configuration even that doesn't match with DMI or whatever identifier.
And when we have two firmware files which differs just by functionality requested by user? Although your method will work, I won't close the possibility to configure everything in udev rather using a hard coded fw load scheme only.
That can be achieved via a module option as Curtis's example, no?
That comes to a question at which stage you want to re-configure the stuff. I suppose that, if the system has been set up, then the firmware cannot be changed on the fly; usually you have to rewind the stuff and re-initialize from the beginning of the driver binding. This corresponds to the device unbind and re-bind procedure (equivalent with hot-unplug and hot-replug). Then you can put your change (e.g. set up module option or a dedicated firmware file) between the unbind and rebind step.
If we make the driver setup as staged (i.e. waiting for the setup via sysfs before moving to the actual firmware loading), a concern with that is the fragility and complexity. If the sysfs set up via sysfs missed or went wrong, all goes south.
Takashi
On 4/9/2021 10:34 AM, Jaroslav Kysela wrote:
Dne 09. 04. 21 v 9:39 Takashi Iwai napsal(a):
On Thu, 08 Apr 2021 20:51:41 +0200, Pierre-Louis Bossart wrote:
When we have a common standard layer for the plug-and-play handling (udev), we should concentrate to allow changing / refining of this information there. Those strings are not used for anything else than the user space. So from my view, there's no reason to create another mechanism to handle the overrides. It should be a safe, fast, flexible and_optional_ solution. The udev can alter the sysfs attributes directly without any hassle with the file modifications or looking for another way to pass / store this information somewhere.
There's one part where I am lost.
The initial idea of udev what to modify kernel parameters to pick a different path for firmware/topology before probing the PCI driver. At
This may be a problematic point. The kernel cmdline cannot be modified from udev (as far as I know). The module parameters can be set using modprobe's config files or when loaded with sysfs attributes (/sys/module/*/parameters). Eventually, you can call the modprobe command with custom module parameters when the appropriate MODALIAS is probed.
Perhaps, I'm missing something here, too. Some example udev rules may help.
see the example shared by Curtis
SUBSYSTEM=="pci", ATTR{vendor}=="0x8086", ATTR{device}=="0xa0c8", ATTR{class}=="0x040100", ATTRS{[dmi/id]board_name}=="Eldrid", RUN+="/sbin/modprobe snd_sof_pci tplg_path=intel/sof-tplg/pdm1"
Those 'path' parameters would have to be set prior to creating the card, making them writable via sysfs would not work, the firmware and topology are already loaded and changing the paths would have no effect.
Couldn't the driver probe the firmware files with some device-specific string suffix at first? e.g. the driver can issue request_firmware() with $base_file-$dmi_board at first, then falls back to the generic $base_file. A similar method was already used in Broadcom WiFi driver.
Also, the driver may do request_firmware() with a fixed path for the custom firmware at first (e.g. "intel/sof-tplg-custom"); then a system integrator may set up a specific configuration even that doesn't match with DMI or whatever identifier.
And when we have two firmware files which differs just by functionality requested by user? Although your method will work, I won't close the possibility to configure everything in udev rather using a hard coded fw load scheme only.
Jaroslav
I've slept on it and now I think I see what you are trying to do.
1. Load FW dependent on platform/user settings 2. Load appropriate topology for FW 3. Have UCM for the FEs and controls exposed by driver
As for 1. I would say that FW should be loaded from one location if there is some platform that requires special FW just add quirks, like it is done with every other driver, and if someone wants to build their own special FW, they just replace it. We can't possibly support hundreds of possible FW modifications if users want them by putting them in separate files. Alternatively allow override via kernel parameters. Overriding FW files via udev would only make sense to me if it was possible to upload new FW at runtime.
I would say that same applies for 2.
This leaves number 3. which would require kernel exposing some kind of information about loaded topology, so user space can use proper UCM. In topology manifest there are few reserved fields (https://elixir.bootlin.com/linux/latest/source/include/uapi/sound/asoc.h#L38...), so we can add some information there which should be unique per topology and then expose it in userspace on topology load, it can be the name of UCM file topology wants to be loaded for example.
For example do something along those lines:
struct snd_soc_tplg_manifest { __le32 size; /* in bytes of this structure */ __le32 control_elems; /* number of control elements */ __le32 widget_elems; /* number of widget elements */ __le32 graph_elems; /* number of graph elements */ __le32 pcm_elems; /* number of PCM elements */ __le32 dai_link_elems; /* number of DAI link elements */ __le32 dai_elems; /* number of physical DAI elements */ __le32 ucm_files; /* UCM files to use with topology */ __le32 reserved[19]; /* reserved for new ABI element types */ struct snd_soc_tplg_private priv; } __attribute__((packed));
struct snd_soc_tplg_ucm_files { struct snd_soc_tplg_ctl_hdr hdr; __le32 size; /* size of struct in bytes */ __le32 count; /* UCM entries */ char ucms[SNDRV_CTL_ELEM_ID_NAME_MAXLEN][]; }
And then expose it somewhere under sysfs after parsing topology.
Dne 09. 04. 21 v 11:09 Amadeusz Sławiński napsal(a):
On 4/9/2021 10:34 AM, Jaroslav Kysela wrote:
Dne 09. 04. 21 v 9:39 Takashi Iwai napsal(a):
On Thu, 08 Apr 2021 20:51:41 +0200, Pierre-Louis Bossart wrote:
> When we have a common standard layer for the plug-and-play handling (udev), we > should concentrate to allow changing / refining of this information there. > Those strings are not used for anything else than the user space. So from my > view, there's no reason to create another mechanism to handle the overrides. > It should be a safe, fast, flexible and_optional_ solution. The udev can > alter the sysfs attributes directly without any hassle with the file > modifications or looking for another way to pass / store this information > somewhere.
There's one part where I am lost.
The initial idea of udev what to modify kernel parameters to pick a different path for firmware/topology before probing the PCI driver. At
This may be a problematic point. The kernel cmdline cannot be modified from udev (as far as I know). The module parameters can be set using modprobe's config files or when loaded with sysfs attributes (/sys/module/*/parameters). Eventually, you can call the modprobe command with custom module parameters when the appropriate MODALIAS is probed.
Perhaps, I'm missing something here, too. Some example udev rules may help.
see the example shared by Curtis
SUBSYSTEM=="pci", ATTR{vendor}=="0x8086", ATTR{device}=="0xa0c8", ATTR{class}=="0x040100", ATTRS{[dmi/id]board_name}=="Eldrid", RUN+="/sbin/modprobe snd_sof_pci tplg_path=intel/sof-tplg/pdm1"
Those 'path' parameters would have to be set prior to creating the card, making them writable via sysfs would not work, the firmware and topology are already loaded and changing the paths would have no effect.
Couldn't the driver probe the firmware files with some device-specific string suffix at first? e.g. the driver can issue request_firmware() with $base_file-$dmi_board at first, then falls back to the generic $base_file. A similar method was already used in Broadcom WiFi driver.
Also, the driver may do request_firmware() with a fixed path for the custom firmware at first (e.g. "intel/sof-tplg-custom"); then a system integrator may set up a specific configuration even that doesn't match with DMI or whatever identifier.
And when we have two firmware files which differs just by functionality requested by user? Although your method will work, I won't close the possibility to configure everything in udev rather using a hard coded fw load scheme only.
JaroslavI've slept on it and now I think I see what you are trying to do.
- Load FW dependent on platform/user settings
- Load appropriate topology for FW
- Have UCM for the FEs and controls exposed by driver
As for 1. I would say that FW should be loaded from one location if there is some platform that requires special FW just add quirks, like it is done with every other driver, and if someone wants to build their own special FW, they just replace it. We can't possibly support hundreds of possible FW modifications if users want them by putting them in separate files. Alternatively allow override via kernel parameters. Overriding FW files via udev would only make sense to me if it was possible to upload new FW at runtime.
I would say that same applies for 2.
This leaves number 3. which would require kernel exposing some kind of information about loaded topology, so user space can use proper UCM. In topology manifest there are few reserved fields (https://elixir.bootlin.com/linux/latest/source/include/uapi/sound/asoc.h#L38...), so we can add some information there which should be unique per topology and then expose it in userspace on topology load, it can be the name of UCM file topology wants to be loaded for example.
For example do something along those lines:
struct snd_soc_tplg_manifest { __le32 size; /* in bytes of this structure */ __le32 control_elems; /* number of control elements */ __le32 widget_elems; /* number of widget elements */ __le32 graph_elems; /* number of graph elements */ __le32 pcm_elems; /* number of PCM elements */ __le32 dai_link_elems; /* number of DAI link elements */ __le32 dai_elems; /* number of physical DAI elements */ __le32 ucm_files; /* UCM files to use with topology */ __le32 reserved[19]; /* reserved for new ABI element types */ struct snd_soc_tplg_private priv; } __attribute__((packed));
struct snd_soc_tplg_ucm_files { struct snd_soc_tplg_ctl_hdr hdr; __le32 size; /* size of struct in bytes */ __le32 count; /* UCM entries */ char ucms[SNDRV_CTL_ELEM_ID_NAME_MAXLEN][]; }
And then expose it somewhere under sysfs after parsing topology.
If the driver exports some extra information via sysfs, it can be already used in the current UCM configs. I welcome any of this activity. It may ease things. But it's a complement to my proposal to allow modify the sound card identification in udev (not only for UCM). I believe that we can support multiple ways and it's up to the maintainer / user of the specific distro to select any.
Jaroslav
Couldn't the driver probe the firmware files with some device-specific string suffix at first? e.g. the driver can issue request_firmware() with $base_file-$dmi_board at first, then falls back to the generic $base_file. A similar method was already used in Broadcom WiFi driver.
Also, the driver may do request_firmware() with a fixed path for the custom firmware at first (e.g. "intel/sof-tplg-custom"); then a system integrator may set up a specific configuration even that doesn't match with DMI or whatever identifier.
And when we have two firmware files which differs just by functionality requested by user? Although your method will work, I won't close the possibility to configure everything in udev rather using a hard coded fw load scheme only.
Jaroslav
I've slept on it and now I think I see what you are trying to do.
- Load FW dependent on platform/user settings
- Load appropriate topology for FW
- Have UCM for the FEs and controls exposed by driver
As for 1. I would say that FW should be loaded from one location if there is some platform that requires special FW just add quirks, like it is done with every other driver, and if someone wants to build their own special FW, they just replace it. We can't possibly support hundreds of possible FW modifications if users want them by putting them in separate files. Alternatively allow override via kernel parameters. Overriding FW files via udev would only make sense to me if it was possible to upload new FW at runtime.
No, replacing firmware files is not viable.
Let me give you a practical example. In the course of SOF development, we routinely copy new test firmware+topology in the location of distribution-managed files. It's classic that when there is a distribution update in the background to install the latest SOF release, our test files are overwritten and it's not usual for developers to lose time trying to figure out why the functionality changed. We do need to have multiple paths and NEVER override what is provided by the distributions. it's the moral equivalent of /usr/bin v. /usr/local/bin.
Likewise, if an OEM has a custom version of firmware+topology, they may want to place it in their own packages that install *separately* from the default distribution. The udev rules provide a means to select the custom firmware, and if no rules are provided the default firmware with no customizations is used.
We are not talking about hundreds of configurations, but typically one directory per OEM. What they do internally is their problem, it's not for us to debate. The key point is that the custom files shall not interfere with the distribution baseline firmware, they have to be handled as separate packages that have their own release cycle and are possibly installed only in OEM custom images.
I would say that same applies for 2.
This leaves number 3. which would require kernel exposing some kind of information about loaded topology, so user space can use proper UCM. In topology manifest there are few reserved fields (https://elixir.bootlin.com/linux/latest/source/include/uapi/sound/asoc.h#L38...), so we can add some information there which should be unique per topology and then expose it in userspace on topology load, it can be the name of UCM file topology wants to be loaded for example.
For example do something along those lines:
struct snd_soc_tplg_manifest { __le32 size; /* in bytes of this structure */ __le32 control_elems; /* number of control elements */ __le32 widget_elems; /* number of widget elements */ __le32 graph_elems; /* number of graph elements */ __le32 pcm_elems; /* number of PCM elements */ __le32 dai_link_elems; /* number of DAI link elements */ __le32 dai_elems; /* number of physical DAI elements */ __le32 ucm_files; /* UCM files to use with topology */ __le32 reserved[19]; /* reserved for new ABI element types */ struct snd_soc_tplg_private priv; } __attribute__((packed));
struct snd_soc_tplg_ucm_files { struct snd_soc_tplg_ctl_hdr hdr; __le32 size; /* size of struct in bytes */ __le32 count; /* UCM entries */ char ucms[SNDRV_CTL_ELEM_ID_NAME_MAXLEN][]; }
And then expose it somewhere under sysfs after parsing topology.
Yes I had a similar idea that we could expose in sysfs information taken from parsing the firmware (e.g. list of module UUIDs) and topology.
But it's not enough. The point is that you may use the same topology+firmware for different platforms, only the tuning varies. I think that's at that level that we should allow the OEMs to set a rule defining what the platform is and what tuning should be applied.
On Fri, 09 Apr 2021 17:55:38 +0200, Pierre-Louis Bossart wrote:
Couldn't the driver probe the firmware files with some device-specific string suffix at first? e.g. the driver can issue request_firmware() with $base_file-$dmi_board at first, then falls back to the generic $base_file. A similar method was already used in Broadcom WiFi driver.
Also, the driver may do request_firmware() with a fixed path for the custom firmware at first (e.g. "intel/sof-tplg-custom"); then a system integrator may set up a specific configuration even that doesn't match with DMI or whatever identifier.
And when we have two firmware files which differs just by functionality requested by user? Although your method will work, I won't close the possibility to configure everything in udev rather using a hard coded fw load scheme only.
Jaroslav
I've slept on it and now I think I see what you are trying to do.
- Load FW dependent on platform/user settings
- Load appropriate topology for FW
- Have UCM for the FEs and controls exposed by driver
As for 1. I would say that FW should be loaded from one location if there is some platform that requires special FW just add quirks, like it is done with every other driver, and if someone wants to build their own special FW, they just replace it. We can't possibly support hundreds of possible FW modifications if users want them by putting them in separate files. Alternatively allow override via kernel parameters. Overriding FW files via udev would only make sense to me if it was possible to upload new FW at runtime.
No, replacing firmware files is not viable.
Let me give you a practical example. In the course of SOF development, we routinely copy new test firmware+topology in the location of distribution-managed files. It's classic that when there is a distribution update in the background to install the latest SOF release, our test files are overwritten and it's not usual for developers to lose time trying to figure out why the functionality changed. We do need to have multiple paths and NEVER override what is provided by the distributions. it's the moral equivalent of /usr/bin v. /usr/local/bin.
Use /lib/firmware/updates/*. That precedes over the standard path /lib/firmware/*. (Also /lib/firmware/updates/$VERSION has even higher priority.)
Takashi
No, replacing firmware files is not viable.
Let me give you a practical example. In the course of SOF development, we routinely copy new test firmware+topology in the location of distribution-managed files. It's classic that when there is a distribution update in the background to install the latest SOF release, our test files are overwritten and it's not usual for developers to lose time trying to figure out why the functionality changed. We do need to have multiple paths and NEVER override what is provided by the distributions. it's the moral equivalent of /usr/bin v. /usr/local/bin.
Use /lib/firmware/updates/*. That precedes over the standard path /lib/firmware/*. (Also /lib/firmware/updates/$VERSION has even higher priority.)
thanks for the feedback Takashi, I had no idea this even existed :-)
The documentation is here: https://www.kernel.org/doc/html/latest/driver-api/firmware/fw_search_path.ht...
I guess that removes the need for udev rules to select the firmware+topology in simple cases, e.g. if you have only one custom version or an image overlay.
On Fri, Apr 09, 2021 at 10:55:38AM -0500, Pierre-Louis Bossart wrote:
For example do something along those lines:
struct snd_soc_tplg_manifest { __le32 size; /* in bytes of this structure */ __le32 control_elems; /* number of control elements */
...
struct snd_soc_tplg_ucm_files { struct snd_soc_tplg_ctl_hdr hdr; __le32 size; /* size of struct in bytes */ __le32 count; /* UCM entries */ char ucms[SNDRV_CTL_ELEM_ID_NAME_MAXLEN][]; }
And then expose it somewhere under sysfs after parsing topology.
Yes I had a similar idea that we could expose in sysfs information taken from parsing the firmware (e.g. list of module UUIDs) and topology.
But it's not enough. The point is that you may use the same topology+firmware for different platforms, only the tuning varies. I think that's at that level that we should allow the OEMs to set a rule defining what the platform is and what tuning should be applied.
So the issue there is trying to put a list of UCM files in the kernel or topology and exposing that (which I agree doesn't seem like something that should come from the kernel), but exposing the information about what's in the loaded topology file seems reasonable. It's just more information that userspace has available to key off when deciding what it wants to do isn't it?
On Thu, Apr 08, 2021 at 05:01:14PM +0200, Jaroslav Kysela wrote:
There are two things to consider (please, don't concentrate to UCM here):
- the card identification
- the user experience
Actually, the generic ASoC drivers are too much generic and they didn't provide a solid information about the hardware.
So if the information provided through the driver is too generic then we should ideally be fixing those drivers/systems to do something sensible. For the DT systems the generic cards have properties that let the system just specify names directly so it will just be a case of setting them properly and it should just be the x86 systems that are a problem. ACPI is a bit of a lost cause here, most of the systems aren't interested in supporting Linux in the first place and the idioms there aren't great, but for DT it's reasonably tractable to push back on people if there's issues and it's much more scalable to do that than telling individual users to do that.
As you can see, the information for both drivers is quite inaccurate and users (including me) may want some flexibility to change those strings to something else. It may resolve some UCM problems, but it's just one small piece of the issue.
It feels like if we're conflating end user configuration and what we're reporting to userspace for userspace to key off we're going to end up causing confusion and/or tying ourselves in knots when people update both places, for example if you're trying to figure out which configuration was used but the values used to select the configuration that was used have changed.
Another issue is when the udev driver loader can change some parameters which modifies the driver behaviour and sound device structure for the given card (as discussed in another thread).
Not sure I've seen that thread, I've certainly not seen anything on github.
When we have a common standard layer for the plug-and-play handling (udev), we should concentrate to allow changing / refining of this information there. Those strings are not used for anything else than the user space. So from my view, there's no reason to create another mechanism to handle the overrides. It should be a safe, fast, flexible and _optional_ solution. The udev can alter the sysfs attributes directly without any hassle with the file modifications or looking for another way to pass / store this information somewhere.
We could add a new sysfs file for user overrides, or alternatively have a new non-overridable file which contains whatever the kernel would set by default so it's always available in case things start to get confused (ie, you can always look at the original value even if it's been overwritten later)?
Didn't we discuss in the past about the possibility to store the profile name in the card component string?
It's already possible to extract any information from the components string. The current UCM is very flexible, but it does not allow to use a missing information.
The main questions is: Can we make those strings writable or not? What prevents us to not allow that?
Like I say I'm nervous about potential confusion if we allow people to change things that were already set and used by the kernel, plus the general desire to encourage better quality of implementation from firmware where we can.
On 4/8/21 11:22 AM, Mark Brown wrote:
Actually, the generic ASoC drivers are too much generic and they didn't provide a solid information about the hardware.
So if the information provided through the driver is too generic then we should ideally be fixing those drivers/systems to do something sensible. For the DT systems the generic cards have properties that let the system just specify names directly so it will just be a case of setting them properly and it should just be the x86 systems that are a problem. ACPI is a bit of a lost cause here, most of the systems aren't interested in supporting Linux in the first place and the idioms there aren't great, but for DT it's reasonably tractable to push back on people if there's issues and it's much more scalable to do that than telling individual users to do that.
Even in the DT case, you may be able to set a specific path for DSP firmware and topology but would you really have enough information to describe what the DSP firmware and topology actually do? That information is part of the DSP firmware manifest and topology.
In addition, the firmware/topology are typically located on the file system, it'd be a hassle to have to edit DT properties every time you have a new distribution update, wouldn't it?
On Thu, Apr 08, 2021 at 11:50:08AM -0500, Pierre-Louis Bossart wrote:
Even in the DT case, you may be able to set a specific path for DSP firmware and topology but would you really have enough information to describe what the DSP firmware and topology actually do? That information is part of the DSP firmware manifest and topology.
I'd have hoped that for information on firmware we'd be keying off descriptive information included in the firmware/topology itself, ideally the kernel would be able to do that itself.
In addition, the firmware/topology are typically located on the file system, it'd be a hassle to have to edit DT properties every time you have a new distribution update, wouldn't it?
You definitely shouldn't be doing that, but I'd also not expect us to have to update anything that isn't the firmware itself.
Dne 08. 04. 21 v 18:22 Mark Brown napsal(a):
On Thu, Apr 08, 2021 at 05:01:14PM +0200, Jaroslav Kysela wrote:
There are two things to consider (please, don't concentrate to UCM here):
- the card identification
- the user experience
Actually, the generic ASoC drivers are too much generic and they didn't provide a solid information about the hardware.
So if the information provided through the driver is too generic then we should ideally be fixing those drivers/systems to do something sensible. For the DT systems the generic cards have properties that let the system just specify names directly so it will just be a case of setting them properly and it should just be the x86 systems that are a problem. ACPI is a bit of a lost cause here, most of the systems aren't interested in supporting Linux in the first place and the idioms there aren't great, but for DT it's reasonably tractable to push back on people if there's issues and it's much more scalable to do that than telling individual users to do that.
DT works only partially. Also, you need the DT compiler to change something, it's easier to overwrite things in the booted system. The user experience is lower with DT.
And as Pierre-louis noted, it may be possible to modify the firmware setup in udev.
As you can see, the information for both drivers is quite inaccurate and users (including me) may want some flexibility to change those strings to something else. It may resolve some UCM problems, but it's just one small piece of the issue.
It feels like if we're conflating end user configuration and what we're reporting to userspace for userspace to key off we're going to end up causing confusion and/or tying ourselves in knots when people update both places, for example if you're trying to figure out which configuration was used but the values used to select the configuration that was used have changed.
Another issue is when the udev driver loader can change some parameters which modifies the driver behaviour and sound device structure for the given card (as discussed in another thread).
Not sure I've seen that thread, I've certainly not seen anything on github.
I meant the e-mail replies to this thread started by Pierre-louis about OEM firmwares for the SOF driver. Sorry for the confusion.
When we have a common standard layer for the plug-and-play handling (udev), we should concentrate to allow changing / refining of this information there. Those strings are not used for anything else than the user space. So from my view, there's no reason to create another mechanism to handle the overrides. It should be a safe, fast, flexible and _optional_ solution. The udev can alter the sysfs attributes directly without any hassle with the file modifications or looking for another way to pass / store this information somewhere.
We could add a new sysfs file for user overrides, or alternatively have a new non-overridable file which contains whatever the kernel would set by default so it's always available in case things start to get confused (ie, you can always look at the original value even if it's been overwritten later)?
It's an interesting idea. Preserving the original strings may be wanted.
Jaroslav
On Thu, Apr 08, 2021 at 07:20:39PM +0200, Jaroslav Kysela wrote:
Dne 08. 04. 21 v 18:22 Mark Brown napsal(a):
So if the information provided through the driver is too generic then we should ideally be fixing those drivers/systems to do something sensible. For the DT systems the generic cards have properties that let the system just specify names directly so it will just be a case of setting them properly and it should just be the x86 systems that are a problem. ACPI is a bit of a lost cause here, most of the systems aren't interested in supporting Linux in the first place and the idioms there aren't great, but for DT it's reasonably tractable to push back on people if there's issues and it's much more scalable to do that than telling individual users to do that.
DT works only partially. Also, you need the DT compiler to change something, it's easier to overwrite things in the booted system. The user experience is lower with DT.
TBH I think the ship sailed on user experience no matter what we do here :) In any case no disagreement that it's useful to have some way to do this at runtime for systems where it's not possible to update the firmware for whatever reason.
Dne 08. 04. 21 v 12:38 Takashi Sakamoto napsal(a):
static DEVICE_ATTR(number, 0444, card_number_show_attr, NULL);
Use DEVICE_ATTR_RO() instead.
My proposal is consistent with the existing code. The cleanups should go on top.
Jaroslav
Dne 08. 04. 21 v 11:43 Jaroslav Kysela napsal(a):
There are several strings which are describing the card. As time goes, we have new universal drivers which probe components in a way, which is disassociated from the card structure (ASoC). Also, some drivers may require to select another firmware depending on the specific platform using udev. The new firmware may change the sound card behaviour.
This patch allows flexible modifications of the card description from the user space to handle the specific boot / plug-in settings.
The original discussion went to different forks, but I'd like summarize some points:
1) those runtime changes may be intrusive 2) even if we allow to change those strings, we should preserve the original 3) generate change events
I hope that we all see the flexibility to change those strings via udev. The nice thing is that we can write to those sysfs attributes before the _control device file_ is created by udev (thus we are pretty sure, that no applications are using this information (if I omit the additional proc and sysfs resources).
It seems to me, that we may just add a policy how to handle those card identification changes. This policy may be controlled using a module parameter (runtime), kernel configuration option (compiled default). The policies will be 'allow changes' and 'disallow changes' so distributions or users may be forced to explicitly enable this behaviour. When all sysfs changes are finished, the udev rules may just set "do not allow other changes" via an additional sysfs sound card attribute (write once) to prevent user errors. Or we may apply the 'write once' mechanism for all strings separately. We should disallow to change the card identification after this point.
Regarding the original value preservation - the udev can save the original strings to it's device environment variables for the later usage. We may handle this in the kernel. but I see the code reduction with this idea and the use will be rate (alsa-info script may be extended to extract this info from the udev database).
The change events are not necessary with this policy. The card identification is supposed to be changed only via udev before any application is active.
Jaroslav
participants (7)
-
Amadeusz Sławiński -
Curtis Malainey -
Jaroslav Kysela -
Mark Brown -
Pierre-Louis Bossart -
Takashi Iwai -
Takashi Sakamoto