| Message ID | 20210215142419.308651-3-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Mon, Feb 15, 2021 at 03:24:19PM +0100, Hans de Goede wrote: > Add support for controlling a speaker and/or microphone mute LED through > LED triggers using the new generic LED trigger module. > - SOC_DOUBLE_EXT("DAC1 Playback Switch", SND_SOC_NOPM, 0, 1, 1, 0, > + SOC_DOUBLE_EXT_ACCESS("DAC1 Playback Switch", SND_SOC_NOPM, 0, 1, 1, 0, > + SNDRV_CTL_ELEM_ACCESS_SPK_LED, > rt5670_dac1_playback_switch_get, rt5670_dac1_playback_switch_put), > - SOC_DOUBLE("ADC Capture Switch", RT5670_STO1_ADC_DIG_VOL, > - RT5670_L_MUTE_SFT, RT5670_R_MUTE_SFT, 1, 1), > + SOC_DOUBLE_ACCESS("ADC Capture Switch", RT5670_STO1_ADC_DIG_VOL, > + RT5670_L_MUTE_SFT, RT5670_R_MUTE_SFT, 1, 1, > + SNDRV_CTL_ELEM_ACCESS_MIC_LED), Why just these particular controls - what if a system has separate mutes for speakers or something?
Hi, On 2/23/21 2:45 PM, Mark Brown wrote: > On Mon, Feb 15, 2021 at 03:24:19PM +0100, Hans de Goede wrote: > >> Add support for controlling a speaker and/or microphone mute LED through >> LED triggers using the new generic LED trigger module. > >> - SOC_DOUBLE_EXT("DAC1 Playback Switch", SND_SOC_NOPM, 0, 1, 1, 0, >> + SOC_DOUBLE_EXT_ACCESS("DAC1 Playback Switch", SND_SOC_NOPM, 0, 1, 1, 0, >> + SNDRV_CTL_ELEM_ACCESS_SPK_LED, >> rt5670_dac1_playback_switch_get, rt5670_dac1_playback_switch_put), > >> - SOC_DOUBLE("ADC Capture Switch", RT5670_STO1_ADC_DIG_VOL, >> - RT5670_L_MUTE_SFT, RT5670_R_MUTE_SFT, 1, 1), >> + SOC_DOUBLE_ACCESS("ADC Capture Switch", RT5670_STO1_ADC_DIG_VOL, >> + RT5670_L_MUTE_SFT, RT5670_R_MUTE_SFT, 1, 1, >> + SNDRV_CTL_ELEM_ACCESS_MIC_LED), > > Why just these particular controls - what if a system has separate mutes > for speakers or something? These are the main volume controls, which are always in the output / input path independent on if we are outputting to e.g. speakers or the headphones. We want to use the main volume control for this, because there always is only 1 output mute LED and 1 input mute LED. Well at least that is the assumption the current ledtrig-audio.c code has. The idea is to only turn the single LED on if we are sure there will be not sound output on any of the outputs, which is why we tie the LED to the mute switch on the main volume control. Regards, Hans
Dne 23. 02. 21 v 15:21 Takashi Iwai napsal(a): > On Tue, 23 Feb 2021 15:09:30 +0100, > Mark Brown wrote: >> >> On Tue, Feb 23, 2021 at 02:59:17PM +0100, Hans de Goede wrote: >>> On 2/23/21 2:45 PM, Mark Brown wrote: >>>> On Mon, Feb 15, 2021 at 03:24:19PM +0100, Hans de Goede wrote: >> >>>> Why just these particular controls - what if a system has separate mutes >>>> for speakers or something? >> >>> These are the main volume controls, which are always in the output / input >>> path independent on if we are outputting to e.g. speakers or the headphones. >> >>> We want to use the main volume control for this, because there always is >>> only 1 output mute LED and 1 input mute LED. Well at least that is the assumption >>> the current ledtrig-audio.c code has. >> >>> The idea is to only turn the single LED on if we are sure there will be not >>> sound output on any of the outputs, which is why we tie the LED to the >>> mute switch on the main volume control. >> >> Right, so that might work well on your particular system with your >> particular configuration but will it work well on other systems with >> different hardware? It's not clear to me that it makes sense to go >> through all the drivers picking controls that might be used for this >> purpose - it seems both time consuming and error prone. Consider a >> mostly digital device which has an ADC/DAC per input/output rather than >> a central ADC/DAC with analogue muxing for example, or a system with >> multiple DACs available for mixing together or analogue bypassess. > > That's one of my concerns in the recent actions for putting the > hard-coded mute LED controls. So far, the only usage of led-audio > trigger is HD-audio, and it's enabled only for selected devices and > setups. OTOH, if we apply the audio-led trigger generically in ASoC > codec driver, it's always done and might misfit; e.g. what happens if > two codecs are present on the system?. That's the abstraction issue. We can use PCI, ACPI, DMI or DT checks at the _right_ place (machine top-level code) to mark those controls with the LED flags in the kernel space. I've never said that the right place is the generic ASoC codec driver. On the other hand, rt5670 is really multi-purpose codec like HDA codecs with the recommended usage/routing from the designer, so it might make sense to add the default LED marking as we do for HDA. > Of course, this implementation would make the integration much easier, > and that's a big benefit. So I have a mixed feeling and not decided > yet whether we should go for it right now... I think that we can reconsider the LED handling implementation later, when someone brings something better on the table. Jaroslav
Hi, On 2/23/21 5:14 PM, Jaroslav Kysela wrote: > Dne 23. 02. 21 v 15:21 Takashi Iwai napsal(a): >> On Tue, 23 Feb 2021 15:09:30 +0100, >> Mark Brown wrote: >>> >>> On Tue, Feb 23, 2021 at 02:59:17PM +0100, Hans de Goede wrote: >>>> On 2/23/21 2:45 PM, Mark Brown wrote: >>>>> On Mon, Feb 15, 2021 at 03:24:19PM +0100, Hans de Goede wrote: >>> >>>>> Why just these particular controls - what if a system has separate mutes >>>>> for speakers or something? >>> >>>> These are the main volume controls, which are always in the output / input >>>> path independent on if we are outputting to e.g. speakers or the headphones. >>> >>>> We want to use the main volume control for this, because there always is >>>> only 1 output mute LED and 1 input mute LED. Well at least that is the assumption >>>> the current ledtrig-audio.c code has. >>> >>>> The idea is to only turn the single LED on if we are sure there will be not >>>> sound output on any of the outputs, which is why we tie the LED to the >>>> mute switch on the main volume control. >>> >>> Right, so that might work well on your particular system with your >>> particular configuration but will it work well on other systems with >>> different hardware? It's not clear to me that it makes sense to go >>> through all the drivers picking controls that might be used for this >>> purpose - it seems both time consuming and error prone. Consider a >>> mostly digital device which has an ADC/DAC per input/output rather than >>> a central ADC/DAC with analogue muxing for example, or a system with >>> multiple DACs available for mixing together or analogue bypassess. >> >> That's one of my concerns in the recent actions for putting the >> hard-coded mute LED controls. So far, the only usage of led-audio >> trigger is HD-audio, and it's enabled only for selected devices and >> setups. OTOH, if we apply the audio-led trigger generically in ASoC >> codec driver, it's always done and might misfit; e.g. what happens if >> two codecs are present on the system?. > > That's the abstraction issue. We can use PCI, ACPI, DMI or DT checks at the > _right_ place (machine top-level code) to mark those controls with the LED > flags in the kernel space. I've never said that the right place is the generic > ASoC codec driver. FWIW I'm more then happy to rework this RFC series to make the setting of the SNDRV_CTL_ELEM_ACCESS_*_LED flag only happen on certain models based on the existing DMI quirks mechanism in the driver. So far I'm aware of only a few 2-in-1 models which actually have a mute LED embedded in the (mic)mute key in their detachable keyboard. Please let me know if you would prefer for me to rework my series to handle the setting of the flag on a per model (DMI match) basis. I see 2 additional advantages of this approach: 1. It would allow us to not modprobe the snd-ctl-led module on devices where we don't need it. 2. The rt5640 codec has 2 I2S interfaces / 2 AIFs and that brings some challenges with it. I currently have a working implementation (which needs some more testing before posting it upstream as RFC) which sets the SNDRV_CTL_ELEM_ACCESS_SPK_LED flag on the main volume control for both the AIF1 and AIF2 paths to the outputs (which have separate main volume controls) and then relies on userspace to always mute the main volume of the unused AIF. This requires the UCM profile to know which AIF is being used, if we do this on a per model basis then we will know which AIF is used and we can set the SNDRV_CTL_ELEM_ACCESS_SPK_LED flag on only one of the 2 main volume-controls, removing the requirement for userspace to mute the unused main volume-control. ### Note that userspace will still need to know which AIF is being used, regardless of the mute-LED discussion, because it needs to know to be able select the right volume-control for hardware volume control (which is something which we really want in general, esp. for the analog mic inputs and is necessary for the mute-LED to work). Since userspace needs to know which AIF is being used, I've written a kernel patch modifying the machine-driver to add a new "aif:1" or "aif:2" string to the components string. I have this all working now, and I will post a RFC upstream once I've run some final tests, for the UCM changes for this, which illustrate the need for userspace to know which AIF is used, see: https://github.com/jwrdegoede/alsa-ucm-conf/commits/master And specifically: https://github.com/jwrdegoede/alsa-ucm-conf/commit/afce963f725a5659eb903508d924d3b19110244a https://github.com/jwrdegoede/alsa-ucm-conf/commit/d2128e5380d9d6128dda67dd94f64bf7244cd3a3 Regards, Hans
Hi, On 2/23/21 6:20 PM, Mark Brown wrote: > On Tue, Feb 23, 2021 at 05:14:32PM +0100, Jaroslav Kysela wrote: >> Dne 23. 02. 21 v 15:21 Takashi Iwai napsal(a): > >>> That's one of my concerns in the recent actions for putting the >>> hard-coded mute LED controls. So far, the only usage of led-audio >>> trigger is HD-audio, and it's enabled only for selected devices and >>> setups. OTOH, if we apply the audio-led trigger generically in ASoC >>> codec driver, it's always done and might misfit; e.g. what happens if >>> two codecs are present on the system?. > >> That's the abstraction issue. We can use PCI, ACPI, DMI or DT checks at the >> _right_ place (machine top-level code) to mark those controls with the LED >> flags in the kernel space. I've never said that the right place is the generic >> ASoC codec driver. > > We already need ACPI and DMI quirks in the CODEC drivers anyway due to > the limitations of ACPI so it wouldn't be particularly surprising to > have stuff there. OTOH this would fix things per machine while with > fancier hardware things might easily be flexible enough that there's > multiple choices that could be made depending on use case. I have a feeling from the discussion here that you would prefer this per model/machine option over the current patch which unconditionally sets the SNDRV_CTL_ELEM_ACCESS_SPK/MIC_LED flag on the main DAC/ADC mute controls ? So I believe that it would be best for me to respin this patch series moving to making this a per model/machine thing, correct? > I'd be a lot more comfortable with ASoC having some runtime control for > overriding which controls get mapped, even if we try to pick defaults > with quirks. The drivers in question already allow overriding their quirks bitmap via a module-parameter. If we are going to enable the mixer-element access-flag which enables LED control on a per-model basis based on DMI quirks, then I plan to simply add 1 new flag in the quirks bitmap for this for each mixer-element on which we need the flag. So for now this would be just 2 new flags, since atm we only need the SNDRV_CTL_ELEM_ACCESS_SPK_LED resp. SNDRV_CTL_ELEM_ACCESS_SPK_LED flag on one mixer-element each. And then the user can always override the settings using the quirk module parameter. This is not exactly runtime control, but IMHO it is close enough and anything else will just overcomplicate things. I'm aware of only 3 model 2-in-1s which need this and on those 3 the implementation is very straight forward. Regards, Hans
Dne 23. 02. 21 v 17:20 Takashi Iwai napsal(a): > So do you mean that the LED feature should be selectively enabled like > the current HD-audio? Yes, it should be enabled only when the machine has the audio LEDs. >>> Of course, this implementation would make the integration much easier, >>> and that's a big benefit. So I have a mixed feeling and not decided >>> yet whether we should go for it right now... >> >> I think that we can reconsider the LED handling implementation later, when >> someone brings something better on the table. > > What worried me is the plan to expose this capability to user-space. > If it's only a kernel-internal, we can fix it in the kernel and > nothing else broken, but if it's a part of API, that's not easy. > > So, if any, I'd like to avoid exposing to the user-space at first. > (But then it comes to the question how to deal with a case like AMD > ACP...) I tried to propose a complete solution and the ACP was one strong reason for this kernel / user space API. So without the user space support, it's just a half solution for known issues. Frankly, I don't see any drawback or a problem even if we remove this API later. The LED group bits are just informal for the user space and it's expected to create the user controls tied to this LED functionality only in alsa-lib/plugins at the moment. The kernel may return an error when the user space tries to set those new bits when the API is deprecated and I believe that the hardware design faults like AMD ACP (without the hardware mute) are rare. Initial alsa-lib support: https://github.com/alsa-project/alsa-lib/pull/121 Jaroslav
On Tue, 23 Feb 2021 21:56:16 +0100, Jaroslav Kysela wrote: > > Dne 23. 02. 21 v 17:20 Takashi Iwai napsal(a): > >>> Of course, this implementation would make the integration much easier, > >>> and that's a big benefit. So I have a mixed feeling and not decided > >>> yet whether we should go for it right now... > >> > >> I think that we can reconsider the LED handling implementation later, when > >> someone brings something better on the table. > > > > What worried me is the plan to expose this capability to user-space. > > If it's only a kernel-internal, we can fix it in the kernel and > > nothing else broken, but if it's a part of API, that's not easy. > > > > So, if any, I'd like to avoid exposing to the user-space at first. > > (But then it comes to the question how to deal with a case like AMD > > ACP...) > > I tried to propose a complete solution and the ACP was one strong reason for > this kernel / user space API. So without the user space support, it's just > a half solution for known issues. > > Frankly, I don't see any drawback or a problem even if we remove this API > later. Removing the user-space API is absolutely no-go. The only exception would be either the case really no one uses it or it's too buggy and unfixable. > The LED group bits are just informal for the user space and it's > expected to create the user controls tied to this LED functionality only in > alsa-lib/plugins at the moment. The kernel may return an error when the user > space tries to set those new bits when the API is deprecated and I believe > that the hardware design faults like AMD ACP (without the hardware mute) are rare. The experience tells us that users are creative enough to (ab)use a new ABI in any unexpected ways, and we have no control for it. So it's not about how alsa-lib is implemented but rather how ABI could be abused :) Takashi
Dne 24. 02. 21 v 8:12 Takashi Iwai napsal(a): > On Tue, 23 Feb 2021 21:56:16 +0100, > Jaroslav Kysela wrote: >> >> Dne 23. 02. 21 v 17:20 Takashi Iwai napsal(a): >>>>> Of course, this implementation would make the integration much easier, >>>>> and that's a big benefit. So I have a mixed feeling and not decided >>>>> yet whether we should go for it right now... >>>> >>>> I think that we can reconsider the LED handling implementation later, when >>>> someone brings something better on the table. >>> >>> What worried me is the plan to expose this capability to user-space. >>> If it's only a kernel-internal, we can fix it in the kernel and >>> nothing else broken, but if it's a part of API, that's not easy. >>> >>> So, if any, I'd like to avoid exposing to the user-space at first. >>> (But then it comes to the question how to deal with a case like AMD >>> ACP...) >> >> I tried to propose a complete solution and the ACP was one strong reason for >> this kernel / user space API. So without the user space support, it's just >> a half solution for known issues. >> >> Frankly, I don't see any drawback or a problem even if we remove this API >> later. > > Removing the user-space API is absolutely no-go. The only exception > would be either the case really no one uses it or it's too buggy and > unfixable. This is a special case. Even if those LED bits are ignored by kernel in future, we expect to be replaced with another layer. Thus the functionality must be retained. >> The LED group bits are just informal for the user space and it's >> expected to create the user controls tied to this LED functionality only in >> alsa-lib/plugins at the moment. The kernel may return an error when the user >> space tries to set those new bits when the API is deprecated and I believe >> that the hardware design faults like AMD ACP (without the hardware mute) are rare. > > The experience tells us that users are creative enough to (ab)use a > new ABI in any unexpected ways, and we have no control for it. So > it's not about how alsa-lib is implemented but rather how ABI could be > abused :) Ok, I don't have other ideas. I don't agree with your argumentation for this particular case, where the functionality is marginal. Ideally, the AMD driver may be recoded to use double-buffering and software mute switch, so we should handle everything in the kernel space. Jaroslav
On Wed, 24 Feb 2021 09:14:41 +0100, Jaroslav Kysela wrote: > > Dne 24. 02. 21 v 8:12 Takashi Iwai napsal(a): > > On Tue, 23 Feb 2021 21:56:16 +0100, > > Jaroslav Kysela wrote: > >> > >> Dne 23. 02. 21 v 17:20 Takashi Iwai napsal(a): > >>>>> Of course, this implementation would make the integration much easier, > >>>>> and that's a big benefit. So I have a mixed feeling and not decided > >>>>> yet whether we should go for it right now... > >>>> > >>>> I think that we can reconsider the LED handling implementation later, when > >>>> someone brings something better on the table. > >>> > >>> What worried me is the plan to expose this capability to user-space. > >>> If it's only a kernel-internal, we can fix it in the kernel and > >>> nothing else broken, but if it's a part of API, that's not easy. > >>> > >>> So, if any, I'd like to avoid exposing to the user-space at first. > >>> (But then it comes to the question how to deal with a case like AMD > >>> ACP...) > >> > >> I tried to propose a complete solution and the ACP was one strong reason for > >> this kernel / user space API. So without the user space support, it's just > >> a half solution for known issues. > >> > >> Frankly, I don't see any drawback or a problem even if we remove this API > >> later. > > > > Removing the user-space API is absolutely no-go. The only exception > > would be either the case really no one uses it or it's too buggy and > > unfixable. > > This is a special case. Even if those LED bits are ignored by kernel in > future, we expect to be replaced with another layer. Thus the functionality > must be retained. Well, we cannot know whether the replacement really happens or happened, and hence we never kill the old one. That's the problem. > >> The LED group bits are just informal for the user space and it's > >> expected to create the user controls tied to this LED functionality only in > >> alsa-lib/plugins at the moment. The kernel may return an error when the user > >> space tries to set those new bits when the API is deprecated and I believe > >> that the hardware design faults like AMD ACP (without the hardware mute) are rare. > > > > The experience tells us that users are creative enough to (ab)use a > > new ABI in any unexpected ways, and we have no control for it. So > > it's not about how alsa-lib is implemented but rather how ABI could be > > abused :) > > Ok, I don't have other ideas. I don't agree with your argumentation for this > particular case, where the functionality is marginal. Ideally, the AMD driver > may be recoded to use double-buffering and software mute switch, so we should > handle everything in the kernel space. My argument is that we're trying to add too much freedom just for this "marginal" problem. Honestly speaking, I would feel rather more comfortable if it were a kernel control element that just does trigger the LED like the original patch from AMD guys. Then you cannot do much wrong. OTOH, creating a virtual capture switch and let alsa-lib handling the software mute, while PA should ignores the soft-mute but dealing only with the assigned mute LED... Sounds too complex to me. thanks, Takashi
Dne 24. 02. 21 v 9:52 Takashi Iwai napsal(a): > On Wed, 24 Feb 2021 09:14:41 +0100, > Jaroslav Kysela wrote: >> >> Dne 24. 02. 21 v 8:12 Takashi Iwai napsal(a): >>> On Tue, 23 Feb 2021 21:56:16 +0100, >>> Jaroslav Kysela wrote: >>>> >>>> Dne 23. 02. 21 v 17:20 Takashi Iwai napsal(a): >>>>>>> Of course, this implementation would make the integration much easier, >>>>>>> and that's a big benefit. So I have a mixed feeling and not decided >>>>>>> yet whether we should go for it right now... >>>>>> >>>>>> I think that we can reconsider the LED handling implementation later, when >>>>>> someone brings something better on the table. >>>>> >>>>> What worried me is the plan to expose this capability to user-space. >>>>> If it's only a kernel-internal, we can fix it in the kernel and >>>>> nothing else broken, but if it's a part of API, that's not easy. >>>>> >>>>> So, if any, I'd like to avoid exposing to the user-space at first. >>>>> (But then it comes to the question how to deal with a case like AMD >>>>> ACP...) >>>> >>>> I tried to propose a complete solution and the ACP was one strong reason for >>>> this kernel / user space API. So without the user space support, it's just >>>> a half solution for known issues. >>>> >>>> Frankly, I don't see any drawback or a problem even if we remove this API >>>> later. >>> >>> Removing the user-space API is absolutely no-go. The only exception >>> would be either the case really no one uses it or it's too buggy and >>> unfixable. >> >> This is a special case. Even if those LED bits are ignored by kernel in >> future, we expect to be replaced with another layer. Thus the functionality >> must be retained. > > Well, we cannot know whether the replacement really happens or > happened, and hence we never kill the old one. That's the problem. > >>>> The LED group bits are just informal for the user space and it's >>>> expected to create the user controls tied to this LED functionality only in >>>> alsa-lib/plugins at the moment. The kernel may return an error when the user >>>> space tries to set those new bits when the API is deprecated and I believe >>>> that the hardware design faults like AMD ACP (without the hardware mute) are rare. >>> >>> The experience tells us that users are creative enough to (ab)use a >>> new ABI in any unexpected ways, and we have no control for it. So >>> it's not about how alsa-lib is implemented but rather how ABI could be >>> abused :) >> >> Ok, I don't have other ideas. I don't agree with your argumentation for this >> particular case, where the functionality is marginal. Ideally, the AMD driver >> may be recoded to use double-buffering and software mute switch, so we should >> handle everything in the kernel space. > > My argument is that we're trying to add too much freedom just for this > "marginal" problem. Honestly speaking, I would feel rather more > comfortable if it were a kernel control element that just does trigger > the LED like the original patch from AMD guys. Then you cannot do > much wrong. OTOH, creating a virtual capture switch and let alsa-lib > handling the software mute, while PA should ignores the soft-mute but We can force the softvol even if PA set the skip flag for this particular PCM stream. > dealing only with the assigned mute LED... Sounds too complex to me. It seems that you misunderstood the number of issues which my code is trying to resolve: 1) set LED based on state from multiple cards (so you cannot trigger LED inside single driver / single control element); we need one arbiter; this is the main argument 2) unifies the audio LED interface 3) reduce the hardware driver code Jaroslav
On Wed, 24 Feb 2021 10:27:13 +0100, Jaroslav Kysela wrote: > > Dne 24. 02. 21 v 9:52 Takashi Iwai napsal(a): > > On Wed, 24 Feb 2021 09:14:41 +0100, > > Jaroslav Kysela wrote: > >> > >> Dne 24. 02. 21 v 8:12 Takashi Iwai napsal(a): > >>> On Tue, 23 Feb 2021 21:56:16 +0100, > >>> Jaroslav Kysela wrote: > >>>> > >>>> Dne 23. 02. 21 v 17:20 Takashi Iwai napsal(a): > >>>>>>> Of course, this implementation would make the integration much easier, > >>>>>>> and that's a big benefit. So I have a mixed feeling and not decided > >>>>>>> yet whether we should go for it right now... > >>>>>> > >>>>>> I think that we can reconsider the LED handling implementation later, when > >>>>>> someone brings something better on the table. > >>>>> > >>>>> What worried me is the plan to expose this capability to user-space. > >>>>> If it's only a kernel-internal, we can fix it in the kernel and > >>>>> nothing else broken, but if it's a part of API, that's not easy. > >>>>> > >>>>> So, if any, I'd like to avoid exposing to the user-space at first. > >>>>> (But then it comes to the question how to deal with a case like AMD > >>>>> ACP...) > >>>> > >>>> I tried to propose a complete solution and the ACP was one strong reason for > >>>> this kernel / user space API. So without the user space support, it's just > >>>> a half solution for known issues. > >>>> > >>>> Frankly, I don't see any drawback or a problem even if we remove this API > >>>> later. > >>> > >>> Removing the user-space API is absolutely no-go. The only exception > >>> would be either the case really no one uses it or it's too buggy and > >>> unfixable. > >> > >> This is a special case. Even if those LED bits are ignored by kernel in > >> future, we expect to be replaced with another layer. Thus the functionality > >> must be retained. > > > > Well, we cannot know whether the replacement really happens or > > happened, and hence we never kill the old one. That's the problem. > > > >>>> The LED group bits are just informal for the user space and it's > >>>> expected to create the user controls tied to this LED functionality only in > >>>> alsa-lib/plugins at the moment. The kernel may return an error when the user > >>>> space tries to set those new bits when the API is deprecated and I believe > >>>> that the hardware design faults like AMD ACP (without the hardware mute) are rare. > >>> > >>> The experience tells us that users are creative enough to (ab)use a > >>> new ABI in any unexpected ways, and we have no control for it. So > >>> it's not about how alsa-lib is implemented but rather how ABI could be > >>> abused :) > >> > >> Ok, I don't have other ideas. I don't agree with your argumentation for this > >> particular case, where the functionality is marginal. Ideally, the AMD driver > >> may be recoded to use double-buffering and software mute switch, so we should > >> handle everything in the kernel space. > > > > My argument is that we're trying to add too much freedom just for this > > "marginal" problem. Honestly speaking, I would feel rather more > > comfortable if it were a kernel control element that just does trigger > > the LED like the original patch from AMD guys. Then you cannot do > > much wrong. OTOH, creating a virtual capture switch and let alsa-lib > > handling the software mute, while PA should ignores the soft-mute but > > We can force the softvol even if PA set the skip flag for this particular PCM > stream. > > > dealing only with the assigned mute LED... Sounds too complex to me. > > It seems that you misunderstood the number of issues which my code is trying > to resolve: > > 1) set LED based on state from multiple cards (so you cannot trigger LED > inside single driver / single control element); we need one arbiter; this is > the main argument > 2) unifies the audio LED interface > 3) reduce the hardware driver code Those purposes are all fine. But they don't need to be exposed for user controls that can be abused. That's the very concern. Takashi
Dne 24. 02. 21 v 10:38 Takashi Iwai napsal(a): >> It seems that you misunderstood the number of issues which my code is trying >> to resolve: >> >> 1) set LED based on state from multiple cards (so you cannot trigger LED >> inside single driver / single control element); we need one arbiter; this is >> the main argument >> 2) unifies the audio LED interface >> 3) reduce the hardware driver code > > Those purposes are all fine. But they don't need to be exposed for > user controls that can be abused. That's the very concern. So, how to handle this feature for AMD ACP without PA / PipeWire modifications? And if we add an user space channel to the audio LED arbiter code, how it differs from my proposed control API extension? We have already locking mechanism for the user control element to one task, so it's possible to create safe user space implementation (depending on the standard task priviledges) on demand even with my proposal. Could you elaborate the abuse you mean? Three bits? Jaroslav
On Wed, 24 Feb 2021 10:49:41 +0100, Jaroslav Kysela wrote: > > Dne 24. 02. 21 v 10:38 Takashi Iwai napsal(a): > > >> It seems that you misunderstood the number of issues which my code is trying > >> to resolve: > >> > >> 1) set LED based on state from multiple cards (so you cannot trigger LED > >> inside single driver / single control element); we need one arbiter; this is > >> the main argument > >> 2) unifies the audio LED interface > >> 3) reduce the hardware driver code > > > > Those purposes are all fine. But they don't need to be exposed for > > user controls that can be abused. That's the very concern. > > So, how to handle this feature for AMD ACP without PA / PipeWire modifications? > > And if we add an user space channel to the audio LED arbiter code, how it > differs from my proposed control API extension? As the early patch does, creating a kernel control (but not a generic "Capture" switch but specific to LED) and control it in UCM profile. What's the practical problem there? That's what I might have missed as the starting point of the discussion. > We have already locking > mechanism for the user control element to one task, so it's possible to create > safe user space implementation (depending on the standard task priviledges) on > demand even with my proposal. > > Could you elaborate the abuse you mean? Three bits? You can create up to 1028 user controls per card and each of them can fire the led trigger... That's an interesting experiment :) So far, a user control is merely storing the value, let read/write via the control API. That's all, and nothing wrong can happen just by that. Now if it interacts with other subsystem... A more serious concern is rather the fragility of the setup; for enabling the mute LED control, you'd have to create a new user-space control, the function of the control has to be ignored by some application and some not, etc. This has to be done on each machine when the system got updated. And, not everyone is using alsa-lib. Does tiny ALSA and other existing backend support the user control element management? Some uncertainty here. thanks, Takashi
Dne 24. 02. 21 v 11:33 Takashi Iwai napsal(a): > On Wed, 24 Feb 2021 10:49:41 +0100, > Jaroslav Kysela wrote: >> >> Dne 24. 02. 21 v 10:38 Takashi Iwai napsal(a): >> >>>> It seems that you misunderstood the number of issues which my code is trying >>>> to resolve: >>>> >>>> 1) set LED based on state from multiple cards (so you cannot trigger LED >>>> inside single driver / single control element); we need one arbiter; this is >>>> the main argument >>>> 2) unifies the audio LED interface >>>> 3) reduce the hardware driver code >>> >>> Those purposes are all fine. But they don't need to be exposed for >>> user controls that can be abused. That's the very concern. >> >> So, how to handle this feature for AMD ACP without PA / PipeWire modifications? >> >> And if we add an user space channel to the audio LED arbiter code, how it >> differs from my proposed control API extension? > > As the early patch does, creating a kernel control (but not a generic > "Capture" switch but specific to LED) and control it in UCM profile. > What's the practical problem there? That's what I might have missed > as the starting point of the discussion. UCM is just a database which does not do any state management for those controls. I've not found a simple way to create an arbiter for UCM without adding more layers to this API. Yes, we have enable/disable sequences, but for LED, you need to create "group" of devices and do the OR state management: Current device enable/disable scheme: Device1 -> enable (LED off) Device2 -> enable (LED off) Device2 -> disable (LED on) --- but Device1 is on, so LED should be off ... LED off - set LED control to off ... LED on - set LED control to on Even the current mechanism fails here, we don't look into the mute switch value in UCM at the moment, so the LED will reflect only device use - not the mute switch. So, as you see, UCM does not cover this. It's just used to activate and deactivate paths, but there's no state management (except for the device on/off). And, it will work only for UCM not for the standard/legacy ALSA setup. Those are reasons for which I ruled out the UCM for the LED control. >> We have already locking >> mechanism for the user control element to one task, so it's possible to create >> safe user space implementation (depending on the standard task priviledges) on >> demand even with my proposal. >> >> Could you elaborate the abuse you mean? Three bits? > > You can create up to 1028 user controls per card and each of them can > fire the led trigger... That's an interesting experiment :) > > So far, a user control is merely storing the value, let read/write via > the control API. That's all, and nothing wrong can happen just by > that. Now if it interacts with other subsystem... > > A more serious concern is rather the fragility of the setup; for > enabling the mute LED control, you'd have to create a new user-space > control, the function of the control has to be ignored by some > application and some not, etc. This has to be done on each machine You're using "ignore", but as I explained before, the user space switch will be used in the whole chain: capture stream -> alsa-lib mute switch / silence PCM stream -> PA mute switch / silence PCM stream So PA can use this switch like the traditional hardware mute switch. And we cannot do much in the user space for a better mute support here. > when the system got updated. And, not everyone is using alsa-lib. > Does tiny ALSA and other existing backend support the user control > element management? Some uncertainty here. It's not an argument. Tiny alsa library does not have all features from alsa-lib. Nobody is restricted to follow the similar mechanism. Jaroslav
On Wed, 24 Feb 2021 11:56:01 +0100, Jaroslav Kysela wrote: > > Dne 24. 02. 21 v 11:33 Takashi Iwai napsal(a): > > On Wed, 24 Feb 2021 10:49:41 +0100, > > Jaroslav Kysela wrote: > >> > >> Dne 24. 02. 21 v 10:38 Takashi Iwai napsal(a): > >> > >>>> It seems that you misunderstood the number of issues which my code is trying > >>>> to resolve: > >>>> > >>>> 1) set LED based on state from multiple cards (so you cannot trigger LED > >>>> inside single driver / single control element); we need one arbiter; this is > >>>> the main argument > >>>> 2) unifies the audio LED interface > >>>> 3) reduce the hardware driver code > >>> > >>> Those purposes are all fine. But they don't need to be exposed for > >>> user controls that can be abused. That's the very concern. > >> > >> So, how to handle this feature for AMD ACP without PA / PipeWire modifications? > >> > >> And if we add an user space channel to the audio LED arbiter code, how it > >> differs from my proposed control API extension? > > > > As the early patch does, creating a kernel control (but not a generic > > "Capture" switch but specific to LED) and control it in UCM profile. > > What's the practical problem there? That's what I might have missed > > as the starting point of the discussion. > > UCM is just a database which does not do any state management for those > controls. I've not found a simple way to create an arbiter for UCM without > adding more layers to this API. Yes, we have enable/disable sequences, but for > LED, you need to create "group" of devices and do the OR state management: > > Current device enable/disable scheme: > > Device1 -> enable (LED off) > Device2 -> enable (LED off) > Device2 -> disable (LED on) --- but Device1 is on, so LED should be off > > ... LED off - set LED control to off > ... LED on - set LED control to on > > Even the current mechanism fails here, we don't look into the mute switch > value in UCM at the moment, so the LED will reflect only device use - not the > mute switch. So, as you see, UCM does not cover this. It's just used to > activate and deactivate paths, but there's no state management (except for the > device on/off). > > And, it will work only for UCM not for the standard/legacy ALSA setup. > > Those are reasons for which I ruled out the UCM for the LED control. Besides the lack of some features in UCM, having the binding of the led trigger managed in the ALSA core is good, and I have no objection. However, again, my concern is the user controls. > >> We have already locking > >> mechanism for the user control element to one task, so it's possible to create > >> safe user space implementation (depending on the standard task priviledges) on > >> demand even with my proposal. > >> > >> Could you elaborate the abuse you mean? Three bits? > > > > You can create up to 1028 user controls per card and each of them can > > fire the led trigger... That's an interesting experiment :) > > > > So far, a user control is merely storing the value, let read/write via > > the control API. That's all, and nothing wrong can happen just by > > that. Now if it interacts with other subsystem... > > > > A more serious concern is rather the fragility of the setup; for > > enabling the mute LED control, you'd have to create a new user-space > > control, the function of the control has to be ignored by some > > application and some not, etc. This has to be done on each machine > > You're using "ignore", but as I explained before, the user space switch will > be used in the whole chain: > > capture stream -> > alsa-lib mute switch / silence PCM stream -> > PA mute switch / silence PCM stream > > So PA can use this switch like the traditional hardware mute switch. Does it mean PA would work as of now without any change? Or does it need patching? > And we cannot do much in the user space for a better mute support here. Right, and I'm not asking about the mute control here. It's all about the LED control. > > when the system got updated. And, not everyone is using alsa-lib. > > Does tiny ALSA and other existing backend support the user control > > element management? Some uncertainty here. > > It's not an argument. Tiny alsa library does not have all features from > alsa-lib. Nobody is restricted to follow the similar mechanism. Yes, but OTOH, if it were implemented in a kernel control, there is no need to worry about the extra setup. So the easiness of the setup is a significant key point. thanks, Takashi
Dne 24. 02. 21 v 12:43 Takashi Iwai napsal(a): >>> So far, a user control is merely storing the value, let read/write via >>> the control API. That's all, and nothing wrong can happen just by >>> that. Now if it interacts with other subsystem... >>> >>> A more serious concern is rather the fragility of the setup; for >>> enabling the mute LED control, you'd have to create a new user-space >>> control, the function of the control has to be ignored by some >>> application and some not, etc. This has to be done on each machine >> >> You're using "ignore", but as I explained before, the user space switch will >> be used in the whole chain: >> >> capture stream -> >> alsa-lib mute switch / silence PCM stream -> >> PA mute switch / silence PCM stream >> >> So PA can use this switch like the traditional hardware mute switch. > > Does it mean PA would work as of now without any change? Or does it > need patching? Yes, no PA modifications are required with my mechanism. The PA will just see the new user space control - mute switch - created in alsa-lib - which will be synced the internal PA path mute state like for the hardware mute switch. I also think that handling LEDs independently (outside the upper layers like PA) is more flexible. Jaroslav
On Wed, 24 Feb 2021 13:08:55 +0100, Jaroslav Kysela wrote: > > Dne 24. 02. 21 v 12:43 Takashi Iwai napsal(a): > > >>> So far, a user control is merely storing the value, let read/write via > >>> the control API. That's all, and nothing wrong can happen just by > >>> that. Now if it interacts with other subsystem... > >>> > >>> A more serious concern is rather the fragility of the setup; for > >>> enabling the mute LED control, you'd have to create a new user-space > >>> control, the function of the control has to be ignored by some > >>> application and some not, etc. This has to be done on each machine > >> > >> You're using "ignore", but as I explained before, the user space switch will > >> be used in the whole chain: > >> > >> capture stream -> > >> alsa-lib mute switch / silence PCM stream -> > >> PA mute switch / silence PCM stream > >> > >> So PA can use this switch like the traditional hardware mute switch. > > > > Does it mean PA would work as of now without any change? Or does it > > need patching? > > Yes, no PA modifications are required with my mechanism. The PA will just see > the new user space control - mute switch - created in alsa-lib - which will be > synced the internal PA path mute state like for the hardware mute > switch. OK, but how would we create and manage the user control element? And why it has to be user control? thanks, Takashi
On Tue, Feb 23, 2021 at 08:03:58PM +0100, Hans de Goede wrote: > On 2/23/21 6:20 PM, Mark Brown wrote: > > We already need ACPI and DMI quirks in the CODEC drivers anyway due to > > the limitations of ACPI so it wouldn't be particularly surprising to > > have stuff there. OTOH this would fix things per machine while with > > fancier hardware things might easily be flexible enough that there's > > multiple choices that could be made depending on use case. > I have a feeling from the discussion here that you would prefer this > per model/machine option over the current patch which unconditionally > sets the SNDRV_CTL_ELEM_ACCESS_SPK/MIC_LED flag on the main DAC/ADC > mute controls ? > So I believe that it would be best for me to respin this patch > series moving to making this a per model/machine thing, correct? Yes, we at least need to be able to do that even if we end up hard coding it in some CODEC drivers as the device is inflexible. There are devices where the concept of "main DAC/ADC" just doesn't apply. > > I'd be a lot more comfortable with ASoC having some runtime control for > > overriding which controls get mapped, even if we try to pick defaults > > with quirks. > The drivers in question already allow overriding their quirks bitmap > via a module-parameter. If we are going to enable the mixer-element I'm not a big fan of module parameters TBH, it's not great for usability. > And then the user can always override the settings using the quirk > module parameter. This is not exactly runtime control, but IMHO it > is close enough and anything else will just overcomplicate things. > I'm aware of only 3 model 2-in-1s which need this and on those > 3 the implementation is very straight forward. The problem I was thinking of is the situation where there are multiple options for the mute control in the hardware and it's a configuration decision which one to use.
Dne 24. 02. 21 v 13:42 Takashi Iwai napsal(a): > On Wed, 24 Feb 2021 13:08:55 +0100, > Jaroslav Kysela wrote: >> >> Dne 24. 02. 21 v 12:43 Takashi Iwai napsal(a): >> >>>>> So far, a user control is merely storing the value, let read/write via >>>>> the control API. That's all, and nothing wrong can happen just by >>>>> that. Now if it interacts with other subsystem... >>>>> >>>>> A more serious concern is rather the fragility of the setup; for >>>>> enabling the mute LED control, you'd have to create a new user-space >>>>> control, the function of the control has to be ignored by some >>>>> application and some not, etc. This has to be done on each machine >>>> >>>> You're using "ignore", but as I explained before, the user space switch will >>>> be used in the whole chain: >>>> >>>> capture stream -> >>>> alsa-lib mute switch / silence PCM stream -> >>>> PA mute switch / silence PCM stream >>>> >>>> So PA can use this switch like the traditional hardware mute switch. >>> >>> Does it mean PA would work as of now without any change? Or does it >>> need patching? >> >> Yes, no PA modifications are required with my mechanism. The PA will just see >> the new user space control - mute switch - created in alsa-lib - which will be >> synced the internal PA path mute state like for the hardware mute >> switch. > > OK, but how would we create and manage the user control element? And > why it has to be user control? The softvol or alsactl can create the user control element. The alsa-lib softvol plugin and PA can silence stream according the state. I see your point to create this control in the kernel space, but any other name than "Mic Capture Switch" (in the ACP case) will be misleading for users, because the user-space does the appropriate real silencing job instead of hw. And if we create "Mic Capture Switch" in the kernel space, it may be misleading for applications (they can think that there's hardware mute control). Perhaps, we can create "Mic Phantom Capture Switch" in kernel which may resolve both problems (no hw mute information + no user confusion). Jaroslav
Hi, On 2/24/21 1:59 PM, Mark Brown wrote: > On Tue, Feb 23, 2021 at 08:03:58PM +0100, Hans de Goede wrote: >> On 2/23/21 6:20 PM, Mark Brown wrote: > >>> We already need ACPI and DMI quirks in the CODEC drivers anyway due to >>> the limitations of ACPI so it wouldn't be particularly surprising to >>> have stuff there. OTOH this would fix things per machine while with >>> fancier hardware things might easily be flexible enough that there's >>> multiple choices that could be made depending on use case. > >> I have a feeling from the discussion here that you would prefer this >> per model/machine option over the current patch which unconditionally >> sets the SNDRV_CTL_ELEM_ACCESS_SPK/MIC_LED flag on the main DAC/ADC >> mute controls ? > >> So I believe that it would be best for me to respin this patch >> series moving to making this a per model/machine thing, correct? > > Yes, we at least need to be able to do that even if we end up hard > coding it in some CODEC drivers as the device is inflexible. There are > devices where the concept of "main DAC/ADC" just doesn't apply. > >>> I'd be a lot more comfortable with ASoC having some runtime control for >>> overriding which controls get mapped, even if we try to pick defaults >>> with quirks. > >> The drivers in question already allow overriding their quirks bitmap >> via a module-parameter. If we are going to enable the mixer-element > > I'm not a big fan of module parameters TBH, it's not great for > usability. > >> And then the user can always override the settings using the quirk >> module parameter. This is not exactly runtime control, but IMHO it >> is close enough and anything else will just overcomplicate things. >> I'm aware of only 3 model 2-in-1s which need this and on those >> 3 the implementation is very straight forward. > > The problem I was thinking of is the situation where there are multiple > options for the mute control in the hardware and it's a configuration > decision which one to use. ATM we have no device where this situation happens, so I would prefer to cross that bridge when we come to it. Regards, Hans
On Wed, Feb 24, 2021 at 08:14:12PM +0100, Hans de Goede wrote: > On 2/24/21 1:59 PM, Mark Brown wrote: > > The problem I was thinking of is the situation where there are multiple > > options for the mute control in the hardware and it's a configuration > > decision which one to use. > ATM we have no device where this situation happens, so I would prefer > to cross that bridge when we come to it. You just added wm5012 machine drivers, that device is going to present issues with this approach.
Hi, On 2/24/21 8:36 PM, Mark Brown wrote: > On Wed, Feb 24, 2021 at 08:14:12PM +0100, Hans de Goede wrote: >> On 2/24/21 1:59 PM, Mark Brown wrote: > >>> The problem I was thinking of is the situation where there are multiple >>> options for the mute control in the hardware and it's a configuration >>> decision which one to use. > >> ATM we have no device where this situation happens, so I would prefer >> to cross that bridge when we come to it. > > You just added wm5012 machine drivers, that device is going to present > issues with this approach. I've no intention to add led-trigger support to the bytcr-wm5102 driver, since to the best of my knowledge there are no mute LEDs on any designs using the bytcr + wm5102 combination. I'm aware of there actually being a mute LED on only 3 2-in-1 models with detachable keyboards (with the mute LEDs sitting inside the media-keys to toggle the mute, like how it is done on thinkpads): 1. The Thinkpad10 bytcr+rt5670 tablet with its USB ultrabook keyboard dock 2. The HP x2 10" clamshell designs with detachable keyboards in 2 variants, bytcr + rt5640 and cht + rt5640. Thats it, and in both cases the codecs have main ADC / DAC volume controls which are the only ones suited for using as the control to drive the led-trigger. Which is why this RFC just hardcoded the trigger to that control. As I said I'll happily respin this RFC series (and the not yet posted similar rt5640 series) to tie the led-trigger to specific controls based on DMI quirks. Given that the use of mute LEDs itself is actually rare and especially the use of mute LEDs in combination with ASoC coming up with some generic configuration mechanism to allow userspace to tie the led-trigger to any random 'Switch' type control is way overkill / overengineering and I've no desire to spend a huge amount of time on implementing this. Not to mention that this would just be punting the actual problem of figuring out which control to use to userspace, while the kernel is actually in a better place to make this decision since the kernel already uses DMI based quirks to deal with model specific configuration. Regards, Hans
On Wed, 24 Feb 2021 18:57:42 +0100, Jaroslav Kysela wrote: > > Dne 24. 02. 21 v 13:42 Takashi Iwai napsal(a): > > On Wed, 24 Feb 2021 13:08:55 +0100, > > Jaroslav Kysela wrote: > >> > >> Dne 24. 02. 21 v 12:43 Takashi Iwai napsal(a): > >> > >>>>> So far, a user control is merely storing the value, let read/write via > >>>>> the control API. That's all, and nothing wrong can happen just by > >>>>> that. Now if it interacts with other subsystem... > >>>>> > >>>>> A more serious concern is rather the fragility of the setup; for > >>>>> enabling the mute LED control, you'd have to create a new user-space > >>>>> control, the function of the control has to be ignored by some > >>>>> application and some not, etc. This has to be done on each machine > >>>> > >>>> You're using "ignore", but as I explained before, the user space switch will > >>>> be used in the whole chain: > >>>> > >>>> capture stream -> > >>>> alsa-lib mute switch / silence PCM stream -> > >>>> PA mute switch / silence PCM stream > >>>> > >>>> So PA can use this switch like the traditional hardware mute switch. > >>> > >>> Does it mean PA would work as of now without any change? Or does it > >>> need patching? > >> > >> Yes, no PA modifications are required with my mechanism. The PA will just see > >> the new user space control - mute switch - created in alsa-lib - which will be > >> synced the internal PA path mute state like for the hardware mute > >> switch. > > > > OK, but how would we create and manage the user control element? And > > why it has to be user control? > > The softvol or alsactl can create the user control element. The alsa-lib > softvol plugin and PA can silence stream according the state. And that's tricky if it's only with PA, as PA won't open a softvol PCM stream... > I see your point to create this control in the kernel space, but any other > name than "Mic Capture Switch" (in the ACP case) will be misleading for users, > because the user-space does the appropriate real silencing job instead of hw. > > And if we create "Mic Capture Switch" in the kernel space, it may be > misleading for applications (they can think that there's hardware mute control). > > Perhaps, we can create "Mic Phantom Capture Switch" in kernel which may > resolve both problems (no hw mute information + no user confusion). Yes, something like that would work. The advantage of in-kernel implementation is that it's self-contained, so just deploying the new kernel makes everything working. thanks, Takashi
On Wed, Feb 24, 2021 at 09:09:36PM +0100, Hans de Goede wrote: > Given that the use of mute LEDs itself is actually rare and especially > the use of mute LEDs in combination with ASoC coming up with some > generic configuration mechanism to allow userspace to tie the This seems like an optimistic set of assumptions - it may reflect current laptops but it sounds like the sort of thing people might deploy on future devices, never mind all the non-laptops that could end up wanting to use this mechanism. > Not to mention that this would just be punting the actual problem > of figuring out which control to use to userspace, while the kernel > is actually in a better place to make this decision since the kernel > already uses DMI based quirks to deal with model specific configuration. Again, this only works in cases where there's only one option for the control that could be used.
Dne 25. 02. 21 v 12:00 Takashi Iwai napsal(a): > On Wed, 24 Feb 2021 18:57:42 +0100, > Jaroslav Kysela wrote: >> >> Dne 24. 02. 21 v 13:42 Takashi Iwai napsal(a): >>> On Wed, 24 Feb 2021 13:08:55 +0100, >>> Jaroslav Kysela wrote: >>>> >>>> Dne 24. 02. 21 v 12:43 Takashi Iwai napsal(a): >>>> >>>>>>> So far, a user control is merely storing the value, let read/write via >>>>>>> the control API. That's all, and nothing wrong can happen just by >>>>>>> that. Now if it interacts with other subsystem... >>>>>>> >>>>>>> A more serious concern is rather the fragility of the setup; for >>>>>>> enabling the mute LED control, you'd have to create a new user-space >>>>>>> control, the function of the control has to be ignored by some >>>>>>> application and some not, etc. This has to be done on each machine >>>>>> >>>>>> You're using "ignore", but as I explained before, the user space switch will >>>>>> be used in the whole chain: >>>>>> >>>>>> capture stream -> >>>>>> alsa-lib mute switch / silence PCM stream -> >>>>>> PA mute switch / silence PCM stream >>>>>> >>>>>> So PA can use this switch like the traditional hardware mute switch. >>>>> >>>>> Does it mean PA would work as of now without any change? Or does it >>>>> need patching? >>>> >>>> Yes, no PA modifications are required with my mechanism. The PA will just see >>>> the new user space control - mute switch - created in alsa-lib - which will be >>>> synced the internal PA path mute state like for the hardware mute >>>> switch. >>> >>> OK, but how would we create and manage the user control element? And >>> why it has to be user control? >> >> The softvol or alsactl can create the user control element. The alsa-lib >> softvol plugin and PA can silence stream according the state. > > And that's tricky if it's only with PA, as PA won't open a softvol PCM > stream... The protection is in alsa-lib, so we can skip to check this hint flag for this particular case like: https://github.com/alsa-project/alsa-lib/pull/121/commits/1acc1c7eccab0359996b25de54a6b6e0aa1e0c17 So it may depend on the softvol config not PA itself. Even for the solution bellow, we need to modify softvol to handle the kernel control elements, too. Actually softvol is not active, when the specified control element is found and this element is not from the user space. >> I see your point to create this control in the kernel space, but any other >> name than "Mic Capture Switch" (in the ACP case) will be misleading for users, >> because the user-space does the appropriate real silencing job instead of hw. >> >> And if we create "Mic Capture Switch" in the kernel space, it may be >> misleading for applications (they can think that there's hardware mute control). >> >> Perhaps, we can create "Mic Phantom Capture Switch" in kernel which may >> resolve both problems (no hw mute information + no user confusion). > > Yes, something like that would work. > The advantage of in-kernel implementation is that it's self-contained, > so just deploying the new kernel makes everything working. Ok, so let settle the naming for those controls which depends on the user space code which does the real work (silencing). Is "Phantom" prefix good - we're using it for jacks, or someone has a better idea? Jaroslav
Hi, On 2/25/21 3:59 PM, Mark Brown wrote: > On Wed, Feb 24, 2021 at 09:09:36PM +0100, Hans de Goede wrote: > >> Given that the use of mute LEDs itself is actually rare and especially >> the use of mute LEDs in combination with ASoC coming up with some >> generic configuration mechanism to allow userspace to tie the > > This seems like an optimistic set of assumptions - it may reflect > current laptops but it sounds like the sort of thing people might > deploy on future devices, never mind all the non-laptops that could end > up wanting to use this mechanism. > >> Not to mention that this would just be punting the actual problem >> of figuring out which control to use to userspace, while the kernel >> is actually in a better place to make this decision since the kernel >> already uses DMI based quirks to deal with model specific configuration. > > Again, this only works in cases where there's only one option for the > control that could be used. Which is the case in all the current models which I'm trying to get the mute LED supported on. In its current form this is all purely handled inside the kernel, so we can easily change / extend the mechanism later without any problems. OTOH adding an interface where userspace can runtime set which control to use for this, would require adding some userspace API for this. To me it seems like a really bad idea to add userspace API for this now, when we don't actually have hardware which needs this. Introducing userspace API for this now introduces a significant risk that we get the API wrong, since we don't actuall have a use-case where we actually need the suggested flexibility. And then if such a use-case does eventually pop-up we might very well have gotten the userspace API for this wrong. I'm not saying that we will never need such flexibility, but we do not need it *now*, so as I said before lets cross that bridge when we reach it. Regards, Hans
On Thu, 25 Feb 2021 19:09:44 +0100, Jaroslav Kysela wrote: > > Dne 25. 02. 21 v 12:00 Takashi Iwai napsal(a): > > On Wed, 24 Feb 2021 18:57:42 +0100, > > Jaroslav Kysela wrote: > >> > >> Dne 24. 02. 21 v 13:42 Takashi Iwai napsal(a): > >>> On Wed, 24 Feb 2021 13:08:55 +0100, > >>> Jaroslav Kysela wrote: > >>>> > >>>> Dne 24. 02. 21 v 12:43 Takashi Iwai napsal(a): > >>>> > >>>>>>> So far, a user control is merely storing the value, let read/write via > >>>>>>> the control API. That's all, and nothing wrong can happen just by > >>>>>>> that. Now if it interacts with other subsystem... > >>>>>>> > >>>>>>> A more serious concern is rather the fragility of the setup; for > >>>>>>> enabling the mute LED control, you'd have to create a new user-space > >>>>>>> control, the function of the control has to be ignored by some > >>>>>>> application and some not, etc. This has to be done on each machine > >>>>>> > >>>>>> You're using "ignore", but as I explained before, the user space switch will > >>>>>> be used in the whole chain: > >>>>>> > >>>>>> capture stream -> > >>>>>> alsa-lib mute switch / silence PCM stream -> > >>>>>> PA mute switch / silence PCM stream > >>>>>> > >>>>>> So PA can use this switch like the traditional hardware mute switch. > >>>>> > >>>>> Does it mean PA would work as of now without any change? Or does it > >>>>> need patching? > >>>> > >>>> Yes, no PA modifications are required with my mechanism. The PA will just see > >>>> the new user space control - mute switch - created in alsa-lib - which will be > >>>> synced the internal PA path mute state like for the hardware mute > >>>> switch. > >>> > >>> OK, but how would we create and manage the user control element? And > >>> why it has to be user control? > >> > >> The softvol or alsactl can create the user control element. The alsa-lib > >> softvol plugin and PA can silence stream according the state. > > > > And that's tricky if it's only with PA, as PA won't open a softvol PCM > > stream... > > The protection is in alsa-lib, so we can skip to check this hint flag for this > particular case like: > > https://github.com/alsa-project/alsa-lib/pull/121/commits/1acc1c7eccab0359996b25de54a6b6e0aa1e0c17 > > So it may depend on the softvol config not PA itself. Thanks, that's what I missed from the big picture. > Even for the solution bellow, we need to modify softvol to handle the kernel > control elements, too. Actually softvol is not active, when the specified > control element is found and this element is not from the user space. Hm, that would certainly work, but as we discussed before, it enforces the softvol PCM process for PA. That isn't too bad for the capture, fortunately, but not ideal, OTOH. And, I'm not sure how PA can take any control as its main capture mute switch, if it's named differently. Wouldn't we need to change mixer path in PA as well? And, if we may change PA side, it sounds more natural to change a control directly in PA's mixer path. The softvol doesn't fit well with PA, after all. > >> I see your point to create this control in the kernel space, but any other > >> name than "Mic Capture Switch" (in the ACP case) will be misleading for users, > >> because the user-space does the appropriate real silencing job instead of hw. > >> > >> And if we create "Mic Capture Switch" in the kernel space, it may be > >> misleading for applications (they can think that there's hardware mute control). > >> > >> Perhaps, we can create "Mic Phantom Capture Switch" in kernel which may > >> resolve both problems (no hw mute information + no user confusion). > > > > Yes, something like that would work. > > The advantage of in-kernel implementation is that it's self-contained, > > so just deploying the new kernel makes everything working. > > Ok, so let settle the naming for those controls which depends on the user > space code which does the real work (silencing). Is "Phantom" prefix good - > we're using it for jacks, or someone has a better idea? If we have to go in this way, that's an acceptable name, I think. thanks, Takashi
Dne 26. 02. 21 v 9:41 Takashi Iwai napsal(a): > On Thu, 25 Feb 2021 19:09:44 +0100, > Jaroslav Kysela wrote: >> >> Dne 25. 02. 21 v 12:00 Takashi Iwai napsal(a): >>> On Wed, 24 Feb 2021 18:57:42 +0100, >>> Jaroslav Kysela wrote: >>>> >>>> Dne 24. 02. 21 v 13:42 Takashi Iwai napsal(a): >>>>> On Wed, 24 Feb 2021 13:08:55 +0100, >>>>> Jaroslav Kysela wrote: >>>>>> >>>>>> Dne 24. 02. 21 v 12:43 Takashi Iwai napsal(a): >>>>>> >>>>>>>>> So far, a user control is merely storing the value, let read/write via >>>>>>>>> the control API. That's all, and nothing wrong can happen just by >>>>>>>>> that. Now if it interacts with other subsystem... >>>>>>>>> >>>>>>>>> A more serious concern is rather the fragility of the setup; for >>>>>>>>> enabling the mute LED control, you'd have to create a new user-space >>>>>>>>> control, the function of the control has to be ignored by some >>>>>>>>> application and some not, etc. This has to be done on each machine >>>>>>>> >>>>>>>> You're using "ignore", but as I explained before, the user space switch will >>>>>>>> be used in the whole chain: >>>>>>>> >>>>>>>> capture stream -> >>>>>>>> alsa-lib mute switch / silence PCM stream -> >>>>>>>> PA mute switch / silence PCM stream >>>>>>>> >>>>>>>> So PA can use this switch like the traditional hardware mute switch. >>>>>>> >>>>>>> Does it mean PA would work as of now without any change? Or does it >>>>>>> need patching? >>>>>> >>>>>> Yes, no PA modifications are required with my mechanism. The PA will just see >>>>>> the new user space control - mute switch - created in alsa-lib - which will be >>>>>> synced the internal PA path mute state like for the hardware mute >>>>>> switch. >>>>> >>>>> OK, but how would we create and manage the user control element? And >>>>> why it has to be user control? >>>> >>>> The softvol or alsactl can create the user control element. The alsa-lib >>>> softvol plugin and PA can silence stream according the state. >>> >>> And that's tricky if it's only with PA, as PA won't open a softvol PCM >>> stream... >> >> The protection is in alsa-lib, so we can skip to check this hint flag for this >> particular case like: >> >> https://github.com/alsa-project/alsa-lib/pull/121/commits/1acc1c7eccab0359996b25de54a6b6e0aa1e0c17 >> >> So it may depend on the softvol config not PA itself. > > Thanks, that's what I missed from the big picture. > >> Even for the solution bellow, we need to modify softvol to handle the kernel >> control elements, too. Actually softvol is not active, when the specified >> control element is found and this element is not from the user space. > > Hm, that would certainly work, but as we discussed before, it enforces > the softvol PCM process for PA. That isn't too bad for the capture, > fortunately, but not ideal, OTOH. PA can work without softvol in this case, but in my opinion, it may be better to force the silence in UCM (do not depend only on the implementation of the UCM application - like PA - more security). As I wrote before, the silencing can be done in alsa-lib AND in PA (simultaneously). If we use only mute switch control configuration (on/off - not volume) for sofvol, then the code in alsa-lib is really effective without any performance penalty. > And, I'm not sure how PA can take any control as its main capture mute > switch, if it's named differently. Wouldn't we need to change mixer > path in PA as well? And, if we may change PA side, it sounds more > natural to change a control directly in PA's mixer path. The softvol > doesn't fit well with PA, after all. The ACP microphone is handled via UCM, so we can add the new control switch identifier to UCM. No PA modifications should be required. And the LED state can be handled via the phantom control (AMD ACP) + legacy HDA capture switch. So two controls from two different sound cards may be marked with the LED microphone group, if it's the correct way for users. Jaroslav
On Thu, Feb 25, 2021 at 07:45:15PM +0100, Hans de Goede wrote: > To me it seems like a really bad idea to add userspace API for this now, > when we don't actually have hardware which needs this. Introducing > userspace API for this now introduces a significant risk that we get the > API wrong, since we don't actuall have a use-case where we actually need > the suggested flexibility. And then if such a use-case does eventually > pop-up we might very well have gotten the userspace API for this wrong. > I'm not saying that we will never need such flexibility, but we do not > need it *now*, so as I said before lets cross that bridge when we reach it. I don't want to get stuck in a cycle of "why can't my system just do what this other system does", or worse end up with problems due to competing system requirements when patches go in on more flexible devices because I didn't notice that the device wasn't a good fit for this sort of thing but people have the expectation that the kernel will transparently handle things.
Hi, On 3/1/21 2:23 PM, Mark Brown wrote: > On Thu, Feb 25, 2021 at 07:45:15PM +0100, Hans de Goede wrote: > >> To me it seems like a really bad idea to add userspace API for this now, >> when we don't actually have hardware which needs this. Introducing >> userspace API for this now introduces a significant risk that we get the >> API wrong, since we don't actuall have a use-case where we actually need >> the suggested flexibility. And then if such a use-case does eventually >> pop-up we might very well have gotten the userspace API for this wrong. > >> I'm not saying that we will never need such flexibility, but we do not >> need it *now*, so as I said before lets cross that bridge when we reach it. > > I don't want to get stuck in a cycle of "why can't my system just do > what this other system does", or worse end up with problems due to > competing system requirements when patches go in on more flexible > devices because I didn't notice that the device wasn't a good fit for > this sort of thing but people have the expectation that the kernel will > transparently handle things. So what do you want / how do you want this to work ? Regards, Hans
Hi, On 3/1/21 9:43 PM, Mark Brown wrote: > On Mon, Mar 01, 2021 at 08:49:34PM +0100, Hans de Goede wrote: >> On 3/1/21 8:15 PM, Mark Brown wrote: > >>> Off the top of my head something like writing a control name into a >>> sysfs file might work, it doesn't scale if you need to use multiple >>> controls as rt5640 does though. > >> Currently ALSA/UCM does not use sysfs files for anything, so this >> feels very inconsistent with how all the rest of this currently works. > > Yes, you'd really want to add string controls in ALSA. Hmm, we already have SNDRV_CTL_ELEM_TYPE_BYTES controls. I think that will work nicely actually, we can have the UCM conf file send a 0 terminated string to the driver that way. It would be nice to have some syntactic sugar on the UCM side to be able to actually specify a string instead of an array of bytes, but I don't think we need any new userspace API for this. This can be combined with a SND_SOC_... macro + helper to add an entry to the usual snd_kcontrol_new <codec-name>_snd_controls[] table for this, which when the control gets set/put will walk over all the controls and find one with a matching name and then add the access flag which Jaroslav's code uses to control the led-trigger to the matching control. I think that that should work nicely for the use-cases which I have ATM and should be flexible enough for future cases. The same control can even be written multiple times to set the flag on multiple controls and we could have a write of an empty string clear the flag on all controls. So in UCM (with the syntactic sugar) for the rt5640 we could then e.g. use something like this: # Clear Speaker Mute LED flags from all controls, then set it # on the Speaker and HP Channel Switches cset "name='Speaker Mute LED Control' ''" cset "name='Speaker Mute LED Control' 'Speaker Channel Switch'" cset "name='Speaker Mute LED Control' 'HP Channel Switch'" Mark, does this sound like an acceptable solution to you ? I know you will want to see the actual code before you can give me a definitive yes on this, but I would like to know that this at least looks like it would be acceptable to you before spending time on coding this out and testing it. Regards, Hans
On Mon, Mar 01, 2021 at 10:26:40PM +0100, Hans de Goede wrote:
> Mark, does this sound like an acceptable solution to you ?
Yes, I think this looks like a reasonable approach.
Dne 01. 03. 21 v 22:26 Hans de Goede napsal(a): > Hi, > > On 3/1/21 9:43 PM, Mark Brown wrote: >> On Mon, Mar 01, 2021 at 08:49:34PM +0100, Hans de Goede wrote: >>> On 3/1/21 8:15 PM, Mark Brown wrote: >> >>>> Off the top of my head something like writing a control name into a >>>> sysfs file might work, it doesn't scale if you need to use multiple >>>> controls as rt5640 does though. >> >>> Currently ALSA/UCM does not use sysfs files for anything, so this >>> feels very inconsistent with how all the rest of this currently works. >> >> Yes, you'd really want to add string controls in ALSA. > > Hmm, we already have SNDRV_CTL_ELEM_TYPE_BYTES controls. I think that will > work nicely actually, we can have the UCM conf file send a 0 terminated > string to the driver that way. It would be nice to have some syntactic > sugar on the UCM side to be able to actually specify a string instead > of an array of bytes, but I don't think we need any new userspace API > for this. The LEDs are controlled per machine not per card. So do we need to create the 'Speaker/Mic LED Control' control for all cards? Also, this change sounds really generic. The interface may be implemented in my proposed control led kernel module, not in the codec drivers. The Mark's sysfs idea is not bad in my opinion. The sequences may be extended in UCM, we have already 'exec' command. Yes, this command is a little heavy for the sysfs writes, but we can add command like 'sysset' or so for sysfs like: # detach all speaker LED controls for card 1 # similar to 'echo -n "card=1,*" > /sysfs/devices/virtual/sound/ctl-led/speaker/detach' sysset "devices/virtual/sound/ctl-led/speaker/detach:card=1,*" # attach the 'Speaker Playback Switch',10 control to speaker LED trigger in card 1 # similar to 'echo -n "card=1,iface=MIXER,name='Speaker Playback Switch',index=10" > /sysfs/devices/virtual/sound/ctl-led/speaker/attach sysset "devices/virtual/sound/ctl-led/speaker/attach:card=1,iface=MIXER,name='Speaker Playback Switch',index=10" Security: The LED-control bindings should be handled only in the boot / init phase (thus in UCM BootSequence section) and the sysfs interface files should be read-only for normal users. Jaroslav
Hi, On 3/2/21 10:14 PM, Jaroslav Kysela wrote: > Dne 01. 03. 21 v 22:26 Hans de Goede napsal(a): >> Hi, >> >> On 3/1/21 9:43 PM, Mark Brown wrote: >>> On Mon, Mar 01, 2021 at 08:49:34PM +0100, Hans de Goede wrote: >>>> On 3/1/21 8:15 PM, Mark Brown wrote: >>> >>>>> Off the top of my head something like writing a control name into a >>>>> sysfs file might work, it doesn't scale if you need to use multiple >>>>> controls as rt5640 does though. >>> >>>> Currently ALSA/UCM does not use sysfs files for anything, so this >>>> feels very inconsistent with how all the rest of this currently works. >>> >>> Yes, you'd really want to add string controls in ALSA. >> >> Hmm, we already have SNDRV_CTL_ELEM_TYPE_BYTES controls. I think that will >> work nicely actually, we can have the UCM conf file send a 0 terminated >> string to the driver that way. It would be nice to have some syntactic >> sugar on the UCM side to be able to actually specify a string instead >> of an array of bytes, but I don't think we need any new userspace API >> for this. > > The LEDs are controlled per machine not per card. So do we need to create the 'Speaker/Mic LED Control' control for all cards? > > Also, this change sounds really generic. The interface may be implemented in my proposed control led kernel module, not in the codec drivers. > > The Mark's sysfs idea is not bad in my opinion. The sequences may be extended in UCM, we have already 'exec' command. Yes, this command is a little heavy for the sysfs writes, but we can add command like 'sysset' or so for sysfs like: Okay, so this would be a sysfs file per card then? Sol we would have for example 2 new sysfs files like this show up when your module is loaded: /sys/class/sounds/card0/spk_mute_led_control /sys/class/sounds/card0/mic_mute_led_control And reading would iterate over all mixer-elements of the card and print the names of those which have the relevant access LED flag set, where as a write would be taken as a control-name to add the access LED flag too? And an empty write would be special and clear the flag on all controls? I guess we don't strictly need that if we only set things up at boot once, but it might still be handy to force things to a clean state. > > # detach all speaker LED controls for card 1 > # similar to 'echo -n "card=1,*" > /sysfs/devices/virtual/sound/ctl-led/speaker/detach' > sysset "devices/virtual/sound/ctl-led/speaker/detach:card=1,*" > > # attach the 'Speaker Playback Switch',10 control to speaker LED trigger in card 1 > # similar to 'echo -n "card=1,iface=MIXER,name='Speaker Playback Switch',index=10" > /sysfs/devices/virtual/sound/ctl-led/speaker/attach > sysset "devices/virtual/sound/ctl-led/speaker/attach:card=1,iface=MIXER,name='Speaker Playback Switch',index=10" I think a sysfs file per card would work better, that would certainly be a lot more inline with how sysfs is normally used... Also do we need the iface=MIXER part ? > Security: The LED-control bindings should be handled only in the boot / init phase (thus in UCM BootSequence section) and the sysfs interface files should be read-only for normal users. Yes that make sense, but it will require some extra helper to that, I guess it could be an extra flag to the alsactl restore command which already gets run at boot, or an extra alsactl command ? Regards, Hans
Dne 04. 03. 21 v 20:39 Hans de Goede napsal(a): > Hi, > > On 3/2/21 10:14 PM, Jaroslav Kysela wrote: >> Dne 01. 03. 21 v 22:26 Hans de Goede napsal(a): >>> Hi, >>> >>> On 3/1/21 9:43 PM, Mark Brown wrote: >>>> On Mon, Mar 01, 2021 at 08:49:34PM +0100, Hans de Goede wrote: >>>>> On 3/1/21 8:15 PM, Mark Brown wrote: >>>> >>>>>> Off the top of my head something like writing a control name into a >>>>>> sysfs file might work, it doesn't scale if you need to use multiple >>>>>> controls as rt5640 does though. >>>> >>>>> Currently ALSA/UCM does not use sysfs files for anything, so this >>>>> feels very inconsistent with how all the rest of this currently works. >>>> >>>> Yes, you'd really want to add string controls in ALSA. >>> >>> Hmm, we already have SNDRV_CTL_ELEM_TYPE_BYTES controls. I think that will >>> work nicely actually, we can have the UCM conf file send a 0 terminated >>> string to the driver that way. It would be nice to have some syntactic >>> sugar on the UCM side to be able to actually specify a string instead >>> of an array of bytes, but I don't think we need any new userspace API >>> for this. >> >> The LEDs are controlled per machine not per card. So do we need to create the 'Speaker/Mic LED Control' control for all cards? >> >> Also, this change sounds really generic. The interface may be implemented in my proposed control led kernel module, not in the codec drivers. >> >> The Mark's sysfs idea is not bad in my opinion. The sequences may be extended in UCM, we have already 'exec' command. Yes, this command is a little heavy for the sysfs writes, but we can add command like 'sysset' or so for sysfs like: > > Okay, so this would be a sysfs file per card then? Sol we would have for example > 2 new sysfs files like this show up when your module is loaded: > > /sys/class/sounds/card0/spk_mute_led_control > /sys/class/sounds/card0/mic_mute_led_control > > And reading would iterate over all mixer-elements of the card and print > the names of those which have the relevant access LED flag set, where > as a write would be taken as a control-name to add the access LED flag > too? It depends if we want to have this feature as an independent add-on (implemented in the generic sound LED module) or if we tie this more to the ALSA's core control code. My proposal creates virtual sound ctl-led driver - thus /sysfs/devices/virtual/sound/ctl-led/ tree. We can add a subdirectory per card there like: /sysfs/devices/virtual/sound/ctl-led/speaker/card0/attach ... > And an empty write would be special and clear the flag on all controls? > I guess we don't strictly need that if we only set things up at boot once, > but it might still be handy to force things to a clean state. The list operations should be probably identified by separate sysfs files. /sysfs/devices/virtual/sound/ctl-led/speaker/card0/attach /sysfs/devices/virtual/sound/ctl-led/speaker/card0/detach /sysfs/devices/virtual/sound/ctl-led/speaker/card0/reset /sysfs/devices/virtual/sound/ctl-led/speaker/card0/controls And /sys/class/sounds/card0/controlC0/led-speaker may be a symlink to /sysfs/devices/virtual/sound/ctl-led/speaker/card0/ . >> # detach all speaker LED controls for card 1 >> # similar to 'echo -n "card=1,*" > /sysfs/devices/virtual/sound/ctl-led/speaker/detach' >> sysset "devices/virtual/sound/ctl-led/speaker/detach:card=1,*" >> >> # attach the 'Speaker Playback Switch',10 control to speaker LED trigger in card 1 >> # similar to 'echo -n "card=1,iface=MIXER,name='Speaker Playback Switch',index=10" > /sysfs/devices/virtual/sound/ctl-led/speaker/attach >> sysset "devices/virtual/sound/ctl-led/speaker/attach:card=1,iface=MIXER,name='Speaker Playback Switch',index=10" > > I think a sysfs file per card would work better, that would certainly be > a lot more inline with how sysfs is normally used... > > Also do we need the iface=MIXER part ? It was just a complete example (element ID specification in a string from alsa-lib/amixer). Of course, the other element types won't be probably used for the LED functionality... >> Security: The LED-control bindings should be handled only in the boot / init phase (thus in UCM BootSequence section) and the sysfs interface files should be read-only for normal users. > > Yes that make sense, but it will require some extra helper to that, I guess it > could be an extra flag to the alsactl restore command which already gets run > at boot, or an extra alsactl command ? The alsactl does the BootSequence initialization when UCM is supported in alsa-lib. But, we have a little issue that the this sequence is executed only if some controls are changed (added or removed) between last alsa state config (/var/lib/alsa/asound.state) or if the state for the given card does not exist to not override the values which may be changed by the user. It really depends on the control API only. Apparently, we need another sequence which will be forcefully executed on boot. ColdSequence , FixedBootSequence, ForcedSequence, ForcedBootSequence ? Jaroslav
Hi, On 3/5/21 2:02 PM, Jaroslav Kysela wrote: > Dne 04. 03. 21 v 20:39 Hans de Goede napsal(a): >> Hi, >> >> On 3/2/21 10:14 PM, Jaroslav Kysela wrote: >>> Dne 01. 03. 21 v 22:26 Hans de Goede napsal(a): >>>> Hi, >>>> >>>> On 3/1/21 9:43 PM, Mark Brown wrote: >>>>> On Mon, Mar 01, 2021 at 08:49:34PM +0100, Hans de Goede wrote: >>>>>> On 3/1/21 8:15 PM, Mark Brown wrote: >>>>> >>>>>>> Off the top of my head something like writing a control name into a >>>>>>> sysfs file might work, it doesn't scale if you need to use multiple >>>>>>> controls as rt5640 does though. >>>>> >>>>>> Currently ALSA/UCM does not use sysfs files for anything, so this >>>>>> feels very inconsistent with how all the rest of this currently works. >>>>> >>>>> Yes, you'd really want to add string controls in ALSA. >>>> >>>> Hmm, we already have SNDRV_CTL_ELEM_TYPE_BYTES controls. I think that will >>>> work nicely actually, we can have the UCM conf file send a 0 terminated >>>> string to the driver that way. It would be nice to have some syntactic >>>> sugar on the UCM side to be able to actually specify a string instead >>>> of an array of bytes, but I don't think we need any new userspace API >>>> for this. >>> >>> The LEDs are controlled per machine not per card. So do we need to create the 'Speaker/Mic LED Control' control for all cards? >>> >>> Also, this change sounds really generic. The interface may be implemented in my proposed control led kernel module, not in the codec drivers. >>> >>> The Mark's sysfs idea is not bad in my opinion. The sequences may be extended in UCM, we have already 'exec' command. Yes, this command is a little heavy for the sysfs writes, but we can add command like 'sysset' or so for sysfs like: >> >> Okay, so this would be a sysfs file per card then? Sol we would have for example >> 2 new sysfs files like this show up when your module is loaded: >> >> /sys/class/sounds/card0/spk_mute_led_control >> /sys/class/sounds/card0/mic_mute_led_control >> >> And reading would iterate over all mixer-elements of the card and print >> the names of those which have the relevant access LED flag set, where >> as a write would be taken as a control-name to add the access LED flag >> too? > > It depends if we want to have this feature as an independent add-on > (implemented in the generic sound LED module) or if we tie this more to > the ALSA's core control code. > > My proposal creates virtual sound ctl-led driver - thus > /sysfs/devices/virtual/sound/ctl-led/ tree. We can add a subdirectory per card > there like: > > /sysfs/devices/virtual/sound/ctl-led/speaker/card0/attach > > ... Ok, I see. >> And an empty write would be special and clear the flag on all controls? >> I guess we don't strictly need that if we only set things up at boot once, >> but it might still be handy to force things to a clean state. > > The list operations should be probably identified by separate sysfs files. > > /sysfs/devices/virtual/sound/ctl-led/speaker/card0/attach > /sysfs/devices/virtual/sound/ctl-led/speaker/card0/detach > /sysfs/devices/virtual/sound/ctl-led/speaker/card0/reset > /sysfs/devices/virtual/sound/ctl-led/speaker/card0/controls Ack, that would be better. > And /sys/class/sounds/card0/controlC0/led-speaker may be a symlink to > /sysfs/devices/virtual/sound/ctl-led/speaker/card0/ . > >>> # detach all speaker LED controls for card 1 >>> # similar to 'echo -n "card=1,*" > /sysfs/devices/virtual/sound/ctl-led/speaker/detach' >>> sysset "devices/virtual/sound/ctl-led/speaker/detach:card=1,*" >>> >>> # attach the 'Speaker Playback Switch',10 control to speaker LED trigger in card 1 >>> # similar to 'echo -n "card=1,iface=MIXER,name='Speaker Playback Switch',index=10" > /sysfs/devices/virtual/sound/ctl-led/speaker/attach >>> sysset "devices/virtual/sound/ctl-led/speaker/attach:card=1,iface=MIXER,name='Speaker Playback Switch',index=10" >> >> I think a sysfs file per card would work better, that would certainly be >> a lot more inline with how sysfs is normally used... >> >> Also do we need the iface=MIXER part ? > > It was just a complete example (element ID specification in a string from > alsa-lib/amixer). Of course, the other element types won't be probably used > for the LED functionality... Ok, the reason I was asking is because if possible we should having to avoid to do string-parsing inside the kernel. So if the sysfs files just take a control-name without any of the name=value pair stuff going on that would be better. > >>> Security: The LED-control bindings should be handled only in the boot / init phase (thus in UCM BootSequence section) and the sysfs interface files should be read-only for normal users. >> >> Yes that make sense, but it will require some extra helper to that, I guess it >> could be an extra flag to the alsactl restore command which already gets run >> at boot, or an extra alsactl command ? > > The alsactl does the BootSequence initialization when UCM is supported in > alsa-lib. But, we have a little issue that the this sequence is executed only > if some controls are changed (added or removed) between last alsa state config > (/var/lib/alsa/asound.state) or if the state for the given card does not exist > to not override the values which may be changed by the user. It really depends > on the control API only. > > Apparently, we need another sequence which will be forcefully executed on > boot. ColdSequence , FixedBootSequence, ForcedSequence, ForcedBootSequence ? FixedBootSequence seems the best. This does seem to have quickly grown into something somewhat complex though. While all that is necessary for the 2 ASoC using devices with actual mute-LEDs which I'm trying to get supported is 2 very simple static assignments of the LED flag. I've just completed some alsa-lib work to deal with the sometimes quirky control-names and with that in place, the spk-mute-led flag can be statically set on the access flags of the Speaker + HP output mute controls as Mark suggested as an alternative. That seems a lot more KISS to me. But I guess the flexibility this gives us will also be helpful for some HDA / other SOF cases like the Dell privacy stuff which also involves tying a mute control on an ASoC codec to a led-trigger ? Regards, Hans
diff --git a/sound/soc/codecs/rt5670.c b/sound/soc/codecs/rt5670.c index feab15d0686a..9233710e3a4f 100644 --- a/sound/soc/codecs/rt5670.c +++ b/sound/soc/codecs/rt5670.c @@ -690,7 +690,8 @@ static const struct snd_kcontrol_new rt5670_snd_controls[] = { /* DAC Digital Volume */ SOC_DOUBLE("DAC2 Playback Switch", RT5670_DAC_CTRL, RT5670_M_DAC_L2_VOL_SFT, RT5670_M_DAC_R2_VOL_SFT, 1, 1), - SOC_DOUBLE_EXT("DAC1 Playback Switch", SND_SOC_NOPM, 0, 1, 1, 0, + SOC_DOUBLE_EXT_ACCESS("DAC1 Playback Switch", SND_SOC_NOPM, 0, 1, 1, 0, + SNDRV_CTL_ELEM_ACCESS_SPK_LED, rt5670_dac1_playback_switch_get, rt5670_dac1_playback_switch_put), SOC_DOUBLE_TLV("DAC1 Playback Volume", RT5670_DAC1_DIG_VOL, RT5670_L_VOL_SFT, RT5670_R_VOL_SFT, @@ -708,8 +709,9 @@ static const struct snd_kcontrol_new rt5670_snd_controls[] = { RT5670_INL_VOL_SFT, RT5670_INR_VOL_SFT, 31, 1, in_vol_tlv), /* ADC Digital Volume Control */ - SOC_DOUBLE("ADC Capture Switch", RT5670_STO1_ADC_DIG_VOL, - RT5670_L_MUTE_SFT, RT5670_R_MUTE_SFT, 1, 1), + SOC_DOUBLE_ACCESS("ADC Capture Switch", RT5670_STO1_ADC_DIG_VOL, + RT5670_L_MUTE_SFT, RT5670_R_MUTE_SFT, 1, 1, + SNDRV_CTL_ELEM_ACCESS_MIC_LED), SOC_DOUBLE_TLV("ADC Capture Volume", RT5670_STO1_ADC_DIG_VOL, RT5670_L_VOL_SFT, RT5670_R_VOL_SFT, 127, 0, adc_vol_tlv), @@ -3253,6 +3255,8 @@ static int rt5670_i2c_probe(struct i2c_client *i2c, } + snd_ctl_led_request(); + pm_runtime_enable(&i2c->dev); pm_request_idle(&i2c->dev);
Add support for controlling a speaker and/or microphone mute LED through LED triggers using the new generic LED trigger module. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- sound/soc/codecs/rt5670.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)