| Message ID | 20180322143642.26883-1-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 22, 2018 at 04:36:42PM +0200, Imre Deak wrote: > After > > commit dd9f31c7a3887950cbd0d49eb9d43f7a1518a356 > Author: Imre Deak <imre.deak@intel.com> > 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@hotmail.com > Fixes: dd9f31c7a388 ("drm/i915/gen9+: Set same power state before hibernation image save/restore") > Cc: amn-bas@hotmail.com > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: <stable@vger.kernel.org> > Signed-off-by: Imre Deak <imre.deak@intel.com> Make sense to me. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> 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
On Thu, Mar 22, 2018 at 04:49:22PM +0000, Patchwork wrote: > == Series Details == > > Series: drm/i915: Fix hibernation with ACPI S0 target state > URL : https://patchwork.freedesktop.org/series/40466/ > State : success > > == Summary == Thanks for the review, I pushed the patch to -dinq. > > ---- Known issues: > > Test kms_cursor_crc: > Subgroup cursor-256x256-suspend: > pass -> INCOMPLETE (shard-hsw) fdo#103375 > Test kms_flip: > Subgroup dpms-vs-vblank-race: > fail -> PASS (shard-hsw) fdo#103060 +1 > Subgroup plain-flip-fb-recreate-interruptible: > pass -> FAIL (shard-hsw) fdo#100368 +2 > Test kms_rotation_crc: > Subgroup sprite-rotation-180: > fail -> PASS (shard-apl) fdo#103925 > Test kms_setmode: > Subgroup basic: > fail -> PASS (shard-apl) fdo#99912 > > fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375 > fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060 > fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368 > fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925 > fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912 > > shard-apl total:3484 pass:1821 dwarn:1 dfail:0 fail:6 skip:1655 time:13132s > shard-hsw total:3477 pass:1770 dwarn:1 dfail:0 fail:2 skip:1702 time:11273s > shard-snb total:3484 pass:1363 dwarn:1 dfail:0 fail:3 skip:2117 time:7243s > Blacklisted hosts: > shard-kbl total:3484 pass:1943 dwarn:1 dfail:0 fail:9 skip:1531 time:10063s > > == Logs == > > For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8451/shards.html
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;
After commit dd9f31c7a3887950cbd0d49eb9d43f7a1518a356 Author: Imre Deak <imre.deak@intel.com> 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@hotmail.com Fixes: dd9f31c7a388 ("drm/i915/gen9+: Set same power state before hibernation image save/restore") Cc: amn-bas@hotmail.com Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: <stable@vger.kernel.org> Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 22 ++++++++++------------ drivers/gpu/drm/i915/i915_drv.h | 2 +- 2 files changed, 11 insertions(+), 13 deletions(-)