[Nouveau] [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 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