[PATCH] PCI: export pci_bridge_d3_possible

[PATCH] PCI: export pci_bridge_d3_possible

[Peter Wu]

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(-)

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);
 
-- 
2.9.0

Re: [PATCH] PCI: export pci_bridge_d3_possible

[Mika Westerberg]

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;

Re: [PATCH] PCI: export pci_bridge_d3_possible

[Peter Wu]

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?
-- 
Kind regards,
Peter Wu
https://lekensteyn.nl

Re: [PATCH] PCI: export pci_bridge_d3_possible

[Mika Westerberg]

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;
	}