Re: [PATCH] drm/i915: make system freeze support depend on CONFIG_ACPI_SLEEP

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Jul 08, 2014 at 01:39:23AM +0200, Rafael J. Wysocki wrote:
> On 7/8/2014 1:13 AM, Rafael J. Wysocki wrote:
> >On 6/24/2014 5:12 PM, Imre Deak wrote:
> >>On Tue, 2014-06-24 at 17:53 +0300, Jani Nikula wrote:
> >>>On Tue, 24 Jun 2014, Imre Deak <imre.deak@intel.com> wrote:
> >>>>On Tue, 2014-06-24 at 16:54 +0300, Jani Nikula wrote:
> >>>>>On Mon, 23 Jun 2014, Imre Deak <imre.deak@intel.com> wrote:
> >>>>>>To achieve further power savings during system freeze (aka connected
> >>>>>>standby, or s0ix) we have to send a PCI_D1 opregion notification. As
> >>>>>>the information about the state we're entering (system freeze,
> >>>>>>suspend to ram or suspend to disk) is only available through the
> >>>>>>ACPI
> >>>>>>subsystem, make this support depend on the relevant kconfig option.
> >>>>>>Things will still work if this option isn't set, albeit with
> >>>>>>less than
> >>>>>>optimial power saving.
> >>>>>>
> >>>>>>This also fixes a compile breakage when the option is not set
> >>>>>>introduced
> >>>>>>in
> >>>>>>
> >>>>>>commit e5747e3adcd67ae27105003ec99fb58cba180105
> >>>>>>Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> >>>>>>Date:   Thu Jun 12 08:35:47 2014 -0700
> >>>>>>
> >>>>>>     drm/i915: send proper opregion notifications on suspend/resume
> >>>>>>
> >>>>>>Reported-by: Randy Dunlap <rdunlap@infradead.org>
> >>>>>>Signed-off-by: Imre Deak <imre.deak@intel.com>
> >>>>>>---
> >>>>>>  drivers/gpu/drm/i915/i915_drv.c | 7 ++++---
> >>>>>>  1 file changed, 4 insertions(+), 3 deletions(-)
> >>>>>>
> >>>>>>diff --git a/drivers/gpu/drm/i915/i915_drv.c
> >>>>>>b/drivers/gpu/drm/i915/i915_drv.c
> >>>>>>index 7ae4e2a..43dc8f7 100644
> >>>>>>--- a/drivers/gpu/drm/i915/i915_drv.c
> >>>>>>+++ b/drivers/gpu/drm/i915/i915_drv.c
> >>>>>>@@ -544,10 +544,11 @@ static int i915_drm_freeze(struct
> >>>>>>drm_device *dev)
> >>>>>>        i915_save_state(dev);
> >>>>>>  -    if (acpi_target_system_state() >= ACPI_STATE_S3)
> >>>>>>-        opregion_target_state = PCI_D3cold;
> >>>>>>-    else
> >>>>>>+    opregion_target_state = PCI_D3cold;
> >>>>>>+#if IS_ENABLED(CONFIG_ACPI_SLEEP)
> >>>>>Maybe this should just check for CONFIG_ACPI?
> >>>>I wanted to send the PCI_D1 signal only if we are sure that the target
> >>>>sleep state is S0ix (or S1/2) and fall back to the old behavior to
> >>>>send
> >>>>PCI_D3cold in all other cases.
> >>>>
> >>>>But you are right, it would make much sense if CONFIG_ACPI_SLEEP=n the
> >>>>target state would be always S0ix. Rafael could you confirm this?
> >
> >The target state should be S0 for CONFIG_ACPI_SLEEP unset.
> >
> 
> Actually, no, it shouldn't.  Or rather it depends on why CONFIG_ACPI_SLEEP
> is unset.
> 
> If that's because CONFIG_ACPI is unset, the target sleep state is undefined
> and
> acpi_target_system_state() should not be called then.

Ok, I've merged Imre's original patch to not call acpi_target_system_state
then.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch