mic_privacy: initial implementation #9788
Conversation
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 <[email protected]>
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 <[email protected]>
Added empty implementation which always returns success Signed-off-by: Michal Bukowski <[email protected]>
…ANGE SW sends this IPC when microphone privacy state is changed for HW_MANAGED mode and SNDW interface. Signed-off-by: Michal Bukowski <[email protected]>
mbukowsk
requested review from
kv2019i,
lyakh,
iuliana-prodan,
dbaluta,
abonislawski,
RanderWang,
pblaszko,
marcinszkudlinski,
lgirdwood,
plbossart,
mmaka1 and
lbetlej
as code owners
| priv_caps.capabilities_length = 1; | ||
| priv_caps.capabilities[0] = mic_privacy_get_policy_register(); | ||
|
|
||
| tlv_value_set(tuple, IPC4_PRIVACY_CAPS_HW_CFG, sizeof(priv_caps), &priv_caps); |
There was a problem hiding this comment.
This sets the privacy to the wrong TLV, this is the FW_CONFIG, not the HW_CONFIG.
IPC4_PRIVACY_CAPS_HW_CFG == IPC4_MAX_ASTATE_COUNT_FW_CFG
| IPC4_UAOL_CAPS_HW_CFG = 10 | ||
| IPC4_UAOL_CAPS_HW_CFG = 10, | ||
| /* Mic privacy capabilities */ | ||
| IPC4_PRIVACY_CAPS_HW_CFG = 11 |
There was a problem hiding this comment.
I would re-name these to be more specific to:
IPC4_INTEL_MIC_PRIVACY_CAPS_HW_CFG (and IPC4_INTEL_UAOL_CAPS_HW_CFG).
it looks to me that we are passing the raw content of the DfMICPVCP register, which is pretty much Intel specific
|
|
||
| priv_caps.privacy_version = 1; | ||
| priv_caps.capabilities_length = 1; | ||
| priv_caps.capabilities[0] = mic_privacy_get_policy_register(); |
There was a problem hiding this comment.
How can this even compile in case of CONFIG_MICROPHONE_PRIVACY=n (everything other than PTL)?
| @@ -88,6 +86,8 @@ int copier_gain_set_fade_params(struct comp_dev *dev, struct dai_data *dd, | |||
| /* Special case for GAIN_ZERO_TRANS_MS to support zero fade-in transition time */ | |||
| gain_params->fade_sg_length = 0; | |||
| return 0; | |||
| } else { | |||
| gain_params->fade_sg_length = frames * fade_period; | |||
| } | ||
| } else { | ||
| gain_params->fade_sg_length = frames * fade_period; | ||
| } |
| @@ -70,6 +70,8 @@ CONFIG_INTEL_ADSP_TIMER=y | |||
| CONFIG_MM_DRV_INTEL_ADSP_TLB_REMAP_UNUSED_RAM=y | |||
| CONFIG_SYS_CLOCK_TICKS_PER_SEC=12000 | |||
|
|
|||
| CONFIG_MICROPHONE_PRIVACY=y | |||
There was a problem hiding this comment.
if this belongs to the "Zephyr / device drivers" then you probably don't need an empty line above it
| if (ret < 0) { | ||
| comp_err(dev, "unable to configure mic privacy"); | ||
| goto error; | ||
| } |
There was a problem hiding this comment.
indentation, more cases below
| struct mic_privacy_data *mic_priv_data = arg; | ||
| struct mic_privacy_settings *mic_privacy_settings = data; | ||
|
|
||
| LOG_INF("mic_privacy_event, arg = %p, data = %p", arg, data); |
There was a problem hiding this comment.
please reduce the number of logs to a minimum
|
|
||
| LOG_MODULE_REGISTER(LOG_DOMAIN); | ||
|
|
||
| #define MAX_INT64 (0x7FFFFFFFFFFFFFFFULL) |
There was a problem hiding this comment.
can you just use INT64_MAX from stdint.h?
| uint32_t mic_privacy_get_policy_register(void) | ||
| { | ||
| if (mic_priv_dev == NULL) | ||
| return -EINVAL; |
There was a problem hiding this comment.
function returning unsigned, change to int?
| return mic_privacy_api->get_dma_data_zeroing_link_select(); | ||
| } | ||
| //hardcoded for FW_MANAGED | ||
| return 0xFFFFFFFF; |
| return 0xFFFFFFFF; | ||
| } | ||
|
|
||
| struct mic_privacy_settings fill_mic_priv_settings(uint32_t mic_disable_status) |
There was a problem hiding this comment.
I think supplying a structure to be filled as a parameter is preferred over returning one
|
|
||
| switch (mic_privacy_policy) { | ||
| case HW_MANAGED: | ||
| { |
There was a problem hiding this comment.
braces unneeded in all case / default clauses
| @@ -0,0 +1,275 @@ | |||
| // SPDX-License-Identifier: BSD-3-Clause | |||
| // | |||
| // Copyright(c) 2023 Intel Corporation. All rights reserved. | |||
| mic_privacy_api->set_fw_managed_mode(true); | ||
| } | ||
|
|
||
| //Enable interrupts |
|
|
||
| else { | ||
| enable_fw_managed_irq(true); | ||
| } |
|
|
||
| #include <zephyr/drivers/mic_privacy.h> | ||
| #include <sof/lib/notifier.h> | ||
| #include <xtensa/tie/xt_hifi4.h> |
There was a problem hiding this comment.
dont think Hifi will be needed for Mic mute API ?
| #include <ipc4/base-config.h> | ||
| #include "../audio/copier/copier_gain.h" | ||
|
|
||
| #define ADSP_RTC_FREQUENCY 32768 |
There was a problem hiding this comment.
This should be in Zephyr as its HW specific.
| }; | ||
|
|
||
| enum mic_privacy_state { | ||
| UNMUTED = 0, |
| int mic_privacy_manager_init(void); | ||
|
|
||
| int mic_privacy_manager_get_policy(void); | ||
|
|
||
| uint32_t mic_privacy_get_policy_register(void); | ||
|
|
||
| void enable_fw_managed_irq(bool enable_irq); | ||
|
|
||
| void enable_dmic_irq(bool enable_irq); | ||
|
|
||
| void handle_dmic_interrupt(void * const self, int a, int b); | ||
|
|
||
| void handle_fw_managed_interrupt(void * const dev); | ||
|
|
||
| void propagate_privacy_settings(struct mic_privacy_settings *mic_privacy_settings); | ||
|
|
||
| uint32_t get_dma_zeroing_wait_time(void); | ||
|
|
||
| uint32_t get_privacy_mask(void); | ||
|
|
||
| struct mic_privacy_settings fill_mic_priv_settings(uint32_t mic_disable_status); | ||
|
|
||
| void set_gtw_mic_state(struct mic_privacy_data *mic_priv_data, uint32_t mic_disable_status); | ||
|
|
||
| void update_gtw_mic_state(struct mic_privacy_data *mic_priv_data, uint32_t hw_mic_disable_status); | ||
|
|
||
| void mic_privacy_process(struct comp_dev *dev, struct mic_privacy_data *mic_priv, struct comp_buffer *buffer, uint32_t copy_samples); | ||
|
|
||
| void mic_priv_gain_input(uint8_t *buff, uint32_t buff_size, uint32_t mic_priv_state, | ||
| const struct ipc4_audio_format *in_fmt); | ||
|
|
There was a problem hiding this comment.
If these are all for client usage of Mic privacy then they should all share the same mic_privacy_ prefix.
| struct processing_module *mod = comp_mod(dev); | ||
| struct copier_data *cd = module_get_private_data(mod); | ||
|
|
||
| if(cd->mic_priv) |
|
|
||
| int stream_copy_from_no_consume(struct comp_dev *dev, struct comp_buffer *source, | ||
| struct comp_buffer *sink, | ||
| dma_process_func process, uint32_t source_bytes, uint32_t chmap); |
| bool last_block, | ||
| uint32_t data_offset_or_size, | ||
| const char *data) | ||
| { |
| @@ -229,7 +229,8 @@ static int copier_dai_init(struct comp_dev *dev, | |||
| } | |||
| cd->dd[index]->gain_data = gain_data; | |||
|
|
|||
| ret = copier_gain_set_params(dev, cd->dd[index]->gain_data, GAIN_DEFAULT_FADE_PERIOD); | |||
| ret = copier_gain_set_params(dev, cd->dd[index]->gain_data, GAIN_DEFAULT_FADE_PERIOD, | |||
| cd->dd[index]->dai->type); | |||
| @@ -344,7 +348,7 @@ int dma_buffer_copy_from(struct comp_buffer *source, | |||
| * cannot be used with Host DMA. Fortunately, remapping conversion is specifically | |||
| * designed for use with IPC4 DAI gateway's device posture feature. IPC4 DAI gateway | |||
| * does not have the same size alignment limitations. Therefore, frame-based calculations | |||
| * are employed only when necessary —- in the case of IPC4 DAI gateway with remapping | |||
| * are employed only when necessary �- in the case of IPC4 DAI gateway with remapping | |||
There was a problem hiding this comment.
Funny looking change, and couple morebellow. Is this intentional? Maybe your editor wants to chane '—' to '�'. I would stick to standard ASC II characters in the code comments, but up to you.
| @@ -288,6 +288,8 @@ enum ipc4_basefw_params { | |||
|
|
|||
| /* Use LARGE_CONFIG_SET to change SDW ownership */ | |||
| IPC4_SDW_OWNERSHIP = 31, | |||
|
|
|||
| IPC4_SET_MIC_PRIVACY_FW_MANAGED_POLICY_MASK =36, | |||
There was a problem hiding this comment.
There are quite big and enlightening comment about the earlier basefw_params. Would be good to ad something here too.
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).