DMIC: Enable power for DMIC for ICL platform.#514
DMIC: Enable power for DMIC for ICL platform.#514lgirdwood merged 1 commit intothesofproject:masterfrom
Conversation
src/platform/intel/cavs/platform.c
Outdated
There was a problem hiding this comment.
We need this also for cannonlake (and suecreek?). It would be better to have some better defines, because it's for cavs1.8+ and listing all these platforms in places like that will hurt us, but for now you can just go for !defined(CONFIG_APOLLOLAKE) because other cavs platforms need this.
There was a problem hiding this comment.
Yeah, I think @singalsu had some definitions for DMIC IP versions ?
There was a problem hiding this comment.
Please comment on feasibility to power/un-power in trigger start/stop instead.
src/platform/intel/cavs/platform.c
Outdated
There was a problem hiding this comment.
Should we enable the power in DMIC start trigger instead to save power? And un-power in stop trigger. The power in sleep (power on, 0 Hz clock) is about 0.1 mW per microphone.
It depends on microphone but if the capture starts from power off the startup time for full quality is a bit longer, e.g. 35 ms -> 50 ms.
There was a problem hiding this comment.
@singalsu You are right, powering on while trigger start would be better. Also shouldn't 300ms default ramp up time (as for now) ease issue with 35-50ms lower quality on start?
There was a problem hiding this comment.
@singalsu, I think it is a good point. I looks like DMIC can not be used if it first wasn't triggered. Therefore enabling DMIC power only during triggering seems like a good idea.
There was a problem hiding this comment.
I also guess this will be included in the runtime PM (since this patch may enable DMIC regardless whether it's being used).
src/platform/intel/cavs/platform.c
Outdated
There was a problem hiding this comment.
Yeah, I think @singalsu had some definitions for DMIC IP versions ?
|
@ranj063, the DMIC shall be turned off as soon as it is no longer needed according to our architectural design https://thesofproject.github.io/latest/developer_guides/drivers/index.html#probing-on-demand. Does it answer your question? |
|
There are multiple threads in kernel land on DMIC management and it would be wise to allow for startup delays, where the PDM clocks are active but the captured samples discarded until a configurable timeout expires. Same for stop, we may need to stop the capture and later on stop the clocks and turn the DMIC block off. |
|
The delay in capture start from clock start to enable the FIFOs (and discard samples for some time) can be tricky since we need to launch the capture with soft reset clear due to an issue in HW. Need to check if we can run the mic clocks while in reset if we need that. The SOF driver contains an internal CIC and FIR mute (currently cleared at 1ms and 2ms from start) controlled by the worker task as well as the gain ramp to conceal the non-valid audio 10BC0 in the beginning. The clock off delay after capture doesn't have such limitation. E.g. a worker task set by trigger stop could do it. The clock on and off delays could be added topology parameters (among min/max PDM clock, min/max duty-cycle %, etc.). The delays are mic specific. Some have such requirements and some not. |
|
@ranj063 Need to find the power rail and regulator (usually 1.8V) from schematic that drives the DMIC power line. If it can be controlled in runtime some power can be saved if the other functionality that shares the power allows shutting it down. |
|
@mrajwa Sounds like we may need to cater for 2 different types of delays
|
src/drivers/intel/cavs/dmic.c
Outdated
There was a problem hiding this comment.
use pm_runtime_put() here, btw where is power enabled in dmic.c It should be with a call to pm_runtime_get_sync().
There was a problem hiding this comment.
@lgirdwood, I had quite a long discussion today with @singalsu about at what point we should turn DMIC power ON. Ideally it should happen right before the trigger for DMIC capture. Unfortunately tests show that in such case the quality of recorded stream is far from acceptable. This issue needs to be further investigated but as for now the power needs to be turned much sooner or else we get low quality captures.
As for, pm_runtime_get_sync() and pm_runtime_put() I don't see any use for them here. For what purpose would you like them here?
src/include/sof/dai.h
Outdated
There was a problem hiding this comment.
Use register bit name instead of DMIC_POWER_ON
There was a problem hiding this comment.
Starting the clock and power simultaneously caused the microphone to capture something that looked like square wave (test condition that @mrajwa uses is loud sine wave). The microphones in the device have a different acoustical to digital dBFS/dBSPL sensitivity for sensor mode and normal mode. The square wave like audio waveform would be explained be erroneously entering the sensor mode. Since this is more complicated than what I expected I agreed that moving power on to earlier is OK despite the small power hit.
We need to study what is the minimum time between the events for correct normal mode start and consider splitting the start sequence and implement such later if there is a significant improvement seen for power. If suspend is used in this platform it might provide already most of the power save opportunity. Just need to ensure the power on and clock start events have the minimum separation needed due to the various system resume delays to not get accidentally what we saw yesterday.
There was a problem hiding this comment.
DMIC_POWER_ON is just a single bit not a register. According to documentation "Set Power Active (SPA)" bit. I think DMIC_POWER_ON is better than a bare 0x01 or SPA as documentation calls it.
|
@lrgirdwo, @singalsu: I took your advices and performed some more testing as well as reworked code. Mainly I used "pm_runtime_*" functions suggested by @lrgirdwo, plus moved power enabling into dmic_probe() function and power disabling into dmic_remove(). After all of this the capture stream is significantly better. @lrgirdwo are you OK with current state? If so please integrate this PR so our validation team can start their tests. |
There was a problem hiding this comment.
Minor change for register bit naming.
There was a problem hiding this comment.
Please name this after the register and bit, e.g. DMICLCTL_SPA as found in the HAS (otherwise it's difficult to follow). Ditto for the others.
There was a problem hiding this comment.
@lgirdwood , I have just applied your suggestion.
Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>
Signed-off-by: Marcin Rajwa marcin.rajwa@linux.intel.com