ASoC: SOF: Intel: hda: fix ordering bug in resume flow - #1764
Conversation
299ac9b to
804e577
Compare
|
No real objections, but wondering what happens in a corner case where you have an HDMI display setup, then remove it and re-plug another one without the SOF PCI device going to suspend/resume. It feels like somehow we don't get a notification of a change on the display side. |
ranj063
left a comment
ranj063
left a comment
There was a problem hiding this comment.
@kv2019i the change looks fine. We used to have this in the suspend/resume sequence a long time ago and removed it when Takashi made some changes in the hdac_hdmi driver to handle this internally.
But the commit message is missing your sign off
|
@plbossart and @ranj063 Thanks for quick feedback. This is a really rough cut to get wider testing and thus a draft only -- so the sign-off is missing on purpose. But interesting to hear about the history. I want to investigate a bit more if it's safe to just keep the controller power reference during the reset actions, or do we keep the reference whenever controller is in active state. I took the latter (safe) approach in this patch version. |
I knew this looked familiar, but what I don't get is why it was removed? @ranj063 do you have pointers on the patches from Takashi? |
@plbossart here're the patches: 687ae9e and 49cddb8 |
|
Thanks @ranj063. Thinking aloud here, I wonder if the fixes done by Takashi/Libin make sense for the hdac_hdmi case, but now that we've transitioned to using patch_hdmi.c something's missing? |
@plbossart yes, that would be my suspicion as well. Maybe we could infer something from the first patch and incorporate something similar in patch_hdmi to avoid having to do this in the SOF driver. Also thinking out loud. @kv2019i any ideas? |
|
@plbossart @ranj063 We are not in the clear yet. This patch fixes the runtime pm case, but we are still hitting issues at probe time that I can't explain. For the older patches, they might not be a good reference as you won't hit this issue on older platforms. You need a very specific setup to hit this. |
Thanks @ranj063 for digging these up! I'm afraid this logic doesn't hold anymore. We can not do any actions on the HDA link (including chip init and link reset) before i915 is powered up. E.g. I think the link resets in get_caps() would be causing problems in early boot, despite my patches as you are generating traffic on the HDA links even before valid link parameters are in place. |
|
@plbossart wrote:
That's ok. As long as audio driver holds a reference (the codec driver does this whenever it is active, and with my patch, we also hold a reference from sof-hda controller code) to i915 acomp, the partition hosting the affected registers is going to stay up no matter what happens with HDMI and displays. So this is safe despite any hotplug activity. The problem only occurs if all users of the partition go away -- i.e. monitors get disconnected (or display put to sleep) and SOF goes to suspend and releases power. If all conditions are met, we lose some register state on i915 side, and then we can hit the bug when resuming if we do actions on the HDA bus before we have ensured i915 is up and running. Current status is that this PR is not yet sufficient. I'll update the series tomorrow. |
|
|
||
| /* get controller capabilities */ | ||
| ret = hda_dsp_ctrl_get_caps(sdev); | ||
| if (ret < 0) |
There was a problem hiding this comment.
@ranj063 @plbossart I'm wondering why we are doing hda_dsp_ctrl_get_caps() in hda_init()..? This is fairly confusing as there is a separate hda_init_caps(). I was thinking should we keep hda_init() as a simple function that just allocates the kernel structures but does not do any hw i/o and then have a separate function that does hda_dsp_ctrl_get_caps() and friends. So far so good, but then I hit the naming issue as we already have hda-init_caps() which does much more.
So something like:
- hda_init() -> remove all hw i/o
- hda_init_caps() -> do the i915 init and hda_dsp_ctrl_get_caps()
- hda_init_probe() -> the current hda_init_caps() which initializes bus'es and runs probe for HDA and SDW
...?
There was a problem hiding this comment.
wow.
@plbossart wrote:
the link resets in get_caps (" ASoC: SOF: Intel: hda-ctrl: add reset cycle before parsing capabilities") > were added to work around SoundWire-related issues, we never root-caused why they were
needed. if they cause problems for HDMI, you can consider removing this...Ack. We actually tried but still got the error. Based on test results so far, link needs to be functional when get_caps is done (i.e. the i915 get_power needs to be done before, which is a bit surprising, but seems to be so). Could also explain your issue (that you had to do a link reset before get_caps). Plus azx is doing it this way.
I have no idea what the issue is. The point is that without the reset cycle, we could not read the capabilities on customer SoundWire-based devices, so we failed to probe. We never had such issues on Intel RVPs. I suspect a BIOS dependency or something.
the link resets in get_caps (" ASoC: SOF: Intel: hda-ctrl: add reset cycle before parsing capabilities") were added to work around SoundWire-related issues, we never root-caused why they were needed. if they cause problems for HDMI, you can consider removing this... |
|
@plbossart wrote:
Ack. We actually tried but still got the error. Based on test results so far, link needs to be functional when get_caps is done (i.e. the i915 get_power needs to be done before, which is a bit surprising, but seems to be so). Could also explain your issue (that you had to do a link reset before get_caps). Plus azx is doing it this way. |
|
@kv2019i is it fairly straightforward to switch to using hdac_hdmi with the current SOF driver and checking if this problem even exists? Is it worth it you think? |
|
@ranj063 wrote:
Hdac-hdmi doesn't support the newer platforms, but I did an experiment and backported TGL/ICL support to it, and on the one setup where I can trigger the problem, it also happens with hdac_hdmi. |
When HDA controller is resumed from suspend, i915 HDMI/DP codec requires that following order of actions is kept: - i915 display power up and configuration of link params - hda link reset and setup Current SOF HDA code delegates display codec power control to the codec driver. This works most of the time, but in runtime PM sequences, the above constraint may be violated. On platforms where BIOS values for HDA link parameters do not match hardware reset defaults, this may lead to errors in HDA verb transactions after resume. Fix the issue by explicitly powering the display codec in the HDA controller runtime resume/suspend calls, thus ensuring correct ordering. Note that early powering of display was removed in commit 687ae9e ("ASoC: intel: skl: Fix display power regression"). This change was also copied to the SOF driver. No failures have resulted as hardware default values for link parameters have worked out of the box. However with recent i915 driver changes like done in commit 87c1694 ("drm/i915: save AUD_FREQ_CNTRL state at audio domain suspend"), this does not hold anymore and errors are hit. Cc: Takashi Iwai <tiwai@suse.de> Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
To be compliant with i915 display driver requirements, i915 power-up must be done before any HDA communication takes place, including parsing the bus capabilities. Move i915 initialization earlier in the SOF HDA sequence. This sequence is now aligned with the snd-hda-intel driver where display_power() calls is before snd_hdac_bus_parse_capabilities() and rest of the capability parsing. Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
99da51a to
670e3c6
Compare
|
Ok, now ready for proper review. |
|
@plbossart wrote:
Oops, I'll hold on until tomorrow. One additional concern is S0ix, I'm afraid this might break that flow -- or at least make it less useful at system level. When SOF goes to D0ix, we do not run hda_suspend() and with my that, that would leave a display power reference on. This would leave display powered on and kind of defeat the original purpose of a low-power state. Right @keyonjie and @ranj063 ? I'll come up with a mitigation for tomorrow. |
@kv2019i from your commit message the issue seems to be a problem only with runtime suspend/resume isnt? We'd never invoke the D0i3 sequence for runtime PM. So we should be good no? |
|
You probably want to look at S0/D0ix and S0ix/D0ix separately. |
Yes @ranj063 @plbossart , I think runtime flow is good, we don't enter D0i3. But S0 is a problem. We do enter D0i3 in SOF even with these patches in PR1764, but as hda-dsp.c:hda_suspend() is not run if s0_suspend is true, SOF will go to D0i3 holding a reference to display power. Now I'm not sure how this impacts at system level, will this prevent system from going to suspend. Will investigate this... UPDATE: ok, it seems i915 will force the power domains down at system suspend, so I think we are good. I'll hold off sending the patches to alsa-devel until tomorrow -- I'll double-check suspend is not blocked (and i915 doesn't go crazy if we do this), double-check what happens at resume and only then send out the patches. |
|
@plbossart @ranj063 wrote:
Tested various scenarios today and basicly functionality seems ok even in the S3 with DSP to D0ix case, but i915 does emit a big warning, so I don't think this is future-proof. I now uploaded #1767 to fix the D0ix case. It touches the helper functions again, so it would be better to squash the patches before sending out. |
|
Sent upstream (squashed and a few more very minor modifications): https://mailman.alsa-project.org/pipermail/alsa-devel/2020-February/162509.html |
3 participants
When HDA controller is resumed from suspend, i915 HDMI/DP
codec requires that following order of sequence is kept:
Current SOF HDA code delegates display codec power control
to the codec driver. This works most of the time, but in
runtime PM sequences, the above constraint may be violated.
On platforms where BIOS values for HDA link parameters do
not match hardware reset defaults, this may lead to errors
in HDA verb transactions after resume.
Fix the issue by explicitly powering the display codec
in the HDA controller resume/suspend calls, thus ensuring
correct ordering.
Fixes #1763