[PATCH 4/4] snd/hda: add runtime suspend/resume on optimus support (v3)
Dave Airlie
airlied at gmail.com
Sun Aug 18 21:50:11 PDT 2013
On Tue, Aug 6, 2013 at 5:14 PM, Takashi Iwai <tiwai at suse.de> wrote:
> 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.
I do wonder how well the intel runtime PM stuff works sometimes, I'm not even
sure how to test it, seems pointless to have runtime support that
nobody turns on,
and doesn't seem to be on by default. how do I best test it? using powertop?
Why would the intel dynamic pm code not be able to handle an irq though, if the
hw is off the 0xffffffff check I put it in should handle it.
>
>> 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.
This is mainly to force the secondary GPU to not actually runtime
suspend without
the CAP, I only want to use the runtime infrastructure to enable
getting the device out of
D3cold, its the only method, in order to do that I don't really want
to use the device driver
runtime PM in normal situtations on these devices, I just want to have
the upper layers
of the stack think the device is runtime suspended when really I've
stuck it into D3cold via
the GPU driver runtime s/r code.
>
>> 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.
Again I don't really want the device driver doing any runtime pm
stuff, its going
to get powered off, so I really just want the upper stack layers to
understand that
the only way to get it back is to call the runtime PM stuff, which will work via
the power domain in the vgaswitcheroo and not via the hda_intel code.
>
>> + /* 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.
Same reason as above, I really don't want the driver doing anything also at this
stage we are already inside a resume handler, as if the device is in D3cold,
config space access will call runtime resume, which will enter the vga
switcheroo
power domain for this device, which will then enter gpu resume, which
will end up
back here via switcheroo paths, we really don't want it re-entering
the driver code again,
hence the get with no resume, as the driver will be resumed no matter
what later.
>
>> 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.
Yeah I wasn't sure, I'm happy to leave this for now until we know,
this stuff can be fragile
enough :-)
>
>
> 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.
I'll get a stable git tree for this stuff soon, I have the patches in git,
I just want to do a bit more testing on them and I'll push it out.
Dave.
More information about the dri-devel
mailing list