Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mic_privacy: initial implementation #9788

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mbukowsk
Copy link
Contributor

Audio privacy feature allows end user to directly
control if user space applications receive actual
data from input devices (microphones). The control
is bypassing application level settings or operating
system controls (like audio endpoint volume).

return mic_privacy_api->get_dma_data_zeroing_link_select();
}
//hardcoded for FW_MANAGED
return 0xFFFFFFFF;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

~0

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbukowsk can you confirm a high level overview of how it works when FW managed. i.e. I assume FW will send an IPC to SW when the mute is enabled/disabled ? What about Mic state at FW boot ?

#include <ipc4/base-config.h>
#include "../audio/copier/copier_gain.h"

#define ADSP_RTC_FREQUENCY 32768
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in Zephyr as its HW specific.

Copy link
Contributor

@jsarha jsarha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No bugs from me (the others already found the serious stuff), but couple of suggestions more.

@abonislawski abonislawski force-pushed the mbukowsk-mic_privacy branch 6 times, most recently from 70c6211 to 37336ba Compare February 8, 2025 16:32
@lgirdwood
Copy link
Member

@mbukowsk can you check CI, several failures. Thanks !

Copy link
Contributor

@ujfalusi ujfalusi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbukowsk, the privacy_capabilities is configured to the wrong struct, that is going to case the host quite a bit of issues as it will not find it in hw_config and there will be two entries with the same ID in fw_config.

@lgirdwood
Copy link
Member

@mbukowsk, the privacy_capabilities is configured to the wrong struct, that is going to case the host quite a bit of issues as it will not find it in hw_config and there will be two entries with the same ID in fw_config.

@ujfalusi ok that's a blocker for kernel. Can you reply with what this should look like for kernel so @mbukowsk can copy.

@ujfalusi
Copy link
Contributor

ujfalusi commented Feb 11, 2025

@mbukowsk, the privacy_capabilities is configured to the wrong struct, that is going to case the host quite a bit of issues as it will not find it in hw_config and there will be two entries with the same ID in fw_config.

@ujfalusi ok that's a blocker for kernel. Can you reply with what this should look like for kernel so @mbukowsk can copy.

No, this is a blocker for this PR.

The firmware places the IPC4_PRIVACY_CAPS_HW_CFG (as is a hardware configuration tuple with ID 11) into the FW_CFG (as is firmware configuration). The ID 11 is IPC4_MAX_ASTATE_COUNT_FW_CFG in firmware config.

These are different configurations, one is queried with SOF_IPC4_FW_PARAM_FW_CONFIG (basefw param 6), the other is SOF_IPC4_FW_PARAM_HW_CONFIG_GET (basefw param 7).

The IPC4_PRIVACY_CAPS_HW_CFG must be placed to the hardware config data in basefw_hw_config() and not in basefw_config() as this PR does.

The corresponding kernel patch is thesofproject/linux@7f00cf9

@lgirdwood
Copy link
Member

@abonislawski any header changes ? Looks unrelated.

-- Unchanged /home/runner/work/sof/sof/tools/testbench/build_testbench/sof_ep/build/generated/include/sof_versions.h
[  1%] Built target check_version_h
[  1%] Built target genconfig
[  1%] Building C object CMakeFiles/sof.dir/src/platform/library/lib/alloc.c.o
[  2%] Building C object CMakeFiles/sof.dir/src/platform/library/lib/clk.c.o
[  3%] Building C object CMakeFiles/sof.dir/src/platform/library/lib/dai.c.o
[  3%] Building C object CMakeFiles/sof.dir/src/platform/library/lib/pm_runtime.c.o
[  4%] Building C object CMakeFiles/sof.dir/src/platform/library/lib/memory.c.o
[  4%] Building C object CMakeFiles/sof.dir/src/platform/library/lib/trace.c.o
In file included from /home/runner/work/sof/sof/posix/include/sof/lib/dai.h:22,
                 from /home/runner/work/sof/sof/src/include/sof/audio/component.h:24,
                 from /home/runner/work/sof/sof/posix/include/sof/lib/dma.h:23,
                 from /home/runner/work/sof/sof/src/include/sof/trace/dma-trace.h:11,
                 from /home/runner/work/sof/sof/src/platform/library/lib/trace.c:12:
/home/runner/work/sof/sof/src/include/sof/lib/dai-legacy.h:177:30: error: field ‘config’ has incomplete type
  177 |         struct dma_sg_config config;
      |                              ^~~~~~
gmake[5]: *** [CMakeFiles/sof.dir/build.make:149: CMakeFiles/sof.dir/src/platform/library/lib/trace.c.o] Error 1
gmake[4]: *** [CMakeFiles/Makefile2:1423: CMakeFiles/sof.dir/all] Error 2
gmake[3]: *** [Makefile:136: all] Error 2
gmake[2]: *** [CMakeFiles/sof_ep.dir/build.make:86: sof_ep/src/sof_ep-stamp/sof_ep-build] Error 2
gmake[1]: *** [CMakeFiles/Makefile2:126: CMakeFiles/sof_ep.dir/all] Error 2

@abonislawski
Copy link
Member

@lgirdwood It seems that it was caused by #include <sof/audio/component.h>, it should work now.

@@ -0,0 +1,3 @@
# SPDX-License-Identifier: BSD-3-Clause

