-
Notifications
You must be signed in to change notification settings - Fork 132
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
ASoC: SOF: Intel: Add support for hardware mic privacy change reporting #5312
base: topic/sof-dev
Are you sure you want to change the base?
ASoC: SOF: Intel: Add support for hardware mic privacy change reporting #5312
Conversation
include/sound/hda-mlink.h
Outdated
void hdac_bus_eml_set_mic_privacy_mask(struct hdac_bus *bus, bool alt, int elid, | ||
unsigned long mask); | ||
bool hdac_bus_eml_check_mic_privacy(struct hdac_bus *bus, bool alt, int elid); | ||
bool hdac_bus_eml_get_mic_privacy(struct hdac_bus *bus, bool alt, int elid); |
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.
What is the difference between check
and get
?
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.
Ok, I got it after reading all the commits. Check
if it is a mic privacy irq and get
the mic privacy status.
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.
Had to go all the way the register spec to get that, but then I did understand that too. A short comment somewhere would not hurt thou.
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.
OK, let me do some renaming and commenting.
|
||
hdac_bus_eml_set_mic_privacy_mask(sof_to_bus(sdev), true, | ||
AZX_REG_ML_LEPTR_ID_SDW, | ||
PTL_MICPVCP_GET_SDW_MASK(micpvcp)); |
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.
Should we only call hdac_bus_eml_set_mic_privacy_mask
when the mic privacy is changed?
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.
hdac_bus_eml_set_mic_privacy_mask() can ignore unchanged mask if needed, but this function should be only called once when the stack is booted up and we have found the mic privacy support in hw config from the firmware.
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.
Only couple of not so important suggestions. LGTM.
{ | ||
struct hdac_bus *bus = sof_to_bus(sdev); | ||
|
||
return hdac_bus_eml_check_interrupt(bus, true, AZX_REG_ML_LEPTR_ID_SDW); | ||
} | ||
EXPORT_SYMBOL_NS(lnl_dsp_check_sdw_irq, "SND_SOC_SOF_INTEL_LNL"); |
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.
Would it be worth mentioning in the commit message that it also unexports the few lnl specific ops and turns them into static?
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.
It only changes the lnl functions that are needed for ptl from static to be exported, which is a must to create the separate ptl.c
include/sound/hda-mlink.h
Outdated
void hdac_bus_eml_set_mic_privacy_mask(struct hdac_bus *bus, bool alt, int elid, | ||
unsigned long mask); | ||
bool hdac_bus_eml_check_mic_privacy(struct hdac_bus *bus, bool alt, int elid); | ||
bool hdac_bus_eml_get_mic_privacy(struct hdac_bus *bus, bool alt, int elid); |
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.
Had to go all the way the register spec to get that, but then I did understand that too. A short comment somewhere would not hurt thou.
3ff7eef
to
85814f7
Compare
Move the sof_mtl_ops and sof_mtl_ops_init() to pci-mtl.c as local static and add a 'generic' sof_mtl_set_ops() function as replacement exported function to fill the dsp_ops structure. Signed-off-by: Peter Ujfalusi <[email protected]>
LunarLake is a next generation in ACE architecture and most of the dsp_ops are the same as it is in previous generation. Use the sof_mtl_set_ops() to get the ops used for mtl and update the ones that needs different functions for LNL. Update pci-ptl at the same time to use the LNL dsp_ops as before. Signed-off-by: Peter Ujfalusi <[email protected]>
There is no need to export individual dsp_ops functions anymore as the callbacks are filled now by sof_mtl_set_ops() Signed-off-by: Peter Ujfalusi <[email protected]>
Create a minimal placeholder to make it possible to add code to handle the new features of Panther Lake compared to MTL/LNL. Signed-off-by: Peter Ujfalusi <[email protected]>
ACE3 (Panther Lake) introduced support for microphone privacy feature which can - in hardware - mute incoming audio data based on a state of a physical switch. The change in the privacy state is delivered through interface IP blocks and can only be handled by the link owner. In Intel platforms Soundwire is for example host owned, so the interrupt can only be handled by the host. Since the input stream is going to be muted by hardware, the host needs to send a message to firmware about the change in privacy so it can execute a fade out/in to enhance user experience. The support for microphone privacy can be queried from the HW_CONFIG data under the INTEL_MIC_PRIVACY_CAP tuple. This is Intel specific data, the core will pass it to platform code if the intel_configure_mic_privacy() callback is provided. Platform code can call sof_ipc4_mic_privacy_state_change() to send the IPC message to the firmware on state change. Signed-off-by: Peter Ujfalusi <[email protected]>
…egisters New register has been introduced with PTL in the vendor specific SHIM registers, outside of the IP itself to control the mic privacy change handling. Via the PVCCS registers it is possible to enable/disable the interrupt generation to the owner of the interface (DSP/host), check the mic privacy status. The PVCCS is provided for each sublink under the IP to make it possible to control the interrupt per sublink bases. The added functions are generic to be future proof if the mic privacy support is extended beyond Soundwire and DMIC links. The interrupt is routed to the IP's interrupt and it needs to be cleared in all links to be able to detect further changes. Signed-off-by: Peter Ujfalusi <[email protected]>
Add generic callback definitions for checking the mic privacy interrupt and status. Implement wrappers for mic privacy reported via the Soundwire interrupt and it's vendor specific SHIM registers. Signed-off-by: Peter Ujfalusi <[email protected]>
Implement the three callbacks that is needed to enable support for reporting the mic privacy change via soundwire. In PTL the mic privacy reporting is supported via soundwire and DMIC and the soundwire is owned by the host, it's interrupt is routed there. To enable the interrupt, the sublink mask needs to be passed to the multilink layer, the check_mic_privacy_irq/process_mic_privacy callbacks needs to be implemented to check and report the mic privacy change. Signed-off-by: Peter Ujfalusi <[email protected]>
Changes since v1:
|
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.
On question inline, but no blockers.
|
||
/* No need to set the mic privacy if it is not enabled or forced */ | ||
if (!(micpvcp & PTL_MICPVCP_DDZE_ENABLED) || | ||
micpvcp & PTL_MICPVCP_DDZE_FORCED) |
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.
So this is just the mask? I guess even in forced state, it is preferably to have FW manage the ramp in/out...?
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.
The firmware will pass the raw value of the MICPVCP register (don't ask). This is configured by a secure entity (BIOS) in case of HW managed mode and contains the link mask of SDW and DMIC that the hardware will mute.
The firmware will only do fade for these links.
If the hardware mute is forced then it is unconditionally muted -> the position of the switch does not matter, always muted.
In FW managed case the mask is not defined and on top, the host does not need to handle interrupts for this as everything is managed in FW (except that the host could send an IPC message to change the mask, to exclude some links since it is expected that all incoming links are FW muted on privacy ON case by default).
So yes, we only enable the interrupt if the policy is HW managed and it is not forced to always mute.
I know...
SOFCI TEST |
@@ -396,6 +396,7 @@ enum sof_ipc4_base_fw_params { | |||
SOF_IPC4_FW_PARAM_MODULES_INFO_GET, | |||
SOF_IPC4_FW_PARAM_LIBRARIES_INFO_GET = 16, | |||
SOF_IPC4_FW_PARAM_SYSTEM_TIME = 20, | |||
SOF_IPC4_FW_PARAM_MIC_PRIVACY_STATE_CHANGE = 35, |
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 is very controversial.
In the case of DMICs, the mute can be completely handled by firmware since it also processes the GPIO that toggles the privacy.
In the case of SoundWire, the mute is expected to be handled in the external codec.
In other words, this IPC does not seem to have a purpose other than academic - and it's mind-blowing that such a split implementation for a 'secure' solution relies on an IPC from the host. Recompile your kernel and the privacy is crippled... That does not sound good, does it.
unsigned long mask); | ||
bool hdac_bus_eml_is_mic_privacy_changed(struct hdac_bus *bus, bool alt, int elid); | ||
bool hdac_bus_eml_get_mic_privacy_state(struct hdac_bus *bus, bool alt, int elid); | ||
|
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.
I think the commit message should be clearer that the information is from hardware to host only. This is different to the previous patch in the sense that the host can select to receive information on the privacy changes, but it is not involved in the privacy flows.
Hi,
This series will change how we set up dsp_ops for ACE versions to make it cleaner what needs to be handled differently in each iteration of the architecture and adds the needed definitions and code to handle the mic privacy via vendor specific SHIM register.