Nouveau - Nov 2016 - [PATCH] drm/nouveau: Drop superfluous DRM_SWITCH_POWER_DYNAMIC_OFF checks

nouveau's ->suspend and ->resume callbacks are currently skipped if
the
device's status is either DRM_SWITCH_POWER_OFF (powered off by
vga_switcheroo manual power control) or DRM_SWITCH_POWER_DYNAMIC_OFF
(runtime suspended).

In the former case this makes sense since the device is powered off
behind the PM core's back:  It will try to execute the ->suspend and
->resume callbacks upon system sleep, not knowing that the device is
inaccessible.  Therefore the callbacks have to become no-ops.

However the latter case doesn't make any sense because the PM core
never calls the ->suspend and ->resume callbacks of runtime suspended
devices:  Such devices are either runtime resumed before going to system
sleep (see call to pm_runtime_resume() in drivers/pci/pci-driver:
pci_pm_suspend()) or they are left runtime suspended over the entire
system suspend/resume process (search for "direct_complete" in
drivers/base/power/main.c).

Consequently the DRM_SWITCH_POWER_DYNAMIC_OFF checks are superfluous.
Drop them.

Signed-off-by: Lukas Wunner <lukas at wunner.de>
---
 drivers/gpu/drm/nouveau/nouveau_drm.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 9876e6f..d91d092 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -666,8 +666,7 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
 	struct drm_device *drm_dev = pci_get_drvdata(pdev);
 	int ret;
 
-	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF ||
-	    drm_dev->switch_power_state == DRM_SWITCH_POWER_DYNAMIC_OFF)
+	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
 		return 0;
 
 	ret = nouveau_do_suspend(drm_dev, false);
@@ -688,8 +687,7 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
 	struct drm_device *drm_dev = pci_get_drvdata(pdev);
 	int ret;
 
-	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF ||
-	    drm_dev->switch_power_state == DRM_SWITCH_POWER_DYNAMIC_OFF)
+	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
 		return 0;
 
 	pci_set_power_state(pdev, PCI_D0);
-- 
2.10.1

On Tue, Nov 08, 2016 at 12:57:00PM +0100, Lukas Wunner
wrote:
> nouveau's ->suspend and ->resume callbacks are currently skipped
if the
> device's status is either DRM_SWITCH_POWER_OFF (powered off by
> vga_switcheroo manual power control) or DRM_SWITCH_POWER_DYNAMIC_OFF
> (runtime suspended).
> 
> In the former case this makes sense since the device is powered off
> behind the PM core's back:  It will try to execute the ->suspend and
> ->resume callbacks upon system sleep, not knowing that the device is
> inaccessible.  Therefore the callbacks have to become no-ops.
> 
> However the latter case doesn't make any sense because the PM core
> never calls the ->suspend and ->resume callbacks of runtime suspended
> devices:  Such devices are either runtime resumed before going to system
> sleep (see call to pm_runtime_resume() in drivers/pci/pci-driver:
> pci_pm_suspend()) or they are left runtime suspended over the entire
> system suspend/resume process (search for "direct_complete" in
> drivers/base/power/main.c).
> 
> Consequently the DRM_SWITCH_POWER_DYNAMIC_OFF checks are superfluous.
> Drop them.
> 
> Signed-off-by: Lukas Wunner <lukas at wunner.de>
It is better to rely on the official documentation rather than the
implementation. Luckily, Documentation/power/pci.txt supports the claim:

    2.4.1. System Suspend

    When the system is going into a sleep state in which the contents of memory
will
    be preserved, such as one of the ACPI sleep states S1-S3, the phases are:

        prepare, suspend, suspend_noirq.
    [..]
    The pci_pm_prepare() routine first puts the device into the "fully
functional"
    state with the help of pm_runtime_resume(). [..]

So indeed we can be sure that the device is runtime-resumed before
suspend. System resume is not documented explicitly, but it seems
reasonable that the device is not runtime-suspended between system
suspend and resume.