add_local_sources(sof mic_privacy_manager_intel.c)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't used by Zephyr builds, right? Because you also add this to zephyr/CMakeLists.txt. So it looks like you also need this file for XTOS builds? But it uses calls like LOG_DBG() which are Zephyr-specific?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This CMake file will be added only if Mic privacy kconfig is set. I think the future plan is to get rid of giant zephyr cmake file and mic privacy cmake is already ready for such move

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abonislawski ok, yes, correct, we plan to move back to a proper cmake tree-based structure, and it's good that you're already thinking about it! It's just that (1) we don't know exactly yet what it will look like, so it might happen that changes will be needed to this file then, and (2) it's a bit confusing now to add unused files. So maybe at least add a comment there along the lines of "currently unused, but will be used in the future when src/audiuo files are removed from zephyr/CMakeLists.txt"

@ujfalusi
Copy link
Contributor

@abonislawski, it looks like this PR is flooding the firmware log during DMIC capture with:

mic_priv: mic_privacy_process: mic_privacy_process invalid state 0

@abonislawski
Copy link
Member

@ujfalusi thanks, wrong log, this is normal state

@ujfalusi
Copy link
Contributor

@abonislawski, there seams to be a firmware panic on the HDA setup when starting a DMIC capture, can be unrelated...

[00:00:09.450,321] <inf> dai_intel_dmic_nhlt: dai_dmic_set_config_nhlt: dmic_set_config_nhlt(): enable0 3, enable1 3
ASSERTION FAIL [!z_soc_irq_is_enabled(irq)] @ ZEPHYR_BASE/arch/common/dynamic_isr.c:23
	IRQ 8 is enabled
[00:00:09.450,355] <err> os: print_fatal_exception:  ** FATAL EXCEPTION
[00:00:09.450,360] <err> os: print_fatal_exception:  ** CPU 0 EXCCAUSE 63 (zephyr exception)
[00:00:09.450,363] <err> os: print_fatal_exception:  **  PC 0xa0030577 VADDR 0xa0031020
[00:00:09.450,366] <err> os: print_fatal_exception:  **  PS 0x60f20
[00:00:09.450,370] <err> os: print_fatal_exception:  **    (INTLEVEL:0 EXCM: 0 UM:1 RING:0 WOE:1 OWB:15 CALLINC:2)
[00:00:09.450,371] <err> os: xtensa_dump_stack:  **  A0 0xa0066070  SP 0xa01184f0  A2 0x4  A3 0xa0118500
[00:00:09.450,375] <err> os: xtensa_dump_st

SDW machine was not available for testing, so that was skipped.

@lyakh
Copy link
Collaborator

lyakh commented Feb 18, 2025

@abonislawski, there seams to be a firmware panic on the HDA setup when starting a DMIC capture, can be unrelated...

@ujfalusi it does seem related, it's a failed assertion in z_isr_install() and it's triggered when configuring a DMIC. In which log have you found that panic?

@abonislawski abonislawski force-pushed the mbukowsk-mic_privacy branch 2 times, most recently from 37f0e45 to 764f2d8 Compare February 18, 2025 08:33
@abonislawski
Copy link
Member

@ujfalusi @lyakh yeap its related. Both DMIC and mic privacy drivers enables the same IRQ so in debug build (linux CI) it can trigger assert for such check but works in normal build (internal CI).
Pushed test commit for west update to verify it.

west.yml Outdated
@@ -43,7 +43,7 @@ manifest:

- name: zephyr
repo-path: zephyr
revision: fe29c40a9366b5ffdcdd2eac26023ce4502413b1
revision: ae0b091459210ae936374f581d6a8f1b3768f958
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you need this and how this should not be merged when part of the PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[DNM]
This is for CI only

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, but we need to test a PR which can be merged at the end

@ujfalusi
Copy link
Contributor

@abonislawski, we still have the crash on dmic start..

@abonislawski abonislawski force-pushed the mbukowsk-mic_privacy branch 2 times, most recently from 524af57 to abe0df8 Compare February 20, 2025 08:51
@ujfalusi
Copy link
Contributor

SOFCI TEST

@lgirdwood lgirdwood added the DNM Do Not Merge tag label Feb 24, 2025
@lgirdwood
Copy link
Member

I've made this DNM until @abonislawski is ready with Zephyr patches merged.

abonislawski and others added 5 commits February 25, 2025 08:53
This pulls in Intel Mic Privacy driver

Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com>
Change type of arguments for gain params setting functions.
DAI data struct is replaced by gain_params. Components other
than DAI can also use copier gain feature (eg. microphone privacy)

Signed-off-by: Michal Bukowski <michal.bukowski@intel.com>
Audio privacy feature allows end user to directly
control if user space applications receive actual
data from input devices (microphones). The control
is bypassing application level settings or operating
system controls (like audio endpoint volume).

Signed-off-by: Michal Bukowski <michal.bukowski@intel.com>
Added empty implementation which always returns success

Signed-off-by: Michal Bukowski <michal.bukowski@intel.com>
…ANGE

SW sends this IPC when microphone privacy state is changed for
HW_MANAGED mode and SNDW interface.

Signed-off-by: Michal Bukowski <michal.bukowski@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DNM Do Not Merge tag
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants