Re: [PATCH 07/16] drm/i915: fix S4 suspend while switcheroo state is off

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Oct 21, 2014 at 04:08:21PM +0300, Imre Deak wrote:
> On Tue, 2014-10-21 at 15:41 +0300, Ville Syrjälä wrote:
> > On Wed, Sep 10, 2014 at 06:17:00PM +0300, Imre Deak wrote:
> > > If the device is suspended already through the switcheroo interface we
> > > shouldn't suspend it again. We have the corresponding check for S3
> > > suspend already, add it for S4 freeze and poweroff too.
> > > 
> > > Note that there is still the problem that the resume path should also
> > > check for the switcheroo-off state and keep the device disabled or in
> > > case of S4 disable it. That is a separate issue, which is not addressed
> > > in this patchset.
> > 
> > That's about RESUME/RESTORE I take it.
> > 
> > But what about .thaw()? I think simply adding the same check to .thaw()
> > would work out just fine since it's always called after .freeze() for
> > THAW/RECOVER.
> 
> Yes, the check is missing from there too (actually by the end of the
> patchset THAW/RESUME/RESTORE are handled the same way). I didn't want to
> fix up that part in this patchset, since I thought avoiding RESTORE in
> case DRM_SWITCH_POWER_OFF is set is not enough during S4, we have to
> actually disable the device if it was enabled. But thinking about it
> again that seems to be not true, since before RESTORE FREEZE is called
> in the loader kernel, which leaves the device in a disabled state.

I was thinking just about fixing THAW/RECOVER since those get executed
from the same kernel as FREEZE/QUIESCE, but I guess you're correct that
we can just handle RESUME/RESTORE the same way since SUSPEND/QUIESCE in
the pre-resume kernel should have disabled the device anyway in case it
was enabled, which means the state will match the switcheroo disabled
state in the resumed kernel. And if switcheroo says the device should
be enabled we enable it normally when resuming. So yeah, sounds like it
should just work (tm).

> 
> So I can put the check to .thaw too (.suspend by the end of the
> patchset), but it'd be easier to do it on top of this patchset if that's
> ok.

Sure. 

> 
> --Imre
> 
> > 
> > > 
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > index ca74d6d..a3addc2 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -994,6 +994,9 @@ static int i915_pm_freeze(struct device *dev)
> > >  		return -ENODEV;
> > >  	}
> > >  
> > > +	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> > > +		return 0;
> > > +
> > >  	return i915_drm_freeze(drm_dev);
> > >  }
> > >  
> > > @@ -1003,6 +1006,9 @@ static int i915_pm_freeze_late(struct device *dev)
> > >  	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> > >  	struct drm_i915_private *dev_priv = drm_dev->dev_private;
> > >  
> > > +	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> > > +		return 0;
> > > +
> > >  	return intel_suspend_complete(dev_priv);
> > >  }
> > >  
> > > @@ -1027,6 +1033,9 @@ static int i915_pm_poweroff(struct device *dev)
> > >  	struct pci_dev *pdev = to_pci_dev(dev);
> > >  	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> > >  
> > > +	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> > > +		return 0;
> > > +
> > >  	return i915_drm_freeze(drm_dev);
> > >  }
> > >  
> > > -- 
> > > 1.8.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> 

-- 
Ville Syrjälä
Intel OTC