| Message ID | 20220929143754.345144-1-perex@perex.cz (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c8d18e44022518ab026338ae86bf14cdf2e71887 |
| Headers | show |
| Series | [v2] ASoC: core: clarify the driver name initialization | expand |
Hi, On Thu, Sep 29, 2022 at 04:37:54PM +0200, Jaroslav Kysela wrote: > The driver field in the struct snd_ctl_card_info is a valid > user space identifier. Actually, many ASoC drivers do not care > and let to initialize this field using a standard wrapping method. > Unfortunately, in this way, this field becomes unusable and > unreadable for the drivers with longer card names. Also, > there is a possibility to have clashes (driver field has > only limit of 15 characters). > > This change will print an error when the wrapping is used. > The developers of the affected drivers should fix the problem. > > Signed-off-by: Jaroslav Kysela <perex@perex.cz> > > v1..v2: > - remove the wrong DMI condition per Mark's review > --- > sound/soc/soc-core.c | 37 +++++++++++++++++++++++-------------- > 1 file changed, 23 insertions(+), 14 deletions(-) > > diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c > index e824ff1a9fc0..590143ebf6df 100644 > --- a/sound/soc/soc-core.c > +++ b/sound/soc/soc-core.c > @@ -1840,21 +1840,22 @@ static void soc_check_tplg_fes(struct snd_soc_card *card) > } > } > > -#define soc_setup_card_name(name, name1, name2, norm) \ > - __soc_setup_card_name(name, sizeof(name), name1, name2, norm) > -static void __soc_setup_card_name(char *name, int len, > - const char *name1, const char *name2, > - int normalization) > +#define soc_setup_card_name(card, name, name1, name2) \ > + __soc_setup_card_name(card, name, sizeof(name), name1, name2) > +static void __soc_setup_card_name(struct snd_soc_card *card, > + char *name, int len, > + const char *name1, const char *name2) > { > + const char *src = name1 ? name1 : name2; > int i; > > - snprintf(name, len, "%s", name1 ? name1 : name2); > + snprintf(name, len, "%s", src); > > - if (!normalization) > + if (name != card->snd_card->driver) > return; The changes do more than the commit message implies by also reworking the need for the normalization flag, and now that you're special casing most of the function based on a particular pointer, I wonder if splitting the below logic to a separate function would make more sense. In any case, Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> Thanks, Nícolas > > /* > - * Name normalization > + * Name normalization (driver field) > * > * The driver name is somewhat special, as it's used as a key for > * searches in the user-space. > @@ -1874,6 +1875,14 @@ static void __soc_setup_card_name(char *name, int len, > break; > } > } > + > + /* > + * The driver field should contain a valid string from the user view. > + * The wrapping usually does not work so well here. Set a smaller string > + * in the specific ASoC driver. > + */ > + if (strlen(src) > len - 1) > + dev_err(card->dev, "ASoC: driver name too long '%s' -> '%s'\n", src, name); > } > > static void soc_cleanup_card_resources(struct snd_soc_card *card) > @@ -2041,12 +2050,12 @@ static int snd_soc_bind_card(struct snd_soc_card *card) > /* try to set some sane longname if DMI is available */ > snd_soc_set_dmi_name(card, NULL); > > - soc_setup_card_name(card->snd_card->shortname, > - card->name, NULL, 0); > - soc_setup_card_name(card->snd_card->longname, > - card->long_name, card->name, 0); > - soc_setup_card_name(card->snd_card->driver, > - card->driver_name, card->name, 1); > + soc_setup_card_name(card, card->snd_card->shortname, > + card->name, NULL); > + soc_setup_card_name(card, card->snd_card->longname, > + card->long_name, card->name); > + soc_setup_card_name(card, card->snd_card->driver, > + card->driver_name, card->name); > > if (card->components) { > /* the current implementation of snd_component_add() accepts */ > -- > 2.35.3 >
On Thu, 29 Sep 2022 16:37:54 +0200, Jaroslav Kysela wrote: > The driver field in the struct snd_ctl_card_info is a valid > user space identifier. Actually, many ASoC drivers do not care > and let to initialize this field using a standard wrapping method. > Unfortunately, in this way, this field becomes unusable and > unreadable for the drivers with longer card names. Also, > there is a possibility to have clashes (driver field has > only limit of 15 characters). > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/1] ASoC: core: clarify the driver name initialization commit: c8d18e44022518ab026338ae86bf14cdf2e71887 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
Hi Jaroslav, On 9/29/22 09:37, Jaroslav Kysela wrote: > The driver field in the struct snd_ctl_card_info is a valid > user space identifier. Actually, many ASoC drivers do not care > and let to initialize this field using a standard wrapping method. > Unfortunately, in this way, this field becomes unusable and > unreadable for the drivers with longer card names. Also, > there is a possibility to have clashes (driver field has > only limit of 15 characters). > > This change will print an error when the wrapping is used. > The developers of the affected drivers should fix the problem. How should we fix this problem? I see all kinds of errors thrown in our first CI results based on 6.1-rc1: [ 12.684893] kernel: cml_rt1011_rt5682 cml_rt1011_rt5682: ASoC: driver name too long 'sof-cml_rt1011_rt5682' -> 'sof-cml_rt1011_' [ 12.219660] kernel: bxt_da7219_max98357a glk_da7219_mx98357a: ASoC: driver name too long 'sof-glkda7219max' -> 'sof-glkda7219ma' I have no idea what is expected here in terms of naming. It's far from obvious to respect the 15-character limit AND have something readable/sensible given the proliferation of hardware skews. Any suggestions? Thanks, -Pierre
On 19. 10. 22 22:06, Pierre-Louis Bossart wrote: > Hi Jaroslav, > > On 9/29/22 09:37, Jaroslav Kysela wrote: >> The driver field in the struct snd_ctl_card_info is a valid >> user space identifier. Actually, many ASoC drivers do not care >> and let to initialize this field using a standard wrapping method. >> Unfortunately, in this way, this field becomes unusable and >> unreadable for the drivers with longer card names. Also, >> there is a possibility to have clashes (driver field has >> only limit of 15 characters). >> >> This change will print an error when the wrapping is used. >> The developers of the affected drivers should fix the problem. > > How should we fix this problem? > > I see all kinds of errors thrown in our first CI results based on 6.1-rc1: > > [ 12.684893] kernel: cml_rt1011_rt5682 cml_rt1011_rt5682: ASoC: driver > name too long 'sof-cml_rt1011_rt5682' -> 'sof-cml_rt1011_' > > [ 12.219660] kernel: bxt_da7219_max98357a glk_da7219_mx98357a: ASoC: > driver name too long 'sof-glkda7219max' -> 'sof-glkda7219ma' > > I have no idea what is expected here in terms of naming. It's far from > obvious to respect the 15-character limit AND have something > readable/sensible given the proliferation of hardware skews. > > Any suggestions? The question is, how deep the driver name should describe the hardware details. The two drivers covering the majority hardware use "HDA-Intel" and "USB-Audio" strings here and there are a lot of variants of the codec / user space devices / mixer controls. The codec chain and eventually the audio bridge can be described in other identification strings like card components or the card name (note that most end users are not able to identify the references to hardware - it's a GUI string). I would use "SOF-Intel-CML" and "SOF-Intel-GLK" strings or just "SOF-Intel" or any other variant as you like (lower case etc.). My opinion is that it's not necessary to have an unique string per driver here (the drivers should have just something common like the SOF core code). Jaroslav
Hi Jaroslav, >>> The driver field in the struct snd_ctl_card_info is a valid >>> user space identifier. Actually, many ASoC drivers do not care >>> and let to initialize this field using a standard wrapping method. >>> Unfortunately, in this way, this field becomes unusable and >>> unreadable for the drivers with longer card names. Also, >>> there is a possibility to have clashes (driver field has >>> only limit of 15 characters). >>> >>> This change will print an error when the wrapping is used. >>> The developers of the affected drivers should fix the problem. >> >> How should we fix this problem? >> >> I see all kinds of errors thrown in our first CI results based on >> 6.1-rc1: >> >> [ 12.684893] kernel: cml_rt1011_rt5682 cml_rt1011_rt5682: ASoC: driver >> name too long 'sof-cml_rt1011_rt5682' -> 'sof-cml_rt1011_' >> >> [ 12.219660] kernel: bxt_da7219_max98357a glk_da7219_mx98357a: ASoC: >> driver name too long 'sof-glkda7219max' -> 'sof-glkda7219ma' >> >> I have no idea what is expected here in terms of naming. It's far from >> obvious to respect the 15-character limit AND have something >> readable/sensible given the proliferation of hardware skews. >> >> Any suggestions? > > The question is, how deep the driver name should describe the hardware > details. The two drivers covering the majority hardware use "HDA-Intel" > and "USB-Audio" strings here and there are a lot of variants of the > codec / user space devices / mixer controls. The codec chain and > eventually the audio bridge can be described in other identification > strings like card components or the card name (note that most end users > are not able to identify the references to hardware - it's a GUI string). > > I would use "SOF-Intel-CML" and "SOF-Intel-GLK" strings or just > "SOF-Intel" or any other variant as you like (lower case etc.). My > opinion is that it's not necessary to have an unique string per driver > here (the drivers should have just something common like the SOF core > code). ok, adding 'SOF-Intel' would be fine, but remind me again how to set the driver name? We already set the card name to e.g. broxton_audio_card.name = "glkda7219max"; then the sof-prefix gets added, and that's what we use for UCM [1] how would I modify the driver name? If I just go ahead and set .driver_name = "SOF-Intel" in the card declaration, isn't this going to impact the calls to soc_setup_card_name(card, card->snd_card->shortname, card->name, NULL); soc_setup_card_name(card, card->snd_card->longname, card->long_name, card->name); soc_setup_card_name(card, card->snd_card->driver, card->driver_name, card->name); #define soc_setup_card_name(card, name, name1, name2) \ __soc_setup_card_name(card, name, sizeof(name), name1, name2) static void __soc_setup_card_name(struct snd_soc_card *card, char *name, int len, const char *name1, const char *name2) { const char *src = name1 ? name1 : name2; int i; snprintf(name, len, "%s", src); if (name != card->snd_card->driver) return; so the shortname and longname would never be used? I am beyond confused on all this :-) [1] https://github.com/alsa-project/alsa-ucm-conf/tree/master/ucm2/Intel/sof-glkda7219max
On 20. 10. 22 18:27, Pierre-Louis Bossart wrote: > Hi Jaroslav, > >>>> The driver field in the struct snd_ctl_card_info is a valid >>>> user space identifier. Actually, many ASoC drivers do not care >>>> and let to initialize this field using a standard wrapping method. >>>> Unfortunately, in this way, this field becomes unusable and >>>> unreadable for the drivers with longer card names. Also, >>>> there is a possibility to have clashes (driver field has >>>> only limit of 15 characters). >>>> >>>> This change will print an error when the wrapping is used. >>>> The developers of the affected drivers should fix the problem. >>> >>> How should we fix this problem? >>> >>> I see all kinds of errors thrown in our first CI results based on >>> 6.1-rc1: >>> >>> [ 12.684893] kernel: cml_rt1011_rt5682 cml_rt1011_rt5682: ASoC: driver >>> name too long 'sof-cml_rt1011_rt5682' -> 'sof-cml_rt1011_' >>> >>> [ 12.219660] kernel: bxt_da7219_max98357a glk_da7219_mx98357a: ASoC: >>> driver name too long 'sof-glkda7219max' -> 'sof-glkda7219ma' >>> >>> I have no idea what is expected here in terms of naming. It's far from >>> obvious to respect the 15-character limit AND have something >>> readable/sensible given the proliferation of hardware skews. >>> >>> Any suggestions? >> >> The question is, how deep the driver name should describe the hardware >> details. The two drivers covering the majority hardware use "HDA-Intel" >> and "USB-Audio" strings here and there are a lot of variants of the >> codec / user space devices / mixer controls. The codec chain and >> eventually the audio bridge can be described in other identification >> strings like card components or the card name (note that most end users >> are not able to identify the references to hardware - it's a GUI string). >> >> I would use "SOF-Intel-CML" and "SOF-Intel-GLK" strings or just >> "SOF-Intel" or any other variant as you like (lower case etc.). My >> opinion is that it's not necessary to have an unique string per driver >> here (the drivers should have just something common like the SOF core >> code). > > ok, adding 'SOF-Intel' would be fine, but remind me again how to set > the driver name? > > We already set the card name to e.g. > broxton_audio_card.name = "glkda7219max"; > then the sof-prefix gets added, and that's what we use for UCM [1] > > how would I modify the driver name? > > If I just go ahead and set .driver_name = "SOF-Intel" in the card > declaration, isn't this going to impact the calls to > > soc_setup_card_name(card, card->snd_card->shortname, > card->name, NULL); > soc_setup_card_name(card, card->snd_card->longname, > card->long_name, card->name); > soc_setup_card_name(card, card->snd_card->driver, > card->driver_name, card->name); > > > #define soc_setup_card_name(card, name, name1, name2) \ > __soc_setup_card_name(card, name, sizeof(name), name1, name2) > static void __soc_setup_card_name(struct snd_soc_card *card, > char *name, int len, > const char *name1, const char *name2) > { > const char *src = name1 ? name1 : name2; > int i; > > snprintf(name, len, "%s", src); > > if (name != card->snd_card->driver) > return; > > so the shortname and longname would never be used? Nope. It's just a short path for the non-driver field to not further process the destination string (the name argument). The snprintf() call sets all field types (it's before the condition). Just set the driver_name field in the soc card structure and you will be fine. The UCM config must be updated to handle the new driver name. The fine selection key should probably use the card name, because long name is set from DMI: old: 1 [sofglkda7219max]: sof-glkda7219ma - sof-glkda7219max Google-Phaser360-rev4 new: 1 [sofglkda7219max]: SOF-Intel - sof-glkda7219max Google-Phaser360-rev4 UCM substitutions: 1 [${CardId} ]: ${CardDriver} - ${CardName} ${CardLongName} UCM conf: mkdir -p ucm2/conf.d/SOF-Intel cat > ucm2/conf.d/SOF-Intel/SOF-Intel.conf <<EOF Syntax 6 Include.0.File "/Intel/\${CardName}/\${CardName}.conf" EOF Jaroslav
Hi Jaroslav, > Nope. It's just a short path for the non-driver field to not further > process the destination string (the name argument). The snprintf() call > sets all field types (it's before the condition). Just set the > driver_name field in the soc card structure and you will be fine. > > The UCM config must be updated to handle the new driver name. The fine > selection key should probably use the card name, because long name is > set from DMI: > > old: > > 1 [sofglkda7219max]: sof-glkda7219ma - sof-glkda7219max > Google-Phaser360-rev4 > > new: > > 1 [sofglkda7219max]: SOF-Intel - sof-glkda7219max > Google-Phaser360-rev4 > > UCM substitutions: > > 1 [${CardId} ]: ${CardDriver} - ${CardName} > ${CardLongName} > > UCM conf: > > mkdir -p ucm2/conf.d/SOF-Intel > cat > ucm2/conf.d/SOF-Intel/SOF-Intel.conf <<EOF > Syntax 6 > Include.0.File "/Intel/\${CardName}/\${CardName}.conf" > EOF I am not following any of this, sorry. The existing UCM configuration uses the card name, e.g. sof-glkda7219max. That works and needs zero extra work. If all the cards registered in sound/soc/intel/boards use the same "SOF-Intel" driver name, then the driver name cannot be used for any UCM selection. What is the point of including all the cardName.conf files at a higher level that brings no obvious value beyond an indirection that we already have with the path ucm2/Intel ? What am I missing ??
On 20. 10. 22 20:13, Pierre-Louis Bossart wrote: > Hi Jaroslav, > >> Nope. It's just a short path for the non-driver field to not further >> process the destination string (the name argument). The snprintf() call >> sets all field types (it's before the condition). Just set the >> driver_name field in the soc card structure and you will be fine. >> >> The UCM config must be updated to handle the new driver name. The fine >> selection key should probably use the card name, because long name is >> set from DMI: >> >> old: >> >> 1 [sofglkda7219max]: sof-glkda7219ma - sof-glkda7219max >> Google-Phaser360-rev4 >> >> new: >> >> 1 [sofglkda7219max]: SOF-Intel - sof-glkda7219max >> Google-Phaser360-rev4 >> >> UCM substitutions: >> >> 1 [${CardId} ]: ${CardDriver} - ${CardName} >> ${CardLongName} >> >> UCM conf: >> >> mkdir -p ucm2/conf.d/SOF-Intel >> cat > ucm2/conf.d/SOF-Intel/SOF-Intel.conf <<EOF >> Syntax 6 >> Include.0.File "/Intel/\${CardName}/\${CardName}.conf" >> EOF > > I am not following any of this, sorry. > > The existing UCM configuration uses the card name, e.g. > sof-glkda7219max. That works and needs zero extra work. Unfortunately, actually the wrapped driver names are used for the primary lookups. The card name is not used at all in ucm2/conf.d. > If all the cards registered in sound/soc/intel/boards use the same > "SOF-Intel" driver name, then the driver name cannot be used for any UCM > selection. It can be used for the first level of the lookup. Eventually, we can add ucm2/conf.d/${CardDriver}/${CardName}.conf search path to ucm2/ucm.conf for the direct lookups to handle this case, but it's just an optimization. I would start with the ${CardName} redirection as I suggested. We can decide / make the ucm.conf change later. > What is the point of including all the cardName.conf files at a higher > level that brings no obvious value beyond an indirection that we already > have with the path ucm2/Intel ? > > What am I missing ?? The goal is to fix the driver names (e.g. "sof-glkda7219ma", "sof-mt8195_r101" etc.). If you like to keep the unique names, it's your decision. I just prefer to have a string which is understandable to users. UCM can handle the finer selection of the configuration at any level now. Examples: sof-soundwire, USB-Audio (ucm2/USB-Audio/USB-Audio.conf), SOF (ucm2/Intel/SOF/HiFi.conf). Jaroslav
On 10/21/22 01:37, Jaroslav Kysela wrote: > On 20. 10. 22 20:13, Pierre-Louis Bossart wrote: >> Hi Jaroslav, >> >>> Nope. It's just a short path for the non-driver field to not further >>> process the destination string (the name argument). The snprintf() call >>> sets all field types (it's before the condition). Just set the >>> driver_name field in the soc card structure and you will be fine. >>> >>> The UCM config must be updated to handle the new driver name. The fine >>> selection key should probably use the card name, because long name is >>> set from DMI: >>> >>> old: >>> >>> 1 [sofglkda7219max]: sof-glkda7219ma - sof-glkda7219max >>> Google-Phaser360-rev4 >>> >>> new: >>> >>> 1 [sofglkda7219max]: SOF-Intel - sof-glkda7219max >>> Google-Phaser360-rev4 >>> >>> UCM substitutions: >>> >>> 1 [${CardId} ]: ${CardDriver} - ${CardName} >>> ${CardLongName} >>> >>> UCM conf: >>> >>> mkdir -p ucm2/conf.d/SOF-Intel >>> cat > ucm2/conf.d/SOF-Intel/SOF-Intel.conf <<EOF >>> Syntax 6 >>> Include.0.File "/Intel/\${CardName}/\${CardName}.conf" >>> EOF >> >> I am not following any of this, sorry. >> >> The existing UCM configuration uses the card name, e.g. >> sof-glkda7219max. That works and needs zero extra work. > > Unfortunately, actually the wrapped driver names are used for the > primary lookups. The card name is not used at all in ucm2/conf.d. ok, that's interesting because I've always used the card name with alsaucm :-) Usage: alsaucm <options> [command] Available options: -h,--help this help -c,--card NAME open card NAME alsaucm -c sof-glkda7219max set _verb HiFi set _enadev Headphone And if we move to use the driver name as the primary lookup then we'd still need the card name for alsaucm to work, so why use the driver name as a primary lookup? If we can report a less confusing driver name to the users, that's fine with me, but I don't get the idea of using the driver name as the first criterion to identify a setup, you'll also need the card name so why not use the card name as primary criterion? >> If all the cards registered in sound/soc/intel/boards use the same >> "SOF-Intel" driver name, then the driver name cannot be used for any UCM >> selection. > > It can be used for the first level of the lookup. Eventually, we can add > ucm2/conf.d/${CardDriver}/${CardName}.conf search path to ucm2/ucm.conf > for the direct lookups to handle this case, but it's just an > optimization. I would start with the ${CardName} redirection as I > suggested. We can decide / make the ucm.conf change later. > >> What is the point of including all the cardName.conf files at a higher >> level that brings no obvious value beyond an indirection that we already >> have with the path ucm2/Intel ? >> >> What am I missing ?? > > The goal is to fix the driver names (e.g. "sof-glkda7219ma", > "sof-mt8195_r101" etc.). If you like to keep the unique names, it's your > decision. I just prefer to have a string which is understandable to > users. UCM can handle the finer selection of the configuration at any > level now. Examples: sof-soundwire, USB-Audio > (ucm2/USB-Audio/USB-Audio.conf), SOF (ucm2/Intel/SOF/HiFi.conf). I don't mind setting a common string, it's not going to be possible to capture all hardware variations in 15 characters. What worries me is backwards compatibility with existing user setups. If someone updates their kernel and the change in driver name ends-up breaking audio that's a big no-no for me. That's a real issue for us in terms of support because we typically ask people reporting SOF/SoundWire/HDA/mic issues if they can installing with our development kernel.
On 21. 10. 22 16:36, Pierre-Louis Bossart wrote: > > > On 10/21/22 01:37, Jaroslav Kysela wrote: >> On 20. 10. 22 20:13, Pierre-Louis Bossart wrote: >>> Hi Jaroslav, >>> >>>> Nope. It's just a short path for the non-driver field to not further >>>> process the destination string (the name argument). The snprintf() call >>>> sets all field types (it's before the condition). Just set the >>>> driver_name field in the soc card structure and you will be fine. >>>> >>>> The UCM config must be updated to handle the new driver name. The fine >>>> selection key should probably use the card name, because long name is >>>> set from DMI: >>>> >>>> old: >>>> >>>> 1 [sofglkda7219max]: sof-glkda7219ma - sof-glkda7219max >>>> Google-Phaser360-rev4 >>>> >>>> new: >>>> >>>> 1 [sofglkda7219max]: SOF-Intel - sof-glkda7219max >>>> Google-Phaser360-rev4 >>>> >>>> UCM substitutions: >>>> >>>> 1 [${CardId} ]: ${CardDriver} - ${CardName} >>>> ${CardLongName} >>>> >>>> UCM conf: >>>> >>>> mkdir -p ucm2/conf.d/SOF-Intel >>>> cat > ucm2/conf.d/SOF-Intel/SOF-Intel.conf <<EOF >>>> Syntax 6 >>>> Include.0.File "/Intel/\${CardName}/\${CardName}.conf" >>>> EOF >>> >>> I am not following any of this, sorry. >>> >>> The existing UCM configuration uses the card name, e.g. >>> sof-glkda7219max. That works and needs zero extra work. >> >> Unfortunately, actually the wrapped driver names are used for the >> primary lookups. The card name is not used at all in ucm2/conf.d. > > ok, that's interesting because I've always used the card name with > alsaucm :-) > > > Usage: alsaucm <options> [command] > > Available options: > -h,--help this help > -c,--card NAME open card NAME > > alsaucm -c sof-glkda7219max set _verb HiFi set _enadev Headphone > > And if we move to use the driver name as the primary lookup then we'd > still need the card name for alsaucm to work, so why use the driver name > as a primary lookup? The driver name is used for the lookups as defined in ucm2/ucm.conf. The driver/card name/card long name lookups are handled in the alsa-lib's code (converted to "hw:X" UCM card name). If you turn this fallback off (use "strict:sof-glkda7219max" UCM card name), things won't work. > If we can report a less confusing driver name to the users, that's fine > with me, but I don't get the idea of using the driver name as the first > criterion to identify a setup, you'll also need the card name so why not > use the card name as primary criterion? It is not usable for the USB driver where every model has own name set from USB descriptors for example. >>> If all the cards registered in sound/soc/intel/boards use the same >>> "SOF-Intel" driver name, then the driver name cannot be used for any UCM >>> selection. >> >> It can be used for the first level of the lookup. Eventually, we can add >> ucm2/conf.d/${CardDriver}/${CardName}.conf search path to ucm2/ucm.conf >> for the direct lookups to handle this case, but it's just an >> optimization. I would start with the ${CardName} redirection as I >> suggested. We can decide / make the ucm.conf change later. >> >>> What is the point of including all the cardName.conf files at a higher >>> level that brings no obvious value beyond an indirection that we already >>> have with the path ucm2/Intel ? >>> >>> What am I missing ?? >> >> The goal is to fix the driver names (e.g. "sof-glkda7219ma", >> "sof-mt8195_r101" etc.). If you like to keep the unique names, it's your >> decision. I just prefer to have a string which is understandable to >> users. UCM can handle the finer selection of the configuration at any >> level now. Examples: sof-soundwire, USB-Audio >> (ucm2/USB-Audio/USB-Audio.conf), SOF (ucm2/Intel/SOF/HiFi.conf). > > I don't mind setting a common string, it's not going to be possible to > capture all hardware variations in 15 characters. > > What worries me is backwards compatibility with existing user setups. If > someone updates their kernel and the change in driver name ends-up > breaking audio that's a big no-no for me. That's a real issue for us in > terms of support because we typically ask people reporting > SOF/SoundWire/HDA/mic issues if they can installing with our development > kernel. We can use a similar mechanism as we did with CONFIG_SND_SOC_INTEL_USER_FRIENDLY_LONG_NAMES . The distributions can enable this when packages when UCM configs are updated. Also, new drivers should use new driver name scheme, it's only for the current drivers. Jaroslav
On 10/24/22 01:19, Jaroslav Kysela wrote: > On 21. 10. 22 16:36, Pierre-Louis Bossart wrote: >> >> >> On 10/21/22 01:37, Jaroslav Kysela wrote: >>> On 20. 10. 22 20:13, Pierre-Louis Bossart wrote: >>>> Hi Jaroslav, >>>> >>>>> Nope. It's just a short path for the non-driver field to not further >>>>> process the destination string (the name argument). The snprintf() >>>>> call >>>>> sets all field types (it's before the condition). Just set the >>>>> driver_name field in the soc card structure and you will be fine. >>>>> >>>>> The UCM config must be updated to handle the new driver name. The fine >>>>> selection key should probably use the card name, because long name is >>>>> set from DMI: >>>>> >>>>> old: >>>>> >>>>> 1 [sofglkda7219max]: sof-glkda7219ma - sof-glkda7219max >>>>> Google-Phaser360-rev4 >>>>> >>>>> new: >>>>> >>>>> 1 [sofglkda7219max]: SOF-Intel - sof-glkda7219max >>>>> Google-Phaser360-rev4 >>>>> >>>>> UCM substitutions: >>>>> >>>>> 1 [${CardId} ]: ${CardDriver} - ${CardName} >>>>> ${CardLongName} >>>>> >>>>> UCM conf: >>>>> >>>>> mkdir -p ucm2/conf.d/SOF-Intel >>>>> cat > ucm2/conf.d/SOF-Intel/SOF-Intel.conf <<EOF >>>>> Syntax 6 >>>>> Include.0.File "/Intel/\${CardName}/\${CardName}.conf" >>>>> EOF >>>> >>>> I am not following any of this, sorry. >>>> >>>> The existing UCM configuration uses the card name, e.g. >>>> sof-glkda7219max. That works and needs zero extra work. >>> >>> Unfortunately, actually the wrapped driver names are used for the >>> primary lookups. The card name is not used at all in ucm2/conf.d. >> >> ok, that's interesting because I've always used the card name with >> alsaucm :-) >> >> >> Usage: alsaucm <options> [command] >> >> Available options: >> -h,--help this help >> -c,--card NAME open card NAME >> >> alsaucm -c sof-glkda7219max set _verb HiFi set _enadev Headphone >> >> And if we move to use the driver name as the primary lookup then we'd >> still need the card name for alsaucm to work, so why use the driver name >> as a primary lookup? > > The driver name is used for the lookups as defined in ucm2/ucm.conf. The > driver/card name/card long name lookups are handled in the alsa-lib's > code (converted to "hw:X" UCM card name). If you turn this fallback off > (use "strict:sof-glkda7219max" UCM card name), things won't work. Fair enough, I wasn't even aware of the existence of this 'strict' directive. >> If we can report a less confusing driver name to the users, that's fine >> with me, but I don't get the idea of using the driver name as the first >> criterion to identify a setup, you'll also need the card name so why not >> use the card name as primary criterion? > > It is not usable for the USB driver where every model has own name set > from USB descriptors for example. How would you use UCM in that context, the use of a driver name would lead to a lot of abstraction potentially, isn't there a risk of not being able to detect specific skews that need variants? >>>> If all the cards registered in sound/soc/intel/boards use the same >>>> "SOF-Intel" driver name, then the driver name cannot be used for any >>>> UCM >>>> selection. >>> >>> It can be used for the first level of the lookup. Eventually, we can add >>> ucm2/conf.d/${CardDriver}/${CardName}.conf search path to ucm2/ucm.conf >>> for the direct lookups to handle this case, but it's just an >>> optimization. I would start with the ${CardName} redirection as I >>> suggested. We can decide / make the ucm.conf change later. >>> >>>> What is the point of including all the cardName.conf files at a higher >>>> level that brings no obvious value beyond an indirection that we >>>> already >>>> have with the path ucm2/Intel ? >>>> >>>> What am I missing ?? >>> >>> The goal is to fix the driver names (e.g. "sof-glkda7219ma", >>> "sof-mt8195_r101" etc.). If you like to keep the unique names, it's your >>> decision. I just prefer to have a string which is understandable to >>> users. UCM can handle the finer selection of the configuration at any >>> level now. Examples: sof-soundwire, USB-Audio >>> (ucm2/USB-Audio/USB-Audio.conf), SOF (ucm2/Intel/SOF/HiFi.conf). >> >> I don't mind setting a common string, it's not going to be possible to >> capture all hardware variations in 15 characters. >> >> What worries me is backwards compatibility with existing user setups. If >> someone updates their kernel and the change in driver name ends-up >> breaking audio that's a big no-no for me. That's a real issue for us in >> terms of support because we typically ask people reporting >> SOF/SoundWire/HDA/mic issues if they can installing with our development >> kernel. > > We can use a similar mechanism as we did with > CONFIG_SND_SOC_INTEL_USER_FRIENDLY_LONG_NAMES . The distributions can > enable this when packages when UCM configs are updated. Also, new > drivers should use new driver name scheme, it's only for the current > drivers. That would be good indeed. FWIW, I reverted this patch in our development tree to remove confusing error messages that make tests fail. That would not be an Intel only option though, right? There are tons of other ASoC machine drivers who don't set the driver name at all, so it could take time to make that transition.
On 24. 10. 22 16:08, Pierre-Louis Bossart wrote: >>> If we can report a less confusing driver name to the users, that's fine >>> with me, but I don't get the idea of using the driver name as the first >>> criterion to identify a setup, you'll also need the card name so why not >>> use the card name as primary criterion? >> >> It is not usable for the USB driver where every model has own name set >> from USB descriptors for example. > > How would you use UCM in that context, the use of a driver name would > lead to a lot of abstraction potentially, isn't there a risk of not > being able to detect specific skews that need variants? The fine USB device ID matching is used. This USB device ID is in the components string. But yes, it's the next level after the basic lookup. >> We can use a similar mechanism as we did with >> CONFIG_SND_SOC_INTEL_USER_FRIENDLY_LONG_NAMES . The distributions can >> enable this when packages when UCM configs are updated. Also, new >> drivers should use new driver name scheme, it's only for the current >> drivers. > > That would be good indeed. FWIW, I reverted this patch in our > development tree to remove confusing error messages that make tests fail. > > That would not be an Intel only option though, right? There are tons of > other ASoC machine drivers who don't set the driver name at all, so it > could take time to make that transition. Yes, but we need to start somewhere. It seems that a most of ASoC drivers do not use card names bigger than 15 characters (I noted this recently in UCM). Jaroslav
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index e824ff1a9fc0..590143ebf6df 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1840,21 +1840,22 @@ static void soc_check_tplg_fes(struct snd_soc_card *card) } } -#define soc_setup_card_name(name, name1, name2, norm) \ - __soc_setup_card_name(name, sizeof(name), name1, name2, norm) -static void __soc_setup_card_name(char *name, int len, - const char *name1, const char *name2, - int normalization) +#define soc_setup_card_name(card, name, name1, name2) \ + __soc_setup_card_name(card, name, sizeof(name), name1, name2) +static void __soc_setup_card_name(struct snd_soc_card *card, + char *name, int len, + const char *name1, const char *name2) { + const char *src = name1 ? name1 : name2; int i; - snprintf(name, len, "%s", name1 ? name1 : name2); + snprintf(name, len, "%s", src); - if (!normalization) + if (name != card->snd_card->driver) return; /* - * Name normalization + * Name normalization (driver field) * * The driver name is somewhat special, as it's used as a key for * searches in the user-space. @@ -1874,6 +1875,14 @@ static void __soc_setup_card_name(char *name, int len, break; } } + + /* + * The driver field should contain a valid string from the user view. + * The wrapping usually does not work so well here. Set a smaller string + * in the specific ASoC driver. + */ + if (strlen(src) > len - 1) + dev_err(card->dev, "ASoC: driver name too long '%s' -> '%s'\n", src, name); } static void soc_cleanup_card_resources(struct snd_soc_card *card) @@ -2041,12 +2050,12 @@ static int snd_soc_bind_card(struct snd_soc_card *card) /* try to set some sane longname if DMI is available */ snd_soc_set_dmi_name(card, NULL); - soc_setup_card_name(card->snd_card->shortname, - card->name, NULL, 0); - soc_setup_card_name(card->snd_card->longname, - card->long_name, card->name, 0); - soc_setup_card_name(card->snd_card->driver, - card->driver_name, card->name, 1); + soc_setup_card_name(card, card->snd_card->shortname, + card->name, NULL); + soc_setup_card_name(card, card->snd_card->longname, + card->long_name, card->name); + soc_setup_card_name(card, card->snd_card->driver, + card->driver_name, card->name); if (card->components) { /* the current implementation of snd_component_add() accepts */
The driver field in the struct snd_ctl_card_info is a valid user space identifier. Actually, many ASoC drivers do not care and let to initialize this field using a standard wrapping method. Unfortunately, in this way, this field becomes unusable and unreadable for the drivers with longer card names. Also, there is a possibility to have clashes (driver field has only limit of 15 characters). This change will print an error when the wrapping is used. The developers of the affected drivers should fix the problem. Signed-off-by: Jaroslav Kysela <perex@perex.cz> v1..v2: - remove the wrong DMI condition per Mark's review --- sound/soc/soc-core.c | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-)