[alsa-devel] HDA controller w/o CLKSTOP and EPSS support

[Takashi Iwai]

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;
 }