ASoC: SOF: hda: forbid codec reset while controller is powered #978
Conversation
The DSP hda controller cannot handle if hda codec is power cycled while the controller is active. After commit b5a236c ("ALSA: hda - Enforces runtime_resume after S3 and S4 for each codec"), frequent IPC timeouts are observed in suspend/resume stress testing. Address the issue by blocking runtime PM in connected codecs while controller is active. Fixes: b5a236c ("ALSA: hda - Enforces runtime_resume after S3 and S4 for each codec") Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
| * power cycling of HDA codecs causes failures in HDA | ||
| * controller logic -> force codecs to be powered | ||
| */ | ||
| snd_hda_set_power_save(hbus, -1); |
There was a problem hiding this comment.
This seems a bit heavy-weight to me. In principle it would be enough to do snd_hda_set_power_save(hbus, SND_SOF_SUSPEND_DELAY_MS); once, and here just do some pm_runtime_get*() / put*(), similar to what is done in azx_vs_set_state():
list_for_each_codec(codec, &chip->bus) {
pm_runtime_suspend(hda_codec_dev(codec));
pm_runtime_disable(hda_codec_dev(codec));
}
...
list_for_each_codec(codec, &chip->bus) {
pm_runtime_enable(hda_codec_dev(codec));
pm_runtime_resume(hda_codec_dev(codec));
}
?
There was a problem hiding this comment.
@lyakh Thanks. That's a good comment. And in fact, this is probably needed. THe current iteration of the patch does not prevent resumes (only suspends) and it seems if I run stress test long enough, I can still trigger problems. Let me work on a update patch.
There was a problem hiding this comment.
@lyakh This proved harded, I tried pm_runtime_get(), pm_runtime_forbid(), pm_runtime_disable() and various combinations, but as this is called from within a suspend/resume, we hit issues.
I'll continue working on Monday. For now, the revert patch is the best cure.
There was a problem hiding this comment.
@kv2019i the order of runtime suspend/resume is dictated by the parent-child relationship of the devices. In our case, the sof-audio-pci dev is the parent of the hda codec device. Therefore, the sof device will be resumed prior to the codec and will be suspended after the codec. I'm not sure it is a good idea to mess with it.
| /* disable hda bus irq and i/o */ | ||
| snd_hdac_bus_stop_chip(bus); | ||
|
|
||
| snd_hda_set_power_save(hbus, SND_SOF_SUSPEND_DELAY_MS); |
There was a problem hiding this comment.
we shouldn't muck with runtime_pm within a runtime_pm routine, this should be done on probe as done in Ranjani's patch.
Also this re-enabled runtime_pm in the suspend case and you disable it in resume, so this is very very odd.
There was a problem hiding this comment.
@plbossart This is trying to enforce more ordering by preventing codec to be runtime-suspended while controller is running. But for many reasons, this patch is a no-go.
| * power cycling of HDA codecs causes failures in HDA | ||
| * controller logic -> force codecs to be powered | ||
| */ | ||
| snd_hda_set_power_save(hbus, -1); |
There was a problem hiding this comment.
this boils down to callling:
pm_runtime_dont_use_autosuspend(dev);
pm_runtime_forbid(dev);
which doesn't seem like a very good idea to me, and invalidates what you do on suspend.
|
Closing the pull request. This patch misuses runtime-pm and does not cover all cases. Other options to address #944 are under investigation. |
5 participants
The DSP hda controller cannot handle if hda codec is power cycled
while the controller is active. After commit b5a236c
("ALSA: hda - Enforces runtime_resume after S3 and S4 for each codec"),
frequent IPC timeouts are observed in suspend/resume stress testing.
Address the issue by blocking runtime PM in connected codecs
while controller is active.
Fixes #944