[alsa-devel] [PATCH v2] ALSA: hda - Enable runtime PM only for discrete GPU
Jian-Hong Pan
jian-hong at endlessm.com
Thu Sep 13 15:27:16 CEST 2018
2018-09-13 14:56 GMT+08:00 Takashi Iwai <tiwai at suse.de>:
> 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 at endlessm.com>
> 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 at suse.de>
> ---
> 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
>
More information about the Alsa-devel
mailing list