Re: HDA controller w/o CLKSTOP and EPSS support

[Lukas Wunner <lukas@wunner.de>]

On Wed, Jun 20, 2018 at 12:14:32PM +0200, Takashi Iwai wrote:
> On Wed, 20 Jun 2018 11:34:52 +0200, Lukas Wunner wrote:
> > 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".
> 
> Supposing that it's AMD GPU, does a fix like below work?
[snip]
> --- 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;
>  }

In hda_intel.c:azx_probe_continue(), we currently do this:

	if (use_vga_switcheroo(hda))
		list_for_each_codec(codec, &chip->bus)
			codec->auto_runtime_pm = 1;

An alternative to setting the flag in patch_atihdmi() would be to
set it here.

One small nit, the code comment you're adding above is in network
subsystem style ("/*" isn't on a line by itself).

Otherwise this is
Reviewed-by: Lukas Wunner <lukas@wunner.de>
Reported-and-tested-by: Henning Kühn <prg@cooco.de>
Fixes: 07f4f97d7b4b ("vga_switcheroo: Use device link for HDA controller")
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106957

Feel free to just copy/paste the problem description from my original
e-mail to the commit message so that you don't have additional work
there.

Thanks so much for the fast response with a working patch!

Lukas