-
Notifications
You must be signed in to change notification settings - Fork 325
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
base: main
Are you sure you want to change the base?
Conversation
return mic_privacy_api->get_dma_data_zeroing_link_select(); | ||
} | ||
//hardcoded for FW_MANAGED | ||
return 0xFFFFFFFF; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
~0
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
70c6211
to
37336ba
Compare
@mbukowsk can you check CI, several failures. Thanks ! |
There was a problem hiding this 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.
@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 The corresponding kernel patch is thesofproject/linux@7f00cf9 |
@abonislawski any header changes ? Looks unrelated.
|
4601d46
to
6b789bc
Compare
@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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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"
@abonislawski, it looks like this PR is flooding the firmware log during DMIC capture with:
|
@ujfalusi thanks, wrong log, this is normal state |
6b789bc
to
f5ca9a1
Compare
@abonislawski, there seams to be a firmware panic on the HDA setup when starting a DMIC capture, can be unrelated...
SDW machine was not available for testing, so that was skipped. |
@ujfalusi it does seem related, it's a failed assertion in |
37f0e45
to
764f2d8
Compare
764f2d8
to
2e166ee
Compare
west.yml
Outdated
@@ -43,7 +43,7 @@ manifest: | |||
|
|||
- name: zephyr | |||
repo-path: zephyr | |||
revision: fe29c40a9366b5ffdcdd2eac26023ce4502413b1 | |||
revision: ae0b091459210ae936374f581d6a8f1b3768f958 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
@abonislawski, we still have the crash on dmic start.. |
524af57
to
abe0df8
Compare
SOFCI TEST |
abe0df8
to
799a0a1
Compare
I've made this DNM until @abonislawski is ready with Zephyr patches merged. |
799a0a1
to
3843767
Compare
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>
3843767
to
52c9e0d
Compare
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).