[PATCH 4/4] snd/hda: add runtime suspend/resume on optimus support (v3)

[Takashi Iwai]

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 at redhat.com>
> ---
>  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