Message ID | 20200206200223.7715-2-kai.vehmanen@linux.intel.com (mailing list archive) |
---|---|
State | Accepted |
Commit | 816938b272b0ac0203e25ce50483bd284ea4a2db |
Headers | show |
Series | [1/3] ALSA: hda: do not override bus codec_mask in link_get() | expand |
On Thu, 06 Feb 2020 21:02:22 +0100, Kai Vehmanen wrote: > > 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> Reviewed-by: Takashi Iwai <tiwai@suse.de> thanks, Takashi
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