* [PATCH] drm/i915/skl: enable PC9/10 power states during suspend-to-idle @ 2015-11-18 15:32 Imre Deak 2015-11-18 16:33 ` Daniel Vetter 0 siblings, 1 reply; 7+ messages in thread From: Imre Deak @ 2015-11-18 15:32 UTC (permalink / raw) To: intel-gfx; +Cc: Nivedita Swaminathan During suspend-to-idle we need to keep the DMC firmware active and DC6 enabled, since otherwise we won't reach deep system power states like PC9/10. The lead for this came from Nivedita who noticed that the kernel's turbostat tool didn't report any PC9/10 residency change across an 'echo freeze > /sys/power/state'. Reported-by: Nivedita Swaminathan <nivedita.swaminathan@intel.com> Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 44 +++++++++++++++++++++++++++++++---------- drivers/gpu/drm/i915/i915_drv.h | 1 + 2 files changed, 35 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 6344dfb..649e20a 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -624,6 +624,14 @@ static int vlv_resume_prepare(struct drm_i915_private *dev_priv, bool rpm_resume); static int bxt_resume_prepare(struct drm_i915_private *dev_priv); +static bool suspend_to_idle(struct drm_i915_private *dev_priv) +{ +#if IS_ENABLED(CONFIG_ACPI_SLEEP) + if (acpi_target_system_state() < ACPI_STATE_S3) + return true; +#endif + return false; +} static int i915_drm_suspend(struct drm_device *dev) { @@ -676,11 +684,7 @@ static int i915_drm_suspend(struct drm_device *dev) i915_save_state(dev); - opregion_target_state = PCI_D3cold; -#if IS_ENABLED(CONFIG_ACPI_SLEEP) - if (acpi_target_system_state() < ACPI_STATE_S3) - opregion_target_state = PCI_D1; -#endif + opregion_target_state = suspend_to_idle(dev_priv) ? PCI_D1 : PCI_D3cold; intel_opregion_notify_adapter(dev, opregion_target_state); intel_uncore_forcewake_reset(dev, false); @@ -701,15 +705,26 @@ static int i915_drm_suspend(struct drm_device *dev) static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation) { struct drm_i915_private *dev_priv = drm_dev->dev_private; + bool fw_csr; int ret; - intel_power_domains_suspend(dev_priv); + fw_csr = 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 + * stay active, it will power down any HW resources as required and + * also enable deeper system power states that would be blocked if the + * firmware was inactive. + */ + if (!fw_csr) + intel_power_domains_suspend(dev_priv); ret = intel_suspend_complete(dev_priv); if (ret) { DRM_ERROR("Suspend complete failed: %d\n", ret); - intel_power_domains_init_hw(dev_priv, true); + if (!fw_csr) + intel_power_domains_init_hw(dev_priv, true); return ret; } @@ -730,6 +745,8 @@ static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation) if (!(hibernation && INTEL_INFO(dev_priv)->gen < 6)) pci_set_power_state(drm_dev->pdev, PCI_D3hot); + dev_priv->suspended_to_idle = suspend_to_idle(dev_priv); + return 0; } @@ -842,8 +859,10 @@ static int i915_drm_resume_early(struct drm_device *dev) * FIXME: This should be solved with a special hdmi sink device or * similar so that power domains can be employed. */ - if (pci_enable_device(dev->pdev)) - return -EIO; + if (pci_enable_device(dev->pdev)) { + ret = -EIO; + goto out; + } pci_set_master(dev->pdev); @@ -861,7 +880,12 @@ static int i915_drm_resume_early(struct drm_device *dev) hsw_disable_pc8(dev_priv); intel_uncore_sanitize(dev); - intel_power_domains_init_hw(dev_priv, true); + + if (!(dev_priv->suspended_to_idle && dev_priv->csr.dmc_payload)) + intel_power_domains_init_hw(dev_priv, true); + +out: + dev_priv->suspended_to_idle = false; return ret; } diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index a32571f..4a50382 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1882,6 +1882,7 @@ struct drm_i915_private { u32 chv_phy_control; u32 suspend_count; + bool suspended_to_idle; struct i915_suspend_saved_registers regfile; struct vlv_s0ix_state vlv_s0ix_state; -- 2.5.0 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915/skl: enable PC9/10 power states during suspend-to-idle 2015-11-18 15:32 [PATCH] drm/i915/skl: enable PC9/10 power states during suspend-to-idle Imre Deak @ 2015-11-18 16:33 ` Daniel Vetter 2015-11-18 16:44 ` Imre Deak 0 siblings, 1 reply; 7+ messages in thread From: Daniel Vetter @ 2015-11-18 16:33 UTC (permalink / raw) To: Imre Deak; +Cc: intel-gfx, Rafael J. Wysocki, Nivedita Swaminathan On Wed, Nov 18, 2015 at 05:32:30PM +0200, Imre Deak wrote: > During suspend-to-idle we need to keep the DMC firmware active and DC6 > enabled, since otherwise we won't reach deep system power states like > PC9/10. The lead for this came from Nivedita who noticed that the > kernel's turbostat tool didn't report any PC9/10 residency change > across an 'echo freeze > /sys/power/state'. > > Reported-by: Nivedita Swaminathan <nivedita.swaminathan@intel.com> > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 44 +++++++++++++++++++++++++++++++---------- > drivers/gpu/drm/i915/i915_drv.h | 1 + > 2 files changed, 35 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 6344dfb..649e20a 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -624,6 +624,14 @@ static int vlv_resume_prepare(struct drm_i915_private *dev_priv, > bool rpm_resume); > static int bxt_resume_prepare(struct drm_i915_private *dev_priv); > > +static bool suspend_to_idle(struct drm_i915_private *dev_priv) > +{ > +#if IS_ENABLED(CONFIG_ACPI_SLEEP) > + if (acpi_target_system_state() < ACPI_STATE_S3) > + return true; > +#endif > + return false; > +} > > static int i915_drm_suspend(struct drm_device *dev) > { > @@ -676,11 +684,7 @@ static int i915_drm_suspend(struct drm_device *dev) > > i915_save_state(dev); > > - opregion_target_state = PCI_D3cold; > -#if IS_ENABLED(CONFIG_ACPI_SLEEP) > - if (acpi_target_system_state() < ACPI_STATE_S3) > - opregion_target_state = PCI_D1; > -#endif > + opregion_target_state = suspend_to_idle(dev_priv) ? PCI_D1 : PCI_D3cold; > intel_opregion_notify_adapter(dev, opregion_target_state); > > intel_uncore_forcewake_reset(dev, false); > @@ -701,15 +705,26 @@ static int i915_drm_suspend(struct drm_device *dev) > static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation) > { > struct drm_i915_private *dev_priv = drm_dev->dev_private; > + bool fw_csr; > int ret; > > - intel_power_domains_suspend(dev_priv); > + fw_csr = 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 > + * stay active, it will power down any HW resources as required and > + * also enable deeper system power states that would be blocked if the > + * firmware was inactive. > + */ > + if (!fw_csr) > + intel_power_domains_suspend(dev_priv); > > ret = intel_suspend_complete(dev_priv); > > if (ret) { > DRM_ERROR("Suspend complete failed: %d\n", ret); > - intel_power_domains_init_hw(dev_priv, true); > + if (!fw_csr) > + intel_power_domains_init_hw(dev_priv, true); > > return ret; > } > @@ -730,6 +745,8 @@ static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation) > if (!(hibernation && INTEL_INFO(dev_priv)->gen < 6)) > pci_set_power_state(drm_dev->pdev, PCI_D3hot); > > + dev_priv->suspended_to_idle = suspend_to_idle(dev_priv); > + > return 0; > } > > @@ -842,8 +859,10 @@ static int i915_drm_resume_early(struct drm_device *dev) > * FIXME: This should be solved with a special hdmi sink device or > * similar so that power domains can be employed. > */ > - if (pci_enable_device(dev->pdev)) > - return -EIO; > + if (pci_enable_device(dev->pdev)) { > + ret = -EIO; > + goto out; > + } > > pci_set_master(dev->pdev); > > @@ -861,7 +880,12 @@ static int i915_drm_resume_early(struct drm_device *dev) > hsw_disable_pc8(dev_priv); > > intel_uncore_sanitize(dev); > - intel_power_domains_init_hw(dev_priv, true); > + > + if (!(dev_priv->suspended_to_idle && dev_priv->csr.dmc_payload)) > + intel_power_domains_init_hw(dev_priv, true); This doesn't work when e.g. system suspend to S3 fails, in which case dmc will also survive. We need the pm core to tell us what really happened, and Rafael Wyzocki was working on patches for that: http://lkml.iu.edu/hypermail/linux/kernel/1510.0/04266.html Specifically we need the pm_suspend/resume_via_firmware helpers. Unfortunately these didn't make it into 4.4, so we need to pick Rafael. Cheers, Daniel > + > +out: > + dev_priv->suspended_to_idle = false; > > return ret; > } > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index a32571f..4a50382 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1882,6 +1882,7 @@ struct drm_i915_private { > u32 chv_phy_control; > > u32 suspend_count; > + bool suspended_to_idle; > struct i915_suspend_saved_registers regfile; > struct vlv_s0ix_state vlv_s0ix_state; > > -- > 2.5.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915/skl: enable PC9/10 power states during suspend-to-idle 2015-11-18 16:33 ` Daniel Vetter @ 2015-11-18 16:44 ` Imre Deak 2015-11-19 13:34 ` Patrik Jakobsson 0 siblings, 1 reply; 7+ messages in thread From: Imre Deak @ 2015-11-18 16:44 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx, Rafael J. Wysocki, Nivedita Swaminathan On ke, 2015-11-18 at 17:33 +0100, Daniel Vetter wrote: > On Wed, Nov 18, 2015 at 05:32:30PM +0200, Imre Deak wrote: > > During suspend-to-idle we need to keep the DMC firmware active and DC6 > > enabled, since otherwise we won't reach deep system power states like > > PC9/10. The lead for this came from Nivedita who noticed that the > > kernel's turbostat tool didn't report any PC9/10 residency change > > across an 'echo freeze > /sys/power/state'. > > > > Reported-by: Nivedita Swaminathan <nivedita.swaminathan@intel.com> > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.c | 44 +++++++++++++++++++++++++++++++---------- > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > 2 files changed, 35 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index 6344dfb..649e20a 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -624,6 +624,14 @@ static int vlv_resume_prepare(struct drm_i915_private *dev_priv, > > bool rpm_resume); > > static int bxt_resume_prepare(struct drm_i915_private *dev_priv); > > > > +static bool suspend_to_idle(struct drm_i915_private *dev_priv) > > +{ > > +#if IS_ENABLED(CONFIG_ACPI_SLEEP) > > + if (acpi_target_system_state() < ACPI_STATE_S3) > > + return true; > > +#endif > > + return false; > > +} > > > > static int i915_drm_suspend(struct drm_device *dev) > > { > > @@ -676,11 +684,7 @@ static int i915_drm_suspend(struct drm_device *dev) > > > > i915_save_state(dev); > > > > - opregion_target_state = PCI_D3cold; > > -#if IS_ENABLED(CONFIG_ACPI_SLEEP) > > - if (acpi_target_system_state() < ACPI_STATE_S3) > > - opregion_target_state = PCI_D1; > > -#endif > > + opregion_target_state = suspend_to_idle(dev_priv) ? PCI_D1 : PCI_D3cold; > > intel_opregion_notify_adapter(dev, opregion_target_state); > > > > intel_uncore_forcewake_reset(dev, false); > > @@ -701,15 +705,26 @@ static int i915_drm_suspend(struct drm_device *dev) > > static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation) > > { > > struct drm_i915_private *dev_priv = drm_dev->dev_private; > > + bool fw_csr; > > int ret; > > > > - intel_power_domains_suspend(dev_priv); > > + fw_csr = 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 > > + * stay active, it will power down any HW resources as required and > > + * also enable deeper system power states that would be blocked if the > > + * firmware was inactive. > > + */ > > + if (!fw_csr) > > + intel_power_domains_suspend(dev_priv); > > > > ret = intel_suspend_complete(dev_priv); > > > > if (ret) { > > DRM_ERROR("Suspend complete failed: %d\n", ret); > > - intel_power_domains_init_hw(dev_priv, true); > > + if (!fw_csr) > > + intel_power_domains_init_hw(dev_priv, true); > > > > return ret; > > } > > @@ -730,6 +745,8 @@ static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation) > > if (!(hibernation && INTEL_INFO(dev_priv)->gen < 6)) > > pci_set_power_state(drm_dev->pdev, PCI_D3hot); > > > > + dev_priv->suspended_to_idle = suspend_to_idle(dev_priv); > > + > > return 0; > > } > > > > @@ -842,8 +859,10 @@ static int i915_drm_resume_early(struct drm_device *dev) > > * FIXME: This should be solved with a special hdmi sink device or > > * similar so that power domains can be employed. > > */ > > - if (pci_enable_device(dev->pdev)) > > - return -EIO; > > + if (pci_enable_device(dev->pdev)) { > > + ret = -EIO; > > + goto out; > > + } > > > > pci_set_master(dev->pdev); > > > > @@ -861,7 +880,12 @@ static int i915_drm_resume_early(struct drm_device *dev) > > hsw_disable_pc8(dev_priv); > > > > intel_uncore_sanitize(dev); > > - intel_power_domains_init_hw(dev_priv, true); > > + > > + if (!(dev_priv->suspended_to_idle && dev_priv->csr.dmc_payload)) > > + intel_power_domains_init_hw(dev_priv, true); > > This doesn't work when e.g. system suspend to S3 fails, in which case dmc > will also survive. We need the pm core to tell us what really happened, > and Rafael Wyzocki was working on patches for that: > > http://lkml.iu.edu/hypermail/linux/kernel/1510.0/04266.html > > Specifically we need the pm_suspend/resume_via_firmware helpers. > Unfortunately these didn't make it into 4.4, so we need to pick Rafael. I think it does work. We deactivate the firmware during S3,S4 suspend in any case and reprogram it during S3,S4 resume, so it doesn't matter if the firmware contents survive or not. What we want is to ensure that during suspend-to-idle we don't deactivate the firmware that is we don't disable DC6 and we can decide that based on acpi_target_system_state(). --Imre > Cheers, Daniel > > > + > > +out: > > + dev_priv->suspended_to_idle = false; > > > > return ret; > > } > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index a32571f..4a50382 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1882,6 +1882,7 @@ struct drm_i915_private { > > u32 chv_phy_control; > > > > u32 suspend_count; > > + bool suspended_to_idle; > > struct i915_suspend_saved_registers regfile; > > struct vlv_s0ix_state vlv_s0ix_state; > > > > -- > > 2.5.0 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915/skl: enable PC9/10 power states during suspend-to-idle 2015-11-18 16:44 ` Imre Deak @ 2015-11-19 13:34 ` Patrik Jakobsson 2015-11-19 14:06 ` Imre Deak 0 siblings, 1 reply; 7+ messages in thread From: Patrik Jakobsson @ 2015-11-19 13:34 UTC (permalink / raw) To: Imre Deak; +Cc: intel-gfx, Rafael J. Wysocki, Nivedita Swaminathan On Wed, Nov 18, 2015 at 06:44:43PM +0200, Imre Deak wrote: > On ke, 2015-11-18 at 17:33 +0100, Daniel Vetter wrote: > > On Wed, Nov 18, 2015 at 05:32:30PM +0200, Imre Deak wrote: > > > During suspend-to-idle we need to keep the DMC firmware active and DC6 > > > enabled, since otherwise we won't reach deep system power states like > > > PC9/10. The lead for this came from Nivedita who noticed that the > > > kernel's turbostat tool didn't report any PC9/10 residency change > > > across an 'echo freeze > /sys/power/state'. > > > > > > Reported-by: Nivedita Swaminathan <nivedita.swaminathan@intel.com> > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_drv.c | 44 +++++++++++++++++++++++++++++++---------- > > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > > 2 files changed, 35 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > index 6344dfb..649e20a 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > @@ -624,6 +624,14 @@ static int vlv_resume_prepare(struct drm_i915_private *dev_priv, > > > bool rpm_resume); > > > static int bxt_resume_prepare(struct drm_i915_private *dev_priv); > > > > > > +static bool suspend_to_idle(struct drm_i915_private *dev_priv) > > > +{ > > > +#if IS_ENABLED(CONFIG_ACPI_SLEEP) > > > + if (acpi_target_system_state() < ACPI_STATE_S3) > > > + return true; > > > +#endif > > > + return false; > > > +} > > > > > > static int i915_drm_suspend(struct drm_device *dev) > > > { > > > @@ -676,11 +684,7 @@ static int i915_drm_suspend(struct drm_device *dev) > > > > > > i915_save_state(dev); > > > > > > - opregion_target_state = PCI_D3cold; > > > -#if IS_ENABLED(CONFIG_ACPI_SLEEP) > > > - if (acpi_target_system_state() < ACPI_STATE_S3) > > > - opregion_target_state = PCI_D1; > > > -#endif > > > + opregion_target_state = suspend_to_idle(dev_priv) ? PCI_D1 : PCI_D3cold; > > > intel_opregion_notify_adapter(dev, opregion_target_state); > > > > > > intel_uncore_forcewake_reset(dev, false); > > > @@ -701,15 +705,26 @@ static int i915_drm_suspend(struct drm_device *dev) > > > static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation) > > > { > > > struct drm_i915_private *dev_priv = drm_dev->dev_private; > > > + bool fw_csr; > > > int ret; > > > > > > - intel_power_domains_suspend(dev_priv); > > > + fw_csr = 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 > > > + * stay active, it will power down any HW resources as required and > > > + * also enable deeper system power states that would be blocked if the > > > + * firmware was inactive. > > > + */ > > > + if (!fw_csr) > > > + intel_power_domains_suspend(dev_priv); > > > > > > ret = intel_suspend_complete(dev_priv); > > > > > > if (ret) { > > > DRM_ERROR("Suspend complete failed: %d\n", ret); > > > - intel_power_domains_init_hw(dev_priv, true); > > > + if (!fw_csr) > > > + intel_power_domains_init_hw(dev_priv, true); > > > > > > return ret; > > > } > > > @@ -730,6 +745,8 @@ static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation) > > > if (!(hibernation && INTEL_INFO(dev_priv)->gen < 6)) > > > pci_set_power_state(drm_dev->pdev, PCI_D3hot); > > > > > > + dev_priv->suspended_to_idle = suspend_to_idle(dev_priv); > > > + > > > return 0; > > > } > > > > > > @@ -842,8 +859,10 @@ static int i915_drm_resume_early(struct drm_device *dev) > > > * FIXME: This should be solved with a special hdmi sink device or > > > * similar so that power domains can be employed. > > > */ > > > - if (pci_enable_device(dev->pdev)) > > > - return -EIO; > > > + if (pci_enable_device(dev->pdev)) { > > > + ret = -EIO; > > > + goto out; > > > + } > > > > > > pci_set_master(dev->pdev); > > > > > > @@ -861,7 +880,12 @@ static int i915_drm_resume_early(struct drm_device *dev) > > > hsw_disable_pc8(dev_priv); > > > > > > intel_uncore_sanitize(dev); > > > - intel_power_domains_init_hw(dev_priv, true); > > > + > > > + if (!(dev_priv->suspended_to_idle && dev_priv->csr.dmc_payload)) > > > + intel_power_domains_init_hw(dev_priv, true); > > > > This doesn't work when e.g. system suspend to S3 fails, in which case dmc > > will also survive. We need the pm core to tell us what really happened, > > and Rafael Wyzocki was working on patches for that: > > > > http://lkml.iu.edu/hypermail/linux/kernel/1510.0/04266.html > > > > Specifically we need the pm_suspend/resume_via_firmware helpers. > > Unfortunately these didn't make it into 4.4, so we need to pick Rafael. > > I think it does work. We deactivate the firmware during S3,S4 suspend > in any case and reprogram it during S3,S4 resume, so it doesn't matter > if the firmware contents survive or not. What we want is to ensure that > during suspend-to-idle we don't deactivate the firmware that is we > don't disable DC6 and we can decide that based > on acpi_target_system_state(). If S3 fails we would reprogram the DMC even though there is no need for it. Do we know if that is allowed? Potentially the DMC could be doing something when we reprogram it and put us in an undefined state. On the other hand, one would hope that it's clever enough to put us back in a valid state when starting again. I haven't seen any problems when reprogramming the DMC so I think this fix is ok for now and we can do the proper fix when Rafael's patches land. -Patrik > > --Imre > > > Cheers, Daniel > > > > > + > > > +out: > > > + dev_priv->suspended_to_idle = false; > > > > > > return ret; > > > } > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > > index a32571f..4a50382 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > @@ -1882,6 +1882,7 @@ struct drm_i915_private { > > > u32 chv_phy_control; > > > > > > u32 suspend_count; > > > + bool suspended_to_idle; > > > struct i915_suspend_saved_registers regfile; > > > struct vlv_s0ix_state vlv_s0ix_state; > > > > > > -- > > > 2.5.0 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915/skl: enable PC9/10 power states during suspend-to-idle 2015-11-19 13:34 ` Patrik Jakobsson @ 2015-11-19 14:06 ` Imre Deak 2015-11-19 18:00 ` Patrik Jakobsson 0 siblings, 1 reply; 7+ messages in thread From: Imre Deak @ 2015-11-19 14:06 UTC (permalink / raw) To: Patrik Jakobsson; +Cc: intel-gfx, Rafael J. Wysocki, Nivedita Swaminathan On to, 2015-11-19 at 14:34 +0100, Patrik Jakobsson wrote: > On Wed, Nov 18, 2015 at 06:44:43PM +0200, Imre Deak wrote: > > On ke, 2015-11-18 at 17:33 +0100, Daniel Vetter wrote: > > > On Wed, Nov 18, 2015 at 05:32:30PM +0200, Imre Deak wrote: > > > > During suspend-to-idle we need to keep the DMC firmware active and DC6 > > > > enabled, since otherwise we won't reach deep system power states like > > > > PC9/10. The lead for this came from Nivedita who noticed that the > > > > kernel's turbostat tool didn't report any PC9/10 residency change > > > > across an 'echo freeze > /sys/power/state'. > > > > > > > > Reported-by: Nivedita Swaminathan > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/i915_drv.c | 44 +++++++++++++++++++++++++++++++---------- > > > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > > > 2 files changed, 35 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > > index 6344dfb..649e20a 100644 > > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > > @@ -624,6 +624,14 @@ static int vlv_resume_prepare(struct drm_i915_private *dev_priv, > > > > bool rpm_resume); > > > > static int bxt_resume_prepare(struct drm_i915_private *dev_priv); > > > > > > > > +static bool suspend_to_idle(struct drm_i915_private *dev_priv) > > > > +{ > > > > +#if IS_ENABLED(CONFIG_ACPI_SLEEP) > > > > + if (acpi_target_system_state() < ACPI_STATE_S3) > > > > + return true; > > > > +#endif > > > > + return false; > > > > +} > > > > > > > > static int i915_drm_suspend(struct drm_device *dev) > > > > { > > > > @@ -676,11 +684,7 @@ static int i915_drm_suspend(struct drm_device *dev) > > > > > > > > i915_save_state(dev); > > > > > > > > - opregion_target_state = PCI_D3cold; > > > > -#if IS_ENABLED(CONFIG_ACPI_SLEEP) > > > > - if (acpi_target_system_state() < ACPI_STATE_S3) > > > > - opregion_target_state = PCI_D1; > > > > -#endif > > > > + opregion_target_state = suspend_to_idle(dev_priv) ? PCI_D1 : PCI_D3cold; > > > > intel_opregion_notify_adapter(dev, opregion_target_state); > > > > > > > > intel_uncore_forcewake_reset(dev, false); > > > > @@ -701,15 +705,26 @@ static int i915_drm_suspend(struct drm_device *dev) > > > > static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation) > > > > { > > > > struct drm_i915_private *dev_priv = drm_dev->dev_private; > > > > + bool fw_csr; > > > > int ret; > > > > > > > > - intel_power_domains_suspend(dev_priv); > > > > + fw_csr = 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 > > > > + * stay active, it will power down any HW resources as required and > > > > + * also enable deeper system power states that would be blocked if the > > > > + * firmware was inactive. > > > > + */ > > > > + if (!fw_csr) > > > > + intel_power_domains_suspend(dev_priv); > > > > > > > > ret = intel_suspend_complete(dev_priv); > > > > > > > > if (ret) { > > > > DRM_ERROR("Suspend complete failed: %d\n", ret); > > > > - intel_power_domains_init_hw(dev_priv, true); > > > > + if (!fw_csr) > > > > + intel_power_domains_init_hw(dev_priv, true); > > > > > > > > return ret; > > > > } > > > > @@ -730,6 +745,8 @@ static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation) > > > > if (!(hibernation && INTEL_INFO(dev_priv)->gen < 6)) > > > > pci_set_power_state(drm_dev->pdev, PCI_D3hot); > > > > > > > > + dev_priv->suspended_to_idle = suspend_to_idle(dev_priv); > > > > + > > > > return 0; > > > > } > > > > > > > > @@ -842,8 +859,10 @@ static int i915_drm_resume_early(struct drm_device *dev) > > > > * FIXME: This should be solved with a special hdmi sink device or > > > > * similar so that power domains can be employed. > > > > */ > > > > - if (pci_enable_device(dev->pdev)) > > > > - return -EIO; > > > > + if (pci_enable_device(dev->pdev)) { > > > > + ret = -EIO; > > > > + goto out; > > > > + } > > > > > > > > pci_set_master(dev->pdev); > > > > > > > > @@ -861,7 +880,12 @@ static int i915_drm_resume_early(struct drm_device *dev) > > > > hsw_disable_pc8(dev_priv); > > > > > > > > intel_uncore_sanitize(dev); > > > > - intel_power_domains_init_hw(dev_priv, true); > > > > + > > > > + if (!(dev_priv->suspended_to_idle && dev_priv->csr.dmc_payload)) > > > > + intel_power_domains_init_hw(dev_priv, true); > > > > > > This doesn't work when e.g. system suspend to S3 fails, in which case dmc > > > will also survive. We need the pm core to tell us what really happened, > > > and Rafael Wyzocki was working on patches for that: > > > > > > http://lkml.iu.edu/hypermail/linux/kernel/1510.0/04266.html > > > > > > Specifically we need the pm_suspend/resume_via_firmware helpers. > > > Unfortunately these didn't make it into 4.4, so we need to pick Rafael. > > > > I think it does work. We deactivate the firmware during S3,S4 suspend > > in any case and reprogram it during S3,S4 resume, so it doesn't matter > > if the firmware contents survive or not. What we want is to ensure that > > during suspend-to-idle we don't deactivate the firmware that is we > > don't disable DC6 and we can decide that based > > on acpi_target_system_state(). > > If S3 fails we would reprogram the DMC even though there is no need for it. Do > we know if that is allowed? Potentially the DMC could be doing something when we > reprogram it and put us in an undefined state. On the other hand, one would hope > that it's clever enough to put us back in a valid state when starting again. > > I haven't seen any problems when reprogramming the DMC so I think this fix is ok > for now and we can do the proper fix when Rafael's patches land. We uninitialize the whole display block and put the device into D3 state, I think after that the DMC should be inactive. I noticed now that dc_state_debugmask_memory_up gets reset after S3/S4. I believe this is what actually activates the firmware. So how about using this flag to decide whether to reprogram the firmware and switch to using the new helpers once Rafael added them? --Imre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915/skl: enable PC9/10 power states during suspend-to-idle 2015-11-19 14:06 ` Imre Deak @ 2015-11-19 18:00 ` Patrik Jakobsson 2015-11-19 18:12 ` Imre Deak 0 siblings, 1 reply; 7+ messages in thread From: Patrik Jakobsson @ 2015-11-19 18:00 UTC (permalink / raw) To: Imre Deak; +Cc: intel-gfx, Rafael J. Wysocki, Nivedita Swaminathan On Thu, Nov 19, 2015 at 04:06:47PM +0200, Imre Deak wrote: > On to, 2015-11-19 at 14:34 +0100, Patrik Jakobsson wrote: > > On Wed, Nov 18, 2015 at 06:44:43PM +0200, Imre Deak wrote: > > > On ke, 2015-11-18 at 17:33 +0100, Daniel Vetter wrote: > > > > On Wed, Nov 18, 2015 at 05:32:30PM +0200, Imre Deak wrote: > > > > > During suspend-to-idle we need to keep the DMC firmware active and DC6 > > > > > enabled, since otherwise we won't reach deep system power states like > > > > > PC9/10. The lead for this came from Nivedita who noticed that the > > > > > kernel's turbostat tool didn't report any PC9/10 residency change > > > > > across an 'echo freeze > /sys/power/state'. > > > > > > > > > > Reported-by: Nivedita Swaminathan > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/i915_drv.c | 44 +++++++++++++++++++++++++++++++---------- > > > > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > > > > 2 files changed, 35 insertions(+), 10 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > > > index 6344dfb..649e20a 100644 > > > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > > > @@ -624,6 +624,14 @@ static int vlv_resume_prepare(struct drm_i915_private *dev_priv, > > > > > bool rpm_resume); > > > > > static int bxt_resume_prepare(struct drm_i915_private *dev_priv); > > > > > > > > > > +static bool suspend_to_idle(struct drm_i915_private *dev_priv) > > > > > +{ > > > > > +#if IS_ENABLED(CONFIG_ACPI_SLEEP) > > > > > + if (acpi_target_system_state() < ACPI_STATE_S3) > > > > > + return true; > > > > > +#endif > > > > > + return false; > > > > > +} > > > > > > > > > > static int i915_drm_suspend(struct drm_device *dev) > > > > > { > > > > > @@ -676,11 +684,7 @@ static int i915_drm_suspend(struct drm_device *dev) > > > > > > > > > > i915_save_state(dev); > > > > > > > > > > - opregion_target_state = PCI_D3cold; > > > > > -#if IS_ENABLED(CONFIG_ACPI_SLEEP) > > > > > - if (acpi_target_system_state() < ACPI_STATE_S3) > > > > > - opregion_target_state = PCI_D1; > > > > > -#endif > > > > > + opregion_target_state = suspend_to_idle(dev_priv) ? PCI_D1 : PCI_D3cold; > > > > > intel_opregion_notify_adapter(dev, opregion_target_state); > > > > > > > > > > intel_uncore_forcewake_reset(dev, false); > > > > > @@ -701,15 +705,26 @@ static int i915_drm_suspend(struct drm_device *dev) > > > > > static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation) > > > > > { > > > > > struct drm_i915_private *dev_priv = drm_dev->dev_private; > > > > > + bool fw_csr; > > > > > int ret; > > > > > > > > > > - intel_power_domains_suspend(dev_priv); > > > > > + fw_csr = 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 > > > > > + * stay active, it will power down any HW resources as required and > > > > > + * also enable deeper system power states that would be blocked if the > > > > > + * firmware was inactive. > > > > > + */ > > > > > + if (!fw_csr) > > > > > + intel_power_domains_suspend(dev_priv); > > > > > > > > > > ret = intel_suspend_complete(dev_priv); > > > > > > > > > > if (ret) { > > > > > DRM_ERROR("Suspend complete failed: %d\n", ret); > > > > > - intel_power_domains_init_hw(dev_priv, true); > > > > > + if (!fw_csr) > > > > > + intel_power_domains_init_hw(dev_priv, true); > > > > > > > > > > return ret; > > > > > } > > > > > @@ -730,6 +745,8 @@ static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation) > > > > > if (!(hibernation && INTEL_INFO(dev_priv)->gen < 6)) > > > > > pci_set_power_state(drm_dev->pdev, PCI_D3hot); > > > > > > > > > > + dev_priv->suspended_to_idle = suspend_to_idle(dev_priv); > > > > > + > > > > > return 0; > > > > > } > > > > > > > > > > @@ -842,8 +859,10 @@ static int i915_drm_resume_early(struct drm_device *dev) > > > > > * FIXME: This should be solved with a special hdmi sink device or > > > > > * similar so that power domains can be employed. > > > > > */ > > > > > - if (pci_enable_device(dev->pdev)) > > > > > - return -EIO; > > > > > + if (pci_enable_device(dev->pdev)) { > > > > > + ret = -EIO; > > > > > + goto out; > > > > > + } > > > > > > > > > > pci_set_master(dev->pdev); > > > > > > > > > > @@ -861,7 +880,12 @@ static int i915_drm_resume_early(struct drm_device *dev) > > > > > hsw_disable_pc8(dev_priv); > > > > > > > > > > intel_uncore_sanitize(dev); > > > > > - intel_power_domains_init_hw(dev_priv, true); > > > > > + > > > > > + if (!(dev_priv->suspended_to_idle && dev_priv->csr.dmc_payload)) > > > > > + intel_power_domains_init_hw(dev_priv, true); > > > > > > > > This doesn't work when e.g. system suspend to S3 fails, in which case dmc > > > > will also survive. We need the pm core to tell us what really happened, > > > > and Rafael Wyzocki was working on patches for that: > > > > > > > > http://lkml.iu.edu/hypermail/linux/kernel/1510.0/04266.html > > > > > > > > Specifically we need the pm_suspend/resume_via_firmware helpers. > > > > Unfortunately these didn't make it into 4.4, so we need to pick Rafael. > > > > > > I think it does work. We deactivate the firmware during S3,S4 suspend > > > in any case and reprogram it during S3,S4 resume, so it doesn't matter > > > if the firmware contents survive or not. What we want is to ensure that > > > during suspend-to-idle we don't deactivate the firmware that is we > > > don't disable DC6 and we can decide that based > > > on acpi_target_system_state(). > > > > If S3 fails we would reprogram the DMC even though there is no need for it. Do > > we know if that is allowed? Potentially the DMC could be doing something when we > > reprogram it and put us in an undefined state. On the other hand, one would hope > > that it's clever enough to put us back in a valid state when starting again. > > > > I haven't seen any problems when reprogramming the DMC so I think this fix is ok > > for now and we can do the proper fix when Rafael's patches land. > > We uninitialize the whole display block and put the device into D3 state, I think > after that the DMC should be inactive. Ok so we are fine on an S3 fail either way. > > I noticed now that dc_state_debugmask_memory_up gets reset after S3/S4. I believe > this is what actually activates the firmware. So how about using this flag to > decide whether to reprogram the firmware and switch to using the new helpers once > Rafael added them? Yes, it seems to hold off any DC5/6 state transistions until we do a dc_state_debugmask_memory_up(). But I'm not sure we can trust those bits to be cleared. Feels a bit like the CSR_PROGRAM(0) check all over again. I'd prefer to keep the patch as is if ok with you so: Reviewed-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> > > --Imre > > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915/skl: enable PC9/10 power states during suspend-to-idle 2015-11-19 18:00 ` Patrik Jakobsson @ 2015-11-19 18:12 ` Imre Deak 0 siblings, 0 replies; 7+ messages in thread From: Imre Deak @ 2015-11-19 18:12 UTC (permalink / raw) To: Patrik Jakobsson; +Cc: intel-gfx, Rafael J. Wysocki, Nivedita Swaminathan On to, 2015-11-19 at 19:00 +0100, Patrik Jakobsson wrote: > On Thu, Nov 19, 2015 at 04:06:47PM +0200, Imre Deak wrote: > > On to, 2015-11-19 at 14:34 +0100, Patrik Jakobsson wrote: > > > On Wed, Nov 18, 2015 at 06:44:43PM +0200, Imre Deak wrote: > > > > On ke, 2015-11-18 at 17:33 +0100, Daniel Vetter wrote: > > > > > On Wed, Nov 18, 2015 at 05:32:30PM +0200, Imre Deak wrote: > > > > > > During suspend-to-idle we need to keep the DMC firmware active and DC6 > > > > > > enabled, since otherwise we won't reach deep system power states like > > > > > > PC9/10. The lead for this came from Nivedita who noticed that the > > > > > > kernel's turbostat tool didn't report any PC9/10 residency change > > > > > > across an 'echo freeze > /sys/power/state'. > > > > > > > > > > > > Reported-by: Nivedita Swaminathan > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > > > --- > > > > > > drivers/gpu/drm/i915/i915_drv.c | 44 +++++++++++++++++++++++++++++++---------- > > > > > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > > > > > 2 files changed, 35 insertions(+), 10 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > > > > index 6344dfb..649e20a 100644 > > > > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > > > > @@ -624,6 +624,14 @@ static int vlv_resume_prepare(struct drm_i915_private *dev_priv, > > > > > > bool rpm_resume); > > > > > > static int bxt_resume_prepare(struct drm_i915_private *dev_priv); > > > > > > > > > > > > +static bool suspend_to_idle(struct drm_i915_private *dev_priv) > > > > > > +{ > > > > > > +#if IS_ENABLED(CONFIG_ACPI_SLEEP) > > > > > > + if (acpi_target_system_state() < ACPI_STATE_S3) > > > > > > + return true; > > > > > > +#endif > > > > > > + return false; > > > > > > +} > > > > > > > > > > > > static int i915_drm_suspend(struct drm_device *dev) > > > > > > { > > > > > > @@ -676,11 +684,7 @@ static int i915_drm_suspend(struct drm_device *dev) > > > > > > > > > > > > i915_save_state(dev); > > > > > > > > > > > > - opregion_target_state = PCI_D3cold; > > > > > > -#if IS_ENABLED(CONFIG_ACPI_SLEEP) > > > > > > - if (acpi_target_system_state() < ACPI_STATE_S3) > > > > > > - opregion_target_state = PCI_D1; > > > > > > -#endif > > > > > > + opregion_target_state = suspend_to_idle(dev_priv) ? PCI_D1 : PCI_D3cold; > > > > > > intel_opregion_notify_adapter(dev, opregion_target_state); > > > > > > > > > > > > intel_uncore_forcewake_reset(dev, false); > > > > > > @@ -701,15 +705,26 @@ static int i915_drm_suspend(struct drm_device *dev) > > > > > > static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation) > > > > > > { > > > > > > struct drm_i915_private *dev_priv = drm_dev->dev_private; > > > > > > + bool fw_csr; > > > > > > int ret; > > > > > > > > > > > > - intel_power_domains_suspend(dev_priv); > > > > > > + fw_csr = 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 > > > > > > + * stay active, it will power down any HW resources as required and > > > > > > + * also enable deeper system power states that would be blocked if the > > > > > > + * firmware was inactive. > > > > > > + */ > > > > > > + if (!fw_csr) > > > > > > + intel_power_domains_suspend(dev_priv); > > > > > > > > > > > > ret = intel_suspend_complete(dev_priv); > > > > > > > > > > > > if (ret) { > > > > > > DRM_ERROR("Suspend complete failed: %d\n", ret); > > > > > > - intel_power_domains_init_hw(dev_priv, true); > > > > > > + if (!fw_csr) > > > > > > + intel_power_domains_init_hw(dev_priv, true); > > > > > > > > > > > > return ret; > > > > > > } > > > > > > @@ -730,6 +745,8 @@ static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation) > > > > > > if (!(hibernation && INTEL_INFO(dev_priv)->gen < 6)) > > > > > > pci_set_power_state(drm_dev->pdev, PCI_D3hot); > > > > > > > > > > > > + dev_priv->suspended_to_idle = suspend_to_idle(dev_priv); > > > > > > + > > > > > > return 0; > > > > > > } > > > > > > > > > > > > @@ -842,8 +859,10 @@ static int i915_drm_resume_early(struct drm_device *dev) > > > > > > * FIXME: This should be solved with a special hdmi sink device or > > > > > > * similar so that power domains can be employed. > > > > > > */ > > > > > > - if (pci_enable_device(dev->pdev)) > > > > > > - return -EIO; > > > > > > + if (pci_enable_device(dev->pdev)) { > > > > > > + ret = -EIO; > > > > > > + goto out; > > > > > > + } > > > > > > > > > > > > pci_set_master(dev->pdev); > > > > > > > > > > > > @@ -861,7 +880,12 @@ static int i915_drm_resume_early(struct drm_device *dev) > > > > > > hsw_disable_pc8(dev_priv); > > > > > > > > > > > > intel_uncore_sanitize(dev); > > > > > > - intel_power_domains_init_hw(dev_priv, true); > > > > > > + > > > > > > + if (!(dev_priv->suspended_to_idle && dev_priv->csr.dmc_payload)) > > > > > > + intel_power_domains_init_hw(dev_priv, true); > > > > > > > > > > This doesn't work when e.g. system suspend to S3 fails, in which case dmc > > > > > will also survive. We need the pm core to tell us what really happened, > > > > > and Rafael Wyzocki was working on patches for that: > > > > > > > > > > http://lkml.iu.edu/hypermail/linux/kernel/1510.0/04266.html > > > > > > > > > > Specifically we need the pm_suspend/resume_via_firmware helpers. > > > > > Unfortunately these didn't make it into 4.4, so we need to pick Rafael. > > > > > > > > I think it does work. We deactivate the firmware during S3,S4 suspend > > > > in any case and reprogram it during S3,S4 resume, so it doesn't matter > > > > if the firmware contents survive or not. What we want is to ensure that > > > > during suspend-to-idle we don't deactivate the firmware that is we > > > > don't disable DC6 and we can decide that based > > > > on acpi_target_system_state(). > > > > > > If S3 fails we would reprogram the DMC even though there is no need for it. Do > > > we know if that is allowed? Potentially the DMC could be doing something when we > > > reprogram it and put us in an undefined state. On the other hand, one would hope > > > that it's clever enough to put us back in a valid state when starting again. > > > > > > I haven't seen any problems when reprogramming the DMC so I think this fix is ok > > > for now and we can do the proper fix when Rafael's patches land. > > > > We uninitialize the whole display block and put the device into D3 state, I think > > after that the DMC should be inactive. > > Ok so we are fine on an S3 fail either way. > > > > > I noticed now that dc_state_debugmask_memory_up gets reset after S3/S4. I believe > > this is what actually activates the firmware. So how about using this flag to > > decide whether to reprogram the firmware and switch to using the new helpers once > > Rafael added them? > > Yes, it seems to hold off any DC5/6 state transistions until we do a > dc_state_debugmask_memory_up(). But I'm not sure we can trust those bits to be > cleared. Feels a bit like the CSR_PROGRAM(0) check all over again. It's a bit different though: the programming sequence says we need to first program the firmware and then set this flag. From which I assume it shouldn't be set while we are programming it. But yea, it's not too clear in any case in the spec, I will try to clarify it with the FW people. > I'd prefer to > keep the patch as is if ok with you so: > > Reviewed-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> Ok, thanks for the review. > > > > > --Imre > > > > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-11-19 18:12 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-11-18 15:32 [PATCH] drm/i915/skl: enable PC9/10 power states during suspend-to-idle Imre Deak 2015-11-18 16:33 ` Daniel Vetter 2015-11-18 16:44 ` Imre Deak 2015-11-19 13:34 ` Patrik Jakobsson 2015-11-19 14:06 ` Imre Deak 2015-11-19 18:00 ` Patrik Jakobsson 2015-11-19 18:12 ` Imre Deak
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox