[PATCH] drm/nouveau: Drop superfluous DRM_SWITCH_POWER_DYNAMIC_OFF checks

[PATCH] drm/nouveau: Drop superfluous DRM_SWITCH_POWER_DYNAMIC_OFF checks

[Lukas Wunner]

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@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

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH] drm/nouveau: Drop superfluous DRM_SWITCH_POWER_DYNAMIC_OFF checks

[Peter Wu]

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@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@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
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH] drm/nouveau: Drop superfluous DRM_SWITCH_POWER_DYNAMIC_OFF checks

[Lukas Wunner]

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@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@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
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH] drm/nouveau: Drop superfluous DRM_SWITCH_POWER_DYNAMIC_OFF checks

[Lukas Wunner]

Hi Ben,

On Wed, Dec 14, 2016 at 08:00:16PM +0100, Lukas Wunner wrote:
> 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@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?

Forgot to mention:  The rationale of this commit is that Alex Deucher
added the same superfluous code to radeon and amdgpu with commits
5e0b1617fc38 and f46cf3735f4c.  When asked he specifically pointed to
nouveau doing the same.  The code was subsequently removed again from
radeon and amdgpu with commits f2aba352a954 and e313de7e8997.

By removing the superfluous code from nouveau I would like to prevent
it from being cargo-culted to further drivers.

If you'd like me to resend with this additional explanation please shout.

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@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
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau