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

[Takashi Iwai]

On Thu, 21 Jun 2018 11:18:59 +0200,
Lukas Wunner wrote:
> 
> 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.

I think it's safer to put in patch_atihdmi().  It's about AMD GPU
specific, and the code in azx_probe_continue() may touch all codecs.

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

It's OK in the sound tree, too.  I like that style :)

> Otherwise this is
> Reviewed-by: Lukas Wunner <lukas at wunner.de>
> Reported-and-tested-by: Henning Kühn <prg at 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.

OK, I'll cook up the proper patch and submit/merge later.

Thanks!


Takashi