Reviewed-by: Peter Wu <peter at lekensteyn.nl>
> ---
>  drivers/gpu/drm/nouveau/nouveau_drm.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index 9876e6f..d91d092 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -666,8 +666,7 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
>  	struct drm_device *drm_dev = pci_get_drvdata(pdev);
>  	int ret;
>  
> -	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF ||
> -	    drm_dev->switch_power_state == DRM_SWITCH_POWER_DYNAMIC_OFF)
> +	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
>  		return 0;
>  
>  	ret = nouveau_do_suspend(drm_dev, false);
> @@ -688,8 +687,7 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
>  	struct drm_device *drm_dev = pci_get_drvdata(pdev);
>  	int ret;
>  
> -	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF ||
> -	    drm_dev->switch_power_state == DRM_SWITCH_POWER_DYNAMIC_OFF)
> +	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
>  		return 0;
>  
>  	pci_set_power_state(pdev, PCI_D0);
> -- 
> 2.10.1

Hi Ben,

On Tue, Nov 08, 2016 at 09:29:38PM +0100, Peter Wu
wrote:
> On Tue, Nov 08, 2016 at 12:57:00PM +0100, Lukas Wunner wrote:
> > nouveau's ->suspend and ->resume callbacks are currently
skipped if the
> > device's status is either DRM_SWITCH_POWER_OFF (powered off by
> > vga_switcheroo manual power control) or DRM_SWITCH_POWER_DYNAMIC_OFF
> > (runtime suspended).
> > 
> > In the former case this makes sense since the device is powered off
> > behind the PM core's back:  It will try to execute the
->suspend and
> > ->resume callbacks upon system sleep, not knowing that the device
is
> > inaccessible.  Therefore the callbacks have to become no-ops.
> > 
> > However the latter case doesn't make any sense because the PM core
> > never calls the ->suspend and ->resume callbacks of runtime
suspended
> > devices:  Such devices are either runtime resumed before going to
system
> > sleep (see call to pm_runtime_resume() in drivers/pci/pci-driver:
> > pci_pm_suspend()) or they are left runtime suspended over the entire
> > system suspend/resume process (search for "direct_complete"
in
> > drivers/base/power/main.c).
> > 
> > Consequently the DRM_SWITCH_POWER_DYNAMIC_OFF checks are superfluous.
> > Drop them.
> > 
> > Signed-off-by: Lukas Wunner <lukas at wunner.de>
Just a gentle ping:  This patch was posted a month ago and I'm not
seeing it in your tree.  Do you have objections?  Should I repost
with Peter's Reviewed-by?

Thanks,

Lukas
> 
> It is better to rely on the official documentation rather than the
> implementation. Luckily, Documentation/power/pci.txt supports the claim:
> 
>     2.4.1. System Suspend
> 
>     When the system is going into a sleep state in which the contents of
memory will
>     be preserved, such as one of the ACPI sleep states S1-S3, the phases
are:
> 
>         prepare, suspend, suspend_noirq.
>     [..]
>     The pci_pm_prepare() routine first puts the device into the "fully
functional"
>     state with the help of pm_runtime_resume(). [..]
> 
> So indeed we can be sure that the device is runtime-resumed before
> suspend. System resume is not documented explicitly, but it seems
> reasonable that the device is not runtime-suspended between system
> suspend and resume.
> 
> Reviewed-by: Peter Wu <peter at lekensteyn.nl>
> 
> > ---
> >  drivers/gpu/drm/nouveau/nouveau_drm.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > index 9876e6f..d91d092 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > @@ -666,8 +666,7 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
> >  	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> >  	int ret;
> >  
> > -	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF ||
> > -	    drm_dev->switch_power_state == DRM_SWITCH_POWER_DYNAMIC_OFF)
> > +	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> >  		return 0;
> >  
> >  	ret = nouveau_do_suspend(drm_dev, false);
> > @@ -688,8 +687,7 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
> >  	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> >  	int ret;
> >  
> > -	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF ||
> > -	    drm_dev->switch_power_state == DRM_SWITCH_POWER_DYNAMIC_OFF)
> > +	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> >  		return 0;
> >  
> >  	pci_set_power_state(pdev, PCI_D0);
> > -- 
> > 2.10.1