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.d...@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.jakobs...@linux.intel.com>
>
> --Imre
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
