Re: [PATCH 5/7] vga_switcheroo: Use device link for HDA controller
[+cc George] On Fri, Feb 23, 2018 at 10:51:47AM +0100, Lukas Wunner wrote: > On Tue, Feb 20, 2018 at 04:20:59PM -0600, Bjorn Helgaas wrote: > > On Sun, Feb 18, 2018 at 09:38:32AM +0100, Lukas Wunner wrote: > > > The device link is added in a PCI quirk rather than in hda_intel.c. > > > It is therefore legal for the GPU to runtime suspend to D3cold even if > > > the HDA controller is not bound to a driver or if CONFIG_SND_HDA_INTEL > > > is not enabled, for accesses to the HDA controller will cause the GPU to > > > wake up regardless if they're occurring outside of hda_intel.c (think > > > config space readout via sysfs). > > > > I guess this GPU wakeup happens *if* the path accessing the HDA > > controller calls pm_runtime_get_sync() or similar, right? > > Right. > > > We do have > > that in the sysfs config accessors via pci_config_pm_runtime_get(), > > but not in the sysfs mmap paths. I think that's a latent PCI core > > defect independent of this series. > > Yes, there may be a few places where the parent of the device is > erroneously not runtime resumed when sysfs files are accessed. > I've never used the sysfs mmap feature, so never witnessed issues > with it. > > > We also don't have that in PCI core config accessors. That maybe > > doesn't matter because the core probably shouldn't be touching devices > > after enumeration (although there might be holes there for things like > > ASPM where a sysfs file can cause reconfiguration of several devices). > > I assume with PCI core config accessors you're referring to > pci_read_config_word() and friends? Those are arguably hotpaths > where we wouldn't want the overhead to check runtime PM status > everytime. They might also be called from atomic context, I'm > not sure, and the runtime PM callbacks may sleep by default > (unless pm_runtime_irq_safe() was called). Yes, I was thinking of pci_read_config_word(), etc. They're used by the core during enumeration and occasionally by drivers, and both cases already pay attention to PM. I think we have a few holes in the runtime sysfs area, but I think it's reasonable to deal with those in the callers. So I don't think those need to actually *do* anything with PM status, although it might be interesting to add some (optional) checking there. George Cherian is turning up some issues in this area because he has a root port that reports errors with an exception instead of silently synthesizing all ones data like most hardware does [1]. Bjorn [1] https://lkml.kernel.org/r/1517554846-16703-1-git-send-email-george.cher...@cavium.com ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 5/7] vga_switcheroo: Use device link for HDA controller
On Tue, Feb 20, 2018 at 04:20:59PM -0600, Bjorn Helgaas wrote:
> On Sun, Feb 18, 2018 at 09:38:32AM +0100, Lukas Wunner wrote:
> > The device link is added in a PCI quirk rather than in hda_intel.c.
> > It is therefore legal for the GPU to runtime suspend to D3cold even if
> > the HDA controller is not bound to a driver or if CONFIG_SND_HDA_INTEL
> > is not enabled, for accesses to the HDA controller will cause the GPU to
> > wake up regardless if they're occurring outside of hda_intel.c (think
> > config space readout via sysfs).
>
> I guess this GPU wakeup happens *if* the path accessing the HDA
> controller calls pm_runtime_get_sync() or similar, right?
Right.
> We do have
> that in the sysfs config accessors via pci_config_pm_runtime_get(),
> but not in the sysfs mmap paths. I think that's a latent PCI core
> defect independent of this series.
Yes, there may be a few places where the parent of the device is
erroneously not runtime resumed when sysfs files are accessed.
I've never used the sysfs mmap feature, so never witnessed issues
with it.
> We also don't have that in PCI core config accessors. That maybe
> doesn't matter because the core probably shouldn't be touching devices
> after enumeration (although there might be holes there for things like
> ASPM where a sysfs file can cause reconfiguration of several devices).
I assume with PCI core config accessors you're referring to
pci_read_config_word() and friends? Those are arguably hotpaths
where we wouldn't want the overhead to check runtime PM status
everytime. They might also be called from atomic context, I'm
not sure, and the runtime PM callbacks may sleep by default
(unless pm_runtime_irq_safe() was called).
> > +static void quirk_gpu_hda(struct pci_dev *hda)
> > +{
> > + struct pci_dev *gpu = NULL;
> > +
> > + if (PCI_FUNC(hda->devfn) != 1)
> > + return;
> > +
> > + gpu = pci_get_domain_bus_and_slot(pci_domain_nr(hda->bus),
>
> Unnecessary initialization.
Thanks for spotting this, it's a remnant of an earlier version of the
patch which called pci_dev_put(gpu) in the (PCI_FUNC(hda->devfn) != 1)
case.
Best regards,
Lukas
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 5/7] vga_switcheroo: Use device link for HDA controller
On Sun, Feb 18, 2018 at 09:38:32AM +0100, Lukas Wunner wrote:
> Back in 2013, runtime PM for GPUs with integrated HDA controller was
> introduced with commits 0d69704ae348 ("gpu/vga_switcheroo: add driver
> control power feature. (v3)") and 246efa4a072f ("snd/hda: add runtime
> suspend/resume on optimus support (v4)").
>
> Briefly, the idea was that the HDA controller is forced on and off in
> unison with the GPU.
>
> The original code is mostly still in place even though it was never a
> 100% perfect solution: E.g. on access to the HDA controller, the GPU
> is powered up via vga_switcheroo_runtime_resume_hdmi_audio() but there
> are no provisions to keep it resumed until access to the HDA controller
> has ceased: The GPU autosuspends after 5 seconds, rendering the HDA
> controller inaccessible.
>
> Additionally, a kludge is required when hda_intel.c probes: It has to
> check whether the GPU is powered down (check_hdmi_disabled()) and defer
> probing if so.
>
> However in the meantime (in v4.10) the driver core has gained a feature
> called device links which promises to solve such issues in a clean way:
> It allows us to declare a dependency from the HDA controller (consumer)
> to the GPU (supplier). The PM core then automagically ensures that the
> GPU is runtime resumed as long as the HDA controller's ->probe hook is
> executed and whenever the HDA controller is accessed.
>
> By default, the HDA controller has a dependency on its parent, a PCIe
> Root Port. Adding a device link creates another dependency on its
> sibling:
>
> PCIe Root Port
> ^ ^
> | |
> | |
> HDA ===> GPU
>
> The device link is not only used for runtime PM, it also guarantees that
> on system sleep, the HDA controller suspends before the GPU and resumes
> after the GPU, and on system shutdown the HDA controller's ->shutdown
> hook is executed before the one of the GPU. It is a complete solution.
>
> Using this functionality is as simple as calling device_link_add(),
> which results in a dmesg entry like this:
>
> pci :01:00.1: Linked as a consumer to :01:00.0
>
> The code for the GPU-governed audio power management can thus be removed
> (except where it's still needed for legacy manual power control).
>
> The device link is added in a PCI quirk rather than in hda_intel.c.
> It is therefore legal for the GPU to runtime suspend to D3cold even if
> the HDA controller is not bound to a driver or if CONFIG_SND_HDA_INTEL
> is not enabled, for accesses to the HDA controller will cause the GPU to
> wake up regardless if they're occurring outside of hda_intel.c (think
> config space readout via sysfs).
I guess this GPU wakeup happens *if* the path accessing the HDA
controller calls pm_runtime_get_sync() or similar, right? We do have
that in the sysfs config accessors via pci_config_pm_runtime_get(),
but not in the sysfs mmap paths. I think that's a latent PCI core
defect independent of this series.
We also don't have that in PCI core config accessors. That maybe
doesn't matter because the core probably shouldn't be touching devices
after enumeration (although there might be holes there for things like
ASPM where a sysfs file can cause reconfiguration of several devices).
> Contrary to the previous implementation, the HDA controller's power
> state is now self-governed, rather than GPU-governed, whereas the GPU's
> power state is no longer fully self-governed. (The HDA controller needs
> to runtime suspend before the GPU can.)
>
> It is thus crucial that runtime PM is always activated on the HDA
> controller even if CONFIG_SND_HDA_POWER_SAVE_DEFAULT is set to 0 (which
> is the default), lest the GPU stays awake. This is achieved by setting
> the auto_runtime_pm flag on every codec and the AZX_DCAPS_PM_RUNTIME
> flag on the HDA controller.
>
> A side effect is that power consumption might be reduced if the GPU is
> in use but the HDA controller is not, because the HDA controller is now
> allowed to go to D3hot. Before, it was forced to stay in D0 as long as
> the GPU was in use. (There is no reduction in power consumption on my
> Nvidia GK107, but there might be on other chips.)
>
> The code paths for legacy manual power control are adjusted such that
> runtime PM is disabled during power off, thereby preventing the PM core
> from resuming the HDA controller.
>
> Note that the device link is not only added on vga_switcheroo capable
> systems, but for *any* GPU with integrated HDA controller. The idea is
> that the HDA controller streams audio via connectors located on the GPU,
> so the GPU needs to be on for the HDA controller to do anything useful.
>
> This commit implicitly fixes an unbalanced runtime PM ref upon unbind of
> hda_intel.c: On ->probe, a runtime PM ref was previously released under
> the condition
[PATCH 5/7] vga_switcheroo: Use device link for HDA controller
Back in 2013, runtime PM for GPUs with integrated HDA controller was
introduced with commits 0d69704ae348 ("gpu/vga_switcheroo: add driver
control power feature. (v3)") and 246efa4a072f ("snd/hda: add runtime
suspend/resume on optimus support (v4)").
Briefly, the idea was that the HDA controller is forced on and off in
unison with the GPU.
The original code is mostly still in place even though it was never a
100% perfect solution: E.g. on access to the HDA controller, the GPU
is powered up via vga_switcheroo_runtime_resume_hdmi_audio() but there
are no provisions to keep it resumed until access to the HDA controller
has ceased: The GPU autosuspends after 5 seconds, rendering the HDA
controller inaccessible.
Additionally, a kludge is required when hda_intel.c probes: It has to
check whether the GPU is powered down (check_hdmi_disabled()) and defer
probing if so.
However in the meantime (in v4.10) the driver core has gained a feature
called device links which promises to solve such issues in a clean way:
It allows us to declare a dependency from the HDA controller (consumer)
to the GPU (supplier). The PM core then automagically ensures that the
GPU is runtime resumed as long as the HDA controller's ->probe hook is
executed and whenever the HDA controller is accessed.
By default, the HDA controller has a dependency on its parent, a PCIe
Root Port. Adding a device link creates another dependency on its
sibling:
PCIe Root Port
^ ^
| |
| |
HDA ===> GPU
The device link is not only used for runtime PM, it also guarantees that
on system sleep, the HDA controller suspends before the GPU and resumes
after the GPU, and on system shutdown the HDA controller's ->shutdown
hook is executed before the one of the GPU. It is a complete solution.
Using this functionality is as simple as calling device_link_add(),
which results in a dmesg entry like this:
pci :01:00.1: Linked as a consumer to :01:00.0
The code for the GPU-governed audio power management can thus be removed
(except where it's still needed for legacy manual power control).
The device link is added in a PCI quirk rather than in hda_intel.c.
It is therefore legal for the GPU to runtime suspend to D3cold even if
the HDA controller is not bound to a driver or if CONFIG_SND_HDA_INTEL
is not enabled, for accesses to the HDA controller will cause the GPU to
wake up regardless if they're occurring outside of hda_intel.c (think
config space readout via sysfs).
Contrary to the previous implementation, the HDA controller's power
state is now self-governed, rather than GPU-governed, whereas the GPU's
power state is no longer fully self-governed. (The HDA controller needs
to runtime suspend before the GPU can.)
It is thus crucial that runtime PM is always activated on the HDA
controller even if CONFIG_SND_HDA_POWER_SAVE_DEFAULT is set to 0 (which
is the default), lest the GPU stays awake. This is achieved by setting
the auto_runtime_pm flag on every codec and the AZX_DCAPS_PM_RUNTIME
flag on the HDA controller.
A side effect is that power consumption might be reduced if the GPU is
in use but the HDA controller is not, because the HDA controller is now
allowed to go to D3hot. Before, it was forced to stay in D0 as long as
the GPU was in use. (There is no reduction in power consumption on my
Nvidia GK107, but there might be on other chips.)
The code paths for legacy manual power control are adjusted such that
runtime PM is disabled during power off, thereby preventing the PM core
from resuming the HDA controller.
Note that the device link is not only added on vga_switcheroo capable
systems, but for *any* GPU with integrated HDA controller. The idea is
that the HDA controller streams audio via connectors located on the GPU,
so the GPU needs to be on for the HDA controller to do anything useful.
This commit implicitly fixes an unbalanced runtime PM ref upon unbind of
hda_intel.c: On ->probe, a runtime PM ref was previously released under
the condition "azx_has_pm_runtime(chip) || hda->use_vga_switcheroo", but
on ->remove a runtime PM ref was only acquired under the first of those
conditions. Thus, binding and unbinding the driver twice on a
vga_switcheroo capable system caused the runtime PM refcount to drop
below zero. The issue is resolved because the AZX_DCAPS_PM_RUNTIME flag
is now always set if use_vga_switcheroo is true.
For more information on device links please refer to:
https://www.kernel.org/doc/html/latest/driver-api/device_link.html
Documentation/driver-api/device_link.rst
Cc: Dave Airlie
Cc: Ben Skeggs
Cc: Takashi Iwai
Cc: Peter Wu
Cc: Alex Deucher
Cc: Bjorn Helgaas
Cc: Rafael J. Wysocki
