On Fri, 13 Jul 2018 17:25:39 +0200,
jimqu wrote:
>
>
>
> å?¨ 2018/7/13 16:46, Takashi Iwai å??é??:
> > On Fri, 13 Jul 2018 10:06:02 +0200,
> > Jim Qu wrote:
> >> On modern laptop, there are more and more platforms
> >> have two GPUs, and each of them maybe have audio codec
> >> for HDMP/DP output. For some dGPU which is no output,
> >> audio codec usually is disabled.
> >>
> >> In currect HDA audio driver, it will set all codec as
> >> VGA_SWITCHEROO_DIS, the audio which is binded to UMA
> >> will be suspended if user use debugfs to contorl power
> >>
> >> In HDA driver side, it is difficult to know which GPU
> >> the audio has binded to. So set the bound gpu pci dev
> >> to vgaswitchroo, the correct audio id will be set in
> >> vgaswitchreoo enable function.
> >>
> >> Signed-off-by: Jim Qu <Jim.Qu at amd.com>
> > The idea looks good to me. Just a nitpick:
> >
> >> @@ -1319,15 +1319,16 @@ static const struct vga_switcheroo_client_ops azx_vs_ops = {
> >> static int register_vga_switcheroo(struct azx *chip)
> >> {
> >> struct hda_intel *hda = container_of(chip, struct hda_intel, chip);
> >> + struct pci_dev * p;
> >> int err;
> >> if (!hda->use_vga_switcheroo)
> >> return 0;
> >> - /* FIXME: currently only handling DIS controller
> >> - * is there any machine with two switchable HDMI audio controllers?
> >> - */
> >> - err = vga_switcheroo_register_audio_client(chip->pci, &azx_vs_ops,
> >> - VGA_SWITCHEROO_DIS);
> >> +
> >> + p = get_bound_vga(chip->pci);
> >> + err = vga_switcheroo_register_audio_client(chip->pci, &azx_vs_ops, p);
> >> + if (p)
> >> + pci_dev_put(p);
> > Passing a NULL vga_dev won't work, so the NULL check after calling
> > vga_switcheroo_register_audio_client() is moot. If any, it should be
> > checked before that.
> >
>
> Â The hda->use_vga_switcheroo is set to 1 in init_vga_switcheroo() if
> bound vga pci dev is not NULL. Since the hda->use_vga_switcheroo is
> checked at beginning of the function. so the p which pass into
> vga_switcheroo_register_audio_client() is constant valid value.
So the NULL-check in the patch is also useless, no? :)
Takashi
