On Thu, Mar 22, 2018 at 04:36:42PM +0200, Imre Deak wrote: > After > > commit dd9f31c7a3887950cbd0d49eb9d43f7a1518a356 > Author: Imre Deak <imre.deak@xxxxxxxxx> > Date: Wed Aug 16 17:46:07 2017 +0300 > > drm/i915/gen9+: Set same power state before hibernation image > save/restore > > during hibernation/suspend the power domain functionality got disabled, > after which resume could leave it incorrectly disabled if the ACPI > target state was S0 during suspend and i915 was not loaded by the loader > kernel. > > This was caused by not considering if we resumed from hibernation as the > condition for power domains reiniting. > > Fix this by simply tracking if we suspended power domains during system > suspend and reinit power domains accordingly during resume. This will > result in reiniting power domains always when resuming from hibernation, > regardless of the platform and whether or not i915 is loaded by the > loader kernel. > > The reason we didn't catch this earlier is that the enabled/disabled > state of power domains during PMSG_FREEZE/PMSG_QUIESCE is platform > and kernel config dependent: on my SKL the target state is S4 > during PMSG_FREEZE and (with the driver loaded in the loader kernel) > S0 during PMSG_QUIESCE. On the reporter's machine it's S0 during > PMSG_FREEZE but (contrary to this) power domains are not initialized > during PMSG_QUIESCE since i915 is not loaded in the loader kernel, or > it's loaded but without the DMC firmware being available. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105196 > Reported-and-tested-by: amn-bas@xxxxxxxxxxx > Fixes: dd9f31c7a388 ("drm/i915/gen9+: Set same power state before hibernation image save/restore") > Cc: amn-bas@xxxxxxxxxxx > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > Cc: <stable@xxxxxxxxxxxxxxx> > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx> Make sense to me. Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> On thing I can't quite tell is what happens with switcheroo. Maybe it's not even relevant for platforms with DC6 in which case I suppose it should work correctly. > --- > drivers/gpu/drm/i915/i915_drv.c | 22 ++++++++++------------ > drivers/gpu/drm/i915/i915_drv.h | 2 +- > 2 files changed, 11 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index a7d3275f45d2..f706cff4f01b 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1612,15 +1612,12 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation) > { > struct drm_i915_private *dev_priv = to_i915(dev); > struct pci_dev *pdev = dev_priv->drm.pdev; > - bool fw_csr; > int ret; > > disable_rpm_wakeref_asserts(dev_priv); > > intel_display_set_init_power(dev_priv, false); > > - 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 > * deinit the power domains. This also means the CSR/DMC firmware will > @@ -1628,8 +1625,11 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation) > * also enable deeper system power states that would be blocked if the > * firmware was inactive. > */ > - if (!fw_csr) > + if (IS_GEN9_LP(dev_priv) || hibernation || !suspend_to_idle(dev_priv) || > + dev_priv->csr.dmc_payload == NULL) { > intel_power_domains_suspend(dev_priv); > + dev_priv->power_domains_suspended = true; > + } > > ret = 0; > if (IS_GEN9_LP(dev_priv)) > @@ -1641,8 +1641,10 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation) > > if (ret) { > DRM_ERROR("Suspend complete failed: %d\n", ret); > - if (!fw_csr) > + if (dev_priv->power_domains_suspended) { > intel_power_domains_init_hw(dev_priv, true); > + dev_priv->power_domains_suspended = false; > + } > > goto out; > } > @@ -1663,8 +1665,6 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation) > if (!(hibernation && INTEL_GEN(dev_priv) < 6)) > pci_set_power_state(pdev, PCI_D3hot); > > - dev_priv->suspended_to_idle = suspend_to_idle(dev_priv); > - > out: > enable_rpm_wakeref_asserts(dev_priv); > > @@ -1831,8 +1831,7 @@ static int i915_drm_resume_early(struct drm_device *dev) > intel_uncore_resume_early(dev_priv); > > if (IS_GEN9_LP(dev_priv)) { > - if (!dev_priv->suspended_to_idle) > - gen9_sanitize_dc_state(dev_priv); > + gen9_sanitize_dc_state(dev_priv); > bxt_disable_dc9(dev_priv); > } else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) { > hsw_disable_pc8(dev_priv); > @@ -1840,8 +1839,7 @@ static int i915_drm_resume_early(struct drm_device *dev) > > intel_uncore_sanitize(dev_priv); > > - if (IS_GEN9_LP(dev_priv) || > - !(dev_priv->suspended_to_idle && dev_priv->csr.dmc_payload)) > + if (dev_priv->power_domains_suspended) > intel_power_domains_init_hw(dev_priv, true); > else > intel_display_set_init_power(dev_priv, true); > @@ -1851,7 +1849,7 @@ static int i915_drm_resume_early(struct drm_device *dev) > enable_rpm_wakeref_asserts(dev_priv); > > out: > - dev_priv->suspended_to_idle = false; > + dev_priv->power_domains_suspended = false; > > return ret; > } > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index c9c3b2ba6a86..3acc4bbec6b2 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1851,7 +1851,7 @@ struct drm_i915_private { > u32 bxt_phy_grc; > > u32 suspend_count; > - bool suspended_to_idle; > + bool power_domains_suspended; > struct i915_suspend_saved_registers regfile; > struct vlv_s0ix_state vlv_s0ix_state; > > -- > 2.13.2 -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
