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 resume/suspend calls, thus ensuring correct ordering. Special handling is needed for the D0i3 flow, where display power must be turned off even though DSP is left powered.
Now that we have more invocations of the display power helper functions, the conditional checks surrounding each call have been moved inside hda_codec_i915_display_power(). The two special cases of display powering at initial probe are handled separately. The intent is to avoid powering the display whenever no display codecs are used.
Note that early powering of display was removed in commit 687ae9e287b3 ("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 87c1694533c9 ("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 Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/sof/intel/hda-codec.c | 12 ++++++++---- sound/soc/sof/intel/hda-dsp.c | 11 +++++++++++ sound/soc/sof/intel/hda.c | 2 +- 3 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c index 9106ab8dac6f..ff45075ef720 100644 --- a/sound/soc/sof/intel/hda-codec.c +++ b/sound/soc/sof/intel/hda-codec.c @@ -174,8 +174,10 @@ void hda_codec_i915_display_power(struct snd_sof_dev *sdev, bool enable) { struct hdac_bus *bus = sof_to_bus(sdev);
- dev_dbg(bus->dev, "Turning i915 HDAC power %d\n", enable); - snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, enable); + if (HDA_IDISP_CODEC(bus->codec_mask)) { + dev_dbg(bus->dev, "Turning i915 HDAC power %d\n", enable); + snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, enable); + } } EXPORT_SYMBOL_NS(hda_codec_i915_display_power, SND_SOC_SOF_HDA_AUDIO_CODEC_I915);
@@ -189,7 +191,8 @@ int hda_codec_i915_init(struct snd_sof_dev *sdev) if (ret < 0) return ret;
- hda_codec_i915_display_power(sdev, true); + /* codec_mask not yet known, power up for probe */ + snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, true);
return 0; } @@ -200,7 +203,8 @@ int hda_codec_i915_exit(struct snd_sof_dev *sdev) struct hdac_bus *bus = sof_to_bus(sdev); int ret;
- hda_codec_i915_display_power(sdev, false); + /* power down unconditionally */ + snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false);
ret = snd_hdac_i915_exit(bus);
diff --git a/sound/soc/sof/intel/hda-dsp.c b/sound/soc/sof/intel/hda-dsp.c index 4a4d318f97ff..0848b79967a9 100644 --- a/sound/soc/sof/intel/hda-dsp.c +++ b/sound/soc/sof/intel/hda-dsp.c @@ -428,6 +428,9 @@ static int hda_suspend(struct snd_sof_dev *sdev, bool runtime_suspend) return ret; }
+ /* display codec can powered off after link reset */ + hda_codec_i915_display_power(sdev, false); + return 0; }
@@ -439,6 +442,9 @@ static int hda_resume(struct snd_sof_dev *sdev, bool runtime_resume) #endif int ret;
+ /* display codec must be powered before link reset */ + hda_codec_i915_display_power(sdev, true); + /* * clear TCSEL to clear playback on some HD Audio * codecs. PCI TCSEL is defined in the Intel manuals. @@ -482,6 +488,8 @@ int hda_dsp_resume(struct snd_sof_dev *sdev) struct pci_dev *pci = to_pci_dev(sdev->dev);
if (sdev->s0_suspend) { + hda_codec_i915_display_power(sdev, true); + /* restore L1SEN bit */ if (hda->l1_support_changed) snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR, @@ -531,6 +539,9 @@ int hda_dsp_suspend(struct snd_sof_dev *sdev) int ret;
if (sdev->s0_suspend) { + /* we can't keep a wakeref to display driver at suspend */ + hda_codec_i915_display_power(sdev, false); + /* enable L1SEN to make sure the system can enter S0Ix */ hda->l1_support_changed = snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR, diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 65b86dd044f1..8fddafb5c1d4 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -381,7 +381,7 @@ static int hda_init_caps(struct snd_sof_dev *sdev) hda_codec_probe_bus(sdev, hda_codec_use_common_hdmi);
if (!HDA_IDISP_CODEC(bus->codec_mask)) - hda_codec_i915_display_power(sdev, false); + hda_codec_i915_exit(sdev);
/* * we are done probing so decrement link counts