2018-09-13 14:56 GMT+08:00 Takashi Iwai <tiwai@xxxxxxx>: > The recent change of vga_switcheroo allowed the runtime PM for > HD-audio on AMD GPUs, but this also resulted in a regression. When > the HD-audio controller driver gets runtime-suspended, HD-audio link > is turned off, and the hotplug notification is ignored. This leads to > the inconsistent audio state (the connection isn't notified and ELD is > ignored). > > The best fix would be to implement the proper ELD notification via the > audio component, but it's still not ready. As a quick workaround, > this patch adds the check of runtime_idle and allows the runtime > suspend only when the vga_switcheroo is bound with discrete GPU. > That is, a system with a single GPU and APU would be again without > runtime PM to keep the HD-audio link for the hotplug notification and > ELD read out. > > Also, the codec->auto_runtime_pm flag is set only for the discrete GPU > at the time GPU gets bound via vga_switcheroo (i.e. only dGPU is > forcibly runtime-PM enabled), so that APU can still get the ELD > notification. > > For identifying which GPU is bound, a new vga_switcheroo client > callback, gpu_bound, is implemented. The vga_switcheroo simply calls > this when GPU is bound, and tells whether it's dGPU or APU. Tested-by: Jian-Hong Pan <jian-hong@xxxxxxxxxxxx> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=200945 > Fixes: 07f4f97d7b4b ("vga_switcheroo: Use device link for HDA controller") > Signed-off-by: Takashi Iwai <tiwai@xxxxxxx> > --- > drivers/gpu/vga/vga_switcheroo.c | 2 + > include/linux/vga_switcheroo.h | 3 ++ > sound/pci/hda/hda_intel.c | 86 +++++++++++++++++++++++--------- > sound/pci/hda/hda_intel.h | 1 + > 4 files changed, 69 insertions(+), 23 deletions(-) > > diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c > index a96bf46bc483..cf2a18571d48 100644 > --- a/drivers/gpu/vga/vga_switcheroo.c > +++ b/drivers/gpu/vga/vga_switcheroo.c > @@ -215,6 +215,8 @@ static void vga_switcheroo_enable(void) > return; > > client->id = ret | ID_BIT_AUDIO; > + if (client->ops->gpu_bound) > + client->ops->gpu_bound(client->pdev, ret); > } > > vga_switcheroo_debugfs_init(&vgasr_priv); > diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h > index a34539b7f750..7e6ac0114d55 100644 > --- a/include/linux/vga_switcheroo.h > +++ b/include/linux/vga_switcheroo.h > @@ -133,15 +133,18 @@ struct vga_switcheroo_handler { > * @can_switch: check if the device is in a position to switch now. > * Mandatory. The client should return false if a user space process > * has one of its device files open > + * @gpu_bound: notify the client id to audio client when the GPU is bound. > * > * Client callbacks. A client can be either a GPU or an audio device on a GPU. > * The @set_gpu_state and @can_switch methods are mandatory, @reprobe may be > * set to NULL. For audio clients, the @reprobe member is bogus. > + * OTOH, @gpu_bound is only for audio clients, and not used for GPU clients. > */ > struct vga_switcheroo_client_ops { > void (*set_gpu_state)(struct pci_dev *dev, enum vga_switcheroo_state); > void (*reprobe)(struct pci_dev *dev); > bool (*can_switch)(struct pci_dev *dev); > + void (*gpu_bound)(struct pci_dev *dev, enum vga_switcheroo_client_id); > }; > > #if defined(CONFIG_VGA_SWITCHEROO) > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c > index 1b2ce304152a..aa4c672dbaf7 100644 > --- a/sound/pci/hda/hda_intel.c > +++ b/sound/pci/hda/hda_intel.c > @@ -365,8 +365,10 @@ enum { > */ > #ifdef SUPPORT_VGA_SWITCHEROO > #define use_vga_switcheroo(chip) ((chip)->use_vga_switcheroo) > +#define needs_eld_notify_link(chip) ((chip)->need_eld_notify_link) > #else > #define use_vga_switcheroo(chip) 0 > +#define needs_eld_notify_link(chip) false > #endif > > #define CONTROLLER_IN_GPU(pci) (((pci)->device == 0x0a0c) || \ > @@ -453,6 +455,7 @@ static inline void mark_runtime_wc(struct azx *chip, struct azx_dev *azx_dev, > #endif > > static int azx_acquire_irq(struct azx *chip, int do_disconnect); > +static void set_default_power_save(struct azx *chip); > > /* > * initialize the PCI registers > @@ -1201,6 +1204,10 @@ static int azx_runtime_idle(struct device *dev) > azx_bus(chip)->codec_powered || !chip->running) > return -EBUSY; > > + /* ELD notification gets broken when HD-audio bus is off */ > + if (needs_eld_notify_link(hda)) > + return -EBUSY; > + > return 0; > } > > @@ -1298,6 +1305,36 @@ static bool azx_vs_can_switch(struct pci_dev *pci) > return true; > } > > +/* > + * The discrete GPU cannot power down unless the HDA controller runtime > + * suspends, so activate runtime PM on codecs even if power_save == 0. > + */ > +static void setup_vga_switcheroo_runtime_pm(struct azx *chip) > +{ > + struct hda_intel *hda = container_of(chip, struct hda_intel, chip); > + struct hda_codec *codec; > + > + if (hda->use_vga_switcheroo && !hda->need_eld_notify_link) { > + list_for_each_codec(codec, &chip->bus) > + codec->auto_runtime_pm = 1; > + /* reset the power save setup */ > + if (chip->running) > + set_default_power_save(chip); > + } > +} > + > +static void azx_vs_gpu_bound(struct pci_dev *pci, > + enum vga_switcheroo_client_id client_id) > +{ > + struct snd_card *card = pci_get_drvdata(pci); > + struct azx *chip = card->private_data; > + struct hda_intel *hda = container_of(chip, struct hda_intel, chip); > + > + if (client_id == VGA_SWITCHEROO_DIS) > + hda->need_eld_notify_link = 0; > + setup_vga_switcheroo_runtime_pm(chip); > +} > + > static void init_vga_switcheroo(struct azx *chip) > { > struct hda_intel *hda = container_of(chip, struct hda_intel, chip); > @@ -1306,6 +1343,7 @@ static void init_vga_switcheroo(struct azx *chip) > dev_info(chip->card->dev, > "Handle vga_switcheroo audio client\n"); > hda->use_vga_switcheroo = 1; > + hda->need_eld_notify_link = 1; /* cleared in gpu_bound op */ > chip->driver_caps |= AZX_DCAPS_PM_RUNTIME; > pci_dev_put(p); > } > @@ -1314,6 +1352,7 @@ static void init_vga_switcheroo(struct azx *chip) > static const struct vga_switcheroo_client_ops azx_vs_ops = { > .set_gpu_state = azx_vs_set_state, > .can_switch = azx_vs_can_switch, > + .gpu_bound = azx_vs_gpu_bound, > }; > > static int register_vga_switcheroo(struct azx *chip) > @@ -1339,6 +1378,7 @@ static int register_vga_switcheroo(struct azx *chip) > #define init_vga_switcheroo(chip) /* NOP */ > #define register_vga_switcheroo(chip) 0 > #define check_hdmi_disabled(pci) false > +#define setup_vga_switcheroo_runtime_pm(chip) /* NOP */ > #endif /* SUPPORT_VGA_SWITCHER */ > > /* > @@ -1352,6 +1392,7 @@ static int azx_free(struct azx *chip) > > if (azx_has_pm_runtime(chip) && chip->running) > pm_runtime_get_noresume(&pci->dev); > + chip->running = 0; > > azx_del_card_list(chip); > > @@ -2230,6 +2271,25 @@ static struct snd_pci_quirk power_save_blacklist[] = { > }; > #endif /* CONFIG_PM */ > > +static void set_default_power_save(struct azx *chip) > +{ > + int val = power_save; > + > +#ifdef CONFIG_PM > + if (pm_blacklist) { > + const struct snd_pci_quirk *q; > + > + q = snd_pci_quirk_lookup(chip->pci, power_save_blacklist); > + if (q && val) { > + dev_info(chip->card->dev, "device %04x:%04x is on the power_save blacklist, forcing power_save to 0\n", > + q->subvendor, q->subdevice); > + val = 0; > + } > + } > +#endif /* CONFIG_PM */ > + snd_hda_set_power_save(&chip->bus, val * 1000); > +} > + > /* number of codec slots for each chipset: 0 = default slots (i.e. 4) */ > static unsigned int azx_max_codecs[AZX_NUM_DRIVERS] = { > [AZX_DRIVER_NVIDIA] = 8, > @@ -2241,9 +2301,7 @@ static int azx_probe_continue(struct azx *chip) > struct hda_intel *hda = container_of(chip, struct hda_intel, chip); > struct hdac_bus *bus = azx_bus(chip); > struct pci_dev *pci = chip->pci; > - struct hda_codec *codec; > int dev = chip->dev_index; > - int val; > int err; > > hda->probe_continued = 1; > @@ -2322,31 +2380,13 @@ static int azx_probe_continue(struct azx *chip) > if (err < 0) > goto out_free; > > + setup_vga_switcheroo_runtime_pm(chip); > + > chip->running = 1; > azx_add_card_list(chip); > > - val = power_save; > -#ifdef CONFIG_PM > - if (pm_blacklist) { > - const struct snd_pci_quirk *q; > - > - q = snd_pci_quirk_lookup(chip->pci, power_save_blacklist); > - if (q && val) { > - dev_info(chip->card->dev, "device %04x:%04x is on the power_save blacklist, forcing power_save to 0\n", > - q->subvendor, q->subdevice); > - val = 0; > - } > - } > -#endif /* CONFIG_PM */ > - /* > - * The discrete GPU cannot power down unless the HDA controller runtime > - * suspends, so activate runtime PM on codecs even if power_save == 0. > - */ > - if (use_vga_switcheroo(hda)) > - list_for_each_codec(codec, &chip->bus) > - codec->auto_runtime_pm = 1; > + set_default_power_save(chip); > > - snd_hda_set_power_save(&chip->bus, val * 1000); > if (azx_has_pm_runtime(chip)) > pm_runtime_put_autosuspend(&pci->dev); > > diff --git a/sound/pci/hda/hda_intel.h b/sound/pci/hda/hda_intel.h > index e3a3d318d2e5..f59719e06b91 100644 > --- a/sound/pci/hda/hda_intel.h > +++ b/sound/pci/hda/hda_intel.h > @@ -37,6 +37,7 @@ struct hda_intel { > > /* vga_switcheroo setup */ > unsigned int use_vga_switcheroo:1; > + unsigned int need_eld_notify_link:1; > unsigned int vga_switcheroo_registered:1; > unsigned int init_failed:1; /* delayed init failed */ > > -- > 2.18.0 > _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
