| Message ID | 20180508153604.23711-20-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 063422ca2a9de238401c3848c1b3641c07b6316c |
| Headers | show |
On 5/8/18 10:36 AM, Hans de Goede wrote: > Many X86 devices using a BYT SoC + RT5640 codec are cheap devices with > generic DMI strings, causing snd_soc_set_dmi_name() to fail to set a > long_name, making it impossible for userspace to have a correct UCM > profile which only uses inputs / outputs which are actually hooked up > on the device. > > Our quirks already specify which input the internal mic is connected to > and if a single (mono) speaker is used or if the device has stereo > speakers. > > This commit sets a long_name based on the quirks so that userspace can > have UCM profiles doing the right thing based on the long_name. Isn't this going to be complicated to manage for UCM? Just with this patch alone, you'd need 8 UCM files to cover all the combinations. 16 if you add the 'sof-' prefix. seems like UCM should become more 'dynamic' and get quirk information somehow (sysfs?) to enable/disable endpoints rather than rely on name encoding to select the right profile? > > Note that if we ever encounter the need for a special UCM profile for > some device we can add a quirk to set a specific long_name for the > device, > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > sound/soc/intel/boards/bytcr_rt5640.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c > index cfc520200214..9a1204dcdbc6 100644 > --- a/sound/soc/intel/boards/bytcr_rt5640.c > +++ b/sound/soc/intel/boards/bytcr_rt5640.c > @@ -929,6 +929,7 @@ static struct snd_soc_dai_link byt_rt5640_dais[] = { > static char byt_rt5640_codec_name[SND_ACPI_I2C_ID_LEN]; > static char byt_rt5640_codec_aif_name[12]; /* = "rt5640-aif[1|2]" */ > static char byt_rt5640_cpu_dai_name[10]; /* = "ssp[0|2]-port" */ > +static char byt_rt5640_long_name[40]; /* = "bytcr-rt5640-*-spk-*-mic" */ > > static int byt_rt5640_suspend(struct snd_soc_card *card) > { > @@ -1000,6 +1001,7 @@ struct acpi_chan_package { /* ACPICA seems to require 64 bit integers */ > > static int snd_byt_rt5640_mc_probe(struct platform_device *pdev) > { > + const char * const map_name[] = { "dmic1", "dmic2", "in1", "in3" }; > const struct dmi_system_id *dmi_id; > struct byt_rt5640_private *priv; > struct snd_soc_acpi_mach *mach; > @@ -1163,6 +1165,13 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev) > } > } > > + snprintf(byt_rt5640_long_name, sizeof(byt_rt5640_long_name), > + "bytcr-rt5640-%s-spk-%s-mic", > + (byt_rt5640_quirk & BYT_RT5640_MONO_SPEAKER) ? > + "mono" : "stereo", > + map_name[BYT_RT5640_MAP(byt_rt5640_quirk)]); > + byt_rt5640_card.long_name = byt_rt5640_long_name; > + > ret_val = devm_snd_soc_register_card(&pdev->dev, &byt_rt5640_card); > > if (ret_val) { >
Hi, On 08-05-18 20:35, Pierre-Louis Bossart wrote: > On 5/8/18 10:36 AM, Hans de Goede wrote: >> Many X86 devices using a BYT SoC + RT5640 codec are cheap devices with >> generic DMI strings, causing snd_soc_set_dmi_name() to fail to set a >> long_name, making it impossible for userspace to have a correct UCM >> profile which only uses inputs / outputs which are actually hooked up >> on the device. >> >> Our quirks already specify which input the internal mic is connected to >> and if a single (mono) speaker is used or if the device has stereo >> speakers. >> >> This commit sets a long_name based on the quirks so that userspace can >> have UCM profiles doing the right thing based on the long_name. > > Isn't this going to be complicated to manage for UCM? Just with this patch alone, you'd need 8 UCM files to cover all the combinations. 16 if you add the 'sof-' prefix. > > seems like UCM should become more 'dynamic' and get quirk information somehow (sysfs?) to enable/disable endpoints rather than rely on name encoding to select the right profile? I agree that this is not ideal, but this is an improvement from the current state where we would need 1 UCM profile per board (assuming valid DMI data and thus a proper long-name being set), 6 profiles (dmic2 is not used anywhere sofar) is a whole lot easier to manage then 1 profile per board. So as said I believe this is a step in the right direction. And looking at the foreseeable future I simply don't see any of us having the time to implement an ideal solution for this. I would really like for end users to be able to run the latest upstream kernel + alsa-lib and have things just work, before this hardware becomes obsolete. I know that no-one having time to work on reworking UCM to make it more dynamic is not the best of arguments but it is something to take into consideration. Thinking more about this on the alsa-lib / UCM profile side we could have something like this: /usr/share/alsa/ucm/bytcr-rt5640-mono-spk-in1-mic/bytcr-rt5640-mono-spk-in1-mic.conf: SectionUseCase."HiFi" { File "../bytcr-rt5640/Generic.conf" File "../bytcr-rt5640/MonoSpeaker.conf" File "../bytcr-rt5640/In1Mic.conf" Comment "Play HiFi quality Music" } SectionDefaults [ cdev "hw:bytcrrt5640" ] The only problem I can see with that is that the "ConflictingDevice" sections for the various inputs / outputs then would refer to not present SectionDevice sections. I have not tested this suggestion yet, but I'm willing to write an alsa-lib patch to ignore non present ConflictingDevice references, to make my suggestion work. I think doing things this way, thus avoiding the need to copy and paste a whole lot of UCM code for the 6 profiles it will not be a problem to maintain 6 profiles, as we're really just maintaining 6 config snippets such as the above example and only one complete profile. Would the solution I outlined above be acceptable to you? Regards, Hans >> Note that if we ever encounter the need for a special UCM profile for >> some device we can add a quirk to set a specific long_name for the >> device, >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> sound/soc/intel/boards/bytcr_rt5640.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c >> index cfc520200214..9a1204dcdbc6 100644 >> --- a/sound/soc/intel/boards/bytcr_rt5640.c >> +++ b/sound/soc/intel/boards/bytcr_rt5640.c >> @@ -929,6 +929,7 @@ static struct snd_soc_dai_link byt_rt5640_dais[] = { >> static char byt_rt5640_codec_name[SND_ACPI_I2C_ID_LEN]; >> static char byt_rt5640_codec_aif_name[12]; /* = "rt5640-aif[1|2]" */ >> static char byt_rt5640_cpu_dai_name[10]; /* = "ssp[0|2]-port" */ >> +static char byt_rt5640_long_name[40]; /* = "bytcr-rt5640-*-spk-*-mic" */ >> static int byt_rt5640_suspend(struct snd_soc_card *card) >> { >> @@ -1000,6 +1001,7 @@ struct acpi_chan_package { /* ACPICA seems to require 64 bit integers */ >> static int snd_byt_rt5640_mc_probe(struct platform_device *pdev) >> { >> + const char * const map_name[] = { "dmic1", "dmic2", "in1", "in3" }; >> const struct dmi_system_id *dmi_id; >> struct byt_rt5640_private *priv; >> struct snd_soc_acpi_mach *mach; >> @@ -1163,6 +1165,13 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev) >> } >> } >> + snprintf(byt_rt5640_long_name, sizeof(byt_rt5640_long_name), >> + "bytcr-rt5640-%s-spk-%s-mic", >> + (byt_rt5640_quirk & BYT_RT5640_MONO_SPEAKER) ? >> + "mono" : "stereo", >> + map_name[BYT_RT5640_MAP(byt_rt5640_quirk)]); >> + byt_rt5640_card.long_name = byt_rt5640_long_name; >> + >> ret_val = devm_snd_soc_register_card(&pdev->dev, &byt_rt5640_card); >> if (ret_val) { >> >
On 5/10/18 5:27 AM, Hans de Goede wrote: > Hi, > > On 08-05-18 20:35, Pierre-Louis Bossart wrote: >> On 5/8/18 10:36 AM, Hans de Goede wrote: >>> Many X86 devices using a BYT SoC + RT5640 codec are cheap devices with >>> generic DMI strings, causing snd_soc_set_dmi_name() to fail to set a >>> long_name, making it impossible for userspace to have a correct UCM >>> profile which only uses inputs / outputs which are actually hooked up >>> on the device. >>> >>> Our quirks already specify which input the internal mic is connected to >>> and if a single (mono) speaker is used or if the device has stereo >>> speakers. >>> >>> This commit sets a long_name based on the quirks so that userspace can >>> have UCM profiles doing the right thing based on the long_name. >> >> Isn't this going to be complicated to manage for UCM? Just with this >> patch alone, you'd need 8 UCM files to cover all the combinations. 16 >> if you add the 'sof-' prefix. >> >> seems like UCM should become more 'dynamic' and get quirk information >> somehow (sysfs?) to enable/disable endpoints rather than rely on name >> encoding to select the right profile? > > I agree that this is not ideal, but this is an improvement from the > current state where we would need 1 UCM profile per board > (assuming valid DMI data and thus a proper long-name being set), > 6 profiles (dmic2 is not used anywhere sofar) is a whole lot easier > to manage then 1 profile per board. So as said I believe this is > a step in the right direction. > > And looking at the foreseeable future I simply don't see any of us > having the time to implement an ideal solution for this. I would > really like for end users to be able to run the latest upstream > kernel + alsa-lib and have things just work, before this hardware > becomes obsolete. I know that no-one having time to work on reworking > UCM to make it more dynamic is not the best of arguments but it > is something to take into consideration. > > Thinking more about this on the alsa-lib / UCM profile side we > could have something like this: > > /usr/share/alsa/ucm/bytcr-rt5640-mono-spk-in1-mic/bytcr-rt5640-mono-spk-in1-mic.conf: > > > SectionUseCase."HiFi" { > File "../bytcr-rt5640/Generic.conf" > File "../bytcr-rt5640/MonoSpeaker.conf" > File "../bytcr-rt5640/In1Mic.conf" > Comment "Play HiFi quality Music" > } > > SectionDefaults [ > cdev "hw:bytcrrt5640" > ] > > The only problem I can see with that is that the "ConflictingDevice" > sections for the various inputs / outputs then would refer to not > present SectionDevice sections. I have not tested this suggestion yet, > but I'm willing to write an alsa-lib patch to ignore non present > ConflictingDevice references, to make my suggestion work. > > I think doing things this way, thus avoiding the need to copy and > paste a whole lot of UCM code for the 6 profiles it will not be > a problem to maintain 6 profiles, as we're really just maintaining > 6 config snippets such as the above example and only one complete > profile. > > Would the solution I outlined above be acceptable to you? The includes and disabling conflicting devices that aren't present make sense. I have another issue though: for SOF integration I already prepared a set of files, which are mostly identical to the regular ones except that the platform-side mixer controls are removed (or different) and the name of the card/device is different (sof- prefix). See on github. I was nearly ready to submit all those files to the new repository, I wasn't aware of your work so now need to figure out how to reuse all this once we upstream SOF (any time now if I look at Liam's patches). > > Regards, > > Hans > > > > > >>> Note that if we ever encounter the need for a special UCM profile for >>> some device we can add a quirk to set a specific long_name for the >>> device, >>> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> --- >>> sound/soc/intel/boards/bytcr_rt5640.c | 9 +++++++++ >>> 1 file changed, 9 insertions(+) >>> >>> diff --git a/sound/soc/intel/boards/bytcr_rt5640.c >>> b/sound/soc/intel/boards/bytcr_rt5640.c >>> index cfc520200214..9a1204dcdbc6 100644 >>> --- a/sound/soc/intel/boards/bytcr_rt5640.c >>> +++ b/sound/soc/intel/boards/bytcr_rt5640.c >>> @@ -929,6 +929,7 @@ static struct snd_soc_dai_link byt_rt5640_dais[] = { >>> static char byt_rt5640_codec_name[SND_ACPI_I2C_ID_LEN]; >>> static char byt_rt5640_codec_aif_name[12]; /* = "rt5640-aif[1|2]" */ >>> static char byt_rt5640_cpu_dai_name[10]; /* = "ssp[0|2]-port" */ >>> +static char byt_rt5640_long_name[40]; /* = >>> "bytcr-rt5640-*-spk-*-mic" */ >>> static int byt_rt5640_suspend(struct snd_soc_card *card) >>> { >>> @@ -1000,6 +1001,7 @@ struct acpi_chan_package { /* ACPICA seems to >>> require 64 bit integers */ >>> static int snd_byt_rt5640_mc_probe(struct platform_device *pdev) >>> { >>> + const char * const map_name[] = { "dmic1", "dmic2", "in1", "in3" }; >>> const struct dmi_system_id *dmi_id; >>> struct byt_rt5640_private *priv; >>> struct snd_soc_acpi_mach *mach; >>> @@ -1163,6 +1165,13 @@ static int snd_byt_rt5640_mc_probe(struct >>> platform_device *pdev) >>> } >>> } >>> + snprintf(byt_rt5640_long_name, sizeof(byt_rt5640_long_name), >>> + "bytcr-rt5640-%s-spk-%s-mic", >>> + (byt_rt5640_quirk & BYT_RT5640_MONO_SPEAKER) ? >>> + "mono" : "stereo", >>> + map_name[BYT_RT5640_MAP(byt_rt5640_quirk)]); >>> + byt_rt5640_card.long_name = byt_rt5640_long_name; >>> + >>> ret_val = devm_snd_soc_register_card(&pdev->dev, >>> &byt_rt5640_card); >>> if (ret_val) { >>> >>
Hi, On 10-05-18 17:00, Pierre-Louis Bossart wrote: > On 5/10/18 5:27 AM, Hans de Goede wrote: >> Hi, >> >> On 08-05-18 20:35, Pierre-Louis Bossart wrote: >>> On 5/8/18 10:36 AM, Hans de Goede wrote: >>>> Many X86 devices using a BYT SoC + RT5640 codec are cheap devices with >>>> generic DMI strings, causing snd_soc_set_dmi_name() to fail to set a >>>> long_name, making it impossible for userspace to have a correct UCM >>>> profile which only uses inputs / outputs which are actually hooked up >>>> on the device. >>>> >>>> Our quirks already specify which input the internal mic is connected to >>>> and if a single (mono) speaker is used or if the device has stereo >>>> speakers. >>>> >>>> This commit sets a long_name based on the quirks so that userspace can >>>> have UCM profiles doing the right thing based on the long_name. >>> >>> Isn't this going to be complicated to manage for UCM? Just with this patch alone, you'd need 8 UCM files to cover all the combinations. 16 if you add the 'sof-' prefix. >>> >>> seems like UCM should become more 'dynamic' and get quirk information somehow (sysfs?) to enable/disable endpoints rather than rely on name encoding to select the right profile? >> >> I agree that this is not ideal, but this is an improvement from the >> current state where we would need 1 UCM profile per board >> (assuming valid DMI data and thus a proper long-name being set), >> 6 profiles (dmic2 is not used anywhere sofar) is a whole lot easier >> to manage then 1 profile per board. So as said I believe this is >> a step in the right direction. >> >> And looking at the foreseeable future I simply don't see any of us >> having the time to implement an ideal solution for this. I would >> really like for end users to be able to run the latest upstream >> kernel + alsa-lib and have things just work, before this hardware >> becomes obsolete. I know that no-one having time to work on reworking >> UCM to make it more dynamic is not the best of arguments but it >> is something to take into consideration. >> >> Thinking more about this on the alsa-lib / UCM profile side we >> could have something like this: >> >> /usr/share/alsa/ucm/bytcr-rt5640-mono-spk-in1-mic/bytcr-rt5640-mono-spk-in1-mic.conf: >> >> SectionUseCase."HiFi" { >> File "../bytcr-rt5640/Generic.conf" >> File "../bytcr-rt5640/MonoSpeaker.conf" >> File "../bytcr-rt5640/In1Mic.conf" >> Comment "Play HiFi quality Music" >> } >> >> SectionDefaults [ >> cdev "hw:bytcrrt5640" >> ] >> >> The only problem I can see with that is that the "ConflictingDevice" >> sections for the various inputs / outputs then would refer to not >> present SectionDevice sections. I have not tested this suggestion yet, >> but I'm willing to write an alsa-lib patch to ignore non present >> ConflictingDevice references, to make my suggestion work. >> >> I think doing things this way, thus avoiding the need to copy and >> paste a whole lot of UCM code for the 6 profiles it will not be >> a problem to maintain 6 profiles, as we're really just maintaining >> 6 config snippets such as the above example and only one complete >> profile. >> >> Would the solution I outlined above be acceptable to you? > > The includes and disabling conflicting devices that aren't present make sense. I have another issue though: for SOF integration I already prepared a set of files, which are mostly identical to the regular ones except that the platform-side mixer controls are removed (or different) and the name of the card/device is different (sof- prefix). See on github. Hmm, it might make sense to split the includes in platform and codec includes, so to pick my example again we would get: /usr/share/alsa/ucm/bytcr-rt5640-mono-spk-in1-mic/bytcr-rt5640-mono-spk-in1-mic.conf: SectionUseCase."HiFi" { SectionVerb { EnableSequence [ cdev "hw:bytcrrt5640" File "../bytcr-rt5640/EnableSeq.conf" # This contains the platform mixer settings File "../rt5640/EnableSeq.conf" ] DisableSequence [ ] Value { PlaybackPCM "hw:bytcrrt5640" CapturePCM "hw:bytcrrt5640" } } File "../rt5640/Headset.conf" File "../rt5640/MonoSpeaker.conf" File "../rt5640/In1Mic.conf" Comment "Play HiFi quality Music" } SectionDefaults [ cdev "hw:bytcrrt5640" ] And then for sof you would just need to offer a sof-rt5640/EnableSeq.conf, or maybe even leave it out completely. And we might also be able to merge the platform enable sequences into a generic: bytcr/EnableSeq.conf I think that will at least fly for bytcr-rt5640 and butcr-rt5651, leading us being able to remove more duplicated UCM config. How does this sound? Regards, Hans > > I was nearly ready to submit all those files to the new repository, I wasn't aware of your work so now need to figure out how to reuse all this once we upstream SOF (any time now if I look at Liam's patches). > >> >> Regards, >> >> Hans >> >> >> >> >> >>>> Note that if we ever encounter the need for a special UCM profile for >>>> some device we can add a quirk to set a specific long_name for the >>>> device, >>>> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> --- >>>> sound/soc/intel/boards/bytcr_rt5640.c | 9 +++++++++ >>>> 1 file changed, 9 insertions(+) >>>> >>>> diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c >>>> index cfc520200214..9a1204dcdbc6 100644 >>>> --- a/sound/soc/intel/boards/bytcr_rt5640.c >>>> +++ b/sound/soc/intel/boards/bytcr_rt5640.c >>>> @@ -929,6 +929,7 @@ static struct snd_soc_dai_link byt_rt5640_dais[] = { >>>> static char byt_rt5640_codec_name[SND_ACPI_I2C_ID_LEN]; >>>> static char byt_rt5640_codec_aif_name[12]; /* = "rt5640-aif[1|2]" */ >>>> static char byt_rt5640_cpu_dai_name[10]; /* = "ssp[0|2]-port" */ >>>> +static char byt_rt5640_long_name[40]; /* = "bytcr-rt5640-*-spk-*-mic" */ >>>> static int byt_rt5640_suspend(struct snd_soc_card *card) >>>> { >>>> @@ -1000,6 +1001,7 @@ struct acpi_chan_package { /* ACPICA seems to require 64 bit integers */ >>>> static int snd_byt_rt5640_mc_probe(struct platform_device *pdev) >>>> { >>>> + const char * const map_name[] = { "dmic1", "dmic2", "in1", "in3" }; >>>> const struct dmi_system_id *dmi_id; >>>> struct byt_rt5640_private *priv; >>>> struct snd_soc_acpi_mach *mach; >>>> @@ -1163,6 +1165,13 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev) >>>> } >>>> } >>>> + snprintf(byt_rt5640_long_name, sizeof(byt_rt5640_long_name), >>>> + "bytcr-rt5640-%s-spk-%s-mic", >>>> + (byt_rt5640_quirk & BYT_RT5640_MONO_SPEAKER) ? >>>> + "mono" : "stereo", >>>> + map_name[BYT_RT5640_MAP(byt_rt5640_quirk)]); >>>> + byt_rt5640_card.long_name = byt_rt5640_long_name; >>>> + >>>> ret_val = devm_snd_soc_register_card(&pdev->dev, &byt_rt5640_card); >>>> if (ret_val) { >>>> >>> >
On 5/10/18 10:48 AM, Hans de Goede wrote: > Hi, > > On 10-05-18 17:00, Pierre-Louis Bossart wrote: >> On 5/10/18 5:27 AM, Hans de Goede wrote: >>> Hi, >>> >>> On 08-05-18 20:35, Pierre-Louis Bossart wrote: >>>> On 5/8/18 10:36 AM, Hans de Goede wrote: >>>>> Many X86 devices using a BYT SoC + RT5640 codec are cheap devices with >>>>> generic DMI strings, causing snd_soc_set_dmi_name() to fail to set a >>>>> long_name, making it impossible for userspace to have a correct UCM >>>>> profile which only uses inputs / outputs which are actually hooked up >>>>> on the device. >>>>> >>>>> Our quirks already specify which input the internal mic is >>>>> connected to >>>>> and if a single (mono) speaker is used or if the device has stereo >>>>> speakers. >>>>> >>>>> This commit sets a long_name based on the quirks so that userspace can >>>>> have UCM profiles doing the right thing based on the long_name. >>>> >>>> Isn't this going to be complicated to manage for UCM? Just with this >>>> patch alone, you'd need 8 UCM files to cover all the combinations. >>>> 16 if you add the 'sof-' prefix. >>>> >>>> seems like UCM should become more 'dynamic' and get quirk >>>> information somehow (sysfs?) to enable/disable endpoints rather than >>>> rely on name encoding to select the right profile? >>> >>> I agree that this is not ideal, but this is an improvement from the >>> current state where we would need 1 UCM profile per board >>> (assuming valid DMI data and thus a proper long-name being set), >>> 6 profiles (dmic2 is not used anywhere sofar) is a whole lot easier >>> to manage then 1 profile per board. So as said I believe this is >>> a step in the right direction. >>> >>> And looking at the foreseeable future I simply don't see any of us >>> having the time to implement an ideal solution for this. I would >>> really like for end users to be able to run the latest upstream >>> kernel + alsa-lib and have things just work, before this hardware >>> becomes obsolete. I know that no-one having time to work on reworking >>> UCM to make it more dynamic is not the best of arguments but it >>> is something to take into consideration. >>> >>> Thinking more about this on the alsa-lib / UCM profile side we >>> could have something like this: >>> >>> /usr/share/alsa/ucm/bytcr-rt5640-mono-spk-in1-mic/bytcr-rt5640-mono-spk-in1-mic.conf: >>> >>> >>> SectionUseCase."HiFi" { >>> File "../bytcr-rt5640/Generic.conf" >>> File "../bytcr-rt5640/MonoSpeaker.conf" >>> File "../bytcr-rt5640/In1Mic.conf" >>> Comment "Play HiFi quality Music" >>> } >>> >>> SectionDefaults [ >>> cdev "hw:bytcrrt5640" >>> ] >>> >>> The only problem I can see with that is that the "ConflictingDevice" >>> sections for the various inputs / outputs then would refer to not >>> present SectionDevice sections. I have not tested this suggestion yet, >>> but I'm willing to write an alsa-lib patch to ignore non present >>> ConflictingDevice references, to make my suggestion work. >>> >>> I think doing things this way, thus avoiding the need to copy and >>> paste a whole lot of UCM code for the 6 profiles it will not be >>> a problem to maintain 6 profiles, as we're really just maintaining >>> 6 config snippets such as the above example and only one complete >>> profile. >>> >>> Would the solution I outlined above be acceptable to you? >> >> The includes and disabling conflicting devices that aren't present >> make sense. I have another issue though: for SOF integration I already >> prepared a set of files, which are mostly identical to the regular >> ones except that the platform-side mixer controls are removed (or >> different) and the name of the card/device is different (sof- prefix). >> See on github. > > Hmm, it might make sense to split the includes in platform and codec > includes, so > to pick my example again we would get: > > /usr/share/alsa/ucm/bytcr-rt5640-mono-spk-in1-mic/bytcr-rt5640-mono-spk-in1-mic.conf: > > > SectionUseCase."HiFi" { > SectionVerb { > EnableSequence [ > cdev "hw:bytcrrt5640" > > File "../bytcr-rt5640/EnableSeq.conf" # This contains the > platform mixer settings > File "../rt5640/EnableSeq.conf" > ] > > DisableSequence [ > ] > > Value { > PlaybackPCM "hw:bytcrrt5640" > CapturePCM "hw:bytcrrt5640" > } > } > > File "../rt5640/Headset.conf" > File "../rt5640/MonoSpeaker.conf" > File "../rt5640/In1Mic.conf" > Comment "Play HiFi quality Music" > } > > SectionDefaults [ > cdev "hw:bytcrrt5640" > ] > > And then for sof you would just need to > offer a sof-rt5640/EnableSeq.conf, or > maybe even leave it out completely. > > And we might also be able to merge the platform > enable sequences into a generic: > > bytcr/EnableSeq.conf > > I think that will at least fly for bytcr-rt5640 and > butcr-rt5651, leading us being able to remove more > duplicated UCM config. > > How does this sound? splitting platform and codec sides is a good idea (and something that was done by removing all platform mixer settings from the HiFi files) the problem remains that we have all these cdev strings that are hard-codec with a card name. Same when the match happens based on a DMI string, how would I know which of the platform settings to apply without querying what the platform driver is? > > Regards, > > Hans > > > > > >> >> I was nearly ready to submit all those files to the new repository, I >> wasn't aware of your work so now need to figure out how to reuse all >> this once we upstream SOF (any time now if I look at Liam's patches). >> >>> >>> Regards, >>> >>> Hans >>> >>> >>> >>> >>> >>>>> Note that if we ever encounter the need for a special UCM profile for >>>>> some device we can add a quirk to set a specific long_name for the >>>>> device, >>>>> >>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>> --- >>>>> sound/soc/intel/boards/bytcr_rt5640.c | 9 +++++++++ >>>>> 1 file changed, 9 insertions(+) >>>>> >>>>> diff --git a/sound/soc/intel/boards/bytcr_rt5640.c >>>>> b/sound/soc/intel/boards/bytcr_rt5640.c >>>>> index cfc520200214..9a1204dcdbc6 100644 >>>>> --- a/sound/soc/intel/boards/bytcr_rt5640.c >>>>> +++ b/sound/soc/intel/boards/bytcr_rt5640.c >>>>> @@ -929,6 +929,7 @@ static struct snd_soc_dai_link >>>>> byt_rt5640_dais[] = { >>>>> static char byt_rt5640_codec_name[SND_ACPI_I2C_ID_LEN]; >>>>> static char byt_rt5640_codec_aif_name[12]; /* = >>>>> "rt5640-aif[1|2]" */ >>>>> static char byt_rt5640_cpu_dai_name[10]; /* = "ssp[0|2]-port" */ >>>>> +static char byt_rt5640_long_name[40]; /* = >>>>> "bytcr-rt5640-*-spk-*-mic" */ >>>>> static int byt_rt5640_suspend(struct snd_soc_card *card) >>>>> { >>>>> @@ -1000,6 +1001,7 @@ struct acpi_chan_package { /* ACPICA seems >>>>> to require 64 bit integers */ >>>>> static int snd_byt_rt5640_mc_probe(struct platform_device *pdev) >>>>> { >>>>> + const char * const map_name[] = { "dmic1", "dmic2", "in1", >>>>> "in3" }; >>>>> const struct dmi_system_id *dmi_id; >>>>> struct byt_rt5640_private *priv; >>>>> struct snd_soc_acpi_mach *mach; >>>>> @@ -1163,6 +1165,13 @@ static int snd_byt_rt5640_mc_probe(struct >>>>> platform_device *pdev) >>>>> } >>>>> } >>>>> + snprintf(byt_rt5640_long_name, sizeof(byt_rt5640_long_name), >>>>> + "bytcr-rt5640-%s-spk-%s-mic", >>>>> + (byt_rt5640_quirk & BYT_RT5640_MONO_SPEAKER) ? >>>>> + "mono" : "stereo", >>>>> + map_name[BYT_RT5640_MAP(byt_rt5640_quirk)]); >>>>> + byt_rt5640_card.long_name = byt_rt5640_long_name; >>>>> + >>>>> ret_val = devm_snd_soc_register_card(&pdev->dev, >>>>> &byt_rt5640_card); >>>>> if (ret_val) { >>>>> >>>> >>
Hi, On 10-05-18 19:46, Pierre-Louis Bossart wrote: > On 5/10/18 10:48 AM, Hans de Goede wrote: >> Hi, >> >> On 10-05-18 17:00, Pierre-Louis Bossart wrote: >>> On 5/10/18 5:27 AM, Hans de Goede wrote: >>>> Hi, >>>> >>>> On 08-05-18 20:35, Pierre-Louis Bossart wrote: >>>>> On 5/8/18 10:36 AM, Hans de Goede wrote: >>>>>> Many X86 devices using a BYT SoC + RT5640 codec are cheap devices with >>>>>> generic DMI strings, causing snd_soc_set_dmi_name() to fail to set a >>>>>> long_name, making it impossible for userspace to have a correct UCM >>>>>> profile which only uses inputs / outputs which are actually hooked up >>>>>> on the device. >>>>>> >>>>>> Our quirks already specify which input the internal mic is connected to >>>>>> and if a single (mono) speaker is used or if the device has stereo >>>>>> speakers. >>>>>> >>>>>> This commit sets a long_name based on the quirks so that userspace can >>>>>> have UCM profiles doing the right thing based on the long_name. >>>>> >>>>> Isn't this going to be complicated to manage for UCM? Just with this patch alone, you'd need 8 UCM files to cover all the combinations. 16 if you add the 'sof-' prefix. >>>>> >>>>> seems like UCM should become more 'dynamic' and get quirk information somehow (sysfs?) to enable/disable endpoints rather than rely on name encoding to select the right profile? >>>> >>>> I agree that this is not ideal, but this is an improvement from the >>>> current state where we would need 1 UCM profile per board >>>> (assuming valid DMI data and thus a proper long-name being set), >>>> 6 profiles (dmic2 is not used anywhere sofar) is a whole lot easier >>>> to manage then 1 profile per board. So as said I believe this is >>>> a step in the right direction. >>>> >>>> And looking at the foreseeable future I simply don't see any of us >>>> having the time to implement an ideal solution for this. I would >>>> really like for end users to be able to run the latest upstream >>>> kernel + alsa-lib and have things just work, before this hardware >>>> becomes obsolete. I know that no-one having time to work on reworking >>>> UCM to make it more dynamic is not the best of arguments but it >>>> is something to take into consideration. >>>> >>>> Thinking more about this on the alsa-lib / UCM profile side we >>>> could have something like this: >>>> >>>> /usr/share/alsa/ucm/bytcr-rt5640-mono-spk-in1-mic/bytcr-rt5640-mono-spk-in1-mic.conf: >>>> >>>> SectionUseCase."HiFi" { >>>> File "../bytcr-rt5640/Generic.conf" >>>> File "../bytcr-rt5640/MonoSpeaker.conf" >>>> File "../bytcr-rt5640/In1Mic.conf" >>>> Comment "Play HiFi quality Music" >>>> } >>>> >>>> SectionDefaults [ >>>> cdev "hw:bytcrrt5640" >>>> ] >>>> >>>> The only problem I can see with that is that the "ConflictingDevice" >>>> sections for the various inputs / outputs then would refer to not >>>> present SectionDevice sections. I have not tested this suggestion yet, >>>> but I'm willing to write an alsa-lib patch to ignore non present >>>> ConflictingDevice references, to make my suggestion work. >>>> >>>> I think doing things this way, thus avoiding the need to copy and >>>> paste a whole lot of UCM code for the 6 profiles it will not be >>>> a problem to maintain 6 profiles, as we're really just maintaining >>>> 6 config snippets such as the above example and only one complete >>>> profile. >>>> >>>> Would the solution I outlined above be acceptable to you? >>> >>> The includes and disabling conflicting devices that aren't present make sense. I have another issue though: for SOF integration I already prepared a set of files, which are mostly identical to the regular ones except that the platform-side mixer controls are removed (or different) and the name of the card/device is different (sof- prefix). See on github. >> >> Hmm, it might make sense to split the includes in platform and codec includes, so >> to pick my example again we would get: >> >> /usr/share/alsa/ucm/bytcr-rt5640-mono-spk-in1-mic/bytcr-rt5640-mono-spk-in1-mic.conf: >> >> SectionUseCase."HiFi" { >> SectionVerb { >> EnableSequence [ >> cdev "hw:bytcrrt5640" >> >> File "../bytcr-rt5640/EnableSeq.conf" # This contains the platform mixer settings >> File "../rt5640/EnableSeq.conf" >> ] >> >> DisableSequence [ >> ] >> >> Value { >> PlaybackPCM "hw:bytcrrt5640" >> CapturePCM "hw:bytcrrt5640" >> } >> } >> >> File "../rt5640/Headset.conf" >> File "../rt5640/MonoSpeaker.conf" >> File "../rt5640/In1Mic.conf" >> Comment "Play HiFi quality Music" >> } >> >> SectionDefaults [ >> cdev "hw:bytcrrt5640" >> ] >> >> And then for sof you would just need to >> offer a sof-rt5640/EnableSeq.conf, or >> maybe even leave it out completely. >> >> And we might also be able to merge the platform >> enable sequences into a generic: >> >> bytcr/EnableSeq.conf >> >> I think that will at least fly for bytcr-rt5640 and >> butcr-rt5651, leading us being able to remove more >> duplicated UCM config. >> >> How does this sound? > > splitting platform and codec sides is a good idea (and something that was done by removing all platform mixer settings from the HiFi files) > > the problem remains that we have all these cdev strings that are hard-codec with a card name. Same when the match happens based on a DMI string, how would I know which of the platform settings to apply without querying what the platform driver is? Well the DMI string would uniquely identify a certain model device, when we write the UCM file we should know what the platform + codec for that device is and we can simply hardcode them, like in my example above. But maybe I'm misunderstanding you? Regards, Hans >>>>>> --- >>>>>> sound/soc/intel/boards/bytcr_rt5640.c | 9 +++++++++ >>>>>> 1 file changed, 9 insertions(+) >>>>>> >>>>>> diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c >>>>>> index cfc520200214..9a1204dcdbc6 100644 >>>>>> --- a/sound/soc/intel/boards/bytcr_rt5640.c >>>>>> +++ b/sound/soc/intel/boards/bytcr_rt5640.c >>>>>> @@ -929,6 +929,7 @@ static struct snd_soc_dai_link byt_rt5640_dais[] = { >>>>>> static char byt_rt5640_codec_name[SND_ACPI_I2C_ID_LEN]; >>>>>> static char byt_rt5640_codec_aif_name[12]; /* = "rt5640-aif[1|2]" */ >>>>>> static char byt_rt5640_cpu_dai_name[10]; /* = "ssp[0|2]-port" */ >>>>>> +static char byt_rt5640_long_name[40]; /* = "bytcr-rt5640-*-spk-*-mic" */ >>>>>> static int byt_rt5640_suspend(struct snd_soc_card *card) >>>>>> { >>>>>> @@ -1000,6 +1001,7 @@ struct acpi_chan_package { /* ACPICA seems to require 64 bit integers */ >>>>>> static int snd_byt_rt5640_mc_probe(struct platform_device *pdev) >>>>>> { >>>>>> + const char * const map_name[] = { "dmic1", "dmic2", "in1", "in3" }; >>>>>> const struct dmi_system_id *dmi_id; >>>>>> struct byt_rt5640_private *priv; >>>>>> struct snd_soc_acpi_mach *mach; >>>>>> @@ -1163,6 +1165,13 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev) >>>>>> } >>>>>> } >>>>>> + snprintf(byt_rt5640_long_name, sizeof(byt_rt5640_long_name), >>>>>> + "bytcr-rt5640-%s-spk-%s-mic", >>>>>> + (byt_rt5640_quirk & BYT_RT5640_MONO_SPEAKER) ? >>>>>> + "mono" : "stereo", >>>>>> + map_name[BYT_RT5640_MAP(byt_rt5640_quirk)]); >>>>>> + byt_rt5640_card.long_name = byt_rt5640_long_name; >>>>>> + >>>>>> ret_val = devm_snd_soc_register_card(&pdev->dev, &byt_rt5640_card); >>>>>> if (ret_val) { >>>>>> >>>>> >>> >
On 05/10/2018 01:01 PM, Hans de Goede wrote: > Hi, > > On 10-05-18 19:46, Pierre-Louis Bossart wrote: >> On 5/10/18 10:48 AM, Hans de Goede wrote: >>> Hi, >>> >>> On 10-05-18 17:00, Pierre-Louis Bossart wrote: >>>> On 5/10/18 5:27 AM, Hans de Goede wrote: >>>>> Hi, >>>>> >>>>> On 08-05-18 20:35, Pierre-Louis Bossart wrote: >>>>>> On 5/8/18 10:36 AM, Hans de Goede wrote: >>>>>>> Many X86 devices using a BYT SoC + RT5640 codec are cheap >>>>>>> devices with >>>>>>> generic DMI strings, causing snd_soc_set_dmi_name() to fail to >>>>>>> set a >>>>>>> long_name, making it impossible for userspace to have a correct UCM >>>>>>> profile which only uses inputs / outputs which are actually >>>>>>> hooked up >>>>>>> on the device. >>>>>>> >>>>>>> Our quirks already specify which input the internal mic is >>>>>>> connected to >>>>>>> and if a single (mono) speaker is used or if the device has stereo >>>>>>> speakers. >>>>>>> >>>>>>> This commit sets a long_name based on the quirks so that >>>>>>> userspace can >>>>>>> have UCM profiles doing the right thing based on the long_name. >>>>>> >>>>>> Isn't this going to be complicated to manage for UCM? Just with >>>>>> this patch alone, you'd need 8 UCM files to cover all the >>>>>> combinations. 16 if you add the 'sof-' prefix. >>>>>> >>>>>> seems like UCM should become more 'dynamic' and get quirk >>>>>> information somehow (sysfs?) to enable/disable endpoints rather >>>>>> than rely on name encoding to select the right profile? >>>>> >>>>> I agree that this is not ideal, but this is an improvement from the >>>>> current state where we would need 1 UCM profile per board >>>>> (assuming valid DMI data and thus a proper long-name being set), >>>>> 6 profiles (dmic2 is not used anywhere sofar) is a whole lot easier >>>>> to manage then 1 profile per board. So as said I believe this is >>>>> a step in the right direction. >>>>> >>>>> And looking at the foreseeable future I simply don't see any of us >>>>> having the time to implement an ideal solution for this. I would >>>>> really like for end users to be able to run the latest upstream >>>>> kernel + alsa-lib and have things just work, before this hardware >>>>> becomes obsolete. I know that no-one having time to work on reworking >>>>> UCM to make it more dynamic is not the best of arguments but it >>>>> is something to take into consideration. >>>>> >>>>> Thinking more about this on the alsa-lib / UCM profile side we >>>>> could have something like this: >>>>> >>>>> /usr/share/alsa/ucm/bytcr-rt5640-mono-spk-in1-mic/bytcr-rt5640-mono-spk-in1-mic.conf: >>>>> >>>>> >>>>> SectionUseCase."HiFi" { >>>>> File "../bytcr-rt5640/Generic.conf" >>>>> File "../bytcr-rt5640/MonoSpeaker.conf" >>>>> File "../bytcr-rt5640/In1Mic.conf" >>>>> Comment "Play HiFi quality Music" >>>>> } >>>>> >>>>> SectionDefaults [ >>>>> cdev "hw:bytcrrt5640" >>>>> ] >>>>> >>>>> The only problem I can see with that is that the "ConflictingDevice" >>>>> sections for the various inputs / outputs then would refer to not >>>>> present SectionDevice sections. I have not tested this suggestion >>>>> yet, >>>>> but I'm willing to write an alsa-lib patch to ignore non present >>>>> ConflictingDevice references, to make my suggestion work. >>>>> >>>>> I think doing things this way, thus avoiding the need to copy and >>>>> paste a whole lot of UCM code for the 6 profiles it will not be >>>>> a problem to maintain 6 profiles, as we're really just maintaining >>>>> 6 config snippets such as the above example and only one complete >>>>> profile. >>>>> >>>>> Would the solution I outlined above be acceptable to you? >>>> >>>> The includes and disabling conflicting devices that aren't present >>>> make sense. I have another issue though: for SOF integration I >>>> already prepared a set of files, which are mostly identical to the >>>> regular ones except that the platform-side mixer controls are >>>> removed (or different) and the name of the card/device is different >>>> (sof- prefix). See on github. >>> >>> Hmm, it might make sense to split the includes in platform and codec >>> includes, so >>> to pick my example again we would get: >>> >>> /usr/share/alsa/ucm/bytcr-rt5640-mono-spk-in1-mic/bytcr-rt5640-mono-spk-in1-mic.conf: >>> >>> >>> SectionUseCase."HiFi" { >>> SectionVerb { >>> EnableSequence [ >>> cdev "hw:bytcrrt5640" >>> >>> File "../bytcr-rt5640/EnableSeq.conf" # This contains >>> the platform mixer settings >>> File "../rt5640/EnableSeq.conf" >>> ] >>> >>> DisableSequence [ >>> ] >>> >>> Value { >>> PlaybackPCM "hw:bytcrrt5640" >>> CapturePCM "hw:bytcrrt5640" >>> } >>> } >>> >>> File "../rt5640/Headset.conf" >>> File "../rt5640/MonoSpeaker.conf" >>> File "../rt5640/In1Mic.conf" >>> Comment "Play HiFi quality Music" >>> } >>> >>> SectionDefaults [ >>> cdev "hw:bytcrrt5640" >>> ] >>> >>> And then for sof you would just need to >>> offer a sof-rt5640/EnableSeq.conf, or >>> maybe even leave it out completely. >>> >>> And we might also be able to merge the platform >>> enable sequences into a generic: >>> >>> bytcr/EnableSeq.conf >>> >>> I think that will at least fly for bytcr-rt5640 and >>> butcr-rt5651, leading us being able to remove more >>> duplicated UCM config. >>> >>> How does this sound? >> >> splitting platform and codec sides is a good idea (and something that >> was done by removing all platform mixer settings from the HiFi files) >> >> the problem remains that we have all these cdev strings that are >> hard-codec with a card name. Same when the match happens based on a >> DMI string, how would I know which of the platform settings to apply >> without querying what the platform driver is? > > Well the DMI string would uniquely identify a certain model device, > when we write the UCM file we should know what the platform + codec > for that device is and we can simply hardcode them, like in > my example above. > > But maybe I'm misunderstanding you? For the same DMI device, you could either enable the existing intel/sst or SOF drivers. How would we handle UCM configs then? The DMI name would need to be augmented with a prefix, or we use *something* to add the references to SOF in the platform include and ALSA device string.
On Sat, 12 May 2018 23:21:58 +0200, Pierre-Louis Bossart wrote: > > >> > >> splitting platform and codec sides is a good idea (and something > >> that was done by removing all platform mixer settings from the HiFi > >> files) > >> > >> the problem remains that we have all these cdev strings that are > >> hard-codec with a card name. Same when the match happens based on a > >> DMI string, how would I know which of the platform settings to > >> apply without querying what the platform driver is? > > > > Well the DMI string would uniquely identify a certain model device, > > when we write the UCM file we should know what the platform + codec > > for that device is and we can simply hardcode them, like in > > my example above. > > > > But maybe I'm misunderstanding you? > > For the same DMI device, you could either enable the existing > intel/sst or SOF drivers. How would we handle UCM configs then? The > DMI name would need to be augmented with a prefix, or we use > *something* to add the references to SOF in the platform include and > ALSA device string. You've already added the flavor suffix to snd_soc_set_dmi_name(), so they'll be unique, no? Takashi
Hi, On 05/13/2018 09:11 AM, Takashi Iwai wrote: > On Sat, 12 May 2018 23:21:58 +0200, > Pierre-Louis Bossart wrote: >> >>>> >>>> splitting platform and codec sides is a good idea (and something >>>> that was done by removing all platform mixer settings from the HiFi >>>> files) >>>> >>>> the problem remains that we have all these cdev strings that are >>>> hard-codec with a card name. Same when the match happens based on a >>>> DMI string, how would I know which of the platform settings to >>>> apply without querying what the platform driver is? >>> >>> Well the DMI string would uniquely identify a certain model device, >>> when we write the UCM file we should know what the platform + codec >>> for that device is and we can simply hardcode them, like in >>> my example above. >>> >>> But maybe I'm misunderstanding you? >> >> For the same DMI device, you could either enable the existing >> intel/sst or SOF drivers. How would we handle UCM configs then? The >> DMI name would need to be augmented with a prefix, or we use >> *something* to add the references to SOF in the platform include and >> ALSA device string. > > You've already added the flavor suffix to snd_soc_set_dmi_name(), so > they'll be unique, no? Yes I was going to suggest doing something like that, but if we already have that even better. Note that the patch setting the longname based on the quirks overrides (pre-empts really) snd_soc_set_dmi_name() and does include bytcr_rt5640 prefix, so this patch should make sure that there is no name conflict between the sof and bytcr machine drivers UCM profiles. As mentioned in the commit message this pre-emption means we loose the ability to have an UCM profile optimized for a specific device, but we can work around this if necessary with a flag which sets to not do set the quirk based long-name on some models (if the need ever arises for this). Regards, Hans
Hi Pierre-Louis, On 10-05-18 17:48, Hans de Goede wrote: > Hi, > > On 10-05-18 17:00, Pierre-Louis Bossart wrote: >> On 5/10/18 5:27 AM, Hans de Goede wrote: >>> Hi, >>> >>> On 08-05-18 20:35, Pierre-Louis Bossart wrote: >>>> On 5/8/18 10:36 AM, Hans de Goede wrote: >>>>> Many X86 devices using a BYT SoC + RT5640 codec are cheap devices with >>>>> generic DMI strings, causing snd_soc_set_dmi_name() to fail to set a >>>>> long_name, making it impossible for userspace to have a correct UCM >>>>> profile which only uses inputs / outputs which are actually hooked up >>>>> on the device. >>>>> >>>>> Our quirks already specify which input the internal mic is connected to >>>>> and if a single (mono) speaker is used or if the device has stereo >>>>> speakers. >>>>> >>>>> This commit sets a long_name based on the quirks so that userspace can >>>>> have UCM profiles doing the right thing based on the long_name. >>>> >>>> Isn't this going to be complicated to manage for UCM? Just with this patch alone, you'd need 8 UCM files to cover all the combinations. 16 if you add the 'sof-' prefix. >>>> >>>> seems like UCM should become more 'dynamic' and get quirk information somehow (sysfs?) to enable/disable endpoints rather than rely on name encoding to select the right profile? >>> >>> I agree that this is not ideal, but this is an improvement from the >>> current state where we would need 1 UCM profile per board >>> (assuming valid DMI data and thus a proper long-name being set), >>> 6 profiles (dmic2 is not used anywhere sofar) is a whole lot easier >>> to manage then 1 profile per board. So as said I believe this is >>> a step in the right direction. >>> >>> And looking at the foreseeable future I simply don't see any of us >>> having the time to implement an ideal solution for this. I would >>> really like for end users to be able to run the latest upstream >>> kernel + alsa-lib and have things just work, before this hardware >>> becomes obsolete. I know that no-one having time to work on reworking >>> UCM to make it more dynamic is not the best of arguments but it >>> is something to take into consideration. >>> >>> Thinking more about this on the alsa-lib / UCM profile side we >>> could have something like this: >>> >>> /usr/share/alsa/ucm/bytcr-rt5640-mono-spk-in1-mic/bytcr-rt5640-mono-spk-in1-mic.conf: >>> >>> SectionUseCase."HiFi" { >>> File "../bytcr-rt5640/Generic.conf" >>> File "../bytcr-rt5640/MonoSpeaker.conf" >>> File "../bytcr-rt5640/In1Mic.conf" >>> Comment "Play HiFi quality Music" >>> } >>> >>> SectionDefaults [ >>> cdev "hw:bytcrrt5640" >>> ] >>> >>> The only problem I can see with that is that the "ConflictingDevice" >>> sections for the various inputs / outputs then would refer to not >>> present SectionDevice sections. I have not tested this suggestion yet, >>> but I'm willing to write an alsa-lib patch to ignore non present >>> ConflictingDevice references, to make my suggestion work. >>> >>> I think doing things this way, thus avoiding the need to copy and >>> paste a whole lot of UCM code for the 6 profiles it will not be >>> a problem to maintain 6 profiles, as we're really just maintaining >>> 6 config snippets such as the above example and only one complete >>> profile. >>> >>> Would the solution I outlined above be acceptable to you? >> >> The includes and disabling conflicting devices that aren't present make sense. I have another issue though: for SOF integration I already prepared a set of files, which are mostly identical to the regular ones except that the platform-side mixer controls are removed (or different) and the name of the card/device is different (sof- prefix). See on github. > > Hmm, it might make sense to split the includes in platform and codec includes, so > to pick my example again we would get: > > /usr/share/alsa/ucm/bytcr-rt5640-mono-spk-in1-mic/bytcr-rt5640-mono-spk-in1-mic.conf: > > SectionUseCase."HiFi" { > SectionVerb { > EnableSequence [ > cdev "hw:bytcrrt5640" > > File "../bytcr-rt5640/EnableSeq.conf" # This contains the platform mixer settings > File "../rt5640/EnableSeq.conf" > ] > > DisableSequence [ > ] > > Value { > PlaybackPCM "hw:bytcrrt5640" > CapturePCM "hw:bytcrrt5640" > } > } > > File "../rt5640/Headset.conf" > File "../rt5640/MonoSpeaker.conf" > File "../rt5640/In1Mic.conf" > Comment "Play HiFi quality Music" > } > > SectionDefaults [ > cdev "hw:bytcrrt5640" > ] > > And then for sof you would just need to > offer a sof-rt5640/EnableSeq.conf, or > maybe even leave it out completely. > > And we might also be able to merge the platform > enable sequences into a generic: > > bytcr/EnableSeq.conf > > I think that will at least fly for bytcr-rt5640 and > butcr-rt5651, leading us being able to remove more > duplicated UCM config. > > How does this sound? I've implemented the above scheme, see: https://github.com/jwrdegoede/alsa-lib/commits/master This seems to work well (I still need to test a bit more, but so far the generic and one long-name based profile work fine) and Mark has merged the "ASoC: Intel: bytcr_rt5640: Set card long_name based on quirks" patch in his for-next branch, so I plan to submit the matching alsa-lib patches from my github for upstream alsa-lib inclusion soon. Regards, Hans
On 5/18/18 10:55 AM, Hans de Goede wrote: > Hi Pierre-Louis, > > On 10-05-18 17:48, Hans de Goede wrote: >> Hi, >> >> On 10-05-18 17:00, Pierre-Louis Bossart wrote: >>> On 5/10/18 5:27 AM, Hans de Goede wrote: >>>> Hi, >>>> >>>> On 08-05-18 20:35, Pierre-Louis Bossart wrote: >>>>> On 5/8/18 10:36 AM, Hans de Goede wrote: >>>>>> Many X86 devices using a BYT SoC + RT5640 codec are cheap devices >>>>>> with >>>>>> generic DMI strings, causing snd_soc_set_dmi_name() to fail to set a >>>>>> long_name, making it impossible for userspace to have a correct UCM >>>>>> profile which only uses inputs / outputs which are actually hooked up >>>>>> on the device. >>>>>> >>>>>> Our quirks already specify which input the internal mic is >>>>>> connected to >>>>>> and if a single (mono) speaker is used or if the device has stereo >>>>>> speakers. >>>>>> >>>>>> This commit sets a long_name based on the quirks so that userspace >>>>>> can >>>>>> have UCM profiles doing the right thing based on the long_name. >>>>> >>>>> Isn't this going to be complicated to manage for UCM? Just with >>>>> this patch alone, you'd need 8 UCM files to cover all the >>>>> combinations. 16 if you add the 'sof-' prefix. >>>>> >>>>> seems like UCM should become more 'dynamic' and get quirk >>>>> information somehow (sysfs?) to enable/disable endpoints rather >>>>> than rely on name encoding to select the right profile? >>>> >>>> I agree that this is not ideal, but this is an improvement from the >>>> current state where we would need 1 UCM profile per board >>>> (assuming valid DMI data and thus a proper long-name being set), >>>> 6 profiles (dmic2 is not used anywhere sofar) is a whole lot easier >>>> to manage then 1 profile per board. So as said I believe this is >>>> a step in the right direction. >>>> >>>> And looking at the foreseeable future I simply don't see any of us >>>> having the time to implement an ideal solution for this. I would >>>> really like for end users to be able to run the latest upstream >>>> kernel + alsa-lib and have things just work, before this hardware >>>> becomes obsolete. I know that no-one having time to work on reworking >>>> UCM to make it more dynamic is not the best of arguments but it >>>> is something to take into consideration. >>>> >>>> Thinking more about this on the alsa-lib / UCM profile side we >>>> could have something like this: >>>> >>>> /usr/share/alsa/ucm/bytcr-rt5640-mono-spk-in1-mic/bytcr-rt5640-mono-spk-in1-mic.conf: >>>> >>>> >>>> SectionUseCase."HiFi" { >>>> File "../bytcr-rt5640/Generic.conf" >>>> File "../bytcr-rt5640/MonoSpeaker.conf" >>>> File "../bytcr-rt5640/In1Mic.conf" >>>> Comment "Play HiFi quality Music" >>>> } >>>> >>>> SectionDefaults [ >>>> cdev "hw:bytcrrt5640" >>>> ] >>>> >>>> The only problem I can see with that is that the "ConflictingDevice" >>>> sections for the various inputs / outputs then would refer to not >>>> present SectionDevice sections. I have not tested this suggestion yet, >>>> but I'm willing to write an alsa-lib patch to ignore non present >>>> ConflictingDevice references, to make my suggestion work. >>>> >>>> I think doing things this way, thus avoiding the need to copy and >>>> paste a whole lot of UCM code for the 6 profiles it will not be >>>> a problem to maintain 6 profiles, as we're really just maintaining >>>> 6 config snippets such as the above example and only one complete >>>> profile. >>>> >>>> Would the solution I outlined above be acceptable to you? >>> >>> The includes and disabling conflicting devices that aren't present >>> make sense. I have another issue though: for SOF integration I >>> already prepared a set of files, which are mostly identical to the >>> regular ones except that the platform-side mixer controls are removed >>> (or different) and the name of the card/device is different (sof- >>> prefix). See on github. >> >> Hmm, it might make sense to split the includes in platform and codec >> includes, so >> to pick my example again we would get: >> >> /usr/share/alsa/ucm/bytcr-rt5640-mono-spk-in1-mic/bytcr-rt5640-mono-spk-in1-mic.conf: >> >> >> SectionUseCase."HiFi" { >> SectionVerb { >> EnableSequence [ >> cdev "hw:bytcrrt5640" >> >> File "../bytcr-rt5640/EnableSeq.conf" # This contains >> the platform mixer settings >> File "../rt5640/EnableSeq.conf" >> ] >> >> DisableSequence [ >> ] >> >> Value { >> PlaybackPCM "hw:bytcrrt5640" >> CapturePCM "hw:bytcrrt5640" >> } >> } >> >> File "../rt5640/Headset.conf" >> File "../rt5640/MonoSpeaker.conf" >> File "../rt5640/In1Mic.conf" >> Comment "Play HiFi quality Music" >> } >> >> SectionDefaults [ >> cdev "hw:bytcrrt5640" >> ] >> >> And then for sof you would just need to >> offer a sof-rt5640/EnableSeq.conf, or >> maybe even leave it out completely. >> >> And we might also be able to merge the platform >> enable sequences into a generic: >> >> bytcr/EnableSeq.conf >> >> I think that will at least fly for bytcr-rt5640 and >> butcr-rt5651, leading us being able to remove more >> duplicated UCM config. > >> How does this sound? > > I've implemented the above scheme, see: > https://github.com/jwrdegoede/alsa-lib/commits/master > > This seems to work well (I still need to test a > bit more, but so far the generic and one long-name > based profile work fine) and Mark has merged the > "ASoC: Intel: bytcr_rt5640: Set card long_name based on quirks" > patch in his for-next branch, so I plan to submit > the matching alsa-lib patches from my github for > upstream alsa-lib inclusion soon. This looks pretty good, thanks! The part that is still problematic is the cdev "hw:bytcrrt5640" that is pretty much everywhere. I don't know if it's useful to be repeated in all files. If there was a way to assign it at a higher level or to override it when SOF is present it'd solve most of the issues I see. Please go ahead and submit these patches, I am not going to have iso-functionality for rt5640 and 51 in the next weeks with SOF - I don't have a clean way to deal with SSP0/SSP2 automatic configuration at the firmware level for now, so don't want to stop your work because I have something on the back burner.
diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c index cfc520200214..9a1204dcdbc6 100644 --- a/sound/soc/intel/boards/bytcr_rt5640.c +++ b/sound/soc/intel/boards/bytcr_rt5640.c @@ -929,6 +929,7 @@ static struct snd_soc_dai_link byt_rt5640_dais[] = { static char byt_rt5640_codec_name[SND_ACPI_I2C_ID_LEN]; static char byt_rt5640_codec_aif_name[12]; /* = "rt5640-aif[1|2]" */ static char byt_rt5640_cpu_dai_name[10]; /* = "ssp[0|2]-port" */ +static char byt_rt5640_long_name[40]; /* = "bytcr-rt5640-*-spk-*-mic" */ static int byt_rt5640_suspend(struct snd_soc_card *card) { @@ -1000,6 +1001,7 @@ struct acpi_chan_package { /* ACPICA seems to require 64 bit integers */ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev) { + const char * const map_name[] = { "dmic1", "dmic2", "in1", "in3" }; const struct dmi_system_id *dmi_id; struct byt_rt5640_private *priv; struct snd_soc_acpi_mach *mach; @@ -1163,6 +1165,13 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev) } } + snprintf(byt_rt5640_long_name, sizeof(byt_rt5640_long_name), + "bytcr-rt5640-%s-spk-%s-mic", + (byt_rt5640_quirk & BYT_RT5640_MONO_SPEAKER) ? + "mono" : "stereo", + map_name[BYT_RT5640_MAP(byt_rt5640_quirk)]); + byt_rt5640_card.long_name = byt_rt5640_long_name; + ret_val = devm_snd_soc_register_card(&pdev->dev, &byt_rt5640_card); if (ret_val) {
Many X86 devices using a BYT SoC + RT5640 codec are cheap devices with generic DMI strings, causing snd_soc_set_dmi_name() to fail to set a long_name, making it impossible for userspace to have a correct UCM profile which only uses inputs / outputs which are actually hooked up on the device. Our quirks already specify which input the internal mic is connected to and if a single (mono) speaker is used or if the device has stereo speakers. This commit sets a long_name based on the quirks so that userspace can have UCM profiles doing the right thing based on the long_name. Note that if we ever encounter the need for a special UCM profile for some device we can add a quirk to set a specific long_name for the device, Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- sound/soc/intel/boards/bytcr_rt5640.c | 9 +++++++++ 1 file changed, 9 insertions(+)