| Message ID | 20210211111400.1131020-1-perex@perex.cz (mailing list archive) |
|---|---|
| Headers | show |
| Series | ALSA: control - add generic LED trigger code | expand |
On Thu, 11 Feb 2021 12:13:55 +0100, Jaroslav Kysela wrote: > > This patchset tries to resolve the diversity in the audio LED > control among the ALSA drivers. > > A new control layer registration is introduced which allows > to run additional operations on top of the elementary ALSA > sound controls. > > A new control access group (three bits in the access flags) > was introduced to carry the LED group information for > the sound controls. The low-level sound drivers can just > mark those controls using this access group. This information > is exported to the user space and eventually the user space > can create sound controls which can belong to a LED group. > > The actual state ('route') evaluation is really easy > (the minimal value check for all channels / controls / cards). > If there's more complicated logic for a given hardware, > the card driver may eventually export a new read-only > sound control for the LED group and do the logic itself. > > The new LED trigger control code is completely separated > and possibly optional (there's no symbol dependency). > The full code separation allows eventually to move this > LED trigger control to the user space in future. > Actually it replaces the already present functionality > in the kernel space (HDA drivers) and allows a quick adoption > for the recent hardware (SoundWire ASoC codecs). > > # lsmod | grep snd_ctl_led > snd_ctl_led 16384 0 > > The sound driver implementation is really easy: > > 1) call snd_ctl_led_request() when control LED layer should be > automatically activated > / it calls module_request("snd-ctl-led") on demand / > 2) mark all related kcontrols with > SNDRV_CTL_ELEM_ACCESS_SPK_LED or > SNDRV_CTL_ELEM_ACCESS_MIC_LED > > Original RFC: https://lore.kernel.org/alsa-devel/20210207201157.869972-1-perex@perex.cz/ > > Cc: Hans de Goede <hdegoede@redhat.com> > Cc: Perry Yuan <Perry.Yuan@dell.com> > > Jaroslav Kysela (5): > ALSA: control - introduce snd_ctl_notify_one() helper > ALSA: control - add layer registration routines > ALSA: control - add generic LED trigger module as the new control > layer > ALSA: HDA - remove the custom implementation for the audio LED trigger > ALSA: control - add sysfs support to the LED trigger module Thanks for the patch. I'm afraid that it's a bit too late for 5.12, as the merge window will be likely closed soon. For 5.13, we'll have enough time for get a consensus about the design. Whether this is the best way to go, or we should rather consider user-space solution as Sakamoto-san mentioned: that has to be decided. Back to the design: your new implementation allows the separation and the dynamic opt-in, which is nice. Thanks for that. This looks generic and may be extended for other purposes in future, too. One thing I still miss from the picture is how to deal with the case like AMD ACP. It has no mixer control to bundle with the LED trigger. Your idea is to make a (dummy) user element and tie the LED trigger with it? Another slight concern is the possible regression: by moving the mute-LED mode enum stuff into the sysfs, user will get incompatibilities after the kernel update. And it's not that trivial to change the sysfs entry as default for each user. It needs some detailed documentation or some temporary workaround (e.g. keep providing the controls for now but warns if the value is changed from the default value via the controls). thanks, Takashi
Dne 11. 02. 21 v 18:15 Takashi Iwai napsal(a): >> Jaroslav Kysela (5): >> ALSA: control - introduce snd_ctl_notify_one() helper >> ALSA: control - add layer registration routines >> ALSA: control - add generic LED trigger module as the new control >> layer >> ALSA: HDA - remove the custom implementation for the audio LED trigger >> ALSA: control - add sysfs support to the LED trigger module > One thing I still miss from the picture is how to deal with the case > like AMD ACP. It has no mixer control to bundle with the LED trigger. > Your idea is to make a (dummy) user element and tie the LED trigger > with it? Yes, the user-space code which guarantee the silence stream should create an user space control with the appropriate LED group access bits. The alsa-lib's softvol PCM plugin can do this silencing for example. > Another slight concern is the possible regression: by moving the > mute-LED mode enum stuff into the sysfs, user will get > incompatibilities after the kernel update. And it's not that trivial > to change the sysfs entry as default for each user. > It needs some detailed documentation or some temporary workaround > (e.g. keep providing the controls for now but warns if the value is > changed from the default value via the controls). I don't think that we have a user space application which is using those controls (Pulseaudio or so..) in an abstract way. I think that it's really minor issue. We should probably concentrate for the main designed purpose (notify about the mute / silent state) and handle those add-on features as an experimental stuff. Jaroslav
On Thu, 11 Feb 2021 18:53:20 +0100, Jaroslav Kysela wrote: > > Dne 11. 02. 21 v 18:15 Takashi Iwai napsal(a): > > >> Jaroslav Kysela (5): > >> ALSA: control - introduce snd_ctl_notify_one() helper > >> ALSA: control - add layer registration routines > >> ALSA: control - add generic LED trigger module as the new control > >> layer > >> ALSA: HDA - remove the custom implementation for the audio LED trigger > >> ALSA: control - add sysfs support to the LED trigger module > > > One thing I still miss from the picture is how to deal with the case > > like AMD ACP. It has no mixer control to bundle with the LED trigger. > > Your idea is to make a (dummy) user element and tie the LED trigger > > with it? > > Yes, the user-space code which guarantee the silence stream should create an > user space control with the appropriate LED group access bits. The alsa-lib's > softvol PCM plugin can do this silencing for example. What control would it create? In the case of softvol, it's a volume control that really changes the volume. For the mute LED, it's a control turn on/off the mute? If so, I wonder what makes better than creating it from the kernel driver. (Of course, we can list up like "flexibility", etc, but it has a flip side of "complexity" and "fragility"...) > > Another slight concern is the possible regression: by moving the > > mute-LED mode enum stuff into the sysfs, user will get > > incompatibilities after the kernel update. And it's not that trivial > > to change the sysfs entry as default for each user. > > It needs some detailed documentation or some temporary workaround > > (e.g. keep providing the controls for now but warns if the value is > > changed from the default value via the controls). > > I don't think that we have a user space application which is using those > controls (Pulseaudio or so..) in an abstract way. I think that it's really > minor issue. We should probably concentrate for the main designed purpose > (notify about the mute / silent state) and handle those add-on features as an > experimental stuff. I'm sure that there are users of the reverse mic-mute LED ("follow capture" mode); the feature was added because of the explicit request from my colleague, and this mode works no matter whether ALSA native or PA is used. Not sure about "on" and "off" mode; maybe there can be some users who want to disable the LED. But, yes, this is a minor issue and should be in a lower priority. It's just as a reminder. thanks, Takashi
Dne 12. 02. 21 v 10:23 Takashi Iwai napsal(a): > On Thu, 11 Feb 2021 18:53:20 +0100, > Jaroslav Kysela wrote: >> >> Dne 11. 02. 21 v 18:15 Takashi Iwai napsal(a): >> >>>> Jaroslav Kysela (5): >>>> ALSA: control - introduce snd_ctl_notify_one() helper >>>> ALSA: control - add layer registration routines >>>> ALSA: control - add generic LED trigger module as the new control >>>> layer >>>> ALSA: HDA - remove the custom implementation for the audio LED trigger >>>> ALSA: control - add sysfs support to the LED trigger module >> >>> One thing I still miss from the picture is how to deal with the case >>> like AMD ACP. It has no mixer control to bundle with the LED trigger. >>> Your idea is to make a (dummy) user element and tie the LED trigger >>> with it? >> >> Yes, the user-space code which guarantee the silence stream should create an >> user space control with the appropriate LED group access bits. The alsa-lib's >> softvol PCM plugin can do this silencing for example. > > What control would it create? In the case of softvol, it's a volume > control that really changes the volume. For the mute LED, it's a > control turn on/off the mute? If so, I wonder what makes better than > creating it from the kernel driver. (Of course, we can list up like > "flexibility", etc, but it has a flip side of "complexity" and > "fragility"...) The current code handles both switch / volume for the marked control (assuming that the minimal value - usually zero - is full mute). And actually, there are snd_pcm_areas_silence() calls in the softvol plugin, so we know that the PCM stream is not passed to the application at this point. Condition: if (info.type == SNDRV_CTL_ELEM_TYPE_BOOLEAN || info.type == SNDRV_CTL_ELEM_TYPE_INTEGER) ... value.value.integer.value[i] != info.value.integer.min The softvol plugin may be extended to add the mute switch control, of course. >>> Another slight concern is the possible regression: by moving the >>> mute-LED mode enum stuff into the sysfs, user will get >>> incompatibilities after the kernel update. And it's not that trivial >>> to change the sysfs entry as default for each user. >>> It needs some detailed documentation or some temporary workaround >>> (e.g. keep providing the controls for now but warns if the value is >>> changed from the default value via the controls). >> >> I don't think that we have a user space application which is using those >> controls (Pulseaudio or so..) in an abstract way. I think that it's really >> minor issue. We should probably concentrate for the main designed purpose >> (notify about the mute / silent state) and handle those add-on features as an >> experimental stuff. > > I'm sure that there are users of the reverse mic-mute LED ("follow > capture" mode); the feature was added because of the explicit request > from my colleague, and this mode works no matter whether ALSA native > or PA is used. I see. It looks like a corner case. The proposed sysfs based code is also user space independent. The issue with the HDA code is that it's card based, but the system wide LEDs should not be tied to one sound card. We are seeing that the hardware designers became very creative :-) Jaroslav
On Fri, 12 Feb 2021 11:32:38 +0100, Jaroslav Kysela wrote: > > Dne 12. 02. 21 v 10:23 Takashi Iwai napsal(a): > > On Thu, 11 Feb 2021 18:53:20 +0100, > > Jaroslav Kysela wrote: > >> > >> Dne 11. 02. 21 v 18:15 Takashi Iwai napsal(a): > >> > >>>> Jaroslav Kysela (5): > >>>> ALSA: control - introduce snd_ctl_notify_one() helper > >>>> ALSA: control - add layer registration routines > >>>> ALSA: control - add generic LED trigger module as the new control > >>>> layer > >>>> ALSA: HDA - remove the custom implementation for the audio LED trigger > >>>> ALSA: control - add sysfs support to the LED trigger module > >> > >>> One thing I still miss from the picture is how to deal with the case > >>> like AMD ACP. It has no mixer control to bundle with the LED trigger. > >>> Your idea is to make a (dummy) user element and tie the LED trigger > >>> with it? > >> > >> Yes, the user-space code which guarantee the silence stream should create an > >> user space control with the appropriate LED group access bits. The alsa-lib's > >> softvol PCM plugin can do this silencing for example. > > > > What control would it create? In the case of softvol, it's a volume > > control that really changes the volume. For the mute LED, it's a > > control turn on/off the mute? If so, I wonder what makes better than > > creating it from the kernel driver. (Of course, we can list up like > > "flexibility", etc, but it has a flip side of "complexity" and > > "fragility"...) > > The current code handles both switch / volume for the marked control (assuming > that the minimal value - usually zero - is full mute). And actually, there are > snd_pcm_areas_silence() calls in the softvol plugin, so we know that the PCM > stream is not passed to the application at this point. > > Condition: > > if (info.type == SNDRV_CTL_ELEM_TYPE_BOOLEAN || > info.type == SNDRV_CTL_ELEM_TYPE_INTEGER) > ... value.value.integer.value[i] != info.value.integer.min > > The softvol plugin may be extended to add the mute switch control, of course. Well, my question was what kind of mixer control will be added at all, although the chip does neither volume nor mute function. Would we add a fake volume/switch like softvol, or would we add rather a control that is directly tied with the LED state? > >>> Another slight concern is the possible regression: by moving the > >>> mute-LED mode enum stuff into the sysfs, user will get > >>> incompatibilities after the kernel update. And it's not that trivial > >>> to change the sysfs entry as default for each user. > >>> It needs some detailed documentation or some temporary workaround > >>> (e.g. keep providing the controls for now but warns if the value is > >>> changed from the default value via the controls). > >> > >> I don't think that we have a user space application which is using those > >> controls (Pulseaudio or so..) in an abstract way. I think that it's really > >> minor issue. We should probably concentrate for the main designed purpose > >> (notify about the mute / silent state) and handle those add-on features as an > >> experimental stuff. > > > > I'm sure that there are users of the reverse mic-mute LED ("follow > > capture" mode); the feature was added because of the explicit request > > from my colleague, and this mode works no matter whether ALSA native > > or PA is used. > I see. It looks like a corner case. The proposed sysfs based code is also user > space independent. The issue with the HDA code is that it's card based, but > the system wide LEDs should not be tied to one sound card. We are seeing that > the hardware designers became very creative :-) Heh, the flexiblity of the existing LED subsystem is already allowing to be creative, although I haven't seen much exotic usage in the reality (maybe the exception is the blinking at panic; IIRC, there is Morse code conversion, too? :) Takashi
Hi, On 2/11/21 12:13 PM, Jaroslav Kysela wrote: > This patchset tries to resolve the diversity in the audio LED > control among the ALSA drivers. > > A new control layer registration is introduced which allows > to run additional operations on top of the elementary ALSA > sound controls. > > A new control access group (three bits in the access flags) > was introduced to carry the LED group information for > the sound controls. The low-level sound drivers can just > mark those controls using this access group. This information > is exported to the user space and eventually the user space > can create sound controls which can belong to a LED group. > > The actual state ('route') evaluation is really easy > (the minimal value check for all channels / controls / cards). > If there's more complicated logic for a given hardware, > the card driver may eventually export a new read-only > sound control for the LED group and do the logic itself. > > The new LED trigger control code is completely separated > and possibly optional (there's no symbol dependency). > The full code separation allows eventually to move this > LED trigger control to the user space in future. > Actually it replaces the already present functionality > in the kernel space (HDA drivers) and allows a quick adoption > for the recent hardware (SoundWire ASoC codecs). > > # lsmod | grep snd_ctl_led > snd_ctl_led 16384 0 > > The sound driver implementation is really easy: > > 1) call snd_ctl_led_request() when control LED layer should be > automatically activated > / it calls module_request("snd-ctl-led") on demand / > 2) mark all related kcontrols with > SNDRV_CTL_ELEM_ACCESS_SPK_LED or > SNDRV_CTL_ELEM_ACCESS_MIC_LED So I've been running some tests with this,combined with writing UCM profiles for hw volume control, for some Intel Bay- and CherryTrail devices using Intel's Low Power Engine (LPE) for audio, which uses the ASoC framework. My work / experiments for this are progressing a bit slower then I would like, but that is not the fault of this patch-set, but rather an issue with hw-volume control mapping, see below for details. Leaving the ASoC implementation details aside, this patch-set works quite nicely to get the speaker mute-LED to work. There is one issue, I'm running my kernels with lockdep and this patchset triggers a lockdep warning: [ 24.487200] input: sof-bytcht rt5672 Headset as /devices/platform/80860F28:00/cht-bsw-rt5672/sound/card1/input19 [ 24.532006] ====================================================== [ 24.532010] WARNING: possible circular locking dependency detected [ 24.532015] 5.11.0-rc7+ #248 Tainted: G E [ 24.532019] ------------------------------------------------------ [ 24.532022] systemd-udevd/394 is trying to acquire lock: [ 24.532027] ffff922888ead720 (&card->controls_rwsem){++++}-{3:3}, at: snd_ctl_led_hello+0x65a/0x9f0 [snd_ctl_led] [ 24.532048] but task is already holding lock: [ 24.532051] ffffffffc0b0eb88 (snd_ctl_led_mutex){+.+.}-{3:3}, at: snd_ctl_led_hello+0x453/0x9f0 [snd_ctl_led] [ 24.532066] which lock already depends on the new lock. [ 24.532069] the existing dependency chain (in reverse order) is: [ 24.532072] -> #2 (snd_ctl_led_mutex){+.+.}-{3:3}: [ 24.532083] __mutex_lock+0xb8/0x7f0 [ 24.532094] snd_ctl_led_hello+0x8fd/0x9f0 [snd_ctl_led] [ 24.532100] snd_ctl_register_layer+0x48/0x360 [snd] [ 24.532112] 0xffffffffc078f149 [ 24.532119] do_one_initcall+0x6e/0x2e0 [ 24.532127] do_init_module+0x5c/0x260 [ 24.532135] load_module+0x2570/0x27e0 [ 24.532142] __do_sys_init_module+0x130/0x190 [ 24.532148] do_syscall_64+0x33/0x40 [ 24.532154] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 24.532161] -> #1 (snd_ctl_layer_rwsem){++++}-{3:3}: [ 24.532172] down_read+0x40/0x50 [ 24.532177] snd_ctl_notify_one+0x8d/0x150 [snd] [ 24.532188] snd_ctl_find_id+0x24e/0x350 [snd] [ 24.532197] snd_ctl_find_id+0x2f3/0x350 [snd] [ 24.532207] 0xffffffffc0786bab [ 24.532212] platform_probe+0x3f/0x90 [ 24.532219] really_probe+0xf2/0x440 [ 24.532225] driver_probe_device+0xe1/0x150 [ 24.532231] device_driver_attach+0xa8/0xb0 [ 24.532237] __driver_attach+0x8c/0x150 [ 24.532243] bus_for_each_dev+0x78/0xa0 [ 24.532249] bus_add_driver+0x12e/0x1f0 [ 24.532254] driver_register+0x8f/0xe0 [ 24.532260] do_one_initcall+0x6e/0x2e0 [ 24.532266] do_init_module+0x5c/0x260 [ 24.532272] load_module+0x2570/0x27e0 [ 24.532278] __do_sys_init_module+0x130/0x190 [ 24.532285] do_syscall_64+0x33/0x40 [ 24.532290] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 24.532296] -> #0 (&card->controls_rwsem){++++}-{3:3}: [ 24.532307] __lock_acquire+0x113d/0x1e10 [ 24.532314] lock_acquire+0xe4/0x390 [ 24.532320] down_read+0x40/0x50 [ 24.532325] snd_ctl_led_hello+0x65a/0x9f0 [snd_ctl_led] [ 24.532331] snd_ctl_led_hello+0x959/0x9f0 [snd_ctl_led] [ 24.532337] snd_ctl_register_layer+0x183/0x360 [snd] [ 24.532347] snd_device_register_all+0x4c/0x60 [snd] [ 24.532357] snd_card_register+0x74/0x1d0 [snd] [ 24.532366] snd_soc_set_dmi_name+0xb2a/0xc30 [snd_soc_core] [ 24.532393] devm_snd_soc_register_card+0x43/0x90 [snd_soc_core] [ 24.532412] 0xffffffffc0cf3579 [ 24.532419] platform_probe+0x3f/0x90 [ 24.532424] really_probe+0xf2/0x440 [ 24.532430] driver_probe_device+0xe1/0x150 [ 24.532436] device_driver_attach+0xa8/0xb0 [ 24.532442] __driver_attach+0x8c/0x150 [ 24.532447] bus_for_each_dev+0x78/0xa0 [ 24.532453] bus_add_driver+0x12e/0x1f0 [ 24.532459] driver_register+0x8f/0xe0 [ 24.532465] do_one_initcall+0x6e/0x2e0 [ 24.532471] do_init_module+0x5c/0x260 [ 24.532477] load_module+0x2570/0x27e0 [ 24.532483] __do_sys_init_module+0x130/0x190 [ 24.532489] do_syscall_64+0x33/0x40 [ 24.532494] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 24.532501] other info that might help us debug this: [ 24.532504] Chain exists of: &card->controls_rwsem --> snd_ctl_layer_rwsem --> snd_ctl_led_mutex [ 24.532517] Possible unsafe locking scenario: [ 24.532520] CPU0 CPU1 [ 24.532523] ---- ---- [ 24.532526] lock(snd_ctl_led_mutex); [ 24.532532] lock(snd_ctl_layer_rwsem); [ 24.532538] lock(snd_ctl_led_mutex); [ 24.532544] lock(&card->controls_rwsem); [ 24.532550] *** DEADLOCK *** [ 24.532553] 5 locks held by systemd-udevd/394: [ 24.532558] #0: ffff922883ec2988 (&dev->mutex){....}-{3:3}, at: device_driver_attach+0x3b/0xb0 [ 24.532574] #1: ffffffffc088e1c8 (client_mutex){+.+.}-{3:3}, at: snd_soc_set_dmi_name+0x2f8/0xc30 [snd_soc_core] [ 24.532604] #2: ffffffffc0cf71f0 (&card->mutex){+.+.}-{3:3}, at: snd_soc_set_dmi_name+0x30e/0xc30 [snd_soc_core] [ 24.532632] #3: ffffffffc07562b0 (snd_ctl_layer_rwsem){++++}-{3:3}, at: snd_ctl_register_layer+0x16b/0x360 [snd] [ 24.532652] #4: ffffffffc0b0eb88 (snd_ctl_led_mutex){+.+.}-{3:3}, at: snd_ctl_led_hello+0x453/0x9f0 [snd_ctl_led] [ 24.532668] stack backtrace: [ 24.532672] CPU: 2 PID: 394 Comm: systemd-udevd Tainted: G E 5.11.0-rc7+ #248 [ 24.532679] Hardware name: LENOVO 20C1000VMH/20C1000VMH, BIOS GUET86WW (1.86) 10/21/2019 [ 24.532683] Call Trace: [ 24.532691] dump_stack+0x8b/0xb0 [ 24.532702] check_noncircular+0xfb/0x110 [ 24.532713] __lock_acquire+0x113d/0x1e10 [ 24.532722] ? lock_acquire+0xe4/0x390 [ 24.532730] lock_acquire+0xe4/0x390 [ 24.532737] ? snd_ctl_led_hello+0x65a/0x9f0 [snd_ctl_led] [ 24.532747] down_read+0x40/0x50 [ 24.532754] ? snd_ctl_led_hello+0x65a/0x9f0 [snd_ctl_led] [ 24.532761] snd_ctl_led_hello+0x65a/0x9f0 [snd_ctl_led] [ 24.532771] snd_ctl_led_hello+0x959/0x9f0 [snd_ctl_led] [ 24.532779] snd_ctl_register_layer+0x183/0x360 [snd] [ 24.532791] snd_device_register_all+0x4c/0x60 [snd] [ 24.532803] snd_card_register+0x74/0x1d0 [snd] [ 24.532816] snd_soc_set_dmi_name+0xb2a/0xc30 [snd_soc_core] [ 24.532838] devm_snd_soc_register_card+0x43/0x90 [snd_soc_core] [ 24.532860] 0xffffffffc0cf3579 [ 24.532869] platform_probe+0x3f/0x90 [ 24.532876] really_probe+0xf2/0x440 [ 24.532885] driver_probe_device+0xe1/0x150 [ 24.532893] device_driver_attach+0xa8/0xb0 [ 24.532901] __driver_attach+0x8c/0x150 [ 24.532908] ? device_driver_attach+0xb0/0xb0 [ 24.532914] ? device_driver_attach+0xb0/0xb0 [ 24.532922] bus_for_each_dev+0x78/0xa0 [ 24.532930] bus_add_driver+0x12e/0x1f0 [ 24.532937] driver_register+0x8f/0xe0 [ 24.532944] ? 0xffffffffc0cfa000 [ 24.532950] do_one_initcall+0x6e/0x2e0 [ 24.532961] do_init_module+0x5c/0x260 [ 24.532970] load_module+0x2570/0x27e0 [ 24.532988] ? __do_sys_init_module+0x130/0x190 [ 24.532995] __do_sys_init_module+0x130/0x190 [ 24.533007] do_syscall_64+0x33/0x40 [ 24.533014] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 24.533022] RIP: 0033:0x7f5d8ed4640e [ 24.533030] Code: 48 8b 0d 65 1a 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 49 89 ca b8 af 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 32 1a 0c 00 f7 d8 64 89 01 48 [ 24.533037] RSP: 002b:00007ffc37771de8 EFLAGS: 00000246 ORIG_RAX: 00000000000000af [ 24.533044] RAX: ffffffffffffffda RBX: 000056459db8f3f0 RCX: 00007f5d8ed4640e [ 24.533049] RDX: 00007f5d8eea632c RSI: 00000000000064d0 RDI: 000056459dd220b0 [ 24.533054] RBP: 000056459dd220b0 R08: 000056459dced5f0 R09: 00007ffc3776e160 [ 24.533058] R10: 00005640f99694ad R11: 0000000000000246 R12: 00007f5d8eea632c [ 24.533062] R13: 000056459dbe48c0 R14: 0000000000000007 R15: 000056459dd16a30 Regards, Hans p.s. The promised details of the issues which I'm hitting in implementing this on Intel devs using the ASoC framework: E.g. on the rt5672 the speaker amplifier has no volume control. So I need to use the DAC1 digital volume control, which has no mute bits. I have this working now, but due to there not being enough steps in the hw-vol-control, it reaches 0 when the GNOME UI is displaying that the sound is soft, but not off/muted. Yes the mute-led goes on because the control reaches it lowest value. So I need to fake a mute control in the codec driver for the LED to work reliable it seems...
Dne 12. 02. 21 v 13:28 Takashi Iwai napsal(a): > On Fri, 12 Feb 2021 11:32:38 +0100, > Jaroslav Kysela wrote: >> >> Dne 12. 02. 21 v 10:23 Takashi Iwai napsal(a): >>> On Thu, 11 Feb 2021 18:53:20 +0100, >>> Jaroslav Kysela wrote: >>>> >>>> Dne 11. 02. 21 v 18:15 Takashi Iwai napsal(a): >>>> >>>>>> Jaroslav Kysela (5): >>>>>> ALSA: control - introduce snd_ctl_notify_one() helper >>>>>> ALSA: control - add layer registration routines >>>>>> ALSA: control - add generic LED trigger module as the new control >>>>>> layer >>>>>> ALSA: HDA - remove the custom implementation for the audio LED trigger >>>>>> ALSA: control - add sysfs support to the LED trigger module >>>> >>>>> One thing I still miss from the picture is how to deal with the case >>>>> like AMD ACP. It has no mixer control to bundle with the LED trigger. >>>>> Your idea is to make a (dummy) user element and tie the LED trigger >>>>> with it? >>>> >>>> Yes, the user-space code which guarantee the silence stream should create an >>>> user space control with the appropriate LED group access bits. The alsa-lib's >>>> softvol PCM plugin can do this silencing for example. >>> >>> What control would it create? In the case of softvol, it's a volume >>> control that really changes the volume. For the mute LED, it's a >>> control turn on/off the mute? If so, I wonder what makes better than >>> creating it from the kernel driver. (Of course, we can list up like >>> "flexibility", etc, but it has a flip side of "complexity" and >>> "fragility"...) >> >> The current code handles both switch / volume for the marked control (assuming >> that the minimal value - usually zero - is full mute). And actually, there are >> snd_pcm_areas_silence() calls in the softvol plugin, so we know that the PCM >> stream is not passed to the application at this point. >> >> Condition: >> >> if (info.type == SNDRV_CTL_ELEM_TYPE_BOOLEAN || >> info.type == SNDRV_CTL_ELEM_TYPE_INTEGER) >> ... value.value.integer.value[i] != info.value.integer.min >> >> The softvol plugin may be extended to add the mute switch control, of course. > > Well, my question was what kind of mixer control will be added at all, > although the chip does neither volume nor mute function. Would we add > a fake volume/switch like softvol, or would we add rather a control > that is directly tied with the LED state? I don't understand your question. If the user space marks the own vol/sw control with the LED group, then it's tied with the LED state. I believe that the control should be created in the code which make sure that the PCM stream is silenced (like alsa-lib's softvol plugin). Jaroslav
On Sun, 14 Feb 2021 19:55:21 +0100, Jaroslav Kysela wrote: > > Dne 12. 02. 21 v 13:28 Takashi Iwai napsal(a): > > On Fri, 12 Feb 2021 11:32:38 +0100, > > Jaroslav Kysela wrote: > >> > >> Dne 12. 02. 21 v 10:23 Takashi Iwai napsal(a): > >>> On Thu, 11 Feb 2021 18:53:20 +0100, > >>> Jaroslav Kysela wrote: > >>>> > >>>> Dne 11. 02. 21 v 18:15 Takashi Iwai napsal(a): > >>>> > >>>>>> Jaroslav Kysela (5): > >>>>>> ALSA: control - introduce snd_ctl_notify_one() helper > >>>>>> ALSA: control - add layer registration routines > >>>>>> ALSA: control - add generic LED trigger module as the new control > >>>>>> layer > >>>>>> ALSA: HDA - remove the custom implementation for the audio LED trigger > >>>>>> ALSA: control - add sysfs support to the LED trigger module > >>>> > >>>>> One thing I still miss from the picture is how to deal with the case > >>>>> like AMD ACP. It has no mixer control to bundle with the LED trigger. > >>>>> Your idea is to make a (dummy) user element and tie the LED trigger > >>>>> with it? > >>>> > >>>> Yes, the user-space code which guarantee the silence stream should create an > >>>> user space control with the appropriate LED group access bits. The alsa-lib's > >>>> softvol PCM plugin can do this silencing for example. > >>> > >>> What control would it create? In the case of softvol, it's a volume > >>> control that really changes the volume. For the mute LED, it's a > >>> control turn on/off the mute? If so, I wonder what makes better than > >>> creating it from the kernel driver. (Of course, we can list up like > >>> "flexibility", etc, but it has a flip side of "complexity" and > >>> "fragility"...) > >> > >> The current code handles both switch / volume for the marked control (assuming > >> that the minimal value - usually zero - is full mute). And actually, there are > >> snd_pcm_areas_silence() calls in the softvol plugin, so we know that the PCM > >> stream is not passed to the application at this point. > >> > >> Condition: > >> > >> if (info.type == SNDRV_CTL_ELEM_TYPE_BOOLEAN || > >> info.type == SNDRV_CTL_ELEM_TYPE_INTEGER) > >> ... value.value.integer.value[i] != info.value.integer.min > >> > >> The softvol plugin may be extended to add the mute switch control, of course. > > > > Well, my question was what kind of mixer control will be added at all, > > although the chip does neither volume nor mute function. Would we add > > a fake volume/switch like softvol, or would we add rather a control > > that is directly tied with the LED state? > > I don't understand your question. If the user space marks the own vol/sw > control with the LED group, then it's tied with the LED state. I believe that > the control should be created in the code which make sure that the PCM stream > is silenced (like alsa-lib's softvol plugin). The softvol (or similar effect) is to be ignored by PA, as it's not suitable with the timer-scheduled type of PCM streaming. So the control shouldn't have any actual effect of PCM stream itself but merely for controlling the LED state. If that's the case, it shouldn't be named like "XXX Switch" or "XXX Volume", but it's a control like "Mic Mute LED Status" -- and ironically, that's a kind of thing we didn't want to take in the kernel side implementation... That said, I see the value of a generic code for the LED control that is tied with the existing volume/switch; there is already such an implementation in HD-audio, and applying to the other drivers makes sense. OTOH, for the case for AMD ACP, I'm still not sure what's the best way. thanks, Takashi Takashi > > Jaroslav > > -- > Jaroslav Kysela <perex@perex.cz> > Linux Sound Maintainer; ALSA Project; Red Hat, Inc. >
Hi, On 2/12/21 9:31 PM, Hans de Goede wrote: > On 2/11/21 12:13 PM, Jaroslav Kysela wrote: <snip> >> The sound driver implementation is really easy: >> >> 1) call snd_ctl_led_request() when control LED layer should be >> automatically activated >> / it calls module_request("snd-ctl-led") on demand / >> 2) mark all related kcontrols with >> SNDRV_CTL_ELEM_ACCESS_SPK_LED or >> SNDRV_CTL_ELEM_ACCESS_MIC_LED > > So I've been running some tests with this,combined with writing > UCM profiles for hw volume control, for some Intel Bay- and > CherryTrail devices using Intel's Low Power Engine (LPE) for audio, > which uses the ASoC framework. > > My work / experiments for this are progressing a bit slower then I > would like, but that is not the fault of this patch-set, but rather > an issue with hw-volume control mapping, see below for details. > > Leaving the ASoC implementation details aside, this patch-set > works quite nicely to get the speaker mute-LED to work. I've spend some more time this weekend playing with this and I've also added mic MUTE LED support for the ASoC rt5672 codec driver now using this. I will post a RFC patch series with the ASoC rt5672 codec driver LED support soon, as adding an extra use-case for this will hopefully help with reviewing this. FWIW there were some challenges, but those were not related to the driver API this patch set adds. The driver API works well for ASoC codec drivers. Regards, Hans p.s. One open issue is the lockdep issue which I reported in my previous email.
Dne 15. 02. 21 v 13:02 Hans de Goede napsal(a): > Hi, > > On 2/12/21 9:31 PM, Hans de Goede wrote: >> On 2/11/21 12:13 PM, Jaroslav Kysela wrote: > > <snip> > >>> The sound driver implementation is really easy: >>> >>> 1) call snd_ctl_led_request() when control LED layer should be >>> automatically activated >>> / it calls module_request("snd-ctl-led") on demand / >>> 2) mark all related kcontrols with >>> SNDRV_CTL_ELEM_ACCESS_SPK_LED or >>> SNDRV_CTL_ELEM_ACCESS_MIC_LED >> >> So I've been running some tests with this,combined with writing >> UCM profiles for hw volume control, for some Intel Bay- and >> CherryTrail devices using Intel's Low Power Engine (LPE) for audio, >> which uses the ASoC framework. >> >> My work / experiments for this are progressing a bit slower then I >> would like, but that is not the fault of this patch-set, but rather >> an issue with hw-volume control mapping, see below for details. >> >> Leaving the ASoC implementation details aside, this patch-set >> works quite nicely to get the speaker mute-LED to work. > > I've spend some more time this weekend playing with this and I've also > added mic MUTE LED support for the ASoC rt5672 codec driver now using > this. > > I will post a RFC patch series with the ASoC rt5672 codec driver LED > support soon, as adding an extra use-case for this will hopefully help > with reviewing this. > > FWIW there were some challenges, but those were not related to the > driver API this patch set adds. The driver API works well for ASoC > codec drivers. > > Regards, > > Hans > > > p.s. > > One open issue is the lockdep issue which I reported in my > previous email. Thank you for tests, Hans. I'm working on the lockdep issue - I'll send v2 of the LED patchset soon. The actual diff (I'd like to do more tests): diff --git a/sound/core/control.c b/sound/core/control.c index 4647b3cd41e8..c9f062fada0a 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -2047,6 +2047,7 @@ void snd_ctl_register_layer(struct snd_ctl_layer_ops *lops) down_write(&snd_ctl_layer_rwsem); lops->next = snd_ctl_layer; snd_ctl_layer = lops; + up_write(&snd_ctl_layer_rwsem); for (card_number = 0; card_number < SNDRV_CARDS; card_number++) { card = snd_card_ref(card_number); if (card) { @@ -2054,7 +2055,6 @@ void snd_ctl_register_layer(struct snd_ctl_layer_ops *lops) snd_card_unref(card); } } - up_write(&snd_ctl_layer_rwsem); } EXPORT_SYMBOL_GPL(snd_ctl_register_layer); diff --git a/sound/core/control_led.c b/sound/core/control_led.c index 638808e397fe..093dce721024 100644 --- a/sound/core/control_led.c +++ b/sound/core/control_led.c @@ -75,7 +75,7 @@ static inline unsigned int group_to_access(unsigned int group) return (group + 1) << SNDRV_CTL_ELEM_ACCESS_LED_SHIFT; } -struct snd_ctl_led *snd_ctl_led_get_by_access(unsigned int access) +static struct snd_ctl_led *snd_ctl_led_get_by_access(unsigned int access) { unsigned int group = access_to_group(access); if (group >= MAX_LED) @@ -116,15 +116,20 @@ static int snd_ctl_led_get(struct snd_ctl_led_ctl *lctl) return 0; } -static int snd_ctl_led_get_lock(struct snd_ctl_led_ctl *lctl) +static int snd_ctl_led_get_lock(struct snd_card *ncard, struct snd_ctl_led_ctl *lctl) { struct snd_card *card = lctl->card; int route; - down_read(&card->controls_rwsem); - route = snd_ctl_led_get(lctl); - up_read(&card->controls_rwsem); - return route; + /* the rwsem is already taken for the notification card */ + if (ncard != card) { + down_read(&card->controls_rwsem); + route = snd_ctl_led_get(lctl); + up_read(&card->controls_rwsem); + return route; + } else { + return snd_ctl_led_get(lctl); + } } static void snd_ctl_led_set_state(struct snd_card *card, unsigned int access, @@ -149,7 +154,7 @@ static void snd_ctl_led_set_state(struct snd_card *card, unsigned int access, list_for_each_entry(lctl, &led->controls, list) { if (lctl->kctl == kctl && lctl->index_offset == ioff) found = true; - UPDATE_ROUTE(route, snd_ctl_led_get_lock(lctl)); + UPDATE_ROUTE(route, snd_ctl_led_get_lock(card, lctl)); } if (!found && kctl && card) { lctl = kzalloc(sizeof(*lctl), GFP_KERNEL); @@ -158,7 +163,7 @@ static void snd_ctl_led_set_state(struct snd_card *card, unsigned int access, lctl->kctl = kctl; lctl->index_offset = ioff; list_add(&lctl->list, &led->controls); - UPDATE_ROUTE(route, snd_ctl_led_get_lock(lctl)); + UPDATE_ROUTE(route, snd_ctl_led_get_lock(card, lctl)); } } mutex_unlock(&snd_ctl_led_mutex); @@ -246,10 +251,11 @@ static void snd_ctl_led_register(struct snd_card *card) mutex_lock(&snd_ctl_led_mutex); snd_ctl_led_card_valid[card->number] = true; mutex_unlock(&snd_ctl_led_mutex); - /* the register callback is already called with held rwsem for controls */ + down_read(&card->controls_rwsem); list_for_each_entry(kctl, &card->controls, list) for (ioff = 0; ioff < kctl->count; ioff++) snd_ctl_led_notify(card, SNDRV_CTL_EVENT_MASK_VALUE, kctl, ioff); + up_read(&card->controls_rwsem); snd_ctl_led_refresh(); } @@ -262,17 +268,6 @@ static void snd_ctl_led_disconnect(struct snd_card *card) snd_ctl_led_refresh(); } -/** - * snd_ctl_led_hello - kernel module reference helper - * - * Call this helper in the module init function when the control LED - * code should be activated for the given driver. - */ -void snd_ctl_led_hello(void) -{ -} -EXPORT_SYMBOL(snd_ctl_led_hello); - /* * sysfs */ Jaroslav
Dne 15. 02. 21 v 8:50 Takashi Iwai napsal(a): > On Sun, 14 Feb 2021 19:55:21 +0100, > Jaroslav Kysela wrote: >> >> Dne 12. 02. 21 v 13:28 Takashi Iwai napsal(a): >>> On Fri, 12 Feb 2021 11:32:38 +0100, >>> Jaroslav Kysela wrote: >>>> >>>> Dne 12. 02. 21 v 10:23 Takashi Iwai napsal(a): >>>>> On Thu, 11 Feb 2021 18:53:20 +0100, >>>>> Jaroslav Kysela wrote: >>>>>> >>>>>> Dne 11. 02. 21 v 18:15 Takashi Iwai napsal(a): >>>>>> >>>>>>>> Jaroslav Kysela (5): >>>>>>>> ALSA: control - introduce snd_ctl_notify_one() helper >>>>>>>> ALSA: control - add layer registration routines >>>>>>>> ALSA: control - add generic LED trigger module as the new control >>>>>>>> layer >>>>>>>> ALSA: HDA - remove the custom implementation for the audio LED trigger >>>>>>>> ALSA: control - add sysfs support to the LED trigger module >>>>>> >>>>>>> One thing I still miss from the picture is how to deal with the case >>>>>>> like AMD ACP. It has no mixer control to bundle with the LED trigger. >>>>>>> Your idea is to make a (dummy) user element and tie the LED trigger >>>>>>> with it? >>>>>> >>>>>> Yes, the user-space code which guarantee the silence stream should create an >>>>>> user space control with the appropriate LED group access bits. The alsa-lib's >>>>>> softvol PCM plugin can do this silencing for example. >>>>> >>>>> What control would it create? In the case of softvol, it's a volume >>>>> control that really changes the volume. For the mute LED, it's a >>>>> control turn on/off the mute? If so, I wonder what makes better than >>>>> creating it from the kernel driver. (Of course, we can list up like >>>>> "flexibility", etc, but it has a flip side of "complexity" and >>>>> "fragility"...) >>>> >>>> The current code handles both switch / volume for the marked control (assuming >>>> that the minimal value - usually zero - is full mute). And actually, there are >>>> snd_pcm_areas_silence() calls in the softvol plugin, so we know that the PCM >>>> stream is not passed to the application at this point. >>>> >>>> Condition: >>>> >>>> if (info.type == SNDRV_CTL_ELEM_TYPE_BOOLEAN || >>>> info.type == SNDRV_CTL_ELEM_TYPE_INTEGER) >>>> ... value.value.integer.value[i] != info.value.integer.min >>>> >>>> The softvol plugin may be extended to add the mute switch control, of course. >>> >>> Well, my question was what kind of mixer control will be added at all, >>> although the chip does neither volume nor mute function. Would we add >>> a fake volume/switch like softvol, or would we add rather a control >>> that is directly tied with the LED state? >> >> I don't understand your question. If the user space marks the own vol/sw >> control with the LED group, then it's tied with the LED state. I believe that >> the control should be created in the code which make sure that the PCM stream >> is silenced (like alsa-lib's softvol plugin). > > The softvol (or similar effect) is to be ignored by PA, as it's not > suitable with the timer-scheduled type of PCM streaming. So the > control shouldn't have any actual effect of PCM stream itself but > merely for controlling the LED state. If that's the case, it > shouldn't be named like "XXX Switch" or "XXX Volume", but it's a > control like "Mic Mute LED Status" -- and ironically, that's a kind of > thing we didn't want to take in the kernel side implementation... I see your point now. We can force softvol switch (not volume) for this kind of hardware (AMD ACP) even for PA, so we know, that there's a code which modifies the PCM stream in the chain. Regarding time-scheduled I/O - for the capture, we're fine, because we can silence the samples before they're processed in the user space. We also even know, that the standard use for the microphone input is real-time, so the buffering is really short. For playback, yes, the driver queued samples cannot be altered, but it's usually no big security problem. The latency may be seconds (or a bit more) in the corner case. If we need a proper solution, the kernel PCM driver should be improved to take care (software based silence) or the buffering must be reduced. I don't think that we need to change something for ACP right now. Also note that PA does the own stream silencing when the mixer path is muted, so it's double win here (alsa-lib + PA) and the latency should be identical. Jaroslav
This patchset tries to resolve the diversity in the audio LED control among the ALSA drivers. A new control layer registration is introduced which allows to run additional operations on top of the elementary ALSA sound controls. A new control access group (three bits in the access flags) was introduced to carry the LED group information for the sound controls. The low-level sound drivers can just mark those controls using this access group. This information is exported to the user space and eventually the user space can create sound controls which can belong to a LED group. The actual state ('route') evaluation is really easy (the minimal value check for all channels / controls / cards). If there's more complicated logic for a given hardware, the card driver may eventually export a new read-only sound control for the LED group and do the logic itself. The new LED trigger control code is completely separated and possibly optional (there's no symbol dependency). The full code separation allows eventually to move this LED trigger control to the user space in future. Actually it replaces the already present functionality in the kernel space (HDA drivers) and allows a quick adoption for the recent hardware (SoundWire ASoC codecs). # lsmod | grep snd_ctl_led snd_ctl_led 16384 0 The sound driver implementation is really easy: 1) call snd_ctl_led_request() when control LED layer should be automatically activated / it calls module_request("snd-ctl-led") on demand / 2) mark all related kcontrols with SNDRV_CTL_ELEM_ACCESS_SPK_LED or SNDRV_CTL_ELEM_ACCESS_MIC_LED Original RFC: https://lore.kernel.org/alsa-devel/20210207201157.869972-1-perex@perex.cz/ Cc: Hans de Goede <hdegoede@redhat.com> Cc: Perry Yuan <Perry.Yuan@dell.com> Jaroslav Kysela (5): ALSA: control - introduce snd_ctl_notify_one() helper ALSA: control - add layer registration routines ALSA: control - add generic LED trigger module as the new control layer ALSA: HDA - remove the custom implementation for the audio LED trigger ALSA: control - add sysfs support to the LED trigger module include/sound/control.h | 27 ++- include/uapi/sound/asound.h | 6 +- sound/core/Kconfig | 6 + sound/core/Makefile | 2 + sound/core/control.c | 173 ++++++++++++-- sound/core/control_led.c | 407 ++++++++++++++++++++++++++++++++ sound/pci/hda/Kconfig | 4 +- sound/pci/hda/hda_codec.c | 69 +----- sound/pci/hda/hda_generic.c | 162 ++----------- sound/pci/hda/hda_generic.h | 15 +- sound/pci/hda/hda_local.h | 16 +- sound/pci/hda/patch_ca0132.c | 4 +- sound/pci/hda/patch_realtek.c | 2 +- sound/pci/hda/patch_sigmatel.c | 6 +- sound/pci/hda/thinkpad_helper.c | 2 +- 15 files changed, 638 insertions(+), 263 deletions(-) create mode 100644 sound/core/control_led.c