On Wed, 20 Jun 2018 11:34:52 +0200, Lukas Wunner wrote:
Hi Takashi,
Henning Kühn reports that due to my commit 07f4f97d7b4b ("vga_switcheroo: Use device link for HDA controller"), the discrete GPU on his hybrid graphics laptop no longer runtime suspends.
The root cause is that the single codec of the GPU's HDA controller doesn't support CLKSTOP and EPSS. (The "Supported Power States" are 0x00000009, i.e. CLKSTOP and EPSS bits are not set, cf. page 209 of the HDA spec.)
This means that in hda_codec_runtime_suspend() we do not call snd_hdac_codec_link_down():
if (codec_has_clkstop(codec) && codec_has_epss(codec) && (state & AC_PWRST_CLK_STOP_OK)) snd_hdac_codec_link_down(&codec->core);
If snd_hdac_codec_link_down() isn't called, the bit in the codec_powered bitmask isn't cleared, which in turn prevents the controller from going to PCI_D3hot in azx_runtime_idle():
if (!power_save_controller || !azx_has_pm_runtime(chip) || azx_bus(chip)->codec_powered || !chip->running) return -EBUSY;
The codec does runtime suspend to D3, but the PS-ClkStopOk bit in the response to "Get Power State" is not set. (Response is 0x00000033, cf. page 151 of the HD Audio spec.) Hence the check above "state & AC_PWRST_CLK_STOP_OK" also results in "false".
I'm not familiar enough with the intricacies of the HD Audio spec to fully comprehend the implications of missing EPSS and CLKSTOP support and to come up with a fix. We could quirk any HDA controller in a vga_switcheroo setup to ignore missing EPSS and CLKSTOP support, but would that be safe? E.g. the spec says that if the bus clock does stop, "a full reset shall be performed" when the clock is reenabled. Are we handling this correctly?
I guess it would work with a quirk. The EPSS and CLKSTOP checks are just to assure the modern codec PM, and GPU is always exceptional.
Supposing that it's AMD GPU, does a fix like below work?
thanks,
Takashi
-- 8< -- --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -2899,8 +2899,9 @@ static int hda_codec_runtime_suspend(struct device *dev) list_for_each_entry(pcm, &codec->pcm_list_head, list) snd_pcm_suspend_all(pcm->pcm); state = hda_call_codec_suspend(codec); - if (codec_has_clkstop(codec) && codec_has_epss(codec) && - (state & AC_PWRST_CLK_STOP_OK)) + if (codec->link_down_at_suspend || + (codec_has_clkstop(codec) && codec_has_epss(codec) && + (state & AC_PWRST_CLK_STOP_OK))) snd_hdac_codec_link_down(&codec->core); snd_hdac_link_power(&codec->core, false); return 0; diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h index 681c360f29f9..5b00c1eb857e 100644 --- a/sound/pci/hda/hda_codec.h +++ b/sound/pci/hda/hda_codec.h @@ -258,6 +258,7 @@ struct hda_codec { unsigned int power_save_node:1; /* advanced PM for each widget */ unsigned int auto_runtime_pm:1; /* enable automatic codec runtime pm */ unsigned int force_pin_prefix:1; /* Add location prefix */ + unsigned int link_down_at_suspend:1; /* force to link down at suspend */ #ifdef CONFIG_PM unsigned long power_on_acct; unsigned long power_off_acct; diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 8840daf9c6a3..98e1c411c56a 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -3741,6 +3741,11 @@ static int patch_atihdmi(struct hda_codec *codec)
spec->chmap.channels_max = max(spec->chmap.channels_max, 8u);
+ /* AMD GPUs have neither EPSS nor CLKSTOP bits, hence preventing + * the link-down as is. Tell the core to allow it. + */ + codec->link_down_at_suspend = 1; + return 0; }