[Intel-gfx] [PATCH] drm/i915/gen9+: Set same power state before hibernation image save/restore

[Imre Deak]

On Fri, Sep 15, 2017 at 06:02:33PM +0300, Ville Syrjälä wrote:
> On Wed, Aug 16, 2017 at 05:46:07PM +0300, Imre Deak wrote:
> > Atm, on GEN9 big core platforms before saving the hibernation image we
> > uninitialize the display, disabling power wells manually, while before
> > restoring the image we keep things powered (letting HW/DMC power down
> > things as needed). The state mismatch will trigger the following error:
> > 
> > DC state mismatch (0x0 -> 0x2)
> > 
> > While the restore handler knows how to initialize the display from an
> > unknown state (due to a different loader kernel or not having i915
> > loaded in the loader kernel) we should still use the same state for
> > consistency before image saving and restoring. Do this by uniniting the
> > display before restoring the image too.
> > 
> > Bugzilla: https://bugs.freedesktop.org/attachment.cgi?id=133376
> > Reported-and-tested-by: Wang Wendy <wendy.wang at intel.com>
> > Reported-and-tested-by: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> > Cc: Wang Wendy <wendy.wang at intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > 
> > ---
> > 
> > [ No Fixes: line, since I'm not aware of any other issues caused by this
> >   besides the error message. ]
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 20 +++++++++++++-------
> >  1 file changed, 13 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 43100229613c..d816ba715cb6 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1571,7 +1571,7 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
> >  
> >  	intel_display_set_init_power(dev_priv, false);
> >  
> > -	fw_csr = !IS_GEN9_LP(dev_priv) &&
> > +	fw_csr = !IS_GEN9_LP(dev_priv) && !hibernation &&
> >  		suspend_to_idle(dev_priv) && dev_priv->csr.dmc_payload;
> >  	/*
> >  	 * In case of firmware assisted context save/restore don't manually
> > @@ -2055,11 +2055,14 @@ static int i915_pm_resume(struct device *kdev)
> >  /* freeze: before creating the hibernation_image */
> >  static int i915_pm_freeze(struct device *kdev)
> >  {
> > +	struct drm_device *dev = &kdev_to_i915(kdev)->drm;
> >  	int ret;
> >  
> > -	ret = i915_pm_suspend(kdev);
> > -	if (ret)
> > -		return ret;
> > +	if (dev->switch_power_state != DRM_SWITCH_POWER_OFF) {
> > +		ret = i915_drm_suspend(dev);
> > +		if (ret)
> > +			return ret;
> > +	}
> 
> Hmm. This hunk is here just for consistentency I take it?

Yes, to make i915_pm_freeze() and i915_pm_freeze_late() similar.

> 
> Patch lgtm
> Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> >  
> >  	ret = i915_gem_freeze(kdev_to_i915(kdev));
> >  	if (ret)
> > @@ -2070,11 +2073,14 @@ static int i915_pm_freeze(struct device *kdev)
> >  
> >  static int i915_pm_freeze_late(struct device *kdev)
> >  {
> > +	struct drm_device *dev = &kdev_to_i915(kdev)->drm;
> >  	int ret;
> >  
> > -	ret = i915_pm_suspend_late(kdev);
> > -	if (ret)
> > -		return ret;
> > +	if (dev->switch_power_state != DRM_SWITCH_POWER_OFF) {
> > +		ret = i915_drm_suspend_late(dev, true);
> > +		if (ret)
> > +			return ret;
> > +	}
> >  
> >  	ret = i915_gem_freeze_late(kdev_to_i915(kdev));
> >  	if (ret)
> > -- 
> > 2.13.2
> 
> -- 
> Ville Syrjälä
> Intel OTC