Hi all,
I am fighting actually with the ASoC driver names. The current situation, where each hardware variant (card name) is mapped to the driver name is not very practical (and not correct). The driver name should describe the common part (usually the controller - like 'HDA-Intel' or bus 'USB-Audio').
I am talking about the situation, where the ASoC card name is set via the device tree (snd_soc_of_parse_card_name) like in [1], but the change may be considered for other drivers like Intel SST:
Card driver name: Lenovo-YOGA-C63 Card name: Lenovo-YOGA-C630-13Q50 Card long name: LENOVO-81JL-LenovoYOGAC630_13Q50-LNVNB161216
[1] https://github.com/alsa-project/alsa-tests/pull/3/commits/1b5a552cff06c6039f...
My question is, can we change / add the more apropriate driver names conditionally with a new kernel CONFIG option? Will you accept this change?
The goal is to group the related UCM2 configurations and do the required split inside the UCM2 top-level configuration file based on card components string or other card identificators (related to the driver).
Thanks, Jaroslav
On Wed, 22 Apr 2020 19:04:36 +0200, Jaroslav Kysela wrote:
Hi all,
I am fighting actually with the ASoC driver names. The current situation, where each hardware variant (card name) is mapped to the driver name is not very practical (and not correct). The driver name should describe the common part (usually the controller - like 'HDA-Intel' or bus 'USB-Audio').
I am talking about the situation, where the ASoC card name is set via the device tree (snd_soc_of_parse_card_name) like in [1], but the change may be considered for other drivers like Intel SST:
Card driver name: Lenovo-YOGA-C63 Card name: Lenovo-YOGA-C630-13Q50 Card long name: LENOVO-81JL-LenovoYOGAC630_13Q50-LNVNB161216
[1] https://github.com/alsa-project/alsa-tests/pull/3/commits/1b5a552cff06c6039f...
My question is, can we change / add the more apropriate driver names conditionally with a new kernel CONFIG option? Will you accept this change?
It's a last resort, but it's more or less acceptable, IMO.
The goal is to group the related UCM2 configurations and do the required split inside the UCM2 top-level configuration file based on card components string or other card identificators (related to the driver).
Wasn't an idea to use the component string feasible? e.g. if a driver sets a certain component string, it's preferred over the standard driver name / long name mapping.
thanks,
Takashi
On Wed, Apr 22, 2020 at 07:04:36PM +0200, Jaroslav Kysela wrote:
I am fighting actually with the ASoC driver names. The current situation, where each hardware variant (card name) is mapped to the driver name is not very practical (and not correct). The driver name should describe the common part (usually the controller - like 'HDA-Intel' or bus 'USB-Audio').
With ASoC systems there is no clear controller - you've got a bunch of different components, usually connected by separate buses, and it's not super obvious what if anything should be the singular name that gets picked for some grouping of devices. The whole point of the subsystem is to glue a bunch of independent devices together, we've generally picked that glue as the driver name.
I am talking about the situation, where the ASoC card name is set via the device tree (snd_soc_of_parse_card_name) like in [1], but the change may be considered for other drivers like Intel SST:
Card driver name: Lenovo-YOGA-C63 Card name: Lenovo-YOGA-C630-13Q50 Card long name: LENOVO-81JL-LenovoYOGAC630_13Q50-LNVNB161216
My question is, can we change / add the more apropriate driver names conditionally with a new kernel CONFIG option? Will you accept this change?
Without knowing what you're actually proposing it's hard to know, and there is the risk of userspace breakage here when you change things people are relying on.
The goal is to group the related UCM2 configurations and do the required split inside the UCM2 top-level configuration file based on card components string or other card identificators (related to the driver).
This sounds like you either want some enumeration of the card components or perhaps you're looking for some for some indication of the reference design that an individual board is based off so you can have a generic configuration for that reference design and then override bits of it?
Dne 23. 04. 20 v 13:04 Mark Brown napsal(a):
On Wed, Apr 22, 2020 at 07:04:36PM +0200, Jaroslav Kysela wrote:
I am fighting actually with the ASoC driver names. The current situation, where each hardware variant (card name) is mapped to the driver name is not very practical (and not correct). The driver name should describe the common part (usually the controller - like 'HDA-Intel' or bus 'USB-Audio').
With ASoC systems there is no clear controller - you've got a bunch of different components, usually connected by separate buses, and it's not super obvious what if anything should be the singular name that gets picked for some grouping of devices. The whole point of the subsystem is to glue a bunch of independent devices together, we've generally picked that glue as the driver name.
I refer mostly the top-level code which creates and registers the ASoC card structure. So it seems the platform name for many ASoC drivers should be there.
I am talking about the situation, where the ASoC card name is set via the device tree (snd_soc_of_parse_card_name) like in [1], but the change may be considered for other drivers like Intel SST:
Card driver name: Lenovo-YOGA-C63 Card name: Lenovo-YOGA-C630-13Q50 Card long name: LENOVO-81JL-LenovoYOGAC630_13Q50-LNVNB161216
My question is, can we change / add the more apropriate driver names conditionally with a new kernel CONFIG option? Will you accept this change?
Without knowing what you're actually proposing it's hard to know, and there is the risk of userspace breakage here when you change things people are relying on.
I propose to set the snd_soc_card->driver_name to something more driver (not card name) related like:
diff --git a/sound/soc/qcom/sdm845.c b/sound/soc/qcom/sdm845.c index b2de65c7f95c..f6adc5a05eeb 100644 --- a/sound/soc/qcom/sdm845.c +++ b/sound/soc/qcom/sdm845.c @@ -557,6 +557,9 @@ static int sdm845_snd_platform_probe(struct platform_device *pdev) card->dapm_widgets = sdm845_snd_widgets; card->num_dapm_widgets = ARRAY_SIZE(sdm845_snd_widgets); card->dev = dev; +#ifdef CONFIG_SND_SOC_DRIVER_NAMES + card->driver_name = "SDM845"; +#endif dev_set_drvdata(dev, card); ret = qcom_snd_parse_of(card); if (ret) { diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 843b8b1c89d4..fdf2c4a12e52 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2246,6 +2246,14 @@ int snd_soc_register_card(struct snd_soc_card *card) if (!card->name || !card->dev) return -EINVAL;
+#ifdef CONFIG_SND_SOC_DRIVER_NAMES + if (!card->driver_name) { + dev_warn(card->dev, + "ASoC: Card driver does not set the driver name\n"); + return -EINVAL; + } +#endif + dev_set_drvdata(card->dev, card);
INIT_LIST_HEAD(&card->widgets);
The goal is to group the related UCM2 configurations and do the required split inside the UCM2 top-level configuration file based on card components string or other card identificators (related to the driver).
This sounds like you either want some enumeration of the card components or perhaps you're looking for some for some indication of the reference design that an individual board is based off so you can have a generic configuration for that reference design and then override bits of it?
I'd like to group the related configuration files and it seems that the driver name field in the CTL info structure is misused (duplicate information).
Jaroslav
On Thu, Apr 23, 2020 at 01:19:31PM +0200, Jaroslav Kysela wrote:
Dne 23. 04. 20 v 13:04 Mark Brown napsal(a):
With ASoC systems there is no clear controller - you've got a bunch of different components, usually connected by separate buses, and it's not super obvious what if anything should be the singular name that gets picked for some grouping of devices. The whole point of the subsystem is to glue a bunch of independent devices together, we've generally picked that glue as the driver name.
I refer mostly the top-level code which creates and registers the ASoC card structure. So it seems the platform name for many ASoC drivers should be there.
At that level you could just say ASoC... bear in mind that a huge proportion of cards are going to come out as one of the generic cards (especially on DT platforms where the firmware situation is less shambolic) which I'm not sure is super useful for grouping things.
Without knowing what you're actually proposing it's hard to know, and there is the risk of userspace breakage here when you change things people are relying on.
+#ifdef CONFIG_SND_SOC_DRIVER_NAMES
card->driver_name = "SDM845";+#endif
OK, so really I think your actual need here sounds like reference platforms here. There's obviously some overlap with drivers but only for the things which use DPCM and ideally we'll be able to move these things over to drivers going forwards which would mean that you'd have the same problem again.
The goal is to group the related UCM2 configurations and do the required split inside the UCM2 top-level configuration file based on card components string or other card identificators (related to the driver).
This sounds like you either want some enumeration of the card components or perhaps you're looking for some for some indication of the reference design that an individual board is based off so you can have a generic configuration for that reference design and then override bits of it?
I'd like to group the related configuration files and it seems that the driver name field in the CTL info structure is misused (duplicate information).
For the generic cards you're not going to get a useful grouping based on the name of the machine driver, they are flexible enough that there can be minimal similarities in the underlying hardware or what it looks like to control in userspace. You are likely to be able to get useful groupings based on things like the CODEC or the SoC from them but the machine driver name isn't going to help much.
+#ifdef CONFIG_SND_SOC_DRIVER_NAMES
if (!card->driver_name) {dev_warn(card->dev,"ASoC: Card driver does not set the driver name\n");return -EINVAL;}
I am all for Jaroslav's proposal of making the driver name the basis for UCM identification. we've been working on this since e.g. the addition of the sof- prefix creates a driver name that makes no sense after a truncation of the card name to 16 characters [1] [2] - still WIP.
Making the card name more user-friendly is also a good thing, there's also a nice hidden feature when the card name contains spaces, the last word - typically the codec - is used for the card ID.
But reporting an error when the driver name is not set is a bit extreme and would break all Intel boards. I think we want to encourage people to move to the suggested solution, but do we want to break existing setups? I must admit I also don't see a generic solution when the card is generated from a DT description, it's not straightforward to translate parsed elements into human-readable ones.
While I am at it, I think we should probably avoid using the DMI information for the long card name. It's just awful. It might be a better idea to add it in the component strings (if it fits) so that UCM can use it internally, but it's really horrible. Even with the clean-ups suggested by Jaroslav I ended-up with this horror of a long name on my test device:
root@Zotac:~# cat /proc/asound/cards 0 [rt5640 ]: SOF - sof-bytcht rt5640 ZOTAC-XXXXXX-XX-CherryTrailFFD
If we really wanted to be user-friendly we'd use something like
"SOF card for Baytrail/Cherrytrail devices with Realtek RT5640 codec"
and apply the same pattern for all machine drivers.
[1] https://github.com/thesofproject/linux/pull/2021
[2] https://github.com/alsa-project/alsa-ucm-conf/pull/20/commits/4cc7fe4493c237...
On Thu, Apr 23, 2020 at 11:17:30AM -0500, Pierre-Louis Bossart wrote:
I am all for Jaroslav's proposal of making the driver name the basis for UCM identification. we've been working on this since e.g. the addition of the sof- prefix creates a driver name that makes no sense after a truncation of the card name to 16 characters [1] [2] - still WIP.
Making the card name more user-friendly is also a good thing, there's also a nice hidden feature when the card name contains spaces, the last word - typically the codec - is used for the card ID.
But reporting an error when the driver name is not set is a bit extreme and would break all Intel boards. I think we want to encourage people to move to the suggested solution, but do we want to break existing setups?
That's an issue, yes - it makes me think we need a really strong case to change things.
I must admit I also don't see a generic solution when the card is generated from a DT description, it's not straightforward to translate parsed elements into human-readable ones.
We do let people just fill in an arbatrary string in the generic cards, not actually checked how many do something sensible though. This is a big worry for me, if we're solving the problem by doing something that doesn't work for generic cards that means that as the generic cards get better and we need fewer custom cards whatever issues the current situation causes in userspace will get worse. For them the commonality is likely to come from one or more of the components in the card and the card itself isn't really interesting.
My instinct is that the machine driver name is being used as a proxy for something else here and that if we need to change the ABI perhaps we need to extend it rather than trying to shoehorn things into what's there.
While I am at it, I think we should probably avoid using the DMI information for the long card name. It's just awful. It might be a better idea to add it in the component strings (if it fits) so that UCM can use it internally, but it's really horrible. Even with the clean-ups suggested by Jaroslav I ended-up with this horror of a long name on my test device:
root@Zotac:~# cat /proc/asound/cards 0 [rt5640 ]: SOF - sof-bytcht rt5640 ZOTAC-XXXXXX-XX-CherryTrailFFD
I don't actually appear to have any DMI equipped systems with non-HDA sound cards but looking at the systems I have (a mix of Lenovo and Dell) they all have sensible display names in the DMI corresponding to the model name. But yeah, outside of enterprise users nobody really pays attention to DMI so there's serious QoI issues in general.
If we really wanted to be user-friendly we'd use something like
"SOF card for Baytrail/Cherrytrail devices with Realtek RT5640 codec"
and apply the same pattern for all machine drivers.
It kind of depends on how well filled in the DMI stuff is - if it's badly filled in it's obviously not useful but if you have a clear model name it works fairly well and matches what a lot of Windows systems do.
Dne 23. 04. 20 v 20:40 Mark Brown napsal(a):
On Thu, Apr 23, 2020 at 11:17:30AM -0500, Pierre-Louis Bossart wrote:
I am all for Jaroslav's proposal of making the driver name the basis for UCM identification. we've been working on this since e.g. the addition of the sof- prefix creates a driver name that makes no sense after a truncation of the card name to 16 characters [1] [2] - still WIP.
Making the card name more user-friendly is also a good thing, there's also a nice hidden feature when the card name contains spaces, the last word - typically the codec - is used for the card ID.
But reporting an error when the driver name is not set is a bit extreme and would break all Intel boards. I think we want to encourage people to move to the suggested solution, but do we want to break existing setups?
That's an issue, yes - it makes me think we need a really strong case to change things.
I just wanted to point to the source location, where the check should be added. If you remove the return, then only the warning will be printed.
Also, we can add the .driver_name to all Intel drivers without the driver name change for the stable drivers. I would prefer the massive change (cleanup also those driver names conditionally), but I know, you're a bit paranoid here ;-)
I must admit I also don't see a generic solution when the card is generated from a DT description, it's not straightforward to translate parsed elements into human-readable ones.
We do let people just fill in an arbatrary string in the generic cards, not actually checked how many do something sensible though. This is a big worry for me, if we're solving the problem by doing something that doesn't work for generic cards that means that as the generic cards get better and we need fewer custom cards whatever issues the current situation causes in userspace will get worse. For them the commonality is likely to come from one or more of the components in the card and the card itself isn't really interesting.
My instinct is that the machine driver name is being used as a proxy for something else here and that if we need to change the ABI perhaps we need to extend it rather than trying to shoehorn things into what's there.
My point is that this information is duplicated in the sense, that we have three fields with the similar contents passed from the audio driver by the ASoC drivers whose set only snd_soc_card->name from the device tree.
For generic drivers: They can pass a generic driver name (like 'ASoC Simple') for the simple card driver (soc/generic/simple-card.c).
So my proposal is to change the driver_name to the right contents (it was the initial intention for this field - changed somehow for ASoC). An information about the used driver which is independent on the real configuration (device tree, ACPI, component enumeration etc.). In other words, the name should be more close to the source (top-level driver) code name than the hardware configuration.
The hardware configuration may be described in the card names and components string field (struct snd_ctl_card_info), so that the user space can do a more finer usage decision later.
While I am at it, I think we should probably avoid using the DMI information for the long card name. It's just awful. It might be a better idea to add it in the component strings (if it fits) so that UCM can use it internally, but it's really horrible. Even with the clean-ups suggested by Jaroslav I ended-up with this horror of a long name on my test device:
root@Zotac:~# cat /proc/asound/cards 0 [rt5640 ]: SOF - sof-bytcht rt5640 ZOTAC-XXXXXX-XX-CherryTrailFFD
I don't actually appear to have any DMI equipped systems with non-HDA sound cards but looking at the systems I have (a mix of Lenovo and Dell) they all have sensible display names in the DMI corresponding to the model name. But yeah, outside of enterprise users nobody really pays attention to DMI so there's serious QoI issues in general.
If we really wanted to be user-friendly we'd use something like
"SOF card for Baytrail/Cherrytrail devices with Realtek RT5640 codec"
and apply the same pattern for all machine drivers.
It kind of depends on how well filled in the DMI stuff is - if it's badly filled in it's obviously not useful but if you have a clear model name it works fairly well and matches what a lot of Windows systems do.
The question is if we resist to use this information in this form. UCM2 can read DMI over sysfs (e.g. /sys/class/dmi/id/product_name can be read using ${sys:class/dmi/id/product_name} substitution and do the string / regex matching on it).
I would prefer to have the sound hardware description in the long name field than the whole hardware platform info here, too.
Jaroslav
On Fri, Apr 24, 2020 at 10:52:38AM +0200, Jaroslav Kysela wrote:
Dne 23. 04. 20 v 20:40 Mark Brown napsal(a):
My instinct is that the machine driver name is being used as a proxy for something else here and that if we need to change the ABI perhaps we need to extend it rather than trying to shoehorn things into what's there.
My point is that this information is duplicated in the sense, that we have three fields with the similar contents passed from the audio driver by the ASoC drivers whose set only snd_soc_card->name from the device tree.
For generic drivers: They can pass a generic driver name (like 'ASoC Simple') for the simple card driver (soc/generic/simple-card.c).
So my proposal is to change the driver_name to the right contents (it was the initial intention for this field - changed somehow for ASoC). An information about the used driver which is independent on the real configuration (device tree, ACPI, component enumeration etc.). In other words, the name should be more close to the source (top-level driver) code name than the hardware configuration.
So if it's not really going to be used for anything particularly concrete then I'm having a hard time summoning the enthusiasm for a change. It feels more like a neatness thing than anything else and the postitive case just isn't jumping out at me, certainly not as a thing to force for everything. New stuff, sure. I guess I'm not bothered enough to block any platform that has a burning desire to convert either though if users start coming and complaining about kernel upgrades breaking things we'd have to revert.
I would prefer to have the sound hardware description in the long name field than the whole hardware platform info here, too.
Does it also cope with the DT equivalents (and I guess there's nothing we can do for board file based systems)? This stuff does get used for embedded systems where the plastics are often important for the configuration.
BTW I think the reason why we're putting the board name in as the driver name is that historically there used to be pretty much one machine driver per board, I can't remember if they were just always taken from the same place or if someone noticed duplicate strings all over the place and removed the duplication.
Dne 24. 04. 20 v 18:49 Mark Brown napsal(a):
On Fri, Apr 24, 2020 at 10:52:38AM +0200, Jaroslav Kysela wrote:
Dne 23. 04. 20 v 20:40 Mark Brown napsal(a):
My instinct is that the machine driver name is being used as a proxy for something else here and that if we need to change the ABI perhaps we need to extend it rather than trying to shoehorn things into what's there.
My point is that this information is duplicated in the sense, that we have three fields with the similar contents passed from the audio driver by the ASoC drivers whose set only snd_soc_card->name from the device tree.
For generic drivers: They can pass a generic driver name (like 'ASoC Simple') for the simple card driver (soc/generic/simple-card.c).
So my proposal is to change the driver_name to the right contents (it was the initial intention for this field - changed somehow for ASoC). An information about the used driver which is independent on the real configuration (device tree, ACPI, component enumeration etc.). In other words, the name should be more close to the source (top-level driver) code name than the hardware configuration.
So if it's not really going to be used for anything particularly concrete then I'm having a hard time summoning the enthusiasm for a change.
The driver name is used as the directory name in UCM / UCM2. For DT, it means thousands possible directories (one per board / board + codec variant and so on..). The generic simple asoc card is a good example.
It feels more like a neatness thing than anything else and the postitive case just isn't jumping out at me, certainly not as a thing to force for everything. New stuff, sure. I guess I'm not bothered enough to block any platform that has a burning desire to convert either though if users start coming and complaining about kernel upgrades breaking things we'd have to revert.
:-( I don't propose to force one way. We can conditionally change the driver names using a well documented CONFIG_ option to keep compatibility with the older user space code. The new driver names may be selected manually in the kernel config.
I would prefer to have the sound hardware description in the long name field than the whole hardware platform info here, too.
Does it also cope with the DT equivalents (and I guess there's nothing we can do for board file based systems)? This stuff does get used for embedded systems where the plastics are often important for the configuration.
The author of DT config knows what hardware is described, thus this person should be resposible for the nice GUI sound part name.
Jaroslav
On Fri, Apr 24, 2020 at 07:11:46PM +0200, Jaroslav Kysela wrote:
Dne 24. 04. 20 v 18:49 Mark Brown napsal(a):
So if it's not really going to be used for anything particularly concrete then I'm having a hard time summoning the enthusiasm for a change.
The driver name is used as the directory name in UCM / UCM2. For DT, it means thousands possible directories (one per board / board + codec variant and so on..). The generic simple asoc card is a good example.
Sure, then you end up with a few directories with huge numbers of files which seems similar? TBH if UCM weren't doing the directory thing it'd be easier to see fixing the neatness issue, what UCM is doing means that it's much more likely that people will notice a problem.
It feels more like a neatness thing than anything else and the postitive case just isn't jumping out at me, certainly not as a thing to force for everything. New stuff, sure. I guess I'm not bothered enough to block any platform that has a burning desire to convert either though if users start coming and complaining about kernel upgrades breaking things we'd have to revert.
:-( I don't propose to force one way. We can conditionally change the driver names using a well documented CONFIG_ option to keep compatibility with the older user space code. The new driver names may be selected manually in the kernel config.
That does provide some mitigation, though there's some potential for confusion too. We could try it and see I guess.
I would prefer to have the sound hardware description in the long name field than the whole hardware platform info here, too.
Does it also cope with the DT equivalents (and I guess there's nothing we can do for board file based systems)? This stuff does get used for embedded systems where the plastics are often important for the configuration.
The author of DT config knows what hardware is described, thus this person should be resposible for the nice GUI sound part name.
Assuming they're working on a system where the sound card name will be displayed in a GUI.
Dne 24. 04. 20 v 20:15 Mark Brown napsal(a):
On Fri, Apr 24, 2020 at 07:11:46PM +0200, Jaroslav Kysela wrote:
Dne 24. 04. 20 v 18:49 Mark Brown napsal(a):
So if it's not really going to be used for anything particularly concrete then I'm having a hard time summoning the enthusiasm for a change.
The driver name is used as the directory name in UCM / UCM2. For DT, it means thousands possible directories (one per board / board + codec variant and so on..). The generic simple asoc card is a good example.
Sure, then you end up with a few directories with huge numbers of files which seems similar?
We can add more levels on demand (when the configs will grow) using the conditions in UCM2. For example:
ucm2/ASoC\ Simple/ASoC\ Simple.conf - which can point to (based on other sound driver / sysfs strings): - ucm2/ASoC Simple/platform1/rt999/board1.conf - ucm2/ASoC Simple/platform1/rt999/board2.conf
etc...
The conditions may also shrink the number of configuration files, because the cards might be different only slightly.
The current scheme is "too flat" and constrained.
TBH if UCM weren't doing the directory thing it'd be easier to see fixing the neatness issue, what UCM is doing means that it's much more likely that people will notice a problem.
It feels more like a neatness thing than anything else and the postitive case just isn't jumping out at me, certainly not as a thing to force for everything. New stuff, sure. I guess I'm not bothered enough to block any platform that has a burning desire to convert either though if users start coming and complaining about kernel upgrades breaking things we'd have to revert.
:-( I don't propose to force one way. We can conditionally change the driver names using a well documented CONFIG_ option to keep compatibility with the older user space code. The new driver names may be selected manually in the kernel config.
That does provide some mitigation, though there's some potential for confusion too. We could try it and see I guess.
Thanks.
Jaroslav
participants (4)
-
Jaroslav Kysela -
Mark Brown -
Pierre-Louis Bossart -
Takashi Iwai