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

[Peter Wu <peter-VTkQYDcBqhK7DlmcbJSQ7g@public.gmane.org>]

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