At Mon, 5 Aug 2013 12:56:04 +1000,
Dave Airlie wrote:
>
> Add support for HDMI audio device on VGA cards that powerdown
> to D3cold using non-standard ACPI/PCI infrastructure (optimus).
>
> This does a couple of things to make it work:
>
> a) add a set of power ops for the hdmi domain, and enables them
> via vga_switcheroo when we are a switcheroo controlled card. This
> just replaces the runtime resume operation so that when the card
> is in D3cold the userspace pci config space access via sysfs,
> the vga switcheroon runtime resume gets called first and it calls
> the GPU resume callback before calling the sound card runtime
> resume.
>
> b) standard ACPI/PCI stacks won't put a device into D3cold without
> an ACPI handle, but since the hdmi audio devices on gpus don't have
> an ACPI handle, we need to manually force the device into D3cold
> after suspend from the switcheroo path only.
>
> c) don't try and do runtime s/r when the GPU is off.
>
> d) call runtime suspend/resume during switcheroo suspend/resume
> this is to make sure the runtime stack knows to try and resume
> the hdmi audio device for pci config space access.
>
> v2: fix incorrect runtime call suspend->resume.
>
> v3: rework irq handler to avoid false irq when we are resuming
> but haven't runtime resumed yet, don't bother trying D3cold,
> it won't work, just set it manually ourselves, move runtime s/r
> calls outside the main s/r hook. enable dnyamic pm properly by
> dropping reference.
>
> Signed-off-by: Dave Airlie <airlied@xxxxxxxxxx>
> ---
> sound/pci/hda/hda_intel.c | 36 +++++++++++++++++++++++++++++-------
> 1 file changed, 29 insertions(+), 7 deletions(-)
>
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index 8860dd5..4c4f546 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -555,6 +555,9 @@ struct azx {
> #ifdef CONFIG_SND_HDA_DSP_LOADER
> struct azx_dev saved_azx_dev;
> #endif
> +
> + /* secondary power domain for hdmi audio under vga device */
> + struct dev_pm_domain hdmi_pm_domain;
> };
>
> #define CREATE_TRACE_POINTS
> @@ -1396,11 +1399,6 @@ static irqreturn_t azx_interrupt(int irq, void *dev_id)
> u8 sd_status;
> int i, ok;
>
> -#ifdef CONFIG_PM_RUNTIME
> - if (chip->pci->dev.power.runtime_status != RPM_ACTIVE)
> - return IRQ_NONE;
> -#endif
> -
This brings me a slight concern. It might break Intel's runtime PM
case.
> spin_lock(&chip->reg_lock);
>
> if (chip->disabled) {
> @@ -1409,7 +1407,7 @@ static irqreturn_t azx_interrupt(int irq, void *dev_id)
> }
>
> status = azx_readl(chip, INTSTS);
> - if (status == 0) {
> + if (status == 0 || status == 0xffffffff) {
> spin_unlock(&chip->reg_lock);
> return IRQ_NONE;
> }
> @@ -2971,6 +2969,12 @@ static int azx_runtime_suspend(struct device *dev)
> struct snd_card *card = dev_get_drvdata(dev);
> struct azx *chip = card->private_data;
>
> + if (chip->disabled)
> + return 0;
> +
> + if (!(chip->driver_caps & AZX_DCAPS_PM_RUNTIME))
> + return 0;
Hmm, why this check? I couldn't see the logic clearly.
I thought you didn't add AZX_DCAPS_PM_RUNTIME to Nvidia controllers,
so only Intel (Haswell & co) can pass this check.
> azx_stop_chip(chip);
> azx_enter_link_reset(chip);
> azx_clear_irq_pending(chip);
> @@ -2984,6 +2988,12 @@ static int azx_runtime_resume(struct device *dev)
> struct snd_card *card = dev_get_drvdata(dev);
> struct azx *chip = card->private_data;
>
> + if (chip->disabled)
> + return 0;
> +
> + if (!(chip->driver_caps & AZX_DCAPS_PM_RUNTIME))
> + return 0;
> +
> if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
> hda_display_power(true);
> azx_init_pci(chip);
> @@ -2996,6 +3006,9 @@ static int azx_runtime_idle(struct device *dev)
> struct snd_card *card = dev_get_drvdata(dev);
> struct azx *chip = card->private_data;
>
> + if (chip->disabled)
> + return 0;
> +
> if (!power_save_controller ||
> !(chip->driver_caps & AZX_DCAPS_PM_RUNTIME))
> return -EBUSY;
> @@ -3078,13 +3091,19 @@ static void azx_vs_set_state(struct pci_dev *pci,
> "%s: %s via VGA-switcheroo\n", pci_name(chip->pci),
> disabled ? "Disabling" : "Enabling");
> if (disabled) {
> + pm_runtime_put_sync_suspend(&pci->dev);
> azx_suspend(&pci->dev);
Isn't it better to swap these two calls, first azx_suspend() and then
pm_runtime_put_sync_suspend()? Otherwise the controller may be shut
down before suspending the underlying codecs.
> + /* when we get suspended by vga switcheroo we end up in D3cold,
> + * however we have no ACPI handle, so pci/acpi can't put us there,
> + * put ourselves there */
> + pci->current_state = PCI_D3cold;
> chip->disabled = true;
> if (snd_hda_lock_devices(chip->bus))
> snd_printk(KERN_WARNING SFX "%s: Cannot lock devices!\n",
> pci_name(chip->pci));
> } else {
> snd_hda_unlock_devices(chip->bus);
> + pm_runtime_get_noresume(&pci->dev);
> chip->disabled = false;
This call should be after chip->disabled = false. Otherwise the check
you added in azx_runtime_resume() will always hit.
> azx_resume(&pci->dev);
> }
> @@ -3139,6 +3158,9 @@ static int register_vga_switcheroo(struct azx *chip)
> if (err < 0)
> return err;
> chip->vga_switcheroo_registered = 1;
> +
> + /* register as an optimus hdmi audio power domain */
> + vga_switcheroo_init_domain_pm_optimus_hdmi_audio(&chip->pci->dev, &chip->hdmi_pm_domain);
> return 0;
> }
> #else
> @@ -3887,7 +3909,7 @@ static int azx_probe_continue(struct azx *chip)
> power_down_all_codecs(chip);
> azx_notifier_register(chip);
> azx_add_card_list(chip);
> - if (chip->driver_caps & AZX_DCAPS_PM_RUNTIME)
> + if ((chip->driver_caps & AZX_DCAPS_PM_RUNTIME) || chip->use_vga_switcheroo)
> pm_runtime_put_noidle(&pci->dev);
Maybe we can now call pm_runtime_put_noidle() and
pm_runtime_get_noresume() unconditionally, since there is already a
check in azx_runtime_idle(), so the unexpected runtime suspend should
be already filtered there.
BTW, how would you like to keep patches in git tree(s)?
There's been already some changes about runtime PM in hda_intel.c for
Haswell, so your changes might conflict. If you can give a persistent
git branch I can pull, I'll cut off a new branch for this and merge
with the current Haswell stuff, then merge back for linux-next in
sound git tree.
thanks,
Takashi
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
