| Message ID | 20160707235448.2622-1-peter@lekensteyn.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Fri, Jul 08, 2016 at 01:54:48AM +0200, Peter Wu wrote: > Allow the nouveau driver to find out whether the bridge can put itself > in the D3cold state or whether it should use a specific DSM method to > achieve the same result. > > Cc: Mika Westerberg <mika.westerberg@linux.intel.com> > Signed-off-by: Peter Wu <peter@lekensteyn.nl> > --- > Since it is not yet merged in Linus tree, maybe the patch in pci/pm can be > amended? This is the follow-up patch I had in mind for nouveau: > > --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c > +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c > @@ -223,6 +223,9 @@ static bool nouveau_pr3_present(struct pci_dev *pdev) > if (!parent_pdev) > return false; > > + if (!pci_bridge_d3_possible(parent_pdev)) > + return false; > + Why not check bridge_d3 directly? if (!parent_dev->bridge_d3) return false; -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 08, 2016 at 11:25:03AM +0300, Mika Westerberg wrote: > On Fri, Jul 08, 2016 at 01:54:48AM +0200, Peter Wu wrote: > > Allow the nouveau driver to find out whether the bridge can put itself > > in the D3cold state or whether it should use a specific DSM method to > > achieve the same result. > > > > Cc: Mika Westerberg <mika.westerberg@linux.intel.com> > > Signed-off-by: Peter Wu <peter@lekensteyn.nl> > > --- > > Since it is not yet merged in Linus tree, maybe the patch in pci/pm can be > > amended? This is the follow-up patch I had in mind for nouveau: > > > > --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c > > +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c > > @@ -223,6 +223,9 @@ static bool nouveau_pr3_present(struct pci_dev *pdev) > > if (!parent_pdev) > > return false; > > > > + if (!pci_bridge_d3_possible(parent_pdev)) > > + return false; > > + > > Why not check bridge_d3 directly? > > if (!parent_dev->bridge_d3) > return false; I have thought of that but then dismissed the idea because pci_bridge_d3_update could change it after initialization based on the d3cold_allowed flag on the bridge or its children. Then this could happen: - initially d3cold_allowed is set false by the user - nouveau decides to use DSM - d3cold_allowed is set by user to true - PCI thinks that power resources are OK to use, but that conflicts with nouveau. Hmm, maybe it is usable, but then something like this is needed: /* Initially assume that D3cold is OK. */ pci_d3cold_enable(pdev); if (!parent_dev->bridge_d3) { /* bridge does not support D3cold, keep it disabled. */ pci_d3cold_disable(pdev); return false; } How does that look?
On Fri, Jul 08, 2016 at 10:45:25AM +0200, Peter Wu wrote: > On Fri, Jul 08, 2016 at 11:25:03AM +0300, Mika Westerberg wrote: > > On Fri, Jul 08, 2016 at 01:54:48AM +0200, Peter Wu wrote: > > > Allow the nouveau driver to find out whether the bridge can put itself > > > in the D3cold state or whether it should use a specific DSM method to > > > achieve the same result. > > > > > > Cc: Mika Westerberg <mika.westerberg@linux.intel.com> > > > Signed-off-by: Peter Wu <peter@lekensteyn.nl> > > > --- > > > Since it is not yet merged in Linus tree, maybe the patch in pci/pm can be > > > amended? This is the follow-up patch I had in mind for nouveau: > > > > > > --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c > > > +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c > > > @@ -223,6 +223,9 @@ static bool nouveau_pr3_present(struct pci_dev *pdev) > > > if (!parent_pdev) > > > return false; > > > > > > + if (!pci_bridge_d3_possible(parent_pdev)) > > > + return false; > > > + > > > > Why not check bridge_d3 directly? > > > > if (!parent_dev->bridge_d3) > > return false; > > I have thought of that but then dismissed the idea because > pci_bridge_d3_update could change it after initialization based on the > d3cold_allowed flag on the bridge or its children. Then this could > happen: > > - initially d3cold_allowed is set false by the user > - nouveau decides to use DSM > - d3cold_allowed is set by user to true > - PCI thinks that power resources are OK to use, but that conflicts > with nouveau. Indeed that might happen. However, to be on the safe side I think we should just do something like this: if (!parent_dev->bridge_d3) { /* * Parent PCI bridge is currently not power managed. * Since userspace can change these afterwards to be on * the safe side we stick with _DSM and prevent usage of * _PR3 from the bridge. */ pci_d3cold_disable(pdev); return false; } -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 9ff7183..714701b 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -2178,7 +2178,7 @@ void pci_config_pm_runtime_put(struct pci_dev *pdev) * This function checks if it is possible to move the bridge to D3. * Currently we only allow D3 for recent enough PCIe ports. */ -static bool pci_bridge_d3_possible(struct pci_dev *bridge) +bool pci_bridge_d3_possible(struct pci_dev *bridge) { unsigned int year; @@ -2207,6 +2207,7 @@ static bool pci_bridge_d3_possible(struct pci_dev *bridge) return false; } +EXPORT_SYMBOL_GPL(pci_bridge_d3_possible); static int pci_dev_check_d3cold(struct pci_dev *dev, void *data) { diff --git a/include/linux/pci.h b/include/linux/pci.h index 0a1a9e3..f19761d 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1088,6 +1088,7 @@ int pci_back_from_sleep(struct pci_dev *dev); bool pci_dev_run_wake(struct pci_dev *dev); bool pci_check_pme_status(struct pci_dev *dev); void pci_pme_wakeup_bus(struct pci_bus *bus); +bool pci_bridge_d3_possible(struct pci_dev *bridge); void pci_d3cold_enable(struct pci_dev *dev); void pci_d3cold_disable(struct pci_dev *dev);
Allow the nouveau driver to find out whether the bridge can put itself in the D3cold state or whether it should use a specific DSM method to achieve the same result. Cc: Mika Westerberg <mika.westerberg@linux.intel.com> Signed-off-by: Peter Wu <peter@lekensteyn.nl> --- Since it is not yet merged in Linus tree, maybe the patch in pci/pm can be amended? This is the follow-up patch I had in mind for nouveau: --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c @@ -223,6 +223,9 @@ static bool nouveau_pr3_present(struct pci_dev *pdev) if (!parent_pdev) return false; + if (!pci_bridge_d3_possible(parent_pdev)) + return false; + parent_adev = ACPI_COMPANION(&parent_pdev->dev); if (!parent_adev) return false; Related nouveau patches were sent a few minutes ago, titled "[PATCH v2 0/4] nouveau RPM fixes for Optimus". --- drivers/pci/pci.c | 3 ++- include/linux/pci.h | 1 + 2 files changed, 3 insertions(+), 1 deletion(-)