From: Dave Airlie <[email protected]>
Windows 10 seems to have standardised power control for the
optimus/powerxpress laptops using PR3 power resource hooks.
I'm not sure this is definitely the correct place to be
doing this, but it works for me here.
The ACPI device for the GPU I have is \_SB_.PCI0.PEG_.VID_
but the power resource hooks are on \_SB_.PCI0.PEG_, so
this patch creates a new power domain to turn the GPU
device parent off using standard ACPI calls.
Signed-off-by: Dave Airlie <[email protected]>
---
drivers/gpu/vga/vga_switcheroo.c | 54 +++++++++++++++++++++++++++++++++++++++-
include/linux/vga_switcheroo.h | 3 ++-
2 files changed, 55 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index 665ab9f..be32cb2 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -42,7 +42,7 @@
#include <linux/uaccess.h>
#include <linux/vgaarb.h>
#include <linux/vga_switcheroo.h>
-
+#include <linux/acpi.h>
/**
* DOC: Overview
*
@@ -997,3 +997,55 @@ vga_switcheroo_init_domain_pm_optimus_hdmi_audio(struct device *dev,
return -EINVAL;
}
EXPORT_SYMBOL(vga_switcheroo_init_domain_pm_optimus_hdmi_audio);
+
+/* With Windows 10 the runtime suspend/resume can use power
+ resources on the parent device */
+static int vga_acpi_switcheroo_runtime_suspend(struct device *dev)
+{
+ struct pci_dev *pdev = to_pci_dev(dev);
+ int ret;
+ struct acpi_device *adev;
+
+ ret = dev->bus->pm->runtime_suspend(dev);
+ if (ret)
+ return ret;
+
+ ret = acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev);
+ if (!ret)
+ acpi_device_set_power(adev->parent, ACPI_STATE_D3_COLD);
+ return 0;
+}
+
+static int vga_acpi_switcheroo_runtime_resume(struct device *dev)
+{
+ struct pci_dev *pdev = to_pci_dev(dev);
+ struct acpi_device *adev;
+ int ret;
+
+ ret = acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev);
+ if (!ret)
+ acpi_device_set_power(adev->parent, ACPI_STATE_D0);
+ ret = dev->bus->pm->runtime_resume(dev);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+int vga_switcheroo_init_parent_pr3_ops(struct device *dev,
+ struct dev_pm_domain *domain)
+
+{
+ /* copy over all the bus versions */
+ if (dev->bus && dev->bus->pm) {
+ domain->ops = *dev->bus->pm;
+ domain->ops.runtime_suspend = vga_acpi_switcheroo_runtime_suspend;
+ domain->ops.runtime_resume = vga_acpi_switcheroo_runtime_resume;
+
+ dev_pm_domain_set(dev, domain);
+ return 0;
+ }
+ dev_pm_domain_set(dev, NULL);
+ return -EINVAL;
+}
+EXPORT_SYMBOL(vga_switcheroo_init_parent_pr3_ops);
diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
index 69e1d4a1..5ce0cbe 100644
--- a/include/linux/vga_switcheroo.h
+++ b/include/linux/vga_switcheroo.h
@@ -144,6 +144,7 @@ void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, enum vga_switcheroo
int vga_switcheroo_init_domain_pm_ops(struct device *dev, struct dev_pm_domain *domain);
void vga_switcheroo_fini_domain_pm_ops(struct device *dev);
int vga_switcheroo_init_domain_pm_optimus_hdmi_audio(struct device *dev, struct dev_pm_domain *domain);
+int vga_switcheroo_init_parent_pr3_ops(struct device *dev, struct dev_pm_domain *domain);
#else
static inline void vga_switcheroo_unregister_client(struct pci_dev *dev) {}
@@ -163,6 +164,6 @@ static inline void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, enum
static inline int vga_switcheroo_init_domain_pm_ops(struct device *dev, struct dev_pm_domain *domain) { return -EINVAL; }
static inline void vga_switcheroo_fini_domain_pm_ops(struct device *dev) {}
static inline int vga_switcheroo_init_domain_pm_optimus_hdmi_audio(struct device *dev, struct dev_pm_domain *domain) { return -EINVAL; }
-
+static inline int vga_switcheroo_init_parent_pr3_ops(struct device *dev, struct dev_pm_domain *domain) { return -EINVAL; }
#endif
#endif /* _LINUX_VGA_SWITCHEROO_H_ */
--
2.5.0
From: Dave Airlie <[email protected]>
This fixes GPU auto powerdown on the Lenovo W541,
since we advertise Windows 2013 to the ACPI layer.
Signed-off-by: Dave Airlie <[email protected]>
---
drivers/gpu/drm/nouveau/nouveau_vga.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c b/drivers/gpu/drm/nouveau/nouveau_vga.c
index af89c36..b987427f 100644
--- a/drivers/gpu/drm/nouveau/nouveau_vga.c
+++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
@@ -101,8 +101,12 @@ nouveau_vga_init(struct nouveau_drm *drm)
runtime = true;
vga_switcheroo_register_client(dev->pdev, &nouveau_switcheroo_ops, runtime);
- if (runtime && nouveau_is_v1_dsm() && !nouveau_is_optimus())
- vga_switcheroo_init_domain_pm_ops(drm->dev->dev, &drm->vga_pm_domain);
+ if (runtime) {
+ if (nouveau_is_v1_dsm() && !nouveau_is_optimus())
+ vga_switcheroo_init_domain_pm_ops(drm->dev->dev, &drm->vga_pm_domain);
+ else if (nouveau_is_optimus())
+ vga_switcheroo_init_parent_pr3_ops(drm->dev->dev, &drm->vga_pm_domain);
+ }
}
void
@@ -117,7 +121,7 @@ nouveau_vga_fini(struct nouveau_drm *drm)
runtime = true;
vga_switcheroo_unregister_client(dev->pdev);
- if (runtime && nouveau_is_v1_dsm() && !nouveau_is_optimus())
+ if (runtime && (nouveau_is_v1_dsm() || nouveau_is_optimus()))
vga_switcheroo_fini_domain_pm_ops(drm->dev->dev);
vga_client_register(dev->pdev, NULL, NULL, NULL);
}
--
2.5.0
On Wed, Mar 9, 2016 at 7:14 AM, Dave Airlie <[email protected]> wrote:
> From: Dave Airlie <[email protected]>
>
> Windows 10 seems to have standardised power control for the
> optimus/powerxpress laptops using PR3 power resource hooks.
>
> I'm not sure this is definitely the correct place to be
> doing this, but it works for me here.
>
> The ACPI device for the GPU I have is \_SB_.PCI0.PEG_.VID_
> but the power resource hooks are on \_SB_.PCI0.PEG_, so
> this patch creates a new power domain to turn the GPU
> device parent off using standard ACPI calls.
>
> Signed-off-by: Dave Airlie <[email protected]>
> ---
> drivers/gpu/vga/vga_switcheroo.c | 54 +++++++++++++++++++++++++++++++++++++++-
> include/linux/vga_switcheroo.h | 3 ++-
> 2 files changed, 55 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> index 665ab9f..be32cb2 100644
> --- a/drivers/gpu/vga/vga_switcheroo.c
> +++ b/drivers/gpu/vga/vga_switcheroo.c
> @@ -42,7 +42,7 @@
> #include <linux/uaccess.h>
> #include <linux/vgaarb.h>
> #include <linux/vga_switcheroo.h>
> -
> +#include <linux/acpi.h>
> /**
> * DOC: Overview
> *
> @@ -997,3 +997,55 @@ vga_switcheroo_init_domain_pm_optimus_hdmi_audio(struct device *dev,
> return -EINVAL;
> }
> EXPORT_SYMBOL(vga_switcheroo_init_domain_pm_optimus_hdmi_audio);
> +
> +/* With Windows 10 the runtime suspend/resume can use power
> + resources on the parent device */
> +static int vga_acpi_switcheroo_runtime_suspend(struct device *dev)
> +{
> + struct pci_dev *pdev = to_pci_dev(dev);
> + int ret;
> + struct acpi_device *adev;
> +
> + ret = dev->bus->pm->runtime_suspend(dev);
> + if (ret)
> + return ret;
> +
> + ret = acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev);
You can use ACPI_COMPANION(&pdev->dev) for that.
> + if (!ret)
> + acpi_device_set_power(adev->parent, ACPI_STATE_D3_COLD);
Won't that mess up with the PM of the parent? Or do we know that the
parent won't do its own PM?
> + return 0;
> +}
> +
> +static int vga_acpi_switcheroo_runtime_resume(struct device *dev)
> +{
> + struct pci_dev *pdev = to_pci_dev(dev);
> + struct acpi_device *adev;
> + int ret;
> +
> + ret = acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev);
ACPI_COMPANION(&pdev->dev) here too?
> + if (!ret)
> + acpi_device_set_power(adev->parent, ACPI_STATE_D0);
> + ret = dev->bus->pm->runtime_resume(dev);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +int vga_switcheroo_init_parent_pr3_ops(struct device *dev,
> + struct dev_pm_domain *domain)
> +
> +{
> + /* copy over all the bus versions */
> + if (dev->bus && dev->bus->pm) {
> + domain->ops = *dev->bus->pm;
> + domain->ops.runtime_suspend = vga_acpi_switcheroo_runtime_suspend;
> + domain->ops.runtime_resume = vga_acpi_switcheroo_runtime_resume;
> +
> + dev_pm_domain_set(dev, domain);
> + return 0;
> + }
> + dev_pm_domain_set(dev, NULL);
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL(vga_switcheroo_init_parent_pr3_ops);
> diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
> index 69e1d4a1..5ce0cbe 100644
> --- a/include/linux/vga_switcheroo.h
> +++ b/include/linux/vga_switcheroo.h
> @@ -144,6 +144,7 @@ void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, enum vga_switcheroo
> int vga_switcheroo_init_domain_pm_ops(struct device *dev, struct dev_pm_domain *domain);
> void vga_switcheroo_fini_domain_pm_ops(struct device *dev);
> int vga_switcheroo_init_domain_pm_optimus_hdmi_audio(struct device *dev, struct dev_pm_domain *domain);
> +int vga_switcheroo_init_parent_pr3_ops(struct device *dev, struct dev_pm_domain *domain);
> #else
>
> static inline void vga_switcheroo_unregister_client(struct pci_dev *dev) {}
> @@ -163,6 +164,6 @@ static inline void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, enum
> static inline int vga_switcheroo_init_domain_pm_ops(struct device *dev, struct dev_pm_domain *domain) { return -EINVAL; }
> static inline void vga_switcheroo_fini_domain_pm_ops(struct device *dev) {}
> static inline int vga_switcheroo_init_domain_pm_optimus_hdmi_audio(struct device *dev, struct dev_pm_domain *domain) { return -EINVAL; }
> -
> +static inline int vga_switcheroo_init_parent_pr3_ops(struct device *dev, struct dev_pm_domain *domain) { return -EINVAL; }
> #endif
> #endif /* _LINUX_VGA_SWITCHEROO_H_ */
> --
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 9, 2016 at 7:14 AM, Dave Airlie <[email protected]> wrote:
> From: Dave Airlie <[email protected]>
>
> This fixes GPU auto powerdown on the Lenovo W541,
> since we advertise Windows 2013 to the ACPI layer.
>
> Signed-off-by: Dave Airlie <[email protected]>
> ---
> drivers/gpu/drm/nouveau/nouveau_vga.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c b/drivers/gpu/drm/nouveau/nouveau_vga.c
> index af89c36..b987427f 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_vga.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
> @@ -101,8 +101,12 @@ nouveau_vga_init(struct nouveau_drm *drm)
> runtime = true;
> vga_switcheroo_register_client(dev->pdev, &nouveau_switcheroo_ops, runtime);
>
> - if (runtime && nouveau_is_v1_dsm() && !nouveau_is_optimus())
> - vga_switcheroo_init_domain_pm_ops(drm->dev->dev, &drm->vga_pm_domain);
> + if (runtime) {
> + if (nouveau_is_v1_dsm() && !nouveau_is_optimus())
> + vga_switcheroo_init_domain_pm_ops(drm->dev->dev, &drm->vga_pm_domain);
> + else if (nouveau_is_optimus())
> + vga_switcheroo_init_parent_pr3_ops(drm->dev->dev, &drm->vga_pm_domain);
> + }
> }
>
> void
> @@ -117,7 +121,7 @@ nouveau_vga_fini(struct nouveau_drm *drm)
> runtime = true;
>
> vga_switcheroo_unregister_client(dev->pdev);
> - if (runtime && nouveau_is_v1_dsm() && !nouveau_is_optimus())
> + if (runtime && (nouveau_is_v1_dsm() || nouveau_is_optimus()))
> vga_switcheroo_fini_domain_pm_ops(drm->dev->dev);
> vga_client_register(dev->pdev, NULL, NULL, NULL);
> }
> --
Looks reasonable to me.
Thanks,
Rafael
Hi Dave,
On Wed, Mar 09, 2016 at 04:14:04PM +1000, Dave Airlie wrote:
> From: Dave Airlie <[email protected]>
>
> Windows 10 seems to have standardised power control for the
> optimus/powerxpress laptops using PR3 power resource hooks.
What happened to the Optimus DSM, does this still work? If not,
echoing OFF to the vgaswitcheroo sysfs file won't power down the
GPU now, right? (If runtime pm is disabled in nouveau.)
> I'm not sure this is definitely the correct place to be
> doing this, but it works for me here.
How about implementing an additional vga_switcheroo handler somewhere
in drivers/acpi/, which looks for the PR3 hooks and registers with
vga_switcheroo if found. The ->power_state callback of that handler
would then invoke acpi_device_set_power().
The ACPI bus is scanned in the subsys_initcall section, much earlier
than nouveau is loaded (in the device_initcall section). So the ACPI
vga_switcheroo handler could register before the nouveau DSM handler
and therefore would always have a higher precedence.
In any case this patch should be amended with kerneldoc for
vga_switcheroo_init_parent_pr3_ops(), I've spent considerable time
last year to document everything and kindly ask that this water not
be muddied again. ;-)
One small nit, the headers are sorted alphabetically yet acpi.h is
added at the end.
Thanks,
Lukas
>
> The ACPI device for the GPU I have is \_SB_.PCI0.PEG_.VID_
> but the power resource hooks are on \_SB_.PCI0.PEG_, so
> this patch creates a new power domain to turn the GPU
> device parent off using standard ACPI calls.
>
> Signed-off-by: Dave Airlie <[email protected]>
> ---
> drivers/gpu/vga/vga_switcheroo.c | 54 +++++++++++++++++++++++++++++++++++++++-
> include/linux/vga_switcheroo.h | 3 ++-
> 2 files changed, 55 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> index 665ab9f..be32cb2 100644
> --- a/drivers/gpu/vga/vga_switcheroo.c
> +++ b/drivers/gpu/vga/vga_switcheroo.c
> @@ -42,7 +42,7 @@
> #include <linux/uaccess.h>
> #include <linux/vgaarb.h>
> #include <linux/vga_switcheroo.h>
> -
> +#include <linux/acpi.h>
> /**
> * DOC: Overview
> *
> @@ -997,3 +997,55 @@ vga_switcheroo_init_domain_pm_optimus_hdmi_audio(struct device *dev,
> return -EINVAL;
> }
> EXPORT_SYMBOL(vga_switcheroo_init_domain_pm_optimus_hdmi_audio);
> +
> +/* With Windows 10 the runtime suspend/resume can use power
> + resources on the parent device */
> +static int vga_acpi_switcheroo_runtime_suspend(struct device *dev)
> +{
> + struct pci_dev *pdev = to_pci_dev(dev);
> + int ret;
> + struct acpi_device *adev;
> +
> + ret = dev->bus->pm->runtime_suspend(dev);
> + if (ret)
> + return ret;
> +
> + ret = acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev);
> + if (!ret)
> + acpi_device_set_power(adev->parent, ACPI_STATE_D3_COLD);
> + return 0;
> +}
> +
> +static int vga_acpi_switcheroo_runtime_resume(struct device *dev)
> +{
> + struct pci_dev *pdev = to_pci_dev(dev);
> + struct acpi_device *adev;
> + int ret;
> +
> + ret = acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev);
> + if (!ret)
> + acpi_device_set_power(adev->parent, ACPI_STATE_D0);
> + ret = dev->bus->pm->runtime_resume(dev);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +int vga_switcheroo_init_parent_pr3_ops(struct device *dev,
> + struct dev_pm_domain *domain)
> +
> +{
> + /* copy over all the bus versions */
> + if (dev->bus && dev->bus->pm) {
> + domain->ops = *dev->bus->pm;
> + domain->ops.runtime_suspend = vga_acpi_switcheroo_runtime_suspend;
> + domain->ops.runtime_resume = vga_acpi_switcheroo_runtime_resume;
> +
> + dev_pm_domain_set(dev, domain);
> + return 0;
> + }
> + dev_pm_domain_set(dev, NULL);
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL(vga_switcheroo_init_parent_pr3_ops);
> diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
> index 69e1d4a1..5ce0cbe 100644
> --- a/include/linux/vga_switcheroo.h
> +++ b/include/linux/vga_switcheroo.h
> @@ -144,6 +144,7 @@ void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, enum vga_switcheroo
> int vga_switcheroo_init_domain_pm_ops(struct device *dev, struct dev_pm_domain *domain);
> void vga_switcheroo_fini_domain_pm_ops(struct device *dev);
> int vga_switcheroo_init_domain_pm_optimus_hdmi_audio(struct device *dev, struct dev_pm_domain *domain);
> +int vga_switcheroo_init_parent_pr3_ops(struct device *dev, struct dev_pm_domain *domain);
> #else
>
> static inline void vga_switcheroo_unregister_client(struct pci_dev *dev) {}
> @@ -163,6 +164,6 @@ static inline void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, enum
> static inline int vga_switcheroo_init_domain_pm_ops(struct device *dev, struct dev_pm_domain *domain) { return -EINVAL; }
> static inline void vga_switcheroo_fini_domain_pm_ops(struct device *dev) {}
> static inline int vga_switcheroo_init_domain_pm_optimus_hdmi_audio(struct device *dev, struct dev_pm_domain *domain) { return -EINVAL; }
> -
> +static inline int vga_switcheroo_init_parent_pr3_ops(struct device *dev, struct dev_pm_domain *domain) { return -EINVAL; }
> #endif
> #endif /* _LINUX_VGA_SWITCHEROO_H_ */
> --
> 2.5.0
Hi Dave,
On Wed, Mar 09, 2016 at 04:14:05PM +1000, Dave Airlie wrote:
> From: Dave Airlie <[email protected]>
>
> This fixes GPU auto powerdown on the Lenovo W541,
> since we advertise Windows 2013 to the ACPI layer.
>
> Signed-off-by: Dave Airlie <[email protected]>
> ---
> drivers/gpu/drm/nouveau/nouveau_vga.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c b/drivers/gpu/drm/nouveau/nouveau_vga.c
> index af89c36..b987427f 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_vga.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
> @@ -101,8 +101,12 @@ nouveau_vga_init(struct nouveau_drm *drm)
> runtime = true;
> vga_switcheroo_register_client(dev->pdev, &nouveau_switcheroo_ops, runtime);
>
> - if (runtime && nouveau_is_v1_dsm() && !nouveau_is_optimus())
> - vga_switcheroo_init_domain_pm_ops(drm->dev->dev, &drm->vga_pm_domain);
> + if (runtime) {
> + if (nouveau_is_v1_dsm() && !nouveau_is_optimus())
The " && !nouveau_is_optimus()" can be dropped because a machine cannot
have both. Note the "else" in nouveau_dsm_detect():
if (has_optimus == 1) { ...
nouveau_dsm_priv.optimus_detected = true;
} else if (vga_count == 2 && has_dsm && guid_valid) { ...
nouveau_dsm_priv.dsm_detected = true;
}
> + vga_switcheroo_init_domain_pm_ops(drm->dev->dev, &drm->vga_pm_domain);
> + else if (nouveau_is_optimus())
> + vga_switcheroo_init_parent_pr3_ops(drm->dev->dev, &drm->vga_pm_domain);
You're calling this unconditionally for all Optimus machines yet
I assume pre Windows 10 machines lack the PR3 hooks.
Best regards,
Lukas
> + }
> }
>
> void
> @@ -117,7 +121,7 @@ nouveau_vga_fini(struct nouveau_drm *drm)
> runtime = true;
>
> vga_switcheroo_unregister_client(dev->pdev);
> - if (runtime && nouveau_is_v1_dsm() && !nouveau_is_optimus())
> + if (runtime && (nouveau_is_v1_dsm() || nouveau_is_optimus()))
> vga_switcheroo_fini_domain_pm_ops(drm->dev->dev);
> vga_client_register(dev->pdev, NULL, NULL, NULL);
> }
> --
> 2.5.0
On Wed, Mar 9, 2016 at 9:33 AM, Lukas Wunner <[email protected]> wrote:
> Hi Dave,
>
> On Wed, Mar 09, 2016 at 04:14:04PM +1000, Dave Airlie wrote:
>> From: Dave Airlie <[email protected]>
>>
>> Windows 10 seems to have standardised power control for the
>> optimus/powerxpress laptops using PR3 power resource hooks.
>
> What happened to the Optimus DSM, does this still work? If not,
> echoing OFF to the vgaswitcheroo sysfs file won't power down the
> GPU now, right? (If runtime pm is disabled in nouveau.)
>
If the OS identifies itself as windows 10 or newer when initializing
ACPI, the standardized interfaces should be used if they exist. I'm
not sure if there is any explicit policy on the vendor specific ones,
but I suspect they are probably broken in that case since I doubt the
OEM validates them when the standardized interfaces are available.
Alex
>> I'm not sure this is definitely the correct place to be
>> doing this, but it works for me here.
>
> How about implementing an additional vga_switcheroo handler somewhere
> in drivers/acpi/, which looks for the PR3 hooks and registers with
> vga_switcheroo if found. The ->power_state callback of that handler
> would then invoke acpi_device_set_power().
>
> The ACPI bus is scanned in the subsys_initcall section, much earlier
> than nouveau is loaded (in the device_initcall section). So the ACPI
> vga_switcheroo handler could register before the nouveau DSM handler
> and therefore would always have a higher precedence.
>
> In any case this patch should be amended with kerneldoc for
> vga_switcheroo_init_parent_pr3_ops(), I've spent considerable time
> last year to document everything and kindly ask that this water not
> be muddied again. ;-)
>
> One small nit, the headers are sorted alphabetically yet acpi.h is
> added at the end.
>
> Thanks,
>
> Lukas
>
>>
>> The ACPI device for the GPU I have is \_SB_.PCI0.PEG_.VID_
>> but the power resource hooks are on \_SB_.PCI0.PEG_, so
>> this patch creates a new power domain to turn the GPU
>> device parent off using standard ACPI calls.
>>
>> Signed-off-by: Dave Airlie <[email protected]>
>> ---
>> drivers/gpu/vga/vga_switcheroo.c | 54 +++++++++++++++++++++++++++++++++++++++-
>> include/linux/vga_switcheroo.h | 3 ++-
>> 2 files changed, 55 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
>> index 665ab9f..be32cb2 100644
>> --- a/drivers/gpu/vga/vga_switcheroo.c
>> +++ b/drivers/gpu/vga/vga_switcheroo.c
>> @@ -42,7 +42,7 @@
>> #include <linux/uaccess.h>
>> #include <linux/vgaarb.h>
>> #include <linux/vga_switcheroo.h>
>> -
>> +#include <linux/acpi.h>
>> /**
>> * DOC: Overview
>> *
>> @@ -997,3 +997,55 @@ vga_switcheroo_init_domain_pm_optimus_hdmi_audio(struct device *dev,
>> return -EINVAL;
>> }
>> EXPORT_SYMBOL(vga_switcheroo_init_domain_pm_optimus_hdmi_audio);
>> +
>> +/* With Windows 10 the runtime suspend/resume can use power
>> + resources on the parent device */
>> +static int vga_acpi_switcheroo_runtime_suspend(struct device *dev)
>> +{
>> + struct pci_dev *pdev = to_pci_dev(dev);
>> + int ret;
>> + struct acpi_device *adev;
>> +
>> + ret = dev->bus->pm->runtime_suspend(dev);
>> + if (ret)
>> + return ret;
>> +
>> + ret = acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev);
>> + if (!ret)
>> + acpi_device_set_power(adev->parent, ACPI_STATE_D3_COLD);
>> + return 0;
>> +}
>> +
>> +static int vga_acpi_switcheroo_runtime_resume(struct device *dev)
>> +{
>> + struct pci_dev *pdev = to_pci_dev(dev);
>> + struct acpi_device *adev;
>> + int ret;
>> +
>> + ret = acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev);
>> + if (!ret)
>> + acpi_device_set_power(adev->parent, ACPI_STATE_D0);
>> + ret = dev->bus->pm->runtime_resume(dev);
>> + if (ret)
>> + return ret;
>> +
>> + return 0;
>> +}
>> +
>> +int vga_switcheroo_init_parent_pr3_ops(struct device *dev,
>> + struct dev_pm_domain *domain)
>> +
>> +{
>> + /* copy over all the bus versions */
>> + if (dev->bus && dev->bus->pm) {
>> + domain->ops = *dev->bus->pm;
>> + domain->ops.runtime_suspend = vga_acpi_switcheroo_runtime_suspend;
>> + domain->ops.runtime_resume = vga_acpi_switcheroo_runtime_resume;
>> +
>> + dev_pm_domain_set(dev, domain);
>> + return 0;
>> + }
>> + dev_pm_domain_set(dev, NULL);
>> + return -EINVAL;
>> +}
>> +EXPORT_SYMBOL(vga_switcheroo_init_parent_pr3_ops);
>> diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
>> index 69e1d4a1..5ce0cbe 100644
>> --- a/include/linux/vga_switcheroo.h
>> +++ b/include/linux/vga_switcheroo.h
>> @@ -144,6 +144,7 @@ void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, enum vga_switcheroo
>> int vga_switcheroo_init_domain_pm_ops(struct device *dev, struct dev_pm_domain *domain);
>> void vga_switcheroo_fini_domain_pm_ops(struct device *dev);
>> int vga_switcheroo_init_domain_pm_optimus_hdmi_audio(struct device *dev, struct dev_pm_domain *domain);
>> +int vga_switcheroo_init_parent_pr3_ops(struct device *dev, struct dev_pm_domain *domain);
>> #else
>>
>> static inline void vga_switcheroo_unregister_client(struct pci_dev *dev) {}
>> @@ -163,6 +164,6 @@ static inline void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, enum
>> static inline int vga_switcheroo_init_domain_pm_ops(struct device *dev, struct dev_pm_domain *domain) { return -EINVAL; }
>> static inline void vga_switcheroo_fini_domain_pm_ops(struct device *dev) {}
>> static inline int vga_switcheroo_init_domain_pm_optimus_hdmi_audio(struct device *dev, struct dev_pm_domain *domain) { return -EINVAL; }
>> -
>> +static inline int vga_switcheroo_init_parent_pr3_ops(struct device *dev, struct dev_pm_domain *domain) { return -EINVAL; }
>> #endif
>> #endif /* _LINUX_VGA_SWITCHEROO_H_ */
>> --
>> 2.5.0
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi,
On Wed, Mar 09, 2016 at 11:52:33AM -0500, Alex Deucher wrote:
> On Wed, Mar 9, 2016 at 9:33 AM, Lukas Wunner <[email protected]> wrote:
> > On Wed, Mar 09, 2016 at 04:14:04PM +1000, Dave Airlie wrote:
> >> From: Dave Airlie <[email protected]>
> >>
> >> Windows 10 seems to have standardised power control for the
> >> optimus/powerxpress laptops using PR3 power resource hooks.
> >
> > What happened to the Optimus DSM, does this still work? If not,
> > echoing OFF to the vgaswitcheroo sysfs file won't power down the
> > GPU now, right? (If runtime pm is disabled in nouveau.)
> >
> If the OS identifies itself as windows 10 or newer when initializing
> ACPI, the standardized interfaces should be used if they exist. I'm
> not sure if there is any explicit policy on the vendor specific ones,
> but I suspect they are probably broken in that case since I doubt the
> OEM validates them when the standardized interfaces are available.
The vendor interface (Optimus DSM) must be present, otherwise the call
to nouveau_is_optimus() in patch [2/2] would return false.
But indeed it seems to not be working, otherwise why would the patches
be necessary?
My point is that the chosen approach does not square with vga_switcheroo's
architecture: Normally it's the handler's job to power the GPU up/down.
If runtime pm is enabled then vga_switcheroo_init_domain_pm_ops()
activates runtime_suspend/_resume callbacks which ask the handler to
flip the power switch.
However these two patches add *additional* runtime_suspend/_resume
callbacks which do not rely on the handler at all. The handler is thus
useless. This becomes apparent when loading the nouveau module with
runpm=0: Normally the user is then able to manually power the GPU
up/down by echoing ON or OFF to the vgaswitcheroo sysfs file. With the
chosen approach this will likely not work because the handler only knows
how to invoke the DSM, it doesn't know anything about PR3.
Hence my suggestion to solve this with an additional handler which
gets used in lieu of the nouveau DSM handler.
Best regards,
Lukas
>
> Alex
>
> >> I'm not sure this is definitely the correct place to be
> >> doing this, but it works for me here.
> >
> > How about implementing an additional vga_switcheroo handler somewhere
> > in drivers/acpi/, which looks for the PR3 hooks and registers with
> > vga_switcheroo if found. The ->power_state callback of that handler
> > would then invoke acpi_device_set_power().
> >
> > The ACPI bus is scanned in the subsys_initcall section, much earlier
> > than nouveau is loaded (in the device_initcall section). So the ACPI
> > vga_switcheroo handler could register before the nouveau DSM handler
> > and therefore would always have a higher precedence.
> >
> > In any case this patch should be amended with kerneldoc for
> > vga_switcheroo_init_parent_pr3_ops(), I've spent considerable time
> > last year to document everything and kindly ask that this water not
> > be muddied again. ;-)
> >
> > One small nit, the headers are sorted alphabetically yet acpi.h is
> > added at the end.
> >
> > Thanks,
> >
> > Lukas
> >
> >>
> >> The ACPI device for the GPU I have is \_SB_.PCI0.PEG_.VID_
> >> but the power resource hooks are on \_SB_.PCI0.PEG_, so
> >> this patch creates a new power domain to turn the GPU
> >> device parent off using standard ACPI calls.
> >>
> >> Signed-off-by: Dave Airlie <[email protected]>
> >> ---
> >> drivers/gpu/vga/vga_switcheroo.c | 54 +++++++++++++++++++++++++++++++++++++++-
> >> include/linux/vga_switcheroo.h | 3 ++-
> >> 2 files changed, 55 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> >> index 665ab9f..be32cb2 100644
> >> --- a/drivers/gpu/vga/vga_switcheroo.c
> >> +++ b/drivers/gpu/vga/vga_switcheroo.c
> >> @@ -42,7 +42,7 @@
> >> #include <linux/uaccess.h>
> >> #include <linux/vgaarb.h>
> >> #include <linux/vga_switcheroo.h>
> >> -
> >> +#include <linux/acpi.h>
> >> /**
> >> * DOC: Overview
> >> *
> >> @@ -997,3 +997,55 @@ vga_switcheroo_init_domain_pm_optimus_hdmi_audio(struct device *dev,
> >> return -EINVAL;
> >> }
> >> EXPORT_SYMBOL(vga_switcheroo_init_domain_pm_optimus_hdmi_audio);
> >> +
> >> +/* With Windows 10 the runtime suspend/resume can use power
> >> + resources on the parent device */
> >> +static int vga_acpi_switcheroo_runtime_suspend(struct device *dev)
> >> +{
> >> + struct pci_dev *pdev = to_pci_dev(dev);
> >> + int ret;
> >> + struct acpi_device *adev;
> >> +
> >> + ret = dev->bus->pm->runtime_suspend(dev);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + ret = acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev);
> >> + if (!ret)
> >> + acpi_device_set_power(adev->parent, ACPI_STATE_D3_COLD);
> >> + return 0;
> >> +}
> >> +
> >> +static int vga_acpi_switcheroo_runtime_resume(struct device *dev)
> >> +{
> >> + struct pci_dev *pdev = to_pci_dev(dev);
> >> + struct acpi_device *adev;
> >> + int ret;
> >> +
> >> + ret = acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev);
> >> + if (!ret)
> >> + acpi_device_set_power(adev->parent, ACPI_STATE_D0);
> >> + ret = dev->bus->pm->runtime_resume(dev);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +int vga_switcheroo_init_parent_pr3_ops(struct device *dev,
> >> + struct dev_pm_domain *domain)
> >> +
> >> +{
> >> + /* copy over all the bus versions */
> >> + if (dev->bus && dev->bus->pm) {
> >> + domain->ops = *dev->bus->pm;
> >> + domain->ops.runtime_suspend = vga_acpi_switcheroo_runtime_suspend;
> >> + domain->ops.runtime_resume = vga_acpi_switcheroo_runtime_resume;
> >> +
> >> + dev_pm_domain_set(dev, domain);
> >> + return 0;
> >> + }
> >> + dev_pm_domain_set(dev, NULL);
> >> + return -EINVAL;
> >> +}
> >> +EXPORT_SYMBOL(vga_switcheroo_init_parent_pr3_ops);
> >> diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
> >> index 69e1d4a1..5ce0cbe 100644
> >> --- a/include/linux/vga_switcheroo.h
> >> +++ b/include/linux/vga_switcheroo.h
> >> @@ -144,6 +144,7 @@ void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, enum vga_switcheroo
> >> int vga_switcheroo_init_domain_pm_ops(struct device *dev, struct dev_pm_domain *domain);
> >> void vga_switcheroo_fini_domain_pm_ops(struct device *dev);
> >> int vga_switcheroo_init_domain_pm_optimus_hdmi_audio(struct device *dev, struct dev_pm_domain *domain);
> >> +int vga_switcheroo_init_parent_pr3_ops(struct device *dev, struct dev_pm_domain *domain);
> >> #else
> >>
> >> static inline void vga_switcheroo_unregister_client(struct pci_dev *dev) {}
> >> @@ -163,6 +164,6 @@ static inline void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, enum
> >> static inline int vga_switcheroo_init_domain_pm_ops(struct device *dev, struct dev_pm_domain *domain) { return -EINVAL; }
> >> static inline void vga_switcheroo_fini_domain_pm_ops(struct device *dev) {}
> >> static inline int vga_switcheroo_init_domain_pm_optimus_hdmi_audio(struct device *dev, struct dev_pm_domain *domain) { return -EINVAL; }
> >> -
> >> +static inline int vga_switcheroo_init_parent_pr3_ops(struct device *dev, struct dev_pm_domain *domain) { return -EINVAL; }
> >> #endif
> >> #endif /* _LINUX_VGA_SWITCHEROO_H_ */
> >> --
> >> 2.5.0
On Wed, Mar 9, 2016 at 3:17 PM, Lukas Wunner <[email protected]> wrote:
> Hi,
>
> On Wed, Mar 09, 2016 at 11:52:33AM -0500, Alex Deucher wrote:
>> On Wed, Mar 9, 2016 at 9:33 AM, Lukas Wunner <[email protected]> wrote:
>> > On Wed, Mar 09, 2016 at 04:14:04PM +1000, Dave Airlie wrote:
>> >> From: Dave Airlie <[email protected]>
>> >>
>> >> Windows 10 seems to have standardised power control for the
>> >> optimus/powerxpress laptops using PR3 power resource hooks.
>> >
>> > What happened to the Optimus DSM, does this still work? If not,
>> > echoing OFF to the vgaswitcheroo sysfs file won't power down the
>> > GPU now, right? (If runtime pm is disabled in nouveau.)
>> >
>> If the OS identifies itself as windows 10 or newer when initializing
>> ACPI, the standardized interfaces should be used if they exist. I'm
>> not sure if there is any explicit policy on the vendor specific ones,
>> but I suspect they are probably broken in that case since I doubt the
>> OEM validates them when the standardized interfaces are available.
>
> The vendor interface (Optimus DSM) must be present, otherwise the call
> to nouveau_is_optimus() in patch [2/2] would return false.
>
> But indeed it seems to not be working, otherwise why would the patches
> be necessary?
It may be present, but I doubt it's functional. I can't speak for
nvidia's implementation, but for AMD, ATPX is present, but when you
parse the supported functions there is a flag saying that you need to
use the PR3 interface instead. Maybe nvidia has something similar.
Then the driver could select whether to use the PR3 helpers or the
vendor specific interface.
Alex
>
> My point is that the chosen approach does not square with vga_switcheroo's
> architecture: Normally it's the handler's job to power the GPU up/down.
> If runtime pm is enabled then vga_switcheroo_init_domain_pm_ops()
> activates runtime_suspend/_resume callbacks which ask the handler to
> flip the power switch.
>
> However these two patches add *additional* runtime_suspend/_resume
> callbacks which do not rely on the handler at all. The handler is thus
> useless. This becomes apparent when loading the nouveau module with
> runpm=0: Normally the user is then able to manually power the GPU
> up/down by echoing ON or OFF to the vgaswitcheroo sysfs file. With the
> chosen approach this will likely not work because the handler only knows
> how to invoke the DSM, it doesn't know anything about PR3.
>
> Hence my suggestion to solve this with an additional handler which
> gets used in lieu of the nouveau DSM handler.
>
> Best regards,
>
> Lukas
>
>>
>> Alex
>>
>> >> I'm not sure this is definitely the correct place to be
>> >> doing this, but it works for me here.
>> >
>> > How about implementing an additional vga_switcheroo handler somewhere
>> > in drivers/acpi/, which looks for the PR3 hooks and registers with
>> > vga_switcheroo if found. The ->power_state callback of that handler
>> > would then invoke acpi_device_set_power().
>> >
>> > The ACPI bus is scanned in the subsys_initcall section, much earlier
>> > than nouveau is loaded (in the device_initcall section). So the ACPI
>> > vga_switcheroo handler could register before the nouveau DSM handler
>> > and therefore would always have a higher precedence.
>> >
>> > In any case this patch should be amended with kerneldoc for
>> > vga_switcheroo_init_parent_pr3_ops(), I've spent considerable time
>> > last year to document everything and kindly ask that this water not
>> > be muddied again. ;-)
>> >
>> > One small nit, the headers are sorted alphabetically yet acpi.h is
>> > added at the end.
>> >
>> > Thanks,
>> >
>> > Lukas
>> >
>> >>
>> >> The ACPI device for the GPU I have is \_SB_.PCI0.PEG_.VID_
>> >> but the power resource hooks are on \_SB_.PCI0.PEG_, so
>> >> this patch creates a new power domain to turn the GPU
>> >> device parent off using standard ACPI calls.
>> >>
>> >> Signed-off-by: Dave Airlie <[email protected]>
>> >> ---
>> >> drivers/gpu/vga/vga_switcheroo.c | 54 +++++++++++++++++++++++++++++++++++++++-
>> >> include/linux/vga_switcheroo.h | 3 ++-
>> >> 2 files changed, 55 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
>> >> index 665ab9f..be32cb2 100644
>> >> --- a/drivers/gpu/vga/vga_switcheroo.c
>> >> +++ b/drivers/gpu/vga/vga_switcheroo.c
>> >> @@ -42,7 +42,7 @@
>> >> #include <linux/uaccess.h>
>> >> #include <linux/vgaarb.h>
>> >> #include <linux/vga_switcheroo.h>
>> >> -
>> >> +#include <linux/acpi.h>
>> >> /**
>> >> * DOC: Overview
>> >> *
>> >> @@ -997,3 +997,55 @@ vga_switcheroo_init_domain_pm_optimus_hdmi_audio(struct device *dev,
>> >> return -EINVAL;
>> >> }
>> >> EXPORT_SYMBOL(vga_switcheroo_init_domain_pm_optimus_hdmi_audio);
>> >> +
>> >> +/* With Windows 10 the runtime suspend/resume can use power
>> >> + resources on the parent device */
>> >> +static int vga_acpi_switcheroo_runtime_suspend(struct device *dev)
>> >> +{
>> >> + struct pci_dev *pdev = to_pci_dev(dev);
>> >> + int ret;
>> >> + struct acpi_device *adev;
>> >> +
>> >> + ret = dev->bus->pm->runtime_suspend(dev);
>> >> + if (ret)
>> >> + return ret;
>> >> +
>> >> + ret = acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev);
>> >> + if (!ret)
>> >> + acpi_device_set_power(adev->parent, ACPI_STATE_D3_COLD);
>> >> + return 0;
>> >> +}
>> >> +
>> >> +static int vga_acpi_switcheroo_runtime_resume(struct device *dev)
>> >> +{
>> >> + struct pci_dev *pdev = to_pci_dev(dev);
>> >> + struct acpi_device *adev;
>> >> + int ret;
>> >> +
>> >> + ret = acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev);
>> >> + if (!ret)
>> >> + acpi_device_set_power(adev->parent, ACPI_STATE_D0);
>> >> + ret = dev->bus->pm->runtime_resume(dev);
>> >> + if (ret)
>> >> + return ret;
>> >> +
>> >> + return 0;
>> >> +}
>> >> +
>> >> +int vga_switcheroo_init_parent_pr3_ops(struct device *dev,
>> >> + struct dev_pm_domain *domain)
>> >> +
>> >> +{
>> >> + /* copy over all the bus versions */
>> >> + if (dev->bus && dev->bus->pm) {
>> >> + domain->ops = *dev->bus->pm;
>> >> + domain->ops.runtime_suspend = vga_acpi_switcheroo_runtime_suspend;
>> >> + domain->ops.runtime_resume = vga_acpi_switcheroo_runtime_resume;
>> >> +
>> >> + dev_pm_domain_set(dev, domain);
>> >> + return 0;
>> >> + }
>> >> + dev_pm_domain_set(dev, NULL);
>> >> + return -EINVAL;
>> >> +}
>> >> +EXPORT_SYMBOL(vga_switcheroo_init_parent_pr3_ops);
>> >> diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
>> >> index 69e1d4a1..5ce0cbe 100644
>> >> --- a/include/linux/vga_switcheroo.h
>> >> +++ b/include/linux/vga_switcheroo.h
>> >> @@ -144,6 +144,7 @@ void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, enum vga_switcheroo
>> >> int vga_switcheroo_init_domain_pm_ops(struct device *dev, struct dev_pm_domain *domain);
>> >> void vga_switcheroo_fini_domain_pm_ops(struct device *dev);
>> >> int vga_switcheroo_init_domain_pm_optimus_hdmi_audio(struct device *dev, struct dev_pm_domain *domain);
>> >> +int vga_switcheroo_init_parent_pr3_ops(struct device *dev, struct dev_pm_domain *domain);
>> >> #else
>> >>
>> >> static inline void vga_switcheroo_unregister_client(struct pci_dev *dev) {}
>> >> @@ -163,6 +164,6 @@ static inline void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, enum
>> >> static inline int vga_switcheroo_init_domain_pm_ops(struct device *dev, struct dev_pm_domain *domain) { return -EINVAL; }
>> >> static inline void vga_switcheroo_fini_domain_pm_ops(struct device *dev) {}
>> >> static inline int vga_switcheroo_init_domain_pm_optimus_hdmi_audio(struct device *dev, struct dev_pm_domain *domain) { return -EINVAL; }
>> >> -
>> >> +static inline int vga_switcheroo_init_parent_pr3_ops(struct device *dev, struct dev_pm_domain *domain) { return -EINVAL; }
>> >> #endif
>> >> #endif /* _LINUX_VGA_SWITCHEROO_H_ */
>> >> --
>> >> 2.5.0
On 9 March 2016 at 23:19, Rafael J. Wysocki <[email protected]> wrote:
> On Wed, Mar 9, 2016 at 7:14 AM, Dave Airlie <[email protected]> wrote:
>> From: Dave Airlie <[email protected]>
>>
>> Windows 10 seems to have standardised power control for the
>> optimus/powerxpress laptops using PR3 power resource hooks.
>>
>> I'm not sure this is definitely the correct place to be
>> doing this, but it works for me here.
>>
>> The ACPI device for the GPU I have is \_SB_.PCI0.PEG_.VID_
>> but the power resource hooks are on \_SB_.PCI0.PEG_, so
>> this patch creates a new power domain to turn the GPU
>> device parent off using standard ACPI calls.
>>
>> Signed-off-by: Dave Airlie <[email protected]>
>> ---
>> drivers/gpu/vga/vga_switcheroo.c | 54 +++++++++++++++++++++++++++++++++++++++-
>> include/linux/vga_switcheroo.h | 3 ++-
>> 2 files changed, 55 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
>> index 665ab9f..be32cb2 100644
>> --- a/drivers/gpu/vga/vga_switcheroo.c
>> +++ b/drivers/gpu/vga/vga_switcheroo.c
>> @@ -42,7 +42,7 @@
>> #include <linux/uaccess.h>
>> #include <linux/vgaarb.h>
>> #include <linux/vga_switcheroo.h>
>> -
>> +#include <linux/acpi.h>
>> /**
>> * DOC: Overview
>> *
>> @@ -997,3 +997,55 @@ vga_switcheroo_init_domain_pm_optimus_hdmi_audio(struct device *dev,
>> return -EINVAL;
>> }
>> EXPORT_SYMBOL(vga_switcheroo_init_domain_pm_optimus_hdmi_audio);
>> +
>> +/* With Windows 10 the runtime suspend/resume can use power
>> + resources on the parent device */
>> +static int vga_acpi_switcheroo_runtime_suspend(struct device *dev)
>> +{
>> + struct pci_dev *pdev = to_pci_dev(dev);
>> + int ret;
>> + struct acpi_device *adev;
>> +
>> + ret = dev->bus->pm->runtime_suspend(dev);
>> + if (ret)
>> + return ret;
>> +
>> + ret = acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev);
>
> You can use ACPI_COMPANION(&pdev->dev) for that.
>
>> + if (!ret)
>> + acpi_device_set_power(adev->parent, ACPI_STATE_D3_COLD);
>
> Won't that mess up with the PM of the parent? Or do we know that the
> parent won't do its own PM?
The parent is always going to be pcieport. It doesn't seem to do any runtime PM,
I do wonder if pcieport should be doing it's own runtime PM handling,
but that is a
larger task than I'm thinking to tackle here. Maybe I should be doing
pci_set_power_state(pdev->bus->self, PCI_D3cold) ? I'm not really sure.
I'm guessing on Windows this all happens automatically.
Dave.
On 10 March 2016 at 00:33, Lukas Wunner <[email protected]> wrote:
> Hi Dave,
>
> On Wed, Mar 09, 2016 at 04:14:04PM +1000, Dave Airlie wrote:
>> From: Dave Airlie <[email protected]>
>>
>> Windows 10 seems to have standardised power control for the
>> optimus/powerxpress laptops using PR3 power resource hooks.
>
> What happened to the Optimus DSM, does this still work? If not,
> echoing OFF to the vgaswitcheroo sysfs file won't power down the
> GPU now, right? (If runtime pm is disabled in nouveau.)
No it doesn't work, hence why I had to go dig through this method. When
you report Windows 2013 ACPI support to the BIOS it disables the PS3 method,
and relies on hitting the PR3 method. Unlike ATPX I couldn't find any
indicator bit
in the optimus DSM to say which method to use. So we get to try both methods.
I've confirmed on my older machine that if there are no PR3 methods, then it
has no effect.
> How about implementing an additional vga_switcheroo handler somewhere
> in drivers/acpi/, which looks for the PR3 hooks and registers with
> vga_switcheroo if found. The ->power_state callback of that handler
> would then invoke acpi_device_set_power().
I've nothing to trigger off when to register in that case. I'm not
sure if the PR3
hooks alone are enough to tell you whether you should be using this method.
The ATPX guys have a bit we need to read to know if we should be using this
method.
>
> In any case this patch should be amended with kerneldoc for
> vga_switcheroo_init_parent_pr3_ops(), I've spent considerable time
> last year to document everything and kindly ask that this water not
> be muddied again. ;-)
>
> One small nit, the headers are sorted alphabetically yet acpi.h is
> added at the end.
I'll add those in v2.
Dave.
On 10 March 2016 at 06:17, Lukas Wunner <[email protected]> wrote:
> Hi,
>
> On Wed, Mar 09, 2016 at 11:52:33AM -0500, Alex Deucher wrote:
>> On Wed, Mar 9, 2016 at 9:33 AM, Lukas Wunner <[email protected]> wrote:
>> > On Wed, Mar 09, 2016 at 04:14:04PM +1000, Dave Airlie wrote:
>> >> From: Dave Airlie <[email protected]>
>> >>
>> >> Windows 10 seems to have standardised power control for the
>> >> optimus/powerxpress laptops using PR3 power resource hooks.
>> >
>> > What happened to the Optimus DSM, does this still work? If not,
>> > echoing OFF to the vgaswitcheroo sysfs file won't power down the
>> > GPU now, right? (If runtime pm is disabled in nouveau.)
>> >
>> If the OS identifies itself as windows 10 or newer when initializing
>> ACPI, the standardized interfaces should be used if they exist. I'm
>> not sure if there is any explicit policy on the vendor specific ones,
>> but I suspect they are probably broken in that case since I doubt the
>> OEM validates them when the standardized interfaces are available.
>
> The vendor interface (Optimus DSM) must be present, otherwise the call
> to nouveau_is_optimus() in patch [2/2] would return false.
>
> But indeed it seems to not be working, otherwise why would the patches
> be necessary?
>
> My point is that the chosen approach does not square with vga_switcheroo's
> architecture: Normally it's the handler's job to power the GPU up/down.
> If runtime pm is enabled then vga_switcheroo_init_domain_pm_ops()
> activates runtime_suspend/_resume callbacks which ask the handler to
> flip the power switch.
>
> However these two patches add *additional* runtime_suspend/_resume
> callbacks which do not rely on the handler at all. The handler is thus
> useless. This becomes apparent when loading the nouveau module with
> runpm=0: Normally the user is then able to manually power the GPU
> up/down by echoing ON or OFF to the vgaswitcheroo sysfs file. With the
> chosen approach this will likely not work because the handler only knows
> how to invoke the DSM, it doesn't know anything about PR3.
>
> Hence my suggestion to solve this with an additional handler which
> gets used in lieu of the nouveau DSM handler.
I'll think about this a bit more, but really I don't care for vgaswitcheroo
manual power control anymore. It's in debugfs for a reason, it was a stopgap
until we got dynamic power control.
If dynamic power control isn't working for some people we should fix that,
but supporting nouveau.runpm=0 and manual power control is so far down
the list of things I care about.
Dave.
On 10 March 2016 at 00:40, Lukas Wunner <[email protected]> wrote:
> Hi Dave,
>
> On Wed, Mar 09, 2016 at 04:14:05PM +1000, Dave Airlie wrote:
>> From: Dave Airlie <[email protected]>
>>
>> This fixes GPU auto powerdown on the Lenovo W541,
>> since we advertise Windows 2013 to the ACPI layer.
>>
>> Signed-off-by: Dave Airlie <[email protected]>
>> ---
>> drivers/gpu/drm/nouveau/nouveau_vga.c | 10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c b/drivers/gpu/drm/nouveau/nouveau_vga.c
>> index af89c36..b987427f 100644
>> --- a/drivers/gpu/drm/nouveau/nouveau_vga.c
>> +++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
>> @@ -101,8 +101,12 @@ nouveau_vga_init(struct nouveau_drm *drm)
>> runtime = true;
>> vga_switcheroo_register_client(dev->pdev, &nouveau_switcheroo_ops, runtime);
>>
>> - if (runtime && nouveau_is_v1_dsm() && !nouveau_is_optimus())
>> - vga_switcheroo_init_domain_pm_ops(drm->dev->dev, &drm->vga_pm_domain);
>> + if (runtime) {
>> + if (nouveau_is_v1_dsm() && !nouveau_is_optimus())
>
> The " && !nouveau_is_optimus()" can be dropped because a machine cannot
> have both. Note the "else" in nouveau_dsm_detect():
I'm pretty sure I've seen a machine with both in my past, back in the
Vista/Win7 crossover days.
> You're calling this unconditionally for all Optimus machines yet
> I assume pre Windows 10 machines lack the PR3 hooks.
>
Yes and I've confirmed on my older machine that nothing bad happens
doing it unconditionally,
and I couldn't find any bits in the _DSM flags to tell me if I should
do something different.
Dave.
On Thursday, March 10, 2016 07:56:41 AM Dave Airlie wrote:
> On 9 March 2016 at 23:19, Rafael J. Wysocki <[email protected]> wrote:
> > On Wed, Mar 9, 2016 at 7:14 AM, Dave Airlie <[email protected]> wrote:
> >> From: Dave Airlie <[email protected]>
> >>
> >> Windows 10 seems to have standardised power control for the
> >> optimus/powerxpress laptops using PR3 power resource hooks.
> >>
> >> I'm not sure this is definitely the correct place to be
> >> doing this, but it works for me here.
> >>
> >> The ACPI device for the GPU I have is \_SB_.PCI0.PEG_.VID_
> >> but the power resource hooks are on \_SB_.PCI0.PEG_, so
> >> this patch creates a new power domain to turn the GPU
> >> device parent off using standard ACPI calls.
> >>
> >> Signed-off-by: Dave Airlie <[email protected]>
> >> ---
> >> drivers/gpu/vga/vga_switcheroo.c | 54 +++++++++++++++++++++++++++++++++++++++-
> >> include/linux/vga_switcheroo.h | 3 ++-
> >> 2 files changed, 55 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> >> index 665ab9f..be32cb2 100644
> >> --- a/drivers/gpu/vga/vga_switcheroo.c
> >> +++ b/drivers/gpu/vga/vga_switcheroo.c
> >> @@ -42,7 +42,7 @@
> >> #include <linux/uaccess.h>
> >> #include <linux/vgaarb.h>
> >> #include <linux/vga_switcheroo.h>
> >> -
> >> +#include <linux/acpi.h>
> >> /**
> >> * DOC: Overview
> >> *
> >> @@ -997,3 +997,55 @@ vga_switcheroo_init_domain_pm_optimus_hdmi_audio(struct device *dev,
> >> return -EINVAL;
> >> }
> >> EXPORT_SYMBOL(vga_switcheroo_init_domain_pm_optimus_hdmi_audio);
> >> +
> >> +/* With Windows 10 the runtime suspend/resume can use power
> >> + resources on the parent device */
> >> +static int vga_acpi_switcheroo_runtime_suspend(struct device *dev)
> >> +{
> >> + struct pci_dev *pdev = to_pci_dev(dev);
> >> + int ret;
> >> + struct acpi_device *adev;
> >> +
> >> + ret = dev->bus->pm->runtime_suspend(dev);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + ret = acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev);
> >
> > You can use ACPI_COMPANION(&pdev->dev) for that.
> >
> >> + if (!ret)
> >> + acpi_device_set_power(adev->parent, ACPI_STATE_D3_COLD);
> >
> > Won't that mess up with the PM of the parent? Or do we know that the
> > parent won't do its own PM?
>
> The parent is always going to be pcieport.
I see.
> It doesn't seem to do any runtime PM,
> I do wonder if pcieport should be doing it's own runtime PM handling,
> but that is a
> larger task than I'm thinking to tackle here.
PCIe ports don't do PM - yet. Mika has posted a series of patches to implement
that, however, that are waiting for comments now:
https://patchwork.kernel.org/patch/8453311/
https://patchwork.kernel.org/patch/8453381/
https://patchwork.kernel.org/patch/8453391/
https://patchwork.kernel.org/patch/8453411/
https://patchwork.kernel.org/patch/8453371/
https://patchwork.kernel.org/patch/8453351/
> Maybe I should be doing
>
> pci_set_power_state(pdev->bus->self, PCI_D3cold) ? I'm not really sure.
Using pci_set_power_state() would be more appropriate IMO, but you can get
to the bridge via dev->parent too, can't you?
In any case, it looks like you and Mika need to talk. :-)
> I'm guessing on Windows this all happens automatically.
PCIe ports are power-managend by (newer) Windows AFAICS, but we know for a fact
that this simply doesn't work reliably on some older hardware which is why
we don't do that. I suppose that the Windows in question uses a cut-off date
or similar to decide what do do with PCIe ports PM.
Thanks,
Rafael
On Thu, Mar 10, 2016 at 09:57:09PM +0100, Rafael J. Wysocki wrote:
> > It doesn't seem to do any runtime PM,
> > I do wonder if pcieport should be doing it's own runtime PM handling,
> > but that is a
> > larger task than I'm thinking to tackle here.
>
> PCIe ports don't do PM - yet. Mika has posted a series of patches to implement
> that, however, that are waiting for comments now:
>
> https://patchwork.kernel.org/patch/8453311/
> https://patchwork.kernel.org/patch/8453381/
> https://patchwork.kernel.org/patch/8453391/
> https://patchwork.kernel.org/patch/8453411/
> https://patchwork.kernel.org/patch/8453371/
> https://patchwork.kernel.org/patch/8453351/
>
> > Maybe I should be doing
> >
> > pci_set_power_state(pdev->bus->self, PCI_D3cold) ? I'm not really sure.
>
> Using pci_set_power_state() would be more appropriate IMO, but you can get
> to the bridge via dev->parent too, can't you?
>
> In any case, it looks like you and Mika need to talk. :-)
When the vga_switcheroo device gets runtime suspended (with the above
runtime PM patchs for PCIe root ports) the root port should also be
runtime suspended by the PM core. I don't think there is a need to call
any pci_set_power_state() in this driver but maybe I'm missing
something.
On Friday, March 11, 2016 12:58:15 PM Mika Westerberg wrote:
> On Thu, Mar 10, 2016 at 09:57:09PM +0100, Rafael J. Wysocki wrote:
> > > It doesn't seem to do any runtime PM,
> > > I do wonder if pcieport should be doing it's own runtime PM handling,
> > > but that is a
> > > larger task than I'm thinking to tackle here.
> >
> > PCIe ports don't do PM - yet. Mika has posted a series of patches to implement
> > that, however, that are waiting for comments now:
> >
> > https://patchwork.kernel.org/patch/8453311/
> > https://patchwork.kernel.org/patch/8453381/
> > https://patchwork.kernel.org/patch/8453391/
> > https://patchwork.kernel.org/patch/8453411/
> > https://patchwork.kernel.org/patch/8453371/
> > https://patchwork.kernel.org/patch/8453351/
> >
> > > Maybe I should be doing
> > >
> > > pci_set_power_state(pdev->bus->self, PCI_D3cold) ? I'm not really sure.
> >
> > Using pci_set_power_state() would be more appropriate IMO, but you can get
> > to the bridge via dev->parent too, can't you?
> >
> > In any case, it looks like you and Mika need to talk. :-)
>
> When the vga_switcheroo device gets runtime suspended (with the above
> runtime PM patchs for PCIe root ports) the root port should also be
> runtime suspended by the PM core.
Right, after your patches have been applied, the additional handling
won't be needed.
So Dave, maybe you can check if the Mika's patches help?
Thanks,
Rafael
On 11 March 2016 at 23:45, Rafael J. Wysocki <[email protected]> wrote:
> On Friday, March 11, 2016 12:58:15 PM Mika Westerberg wrote:
>> On Thu, Mar 10, 2016 at 09:57:09PM +0100, Rafael J. Wysocki wrote:
>> > > It doesn't seem to do any runtime PM,
>> > > I do wonder if pcieport should be doing it's own runtime PM handling,
>> > > but that is a
>> > > larger task than I'm thinking to tackle here.
>> >
>> > PCIe ports don't do PM - yet. Mika has posted a series of patches to implement
>> > that, however, that are waiting for comments now:
>> >
>> > https://patchwork.kernel.org/patch/8453311/
>> > https://patchwork.kernel.org/patch/8453381/
>> > https://patchwork.kernel.org/patch/8453391/
>> > https://patchwork.kernel.org/patch/8453411/
>> > https://patchwork.kernel.org/patch/8453371/
>> > https://patchwork.kernel.org/patch/8453351/
>> >
>> > > Maybe I should be doing
>> > >
>> > > pci_set_power_state(pdev->bus->self, PCI_D3cold) ? I'm not really sure.
>> >
>> > Using pci_set_power_state() would be more appropriate IMO, but you can get
>> > to the bridge via dev->parent too, can't you?
>> >
>> > In any case, it looks like you and Mika need to talk. :-)
>>
>> When the vga_switcheroo device gets runtime suspended (with the above
>> runtime PM patchs for PCIe root ports) the root port should also be
>> runtime suspended by the PM core.
>
> Right, after your patches have been applied, the additional handling
> won't be needed.
>
> So Dave, maybe you can check if the Mika's patches help?
Hi Mika,
I tested your patches with a couple of changes on the Lenovo W541.
The attached patch contains the two things I needed to get the same
functionality
as my patches.
I'm really not in love with the per-chipset enablement for this,
really any chipsets
after a certain year should probably be better, as we'll constantly be
adding PCI Ids
for every chipset ever made, and I expect we'll forget some.
Dave.
On Mon, Mar 14, 2016 at 12:19:14PM +1000, Dave Airlie wrote:
> On 11 March 2016 at 23:45, Rafael J. Wysocki <[email protected]> wrote:
> > On Friday, March 11, 2016 12:58:15 PM Mika Westerberg wrote:
> >> On Thu, Mar 10, 2016 at 09:57:09PM +0100, Rafael J. Wysocki wrote:
> >> > > It doesn't seem to do any runtime PM,
> >> > > I do wonder if pcieport should be doing it's own runtime PM handling,
> >> > > but that is a
> >> > > larger task than I'm thinking to tackle here.
> >> >
> >> > PCIe ports don't do PM - yet. Mika has posted a series of patches to implement
> >> > that, however, that are waiting for comments now:
> >> >
> >> > https://patchwork.kernel.org/patch/8453311/
> >> > https://patchwork.kernel.org/patch/8453381/
> >> > https://patchwork.kernel.org/patch/8453391/
> >> > https://patchwork.kernel.org/patch/8453411/
> >> > https://patchwork.kernel.org/patch/8453371/
> >> > https://patchwork.kernel.org/patch/8453351/
> >> >
> >> > > Maybe I should be doing
> >> > >
> >> > > pci_set_power_state(pdev->bus->self, PCI_D3cold) ? I'm not really sure.
> >> >
> >> > Using pci_set_power_state() would be more appropriate IMO, but you can get
> >> > to the bridge via dev->parent too, can't you?
> >> >
> >> > In any case, it looks like you and Mika need to talk. :-)
> >>
> >> When the vga_switcheroo device gets runtime suspended (with the above
> >> runtime PM patchs for PCIe root ports) the root port should also be
> >> runtime suspended by the PM core.
> >
> > Right, after your patches have been applied, the additional handling
> > won't be needed.
> >
> > So Dave, maybe you can check if the Mika's patches help?
>
> Hi Mika,
>
> I tested your patches with a couple of changes on the Lenovo W541.
Thanks for testing.
> The attached patch contains the two things I needed to get the same
> functionality
> as my patches.
>
> I'm really not in love with the per-chipset enablement for this,
> really any chipsets
> after a certain year should probably be better, as we'll constantly be
> adding PCI Ids
> for every chipset ever made, and I expect we'll forget some.
Bjorn also had the same comment. I think I'll change it to use cut-off
date instead of list of PCI IDs of nobody objects.
> Dave.
> From eea412076c9d24262ebd4811f766d5379b728045 Mon Sep 17 00:00:00 2001
> From: Dave Airlie <[email protected]>
> Date: Mon, 14 Mar 2016 23:37:43 +1000
> Subject: [PATCH] [RFC] PCI: enable runtime PM on pcieports to let secondary
> GPUs powerdown.
>
> With secondary GPUs in Windows 10 laptops we need to use runtime PM
> to power down the PCIe ports after the devices goes to D3Cold.
>
> This patch adds the PCI ID for the Haswell 16x PCIE slot found
> in the W541 laptops. I should probably try and gather the PCI IDs,
> for broadwell/skylake variants as well if we can't find them.
>
> This fixes a regression on W541 laptops on top of Mika's PCIE patches
> since we started advertising Windows 2013 ACPI.
>
> [probably should be two patches - hence RFC]
>
> Signed-off-by: Dave Airlie <[email protected]>
> ---
> drivers/pci/pcie/portdrv_pci.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> index 43dd23e..1405de8 100644
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -278,8 +278,10 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
>
> pci_save_state(dev);
>
> - if (pcie_port_runtime_suspend_allowed(dev))
> + if (pcie_port_runtime_suspend_allowed(dev)) {
> + pm_runtime_allow(&dev->dev);
PCI drivers typically have left this decision up to the userspace. I'm
wondering whether it is good idea to deviate from that here? Of course
this allows immediate power savings but could potentially cause problems
as well.
I think we need to add corresponding call to pm_runtime_forbid() in
pcie_portdrv_remove().
> pm_runtime_put_noidle(&dev->dev);
> + }
>
> return 0;
> }
> @@ -436,6 +438,8 @@ static void pcie_portdrv_err_resume(struct pci_dev *dev)
> * LINUX Device Driver Model
> */
> static const struct pci_device_id port_pci_ids[] = {
> + /* Intel Skylake PCIE graphics port */
> + { PCI_VDEVICE(INTEL, 0x0c01), .driver_data = PCIE_PORT_SPT },
> /* Intel Broxton */
> { PCI_VDEVICE(INTEL, 0x1ad6), .driver_data = PCIE_PORT_SPT },
> { PCI_VDEVICE(INTEL, 0x1ad7), .driver_data = PCIE_PORT_SPT },
> --
> 2.5.0
>
>
>> - if (pcie_port_runtime_suspend_allowed(dev))
>> + if (pcie_port_runtime_suspend_allowed(dev)) {
>> + pm_runtime_allow(&dev->dev);
>
> PCI drivers typically have left this decision up to the userspace. I'm
> wondering whether it is good idea to deviate from that here? Of course
> this allows immediate power savings but could potentially cause problems
> as well.
>
No distro has ever shipped userspace to do this, I really think this
is a bad design.
We have wasted countless watts of power on this stupid idea that people will
run powertop, only a few people in the world run powertop, lots of
people use Linux.
The kernel should power stuff down not wait for the user to run powertop,
At least for the GPU it's in the area of 8W of power, and I've got the
GPU drivers doing this themselves,
I could have the GPU driver call runtime allow for it's host bridge I suppose,
if we insist on the userspace cares, but I'd prefer not doing so.
> I think we need to add corresponding call to pm_runtime_forbid() in
> pcie_portdrv_remove().
Yes most likely.
Dave.
>
>> pm_runtime_put_noidle(&dev->dev);
>> + }
>>
>> return 0;
>> }
>> @@ -436,6 +438,8 @@ static void pcie_portdrv_err_resume(struct pci_dev *dev)
>> * LINUX Device Driver Model
>> */
>> static const struct pci_device_id port_pci_ids[] = {
>> + /* Intel Skylake PCIE graphics port */
>> + { PCI_VDEVICE(INTEL, 0x0c01), .driver_data = PCIE_PORT_SPT },
>> /* Intel Broxton */
>> { PCI_VDEVICE(INTEL, 0x1ad6), .driver_data = PCIE_PORT_SPT },
>> { PCI_VDEVICE(INTEL, 0x1ad7), .driver_data = PCIE_PORT_SPT },
>> --
>> 2.5.0
>>
>
On Mon, Mar 14, 2016 at 07:47:39PM +1000, Dave Airlie wrote:
> >
> >> - if (pcie_port_runtime_suspend_allowed(dev))
> >> + if (pcie_port_runtime_suspend_allowed(dev)) {
> >> + pm_runtime_allow(&dev->dev);
> >
> > PCI drivers typically have left this decision up to the userspace. I'm
> > wondering whether it is good idea to deviate from that here? Of course
> > this allows immediate power savings but could potentially cause problems
> > as well.
> >
>
> No distro has ever shipped userspace to do this, I really think this
> is a bad design.
> We have wasted countless watts of power on this stupid idea that people will
> run powertop, only a few people in the world run powertop, lots of
> people use Linux.
>
> The kernel should power stuff down not wait for the user to run powertop,
> At least for the GPU it's in the area of 8W of power, and I've got the
> GPU drivers doing this themselves,
>
> I could have the GPU driver call runtime allow for it's host bridge I suppose,
> if we insist on the userspace cares, but I'd prefer not doing so.
Yes, fully agreed. Runtime PM that's not enabled by default is useless,
since no one runs powertop, which also means it's buggy because no one
tests it. Not enabling power saving features by default is imo just a lame
excuse. Shit better just work, and tuneables should only exposed if
there's a real reason why autotuning is infeasible. E.g. we auto-ramp the
gpu clock down/up in i915 in software to compensate for some of the
silliness in how the hw does it alone. And the sysfs tunables are mostly
just to make benchmarking (where slower, but without any thermal
throttling) is sometimes preferred.
And yes i915 hasn't enabled rpm yet by default because there's bugs left
:(
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Mon, Mar 14, 2016 at 07:47:39PM +1000, Dave Airlie wrote:
> >
> >> - if (pcie_port_runtime_suspend_allowed(dev))
> >> + if (pcie_port_runtime_suspend_allowed(dev)) {
> >> + pm_runtime_allow(&dev->dev);
> >
> > PCI drivers typically have left this decision up to the userspace. I'm
> > wondering whether it is good idea to deviate from that here? Of course
> > this allows immediate power savings but could potentially cause problems
> > as well.
> >
>
> No distro has ever shipped userspace to do this, I really think this
> is a bad design.
> We have wasted countless watts of power on this stupid idea that people will
> run powertop, only a few people in the world run powertop, lots of
> people use Linux.
That is a fair point.
I do not have anything against calling pm_runtime_allow() here. In fact
we already do the same in Intel LPSS drivers. I just wanted to bring
that up.
Rafael, what do you think?
If we anyway are going to add cut-off date to enable runtime PM we
should expect that the hardware is also capable of doing so (and if not
we can always blacklist the exceptions).
> The kernel should power stuff down not wait for the user to run powertop,
> At least for the GPU it's in the area of 8W of power, and I've got the
> GPU drivers doing this themselves,
>
> I could have the GPU driver call runtime allow for it's host bridge I suppose,
> if we insist on the userspace cares, but I'd prefer not doing so.
>
> > I think we need to add corresponding call to pm_runtime_forbid() in
> > pcie_portdrv_remove().
>
> Yes most likely.
BTW, I can add both calls to the next version of PCIe runtime PM patches
if you are OK with that, and all agree this is a good idea.
On Mon, Mar 14, 2016 at 11:23 AM, Mika Westerberg
<[email protected]> wrote:
> On Mon, Mar 14, 2016 at 07:47:39PM +1000, Dave Airlie wrote:
>> >
>> >> - if (pcie_port_runtime_suspend_allowed(dev))
>> >> + if (pcie_port_runtime_suspend_allowed(dev)) {
>> >> + pm_runtime_allow(&dev->dev);
>> >
>> > PCI drivers typically have left this decision up to the userspace. I'm
>> > wondering whether it is good idea to deviate from that here? Of course
>> > this allows immediate power savings but could potentially cause problems
>> > as well.
>> >
>>
>> No distro has ever shipped userspace to do this, I really think this
>> is a bad design.
>> We have wasted countless watts of power on this stupid idea that people will
>> run powertop, only a few people in the world run powertop, lots of
>> people use Linux.
>
> That is a fair point.
>
> I do not have anything against calling pm_runtime_allow() here. In fact
> we already do the same in Intel LPSS drivers. I just wanted to bring
> that up.
>
> Rafael, what do you think?
We can do that to start with. If there are no problems in the field
with it, I don't see any problems in principle.
> If we anyway are going to add cut-off date to enable runtime PM we
> should expect that the hardware is also capable of doing so (and if not
> we can always blacklist the exceptions).
Sounds reasonable.
>> The kernel should power stuff down not wait for the user to run powertop,
>> At least for the GPU it's in the area of 8W of power, and I've got the
>> GPU drivers doing this themselves,
>>
>> I could have the GPU driver call runtime allow for it's host bridge I suppose,
>> if we insist on the userspace cares, but I'd prefer not doing so.
>>
>> > I think we need to add corresponding call to pm_runtime_forbid() in
>> > pcie_portdrv_remove().
>>
>> Yes most likely.
>
> BTW, I can add both calls to the next version of PCIe runtime PM patches
> if you are OK with that, and all agree this is a good idea.
That would be fine by me.
Thanks,
Rafael
On Sun, Mar 13, 2016 at 10:19 PM, Dave Airlie <[email protected]> wrote:
> On 11 March 2016 at 23:45, Rafael J. Wysocki <[email protected]> wrote:
>> On Friday, March 11, 2016 12:58:15 PM Mika Westerberg wrote:
>>> On Thu, Mar 10, 2016 at 09:57:09PM +0100, Rafael J. Wysocki wrote:
>>> > > It doesn't seem to do any runtime PM,
>>> > > I do wonder if pcieport should be doing it's own runtime PM handling,
>>> > > but that is a
>>> > > larger task than I'm thinking to tackle here.
>>> >
>>> > PCIe ports don't do PM - yet. Mika has posted a series of patches to implement
>>> > that, however, that are waiting for comments now:
>>> >
>>> > https://patchwork.kernel.org/patch/8453311/
>>> > https://patchwork.kernel.org/patch/8453381/
>>> > https://patchwork.kernel.org/patch/8453391/
>>> > https://patchwork.kernel.org/patch/8453411/
>>> > https://patchwork.kernel.org/patch/8453371/
>>> > https://patchwork.kernel.org/patch/8453351/
>>> >
>>> > > Maybe I should be doing
>>> > >
>>> > > pci_set_power_state(pdev->bus->self, PCI_D3cold) ? I'm not really sure.
>>> >
>>> > Using pci_set_power_state() would be more appropriate IMO, but you can get
>>> > to the bridge via dev->parent too, can't you?
>>> >
>>> > In any case, it looks like you and Mika need to talk. :-)
>>>
>>> When the vga_switcheroo device gets runtime suspended (with the above
>>> runtime PM patchs for PCIe root ports) the root port should also be
>>> runtime suspended by the PM core.
>>
>> Right, after your patches have been applied, the additional handling
>> won't be needed.
>>
>> So Dave, maybe you can check if the Mika's patches help?
>
> Hi Mika,
>
> I tested your patches with a couple of changes on the Lenovo W541.
>
> The attached patch contains the two things I needed to get the same
> functionality
> as my patches.
>
> I'm really not in love with the per-chipset enablement for this,
> really any chipsets
> after a certain year should probably be better, as we'll constantly be
> adding PCI Ids
> for every chipset ever made, and I expect we'll forget some.
Yeah, I don't care of that either. There are always going to be
chipsets falling through the cracks.
Alex
>
> Dave.
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
On Mon, Mar 14, 2016 at 01:50:41PM +0100, Rafael J. Wysocki wrote:
> On Mon, Mar 14, 2016 at 11:23 AM, Mika Westerberg
> <[email protected]> wrote:
> > On Mon, Mar 14, 2016 at 07:47:39PM +1000, Dave Airlie wrote:
> >> >
> >> >> - if (pcie_port_runtime_suspend_allowed(dev))
> >> >> + if (pcie_port_runtime_suspend_allowed(dev)) {
> >> >> + pm_runtime_allow(&dev->dev);
> >> >
> >> > PCI drivers typically have left this decision up to the userspace. I'm
> >> > wondering whether it is good idea to deviate from that here? Of course
> >> > this allows immediate power savings but could potentially cause problems
> >> > as well.
> >> >
> >>
> >> No distro has ever shipped userspace to do this, I really think this
> >> is a bad design.
> >> We have wasted countless watts of power on this stupid idea that people will
> >> run powertop, only a few people in the world run powertop, lots of
> >> people use Linux.
> >
> > That is a fair point.
> >
> > I do not have anything against calling pm_runtime_allow() here. In fact
> > we already do the same in Intel LPSS drivers. I just wanted to bring
> > that up.
> >
> > Rafael, what do you think?
>
> We can do that to start with. If there are no problems in the field
> with it, I don't see any problems in principle.
>
> > If we anyway are going to add cut-off date to enable runtime PM we
> > should expect that the hardware is also capable of doing so (and if not
> > we can always blacklist the exceptions).
>
> Sounds reasonable.
>
> >> The kernel should power stuff down not wait for the user to run powertop,
> >> At least for the GPU it's in the area of 8W of power, and I've got the
> >> GPU drivers doing this themselves,
> >>
> >> I could have the GPU driver call runtime allow for it's host bridge I suppose,
> >> if we insist on the userspace cares, but I'd prefer not doing so.
> >>
> >> > I think we need to add corresponding call to pm_runtime_forbid() in
> >> > pcie_portdrv_remove().
> >>
> >> Yes most likely.
> >
> > BTW, I can add both calls to the next version of PCIe runtime PM patches
> > if you are OK with that, and all agree this is a good idea.
>
> That would be fine by me.
OK thanks.
I'll do these changes to the next version of the patch series then.
Hi Mika,
On Mon, Mar 14, 2016 at 11:43:35AM +0200, Mika Westerberg wrote:
> On Mon, Mar 14, 2016 at 12:19:14PM +1000, Dave Airlie wrote:
> > On 11 March 2016 at 23:45, Rafael J. Wysocki <[email protected]> wrote:
> > > On Friday, March 11, 2016 12:58:15 PM Mika Westerberg wrote:
> > >> On Thu, Mar 10, 2016 at 09:57:09PM +0100, Rafael J. Wysocki wrote:
> > >> > > It doesn't seem to do any runtime PM,
> > >> > > I do wonder if pcieport should be doing it's own runtime PM handling,
> > >> > > but that is a
> > >> > > larger task than I'm thinking to tackle here.
> > >> >
> > >> > PCIe ports don't do PM - yet. Mika has posted a series of patches to implement
> > >> > that, however, that are waiting for comments now:
> > >> >
> > >> > https://patchwork.kernel.org/patch/8453311/
> > >> > https://patchwork.kernel.org/patch/8453381/
> > >> > https://patchwork.kernel.org/patch/8453391/
> > >> > https://patchwork.kernel.org/patch/8453411/
> > >> > https://patchwork.kernel.org/patch/8453371/
> > >> > https://patchwork.kernel.org/patch/8453351/
If a pciehp port is runtime suspended and pciehp_poll_mode is enabled,
the poll timer needs to be disabled and later reenabled on runtime resume.
Don't we need to add runtime pm callbacks to struct pcie_port_service_driver
for that, and call them from pcie_port_runtime_*()?
Best regards,
Lukas
On Tue, Mar 15, 2016 at 02:39:58PM +0100, Lukas Wunner wrote:
> Hi Mika,
>
> On Mon, Mar 14, 2016 at 11:43:35AM +0200, Mika Westerberg wrote:
> > On Mon, Mar 14, 2016 at 12:19:14PM +1000, Dave Airlie wrote:
> > > On 11 March 2016 at 23:45, Rafael J. Wysocki <[email protected]> wrote:
> > > > On Friday, March 11, 2016 12:58:15 PM Mika Westerberg wrote:
> > > >> On Thu, Mar 10, 2016 at 09:57:09PM +0100, Rafael J. Wysocki wrote:
> > > >> > > It doesn't seem to do any runtime PM,
> > > >> > > I do wonder if pcieport should be doing it's own runtime PM handling,
> > > >> > > but that is a
> > > >> > > larger task than I'm thinking to tackle here.
> > > >> >
> > > >> > PCIe ports don't do PM - yet. Mika has posted a series of patches to implement
> > > >> > that, however, that are waiting for comments now:
> > > >> >
> > > >> > https://patchwork.kernel.org/patch/8453311/
> > > >> > https://patchwork.kernel.org/patch/8453381/
> > > >> > https://patchwork.kernel.org/patch/8453391/
> > > >> > https://patchwork.kernel.org/patch/8453411/
> > > >> > https://patchwork.kernel.org/patch/8453371/
> > > >> > https://patchwork.kernel.org/patch/8453351/
>
> If a pciehp port is runtime suspended and pciehp_poll_mode is enabled,
> the poll timer needs to be disabled and later reenabled on runtime resume.
If we disable the timer then we can't detect when a new device is
connected to the port.
I think in this case it might be better not to enable runtime PM for the
port at all.
Hi Dave,
On Thu, Mar 10, 2016 at 08:04:26AM +1000, Dave Airlie wrote:
> On 10 March 2016 at 00:40, Lukas Wunner <[email protected]> wrote:
> > On Wed, Mar 09, 2016 at 04:14:05PM +1000, Dave Airlie wrote:
> >> From: Dave Airlie <[email protected]>
> >>
> >> This fixes GPU auto powerdown on the Lenovo W541,
> >> since we advertise Windows 2013 to the ACPI layer.
> >>
> >> Signed-off-by: Dave Airlie <[email protected]>
> >> ---
> >> drivers/gpu/drm/nouveau/nouveau_vga.c | 10 +++++++---
> >> 1 file changed, 7 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c b/drivers/gpu/drm/nouveau/nouveau_vga.c
> >> index af89c36..b987427f 100644
> >> --- a/drivers/gpu/drm/nouveau/nouveau_vga.c
> >> +++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
> >> @@ -101,8 +101,12 @@ nouveau_vga_init(struct nouveau_drm *drm)
> >> runtime = true;
> >> vga_switcheroo_register_client(dev->pdev, &nouveau_switcheroo_ops, runtime);
> >>
> >> - if (runtime && nouveau_is_v1_dsm() && !nouveau_is_optimus())
> >> - vga_switcheroo_init_domain_pm_ops(drm->dev->dev, &drm->vga_pm_domain);
> >> + if (runtime) {
> >> + if (nouveau_is_v1_dsm() && !nouveau_is_optimus())
> >
> > The " && !nouveau_is_optimus()" can be dropped because a machine cannot
> > have both. Note the "else" in nouveau_dsm_detect():
>
> I'm pretty sure I've seen a machine with both in my past, back in the
> Vista/Win7 crossover days.
Yes, but the code in nouveau_dsm_detect() is such that you'll never have
both nouveau_is_v1_dsm() and nouveau_is_optimus() return true.
So you can drop the " && !nouveau_is_optimus()".
Best regards,
Lukas
>
> > You're calling this unconditionally for all Optimus machines yet
> > I assume pre Windows 10 machines lack the PR3 hooks.
> >
>
> Yes and I've confirmed on my older machine that nothing bad happens
> doing it unconditionally,
> and I couldn't find any bits in the _DSM flags to tell me if I should
> do something different.
>
> Dave.