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@xxxxxxxxx>
> Reported-and-tested-by: Henning Kühn <prg@xxxxxxxx>
> 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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
