[Intel-gfx] [PATCH] drm/i915/skl: enable PC9/10 power states during suspend-to-idle
Imre Deak
imre.deak at intel.com
Thu Nov 19 10:12:47 PST 2015
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 at 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 at linux.intel.com>
Ok, thanks for the review.
>
> >
> > --Imre
> >
> >
More information about the Intel-gfx
mailing list