[PATCH 1/4] drm/i915: disable power wells on suspend

[PATCH 1/4] drm/i915: disable power wells on suspend

[Jesse Barnes]

From: Kristen Carlson Accardi <kristen@linux.intel.com>

We want to make sure everything is disabled and at its lowest power when
freezing.

Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_drv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e2bfdda..66c6ffb 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -551,6 +551,8 @@ static int i915_drm_freeze(struct drm_device *dev)
 
 	dev_priv->suspend_count++;
 
+	intel_display_set_init_power(dev_priv, false);
+
 	return 0;
 }
 
-- 
1.9.1

[PATCH 2/4] drm/i915: leave rc6 enabled at suspend time

[Jesse Barnes]

From: Kristen Carlson Accardi <kristen@linux.intel.com>

This allows the system to enter the lowest power mode during system freeze.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_drv.c  |  3 ---
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c  | 16 +++++++++++-----
 3 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 66c6ffb..433bdfa 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -521,8 +521,6 @@ static int i915_drm_freeze(struct drm_device *dev)
 		drm_irq_uninstall(dev);
 		dev_priv->enable_hotplug_processing = false;
 
-		intel_disable_gt_powersave(dev);
-
 		/*
 		 * Disable CRTCs directly since we want to preserve sw state
 		 * for _thaw.
@@ -543,7 +541,6 @@ static int i915_drm_freeze(struct drm_device *dev)
 	i915_save_state(dev);
 
 	intel_opregion_fini(dev);
-	intel_uncore_fini(dev);
 
 	console_lock();
 	intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c597b0d..bf90e7d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -956,6 +956,7 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv);
 void intel_init_gt_powersave(struct drm_device *dev);
 void intel_cleanup_gt_powersave(struct drm_device *dev);
 void intel_enable_gt_powersave(struct drm_device *dev);
+void intel_enable_gt_powersave_sync(struct drm_device *dev);
 void intel_disable_gt_powersave(struct drm_device *dev);
 void intel_reset_gt_powersave(struct drm_device *dev);
 void ironlake_teardown_rc6(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 1840d15..8d9e036 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4891,12 +4891,9 @@ void intel_disable_gt_powersave(struct drm_device *dev)
 	}
 }
 
-static void intel_gen6_powersave_work(struct work_struct *work)
+void intel_enable_gt_powersave_sync(struct drm_device *dev)
 {
-	struct drm_i915_private *dev_priv =
-		container_of(work, struct drm_i915_private,
-			     rps.delayed_resume_work.work);
-	struct drm_device *dev = dev_priv->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	mutex_lock(&dev_priv->rps.hw_lock);
 
@@ -4917,6 +4914,15 @@ static void intel_gen6_powersave_work(struct work_struct *work)
 	intel_runtime_pm_put(dev_priv);
 }
 
+static void intel_gen6_powersave_work(struct work_struct *work)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(work, struct drm_i915_private,
+			     rps.delayed_resume_work.work);
+
+	intel_enable_gt_powersave_sync(dev_priv->dev);
+}
+
 void intel_enable_gt_powersave(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-- 
1.9.1

[PATCH 3/4] drm/i915: send proper opregion notifications on suspend/resume

[Jesse Barnes]

From: Kristen Carlson Accardi <kristen@linux.intel.com>

This indicates to the firmware that it can power down various other
components or bring them back up, depending on the target system state.

Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/acpi/sleep.c            |  1 +
 drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index c40fb2e..807f333 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -89,6 +89,7 @@ u32 acpi_target_system_state(void)
 {
 	return acpi_target_sleep_state;
 }
+EXPORT_SYMBOL(acpi_target_system_state);
 
 static bool pwr_btn_event_pending;
 
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 433bdfa..b6211d7 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -28,6 +28,7 @@
  */
 
 #include <linux/device.h>
+#include <linux/acpi.h>
 #include <drm/drmP.h>
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
@@ -491,6 +492,7 @@ static int i915_drm_freeze(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
+	pci_power_t opregion_target_state;
 
 	intel_runtime_pm_get(dev_priv);
 
@@ -540,6 +542,12 @@ 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_D1;
+	intel_opregion_notify_adapter(dev, opregion_target_state);
+
 	intel_opregion_fini(dev);
 
 	console_lock();
@@ -671,6 +679,8 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
 	dev_priv->modeset_restore = MODESET_DONE;
 	mutex_unlock(&dev_priv->modeset_restore_lock);
 
+	intel_opregion_notify_adapter(dev, PCI_D0);
+
 	intel_runtime_pm_put(dev_priv);
 	return 0;
 }
-- 
1.9.1

[PATCH 4/4] drm/i915: make sure PC8 is enabled on suspend and disabled on resume

[Jesse Barnes]

From: Kristen Carlson Accardi <kristen@linux.intel.com>

This matches the runtime suspend paths and allows the system to enter
the lowest power mode at freeze time.

Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_drv.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index b6211d7..24dc856 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -558,6 +558,9 @@ static int i915_drm_freeze(struct drm_device *dev)
 
 	intel_display_set_init_power(dev_priv, false);
 
+	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
+		hsw_enable_pc8(dev_priv);
+
 	return 0;
 }
 
@@ -618,6 +621,9 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
+		hsw_disable_pc8(dev_priv);
+
 	if (drm_core_check_feature(dev, DRIVER_MODESET) &&
 	    restore_gtt_mappings) {
 		mutex_lock(&dev->struct_mutex);
-- 
1.9.1

[PATCH 3/5] ACPI: export target system state for use by drivers

[Jesse Barnes]

From: Kristen Carlson Accardi <kristen@linux.intel.com>

Needed in ->freeze routines to figure out the target system state and
take appropriate action.

v2: split out ACPI patch

Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/acpi/sleep.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index c40fb2e..807f333 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -89,6 +89,7 @@ u32 acpi_target_system_state(void)
 {
 	return acpi_target_sleep_state;
 }
+EXPORT_SYMBOL(acpi_target_system_state);
 
 static bool pwr_btn_event_pending;
 
-- 
1.9.1

[PATCH 4/5] drm/i915: send proper opregion notifications on suspend/resume

[Jesse Barnes]

This indicates to the firmware that it can power down various other
components or bring them back up, depending on the target system state.

Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 433bdfa..b6211d7 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -28,6 +28,7 @@
  */
 
 #include <linux/device.h>
+#include <linux/acpi.h>
 #include <drm/drmP.h>
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
@@ -491,6 +492,7 @@ static int i915_drm_freeze(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
+	pci_power_t opregion_target_state;
 
 	intel_runtime_pm_get(dev_priv);
 
@@ -540,6 +542,12 @@ 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_D1;
+	intel_opregion_notify_adapter(dev, opregion_target_state);
+
 	intel_opregion_fini(dev);
 
 	console_lock();
@@ -671,6 +679,8 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
 	dev_priv->modeset_restore = MODESET_DONE;
 	mutex_unlock(&dev_priv->modeset_restore_lock);
 
+	intel_opregion_notify_adapter(dev, PCI_D0);
+
 	intel_runtime_pm_put(dev_priv);
 	return 0;
 }
-- 
1.9.1

Re: [PATCH 3/5] ACPI: export target system state for use by drivers

[Rafael J. Wysocki]

On Thursday, May 29, 2014 02:48:44 PM Jesse Barnes wrote:
> From: Kristen Carlson Accardi <kristen@linux.intel.com>
> 
> Needed in ->freeze routines to figure out the target system state and
> take appropriate action.
> 
> v2: split out ACPI patch
> 
> Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

This is in my linux-next branch already.

I can put out a separate branch with it for you to pull from if
necessary.

Rafael


> ---
>  drivers/acpi/sleep.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index c40fb2e..807f333 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -89,6 +89,7 @@ u32 acpi_target_system_state(void)
>  {
>  	return acpi_target_sleep_state;
>  }
> +EXPORT_SYMBOL(acpi_target_system_state);
>  
>  static bool pwr_btn_event_pending;
>  
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/4] drm/i915: disable power wells on suspend

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 893 bytes --]

On Thu, 2014-05-29 at 14:11 -0700, Jesse Barnes wrote:
> From: Kristen Carlson Accardi <kristen@linux.intel.com>
> 
> We want to make sure everything is disabled and at its lowest power when
> freezing.
> 
> Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Looks ok to me:
Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e2bfdda..66c6ffb 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -551,6 +551,8 @@ static int i915_drm_freeze(struct drm_device *dev)
>  
>  	dev_priv->suspend_count++;
>  
> +	intel_display_set_init_power(dev_priv, false);
> +
>  	return 0;
>  }
>  


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915: leave rc6 enabled at suspend time

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 3897 bytes --]

On Thu, 2014-05-29 at 14:11 -0700, Jesse Barnes wrote:
> From: Kristen Carlson Accardi <kristen@linux.intel.com>
> 
> This allows the system to enter the lowest power mode during system freeze.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.c  |  3 ---
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_pm.c  | 16 +++++++++++-----
>  3 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 66c6ffb..433bdfa 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -521,8 +521,6 @@ static int i915_drm_freeze(struct drm_device *dev)
>  		drm_irq_uninstall(dev);
>  		dev_priv->enable_hotplug_processing = false;
>  
> -		intel_disable_gt_powersave(dev);
> -

I wonder what was the reason for this call. One possibility is that
i915_save_state() depends on it to save the correct registers, but it
would be good to clarify this.

It also cancels some deferred works which we do need here. But we could
also add that to intel_enable_gt_powersave_sync() in this patch.

>  		/*
>  		 * Disable CRTCs directly since we want to preserve sw state
>  		 * for _thaw.
> @@ -543,7 +541,6 @@ static int i915_drm_freeze(struct drm_device *dev)
>  	i915_save_state(dev);
>  
>  	intel_opregion_fini(dev);
> -	intel_uncore_fini(dev);

Some stuff in the above call is unrelated to this patch, like
intel_uncore_forcewake_reset(). At least canceling force_wake_timer
there seems to be needed here. In any case it would be better to have
the above two changes in a separate patch.

With that fixed this patch looks ok to me. The original patch was from
me, so fwiw:
Reviewed-by: Imre Deak <imre.deak@intel.com>
 
>  	console_lock();
>  	intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index c597b0d..bf90e7d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -956,6 +956,7 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv);
>  void intel_init_gt_powersave(struct drm_device *dev);
>  void intel_cleanup_gt_powersave(struct drm_device *dev);
>  void intel_enable_gt_powersave(struct drm_device *dev);
> +void intel_enable_gt_powersave_sync(struct drm_device *dev);
>  void intel_disable_gt_powersave(struct drm_device *dev);
>  void intel_reset_gt_powersave(struct drm_device *dev);
>  void ironlake_teardown_rc6(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 1840d15..8d9e036 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4891,12 +4891,9 @@ void intel_disable_gt_powersave(struct drm_device *dev)
>  	}
>  }
>  
> -static void intel_gen6_powersave_work(struct work_struct *work)
> +void intel_enable_gt_powersave_sync(struct drm_device *dev)
>  {
> -	struct drm_i915_private *dev_priv =
> -		container_of(work, struct drm_i915_private,
> -			     rps.delayed_resume_work.work);
> -	struct drm_device *dev = dev_priv->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  
>  	mutex_lock(&dev_priv->rps.hw_lock);
>  
> @@ -4917,6 +4914,15 @@ static void intel_gen6_powersave_work(struct work_struct *work)
>  	intel_runtime_pm_put(dev_priv);
>  }
>  
> +static void intel_gen6_powersave_work(struct work_struct *work)
> +{
> +	struct drm_i915_private *dev_priv =
> +		container_of(work, struct drm_i915_private,
> +			     rps.delayed_resume_work.work);
> +
> +	intel_enable_gt_powersave_sync(dev_priv->dev);
> +}
> +
>  void intel_enable_gt_powersave(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: make sure PC8 is enabled on suspend and disabled on resume

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 1546 bytes --]

On Thu, 2014-05-29 at 14:11 -0700, Jesse Barnes wrote:
> From: Kristen Carlson Accardi <kristen@linux.intel.com>
> 
> This matches the runtime suspend paths and allows the system to enter
> the lowest power mode at freeze time.
> 
> Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index b6211d7..24dc856 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -558,6 +558,9 @@ static int i915_drm_freeze(struct drm_device *dev)
>  
>  	intel_display_set_init_power(dev_priv, false);
>  
> +	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> +		hsw_enable_pc8(dev_priv);
> +
>  	return 0;
>  }
>  
> @@ -618,6 +621,9 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> +	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> +		hsw_disable_pc8(dev_priv);

I would put this before we access any of the HW regs in thaw_early() and
correspondingly the above call to hsw_enable_pc8() to suspend_late()
before we call pci_disable_device().

With that change this is:
Reviewed-by: Imre Deak <imre.deak@intel.com>

> +
>  	if (drm_core_check_feature(dev, DRIVER_MODESET) &&
>  	    restore_gtt_mappings) {
>  		mutex_lock(&dev->struct_mutex);


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: send proper opregion notifications on suspend/resume

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 2491 bytes --]

On Thu, 2014-05-29 at 14:11 -0700, Jesse Barnes wrote:
> From: Kristen Carlson Accardi <kristen@linux.intel.com>
> 
> This indicates to the firmware that it can power down various other
> components or bring them back up, depending on the target system state.
> 
> Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/acpi/sleep.c            |  1 +
>  drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index c40fb2e..807f333 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -89,6 +89,7 @@ u32 acpi_target_system_state(void)
>  {
>  	return acpi_target_sleep_state;
>  }
> +EXPORT_SYMBOL(acpi_target_system_state);
>  
>  static bool pwr_btn_event_pending;
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 433bdfa..b6211d7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -28,6 +28,7 @@
>   */
>  
>  #include <linux/device.h>
> +#include <linux/acpi.h>
>  #include <drm/drmP.h>
>  #include <drm/i915_drm.h>
>  #include "i915_drv.h"
> @@ -491,6 +492,7 @@ static int i915_drm_freeze(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *crtc;
> +	pci_power_t opregion_target_state;
>  
>  	intel_runtime_pm_get(dev_priv);
>  
> @@ -540,6 +542,12 @@ 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_D1;
> +	intel_opregion_notify_adapter(dev, opregion_target_state);
> +
>  	intel_opregion_fini(dev);
>  
>  	console_lock();
> @@ -671,6 +679,8 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
>  	dev_priv->modeset_restore = MODESET_DONE;
>  	mutex_unlock(&dev_priv->modeset_restore_lock);
>  
> +	intel_opregion_notify_adapter(dev, PCI_D0);
> +

This could be moved after intel_opregion_init() just for clarity, but
I'm also fine keeping it here.

This patch depends on Rafael's change in his PM tree to export
acpi_target_system_state(), other than that this is:
Reviewed-by: Imre Deak <imre.deak@intel.com>
 
>  	intel_runtime_pm_put(dev_priv);
>  	return 0;
>  }


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915: leave rc6 enabled at suspend time

[Jesse Barnes]

On Fri, 30 May 2014 15:54:37 +0300
Imre Deak <imre.deak@intel.com> wrote:

> On Thu, 2014-05-29 at 14:11 -0700, Jesse Barnes wrote:
> > From: Kristen Carlson Accardi <kristen@linux.intel.com>
> > 
> > This allows the system to enter the lowest power mode during system freeze.
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c  |  3 ---
> >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> >  drivers/gpu/drm/i915/intel_pm.c  | 16 +++++++++++-----
> >  3 files changed, 12 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 66c6ffb..433bdfa 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -521,8 +521,6 @@ static int i915_drm_freeze(struct drm_device *dev)
> >  		drm_irq_uninstall(dev);
> >  		dev_priv->enable_hotplug_processing = false;
> >  
> > -		intel_disable_gt_powersave(dev);
> > -
> 
> I wonder what was the reason for this call. One possibility is that
> i915_save_state() depends on it to save the correct registers, but it
> would be good to clarify this.
> 
> It also cancels some deferred works which we do need here. But we could
> also add that to intel_enable_gt_powersave_sync() in this patch.

Yeah I was worried about that too, but then we do the reset on resume
anyway, and I didn't see anything in my logs in testing...

But I can split that out if there's a reason to.  Seems like we do a
bit too much teardown at suspend these days (like tearing down opregion
state), I'd like to trim it back if possible and share between runtime
and system suspend/freeze.

I'll look into the forcewake bits.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/4] drm/i915: leave rc6 enabled at suspend time

[Chris Wilson]

On Fri, May 30, 2014 at 08:32:20AM -0700, Jesse Barnes wrote:
> But I can split that out if there's a reason to.  Seems like we do a
> bit too much teardown at suspend these days (like tearing down opregion
> state), I'd like to trim it back if possible and share between runtime
> and system suspend/freeze.

I guess the answer is to move all the paranoid bits from suspend into
hibernate-resume.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/4] drm/i915: leave rc6 enabled at suspend time

[Jesse Barnes]

On Fri, 30 May 2014 16:40:27 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Fri, May 30, 2014 at 08:32:20AM -0700, Jesse Barnes wrote:
> > But I can split that out if there's a reason to.  Seems like we do a
> > bit too much teardown at suspend these days (like tearing down opregion
> > state), I'd like to trim it back if possible and share between runtime
> > and system suspend/freeze.
> 
> I guess the answer is to move all the paranoid bits from suspend into
> hibernate-resume.

Well and probably move a bunch of stuff out of _freeze and into a
shared _suspend function for use at runtime suspend, suspend, and
hibernate.

Separate patches for all of that though so all the breakage is easily
bisected. :)

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 4/4] drm/i915: make sure PC8 is enabled on suspend and disabled on resume

[Jesse Barnes]

On Fri, 30 May 2014 16:37:53 +0300
Imre Deak <imre.deak@intel.com> wrote:

> On Thu, 2014-05-29 at 14:11 -0700, Jesse Barnes wrote:
> > From: Kristen Carlson Accardi <kristen@linux.intel.com>
> > 
> > This matches the runtime suspend paths and allows the system to enter
> > the lowest power mode at freeze time.
> > 
> > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index b6211d7..24dc856 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -558,6 +558,9 @@ static int i915_drm_freeze(struct drm_device *dev)
> >  
> >  	intel_display_set_init_power(dev_priv, false);
> >  
> > +	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > +		hsw_enable_pc8(dev_priv);
> > +
> >  	return 0;
> >  }
> >  
> > @@ -618,6 +621,9 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  
> > +	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > +		hsw_disable_pc8(dev_priv);
> 
> I would put this before we access any of the HW regs in thaw_early() and
> correspondingly the above call to hsw_enable_pc8() to suspend_late()
> before we call pci_disable_device().
> 
> With that change this is:
> Reviewed-by: Imre Deak <imre.deak@intel.com>

For the thaw side I think that makes sense.

But for the freeze side, putting it in suspend_late won't get us the
freeze behavior we want.  I think Rafael and Kristen are thinking of
re-using the freeze infrastructure for some kind of connected standby
feature, where most stuff is frozen, but the system isn't in S3 or S4,
so we need the enable_pc8 call in the _freeze path as well.

Rafael, is that correct?

I'll add a late_freeze and put it there instead, so it doesn't pollute
the S3 suspend path.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH] drm/i915: make sure PC8 is enabled on suspend and disabled on resume v2

[Jesse Barnes]

From: Kristen Carlson Accardi <kristen@linux.intel.com>

This matches the runtime suspend paths and allows the system to enter
the lowest power mode at freeze time.

v2: move disable_pc8 call to thaw_early (Imre)
    move enable_pc8 to freeze_late (Imre/Jesse)

Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_drv.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index a573f5a..0a8925e 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -559,6 +559,9 @@ static int i915_drm_freeze(struct drm_device *dev)
 
 	intel_display_set_init_power(dev_priv, false);
 
+	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
+		hsw_enable_pc8(dev_priv);
+
 	return 0;
 }
 
@@ -608,6 +611,9 @@ static int i915_drm_thaw_early(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
+		hsw_disable_pc8(dev_priv);
+
 	intel_uncore_early_sanitize(dev);
 	intel_uncore_sanitize(dev);
 	intel_power_domains_init_hw(dev_priv);
@@ -939,6 +945,21 @@ static int i915_pm_freeze(struct device *dev)
 	return i915_drm_freeze(drm_dev);
 }
 
+static int i915_pm_freeze_late(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct drm_device *drm_dev = pci_get_drvdata(pdev);
+	struct drm_i915_private *dev_priv = drm_dev->dev_private;
+
+	if (IS_HASWELL(drm_dev) || IS_BROADWELL(drm_dev))
+		hsw_enable_pc8(dev_priv);
+
+	pci_disable_device(pdev);
+	pci_set_power_state(pdev, PCI_D3hot);
+
+	return 0;
+}
+
 static int i915_pm_thaw_early(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
@@ -1476,6 +1497,7 @@ static const struct dev_pm_ops i915_pm_ops = {
 	.resume_early = i915_pm_resume_early,
 	.resume = i915_pm_resume,
 	.freeze = i915_pm_freeze,
+	.freeze_late = i915_pm_freeze_late,
 	.thaw_early = i915_pm_thaw_early,
 	.thaw = i915_pm_thaw,
 	.poweroff = i915_pm_poweroff,
-- 
1.9.1

[PATCH] drm/i915: leave rc6 enabled at suspend time v2

[Jesse Barnes]

From: Kristen Carlson Accardi <kristen@linux.intel.com>

This allows the system to enter the lowest power mode during system freeze.

v2: delete force wake timer at suspend (Imre)

Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_drv.c  |  4 +---
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c  | 16 +++++++++++-----
 3 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 66c6ffb..c65fc68 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -521,8 +521,6 @@ static int i915_drm_freeze(struct drm_device *dev)
 		drm_irq_uninstall(dev);
 		dev_priv->enable_hotplug_processing = false;
 
-		intel_disable_gt_powersave(dev);
-
 		/*
 		 * Disable CRTCs directly since we want to preserve sw state
 		 * for _thaw.
@@ -542,8 +540,8 @@ static int i915_drm_freeze(struct drm_device *dev)
 
 	i915_save_state(dev);
 
+	del_timer_sync(&dev_priv->uncore.force_wake_timer);
 	intel_opregion_fini(dev);
-	intel_uncore_fini(dev);
 
 	console_lock();
 	intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c597b0d..bf90e7d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -956,6 +956,7 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv);
 void intel_init_gt_powersave(struct drm_device *dev);
 void intel_cleanup_gt_powersave(struct drm_device *dev);
 void intel_enable_gt_powersave(struct drm_device *dev);
+void intel_enable_gt_powersave_sync(struct drm_device *dev);
 void intel_disable_gt_powersave(struct drm_device *dev);
 void intel_reset_gt_powersave(struct drm_device *dev);
 void ironlake_teardown_rc6(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 1840d15..8d9e036 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4891,12 +4891,9 @@ void intel_disable_gt_powersave(struct drm_device *dev)
 	}
 }
 
-static void intel_gen6_powersave_work(struct work_struct *work)
+void intel_enable_gt_powersave_sync(struct drm_device *dev)
 {
-	struct drm_i915_private *dev_priv =
-		container_of(work, struct drm_i915_private,
-			     rps.delayed_resume_work.work);
-	struct drm_device *dev = dev_priv->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	mutex_lock(&dev_priv->rps.hw_lock);
 
@@ -4917,6 +4914,15 @@ static void intel_gen6_powersave_work(struct work_struct *work)
 	intel_runtime_pm_put(dev_priv);
 }
 
+static void intel_gen6_powersave_work(struct work_struct *work)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(work, struct drm_i915_private,
+			     rps.delayed_resume_work.work);
+
+	intel_enable_gt_powersave_sync(dev_priv->dev);
+}
+
 void intel_enable_gt_powersave(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-- 
1.9.1

[PATCH] drm/i915: make sure PC8 is enabled on suspend and disabled on resume v3

[Jesse Barnes]

From: Kristen Carlson Accardi <kristen@linux.intel.com>

This matches the runtime suspend paths and allows the system to enter
the lowest power mode at freeze time.

v2: move disable_pc8 call to thaw_early (Imre)
    move enable_pc8 to freeze_late (Imre/Jesse)
v3: drop spurious hunk from _freeze now that we have freeze_late (Jesse)

Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_drv.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index a573f5a..ff291c0 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -608,6 +608,9 @@ static int i915_drm_thaw_early(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
+		hsw_disable_pc8(dev_priv);
+
 	intel_uncore_early_sanitize(dev);
 	intel_uncore_sanitize(dev);
 	intel_power_domains_init_hw(dev_priv);
@@ -939,6 +942,21 @@ static int i915_pm_freeze(struct device *dev)
 	return i915_drm_freeze(drm_dev);
 }
 
+static int i915_pm_freeze_late(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct drm_device *drm_dev = pci_get_drvdata(pdev);
+	struct drm_i915_private *dev_priv = drm_dev->dev_private;
+
+	if (IS_HASWELL(drm_dev) || IS_BROADWELL(drm_dev))
+		hsw_enable_pc8(dev_priv);
+
+	pci_disable_device(pdev);
+	pci_set_power_state(pdev, PCI_D3hot);
+
+	return 0;
+}
+
 static int i915_pm_thaw_early(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
@@ -1476,6 +1494,7 @@ static const struct dev_pm_ops i915_pm_ops = {
 	.resume_early = i915_pm_resume_early,
 	.resume = i915_pm_resume,
 	.freeze = i915_pm_freeze,
+	.freeze_late = i915_pm_freeze_late,
 	.thaw_early = i915_pm_thaw_early,
 	.thaw = i915_pm_thaw,
 	.poweroff = i915_pm_poweroff,
-- 
1.9.1

[PATCH] drm/i915: make sure PC8 is enabled on suspend and disabled on resume v4

[Jesse Barnes]

From: Kristen Carlson Accardi <kristen@linux.intel.com>

This matches the runtime suspend paths and allows the system to enter
the lowest power mode at freeze time.

v2: move disable_pc8 call to thaw_early (Imre)
    move enable_pc8 to freeze_late (Imre/Jesse)
v3: drop spurious hunk from _freeze now that we have freeze_late (Jesse)
v4: move back to suspend_late (Imre was right)

Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_drv.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index a573f5a..2583442 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -608,6 +608,9 @@ static int i915_drm_thaw_early(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
+		hsw_disable_pc8(dev_priv);
+
 	intel_uncore_early_sanitize(dev);
 	intel_uncore_sanitize(dev);
 	intel_power_domains_init_hw(dev_priv);
@@ -891,6 +894,7 @@ static int i915_pm_suspend_late(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct drm_device *drm_dev = pci_get_drvdata(pdev);
+	struct drm_i915_private *dev_priv = drm_dev->dev_private;
 
 	/*
 	 * We have a suspedn ordering issue with the snd-hda driver also
@@ -904,6 +908,9 @@ static int i915_pm_suspend_late(struct device *dev)
 	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
 		return 0;
 
+	if (IS_HASWELL(drm_dev) || IS_BROADWELL(drm_dev))
+		hsw_enable_pc8(dev_priv);
+
 	pci_disable_device(pdev);
 	pci_set_power_state(pdev, PCI_D3hot);
 
-- 
1.9.1

Re: [PATCH 1/4] drm/i915: disable power wells on suspend

[Kristen Carlson Accardi]

On Fri, 30 May 2014 15:48:23 +0300
Imre Deak <imre.deak@intel.com> wrote:

> On Thu, 2014-05-29 at 14:11 -0700, Jesse Barnes wrote:
> > From: Kristen Carlson Accardi <kristen@linux.intel.com>
> > 
> > We want to make sure everything is disabled and at its lowest power when
> > freezing.
> > 
> > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> Looks ok to me:
> Reviewed-by: Imre Deak <imre.deak@intel.com>
> 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index e2bfdda..66c6ffb 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -551,6 +551,8 @@ static int i915_drm_freeze(struct drm_device *dev)
> >  
> >  	dev_priv->suspend_count++;
> >  
> > +	intel_display_set_init_power(dev_priv, false);
> > +
> >  	return 0;
> >  }
> >  
> 

This was actually Imre's patch not mine.

Re: [PATCH 2/4] drm/i915: leave rc6 enabled at suspend time

[Kristen Carlson Accardi]

On Thu, 29 May 2014 14:11:35 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> From: Kristen Carlson Accardi <kristen@linux.intel.com>

Imre is the author.

> 
> This allows the system to enter the lowest power mode during system freeze.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.c  |  3 ---
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_pm.c  | 16 +++++++++++-----
>  3 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 66c6ffb..433bdfa 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -521,8 +521,6 @@ static int i915_drm_freeze(struct drm_device *dev)
>  		drm_irq_uninstall(dev);
>  		dev_priv->enable_hotplug_processing = false;Linux-3.16
>  
> -		intel_disable_gt_powersave(dev);
> -
>  		/*
>  		 * Disable CRTCs directly since we want to preserve sw state
>  		 * for _thaw.
> @@ -543,7 +541,6 @@ static int i915_drm_freeze(struct drm_device *dev)
>  	i915_save_state(dev);
>  
>  	intel_opregion_fini(dev);
> -	intel_uncore_fini(dev);
>  
>  	console_lock();
>  	intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index c597b0d..bf90e7d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -956,6 +956,7 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv);
>  void intel_init_gt_powersave(struct drm_device *dev);
>  void intel_cleanup_gt_powersave(struct drm_device *dev);
>  void intel_enable_gt_powersave(struct drm_device *dev);
> +void intel_enable_gt_powersave_sync(struct drm_device *dev);
>  void intel_disable_gt_powersave(struct drm_device *dev);
>  void intel_reset_gt_powersave(struct drm_device *dev);
>  void ironlake_teardown_rc6(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 1840d15..8d9e036 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4891,12 +4891,9 @@ void intel_disable_gt_powersave(struct drm_device *dev)
>  	}
>  }
>  
> -static void intel_gen6_powersave_work(struct work_struct *work)
> +void intel_enable_gt_powersave_sync(struct drm_device *dev)
>  {
> -	struct drm_i915_private *dev_priv =
> -		container_of(work, struct drm_i915_private,
> -			     rps.delayed_resume_work.work);
> -	struct drm_device *dev = dev_priv->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  
>  	mutex_lock(&dev_priv->rps.hw_lock);
>  
> @@ -4917,6 +4914,15 @@ static void intel_gen6_powersave_work(struct work_struct *work)
>  	intel_runtime_pm_put(dev_priv);
>  }
>  
> +static void intel_gen6_powersave_work(struct work_struct *work)
> +{
> +	struct drm_i915_private *dev_priv =
> +		container_of(work, struct drm_i915_private,
> +			     rps.delayed_resume_work.work);
> +
> +	intel_enable_gt_powersave_sync(dev_priv->dev);
> +}
> +
>  void intel_enable_gt_powersave(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;

Re: [PATCH 3/4] drm/i915: send proper opregion notifications on suspend/resume

[Kristen Carlson Accardi]

On Thu, 29 May 2014 14:11:36 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> From: Kristen Carlson Accardi <kristen@linux.intel.com>

This one was based on a patch from Imre - I just added the D0 on
the resume path.

> 
> This indicates to the firmware that it can power down various other
> components or bring them back up, depending on the target system state.
> 
> Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/acpi/sleep.c            |  1 +
>  drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index c40fb2e..807f333 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -89,6 +89,7 @@ u32 acpi_target_system_state(void)
>  {
>  	return acpi_target_sleep_state;
>  }
> +EXPORT_SYMBOL(acpi_target_system_state);
>  
>  static bool pwr_btn_event_pending;
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 433bdfa..b6211d7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -28,6 +28,7 @@
>   */
>  
>  #include <linux/device.h>
> +#include <linux/acpi.h>
>  #include <drm/drmP.h>
>  #include <drm/i915_drm.h>
>  #include "i915_drv.h"
> @@ -491,6 +492,7 @@ static int i915_drm_freeze(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *crtc;
> +	pci_power_t opregion_target_state;
>  
>  	intel_runtime_pm_get(dev_priv);
>  
> @@ -540,6 +542,12 @@ 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_D1;
> +	intel_opregion_notify_adapter(dev, opregion_target_state);
> +
>  	intel_opregion_fini(dev);
>  
>  	console_lock();
> @@ -671,6 +679,8 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
>  	dev_priv->modeset_restore = MODESET_DONE;
>  	mutex_unlock(&dev_priv->modeset_restore_lock);
>  
> +	intel_opregion_notify_adapter(dev, PCI_D0);
> +
>  	intel_runtime_pm_put(dev_priv);
>  	return 0;
>  }

Re: [PATCH 4/4] drm/i915: make sure PC8 is enabled on suspend and disabled on resume

[Jesse Barnes]

On Fri, 30 May 2014 23:12:45 +0200
"Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:

> On Friday, May 30, 2014 11:29:15 AM Jesse Barnes wrote:
> > On Fri, 30 May 2014 16:37:53 +0300
> > Imre Deak <imre.deak@intel.com> wrote:
> > 
> > > On Thu, 2014-05-29 at 14:11 -0700, Jesse Barnes wrote:
> > > > From: Kristen Carlson Accardi <kristen@linux.intel.com>
> > > > 
> > > > This matches the runtime suspend paths and allows the system to enter
> > > > the lowest power mode at freeze time.
> > > > 
> > > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.c | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > > index b6211d7..24dc856 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > @@ -558,6 +558,9 @@ static int i915_drm_freeze(struct drm_device *dev)
> > > >  
> > > >  	intel_display_set_init_power(dev_priv, false);
> > > >  
> > > > +	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > > > +		hsw_enable_pc8(dev_priv);
> > > > +
> > > >  	return 0;
> > > >  }
> > > >  
> > > > @@ -618,6 +621,9 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
> > > >  {
> > > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > >  
> > > > +	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > > > +		hsw_disable_pc8(dev_priv);
> > > 
> > > I would put this before we access any of the HW regs in thaw_early() and
> > > correspondingly the above call to hsw_enable_pc8() to suspend_late()
> > > before we call pci_disable_device().
> > > 
> > > With that change this is:
> > > Reviewed-by: Imre Deak <imre.deak@intel.com>
> > 
> > For the thaw side I think that makes sense.
> > 
> > But for the freeze side, putting it in suspend_late won't get us the
> > freeze behavior we want.  I think Rafael and Kristen are thinking of
> > re-using the freeze infrastructure for some kind of connected standby
> > feature, where most stuff is frozen, but the system isn't in S3 or S4,
> > so we need the enable_pc8 call in the _freeze path as well.
> > 
> > Rafael, is that correct?
> 
> No, it isn't.  The .freeze()/.thaw() callbacks are hibernation-specific.
> There are no plans for using this in PM beyond hibernation.
> 
> What we're going to use are .suspend/_late/_noirq() and the corresponding
> resume callbacks and runtime PM.
> 
> > I'll add a late_freeze and put it there instead, so it doesn't pollute
> > the S3 suspend path.
> 
> The freeze/thaw stuff need not do any PM BTW, it just needs to quiesce the
> device to prevent it from doing DMA etc and then bring it back to life.

Ok thanks.  Kristen corrected me on IRC too.  The latest patch I sent
should do what we want then, now that I've removed the freeze_late
function and put our PC8 enable in suspend_late like Imre suggested
initially.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 4/4] drm/i915: make sure PC8 is enabled on suspend and disabled on resume

[Rafael J. Wysocki]

On Friday, May 30, 2014 11:29:15 AM Jesse Barnes wrote:
> On Fri, 30 May 2014 16:37:53 +0300
> Imre Deak <imre.deak@intel.com> wrote:
> 
> > On Thu, 2014-05-29 at 14:11 -0700, Jesse Barnes wrote:
> > > From: Kristen Carlson Accardi <kristen@linux.intel.com>
> > > 
> > > This matches the runtime suspend paths and allows the system to enter
> > > the lowest power mode at freeze time.
> > > 
> > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > index b6211d7..24dc856 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -558,6 +558,9 @@ static int i915_drm_freeze(struct drm_device *dev)
> > >  
> > >  	intel_display_set_init_power(dev_priv, false);
> > >  
> > > +	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > > +		hsw_enable_pc8(dev_priv);
> > > +
> > >  	return 0;
> > >  }
> > >  
> > > @@ -618,6 +621,9 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
> > >  {
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > >  
> > > +	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > > +		hsw_disable_pc8(dev_priv);
> > 
> > I would put this before we access any of the HW regs in thaw_early() and
> > correspondingly the above call to hsw_enable_pc8() to suspend_late()
> > before we call pci_disable_device().
> > 
> > With that change this is:
> > Reviewed-by: Imre Deak <imre.deak@intel.com>
> 
> For the thaw side I think that makes sense.
> 
> But for the freeze side, putting it in suspend_late won't get us the
> freeze behavior we want.  I think Rafael and Kristen are thinking of
> re-using the freeze infrastructure for some kind of connected standby
> feature, where most stuff is frozen, but the system isn't in S3 or S4,
> so we need the enable_pc8 call in the _freeze path as well.
> 
> Rafael, is that correct?

No, it isn't.  The .freeze()/.thaw() callbacks are hibernation-specific.
There are no plans for using this in PM beyond hibernation.

What we're going to use are .suspend/_late/_noirq() and the corresponding
resume callbacks and runtime PM.

> I'll add a late_freeze and put it there instead, so it doesn't pollute
> the S3 suspend path.

The freeze/thaw stuff need not do any PM BTW, it just needs to quiesce the
device to prevent it from doing DMA etc and then bring it back to life.

Rafael

Re: [PATCH 2/4] drm/i915: leave rc6 enabled at suspend time

[Daniel Vetter]

On Fri, May 30, 2014 at 08:32:20AM -0700, Jesse Barnes wrote:
> On Fri, 30 May 2014 15:54:37 +0300
> Imre Deak <imre.deak@intel.com> wrote:
> 
> > On Thu, 2014-05-29 at 14:11 -0700, Jesse Barnes wrote:
> > > From: Kristen Carlson Accardi <kristen@linux.intel.com>
> > > 
> > > This allows the system to enter the lowest power mode during system freeze.
> > > 
> > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.c  |  3 ---
> > >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> > >  drivers/gpu/drm/i915/intel_pm.c  | 16 +++++++++++-----
> > >  3 files changed, 12 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > index 66c6ffb..433bdfa 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -521,8 +521,6 @@ static int i915_drm_freeze(struct drm_device *dev)
> > >  		drm_irq_uninstall(dev);
> > >  		dev_priv->enable_hotplug_processing = false;
> > >  
> > > -		intel_disable_gt_powersave(dev);
> > > -
> > 
> > I wonder what was the reason for this call. One possibility is that
> > i915_save_state() depends on it to save the correct registers, but it
> > would be good to clarify this.

save_state needs to die. Pretty much because it's fragile like you've just
pointed out.

> > It also cancels some deferred works which we do need here. But we could
> > also add that to intel_enable_gt_powersave_sync() in this patch.
> 
> Yeah I was worried about that too, but then we do the reset on resume
> anyway, and I didn't see anything in my logs in testing...
> 
> But I can split that out if there's a reason to.  Seems like we do a
> bit too much teardown at suspend these days (like tearing down opregion
> state), I'd like to trim it back if possible and share between runtime
> and system suspend/freeze.

Yeah, that's the direction I'm pushing towards, too - we should only stop
timers, work, interrupts and stuff like that, but never tear down
structures. So if you can use this opportunity to fix a few of the
offenders (like opregion) I'd be very happy.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/4] drm/i915: make sure PC8 is enabled on suspend and disabled on resume

[Daniel Vetter]

On Thu, May 29, 2014 at 02:11:37PM -0700, Jesse Barnes wrote:
> From: Kristen Carlson Accardi <kristen@linux.intel.com>
> 
> This matches the runtime suspend paths and allows the system to enter
> the lowest power mode at freeze time.
> 
> Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

pc8 is fully subsumed into runtime pm by now. Do we _really_ still need
this?
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index b6211d7..24dc856 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -558,6 +558,9 @@ static int i915_drm_freeze(struct drm_device *dev)
>  
>  	intel_display_set_init_power(dev_priv, false);
>  
> +	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> +		hsw_enable_pc8(dev_priv);
> +
>  	return 0;
>  }
>  
> @@ -618,6 +621,9 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> +	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> +		hsw_disable_pc8(dev_priv);
> +
>  	if (drm_core_check_feature(dev, DRIVER_MODESET) &&
>  	    restore_gtt_mappings) {
>  		mutex_lock(&dev->struct_mutex);
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/4] drm/i915: make sure PC8 is enabled on suspend and disabled on resume

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 1850 bytes --]

On Mon, 2014-06-02 at 10:45 +0200, Daniel Vetter wrote:
> On Thu, May 29, 2014 at 02:11:37PM -0700, Jesse Barnes wrote:
> > From: Kristen Carlson Accardi <kristen@linux.intel.com>
> > 
> > This matches the runtime suspend paths and allows the system to enter
> > the lowest power mode at freeze time.
> > 
> > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> pc8 is fully subsumed into runtime pm by now. Do we _really_ still need
> this?

Yes, since the system suspend/resume handlers are called with an RPM ref
held and thus PC8 disabled.

--Imre

> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index b6211d7..24dc856 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -558,6 +558,9 @@ static int i915_drm_freeze(struct drm_device *dev)
> >  
> >  	intel_display_set_init_power(dev_priv, false);
> >  
> > +	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > +		hsw_enable_pc8(dev_priv);
> > +
> >  	return 0;
> >  }
> >  
> > @@ -618,6 +621,9 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  
> > +	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > +		hsw_disable_pc8(dev_priv);
> > +
> >  	if (drm_core_check_feature(dev, DRIVER_MODESET) &&
> >  	    restore_gtt_mappings) {
> >  		mutex_lock(&dev->struct_mutex);
> > -- 
> > 1.9.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: make sure PC8 is enabled on suspend and disabled on resume

[Daniel Vetter]

On Mon, Jun 02, 2014 at 02:37:35PM +0300, Imre Deak wrote:
> On Mon, 2014-06-02 at 10:45 +0200, Daniel Vetter wrote:
> > On Thu, May 29, 2014 at 02:11:37PM -0700, Jesse Barnes wrote:
> > > From: Kristen Carlson Accardi <kristen@linux.intel.com>
> > > 
> > > This matches the runtime suspend paths and allows the system to enter
> > > the lowest power mode at freeze time.
> > > 
> > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > 
> > pc8 is fully subsumed into runtime pm by now. Do we _really_ still need
> > this?
> 
> Yes, since the system suspend/resume handlers are called with an RPM ref
> held and thus PC8 disabled.

But doesn't patch 1 try to fix that? Imo we should have this here but
instead go through highl-level the runtime pm functions to shut off the
chip on all platforms.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/4] drm/i915: make sure PC8 is enabled on suspend and disabled on resume

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 1428 bytes --]

On Mon, 2014-06-02 at 17:32 +0200, Daniel Vetter wrote:
> On Mon, Jun 02, 2014 at 02:37:35PM +0300, Imre Deak wrote:
> > On Mon, 2014-06-02 at 10:45 +0200, Daniel Vetter wrote:
> > > On Thu, May 29, 2014 at 02:11:37PM -0700, Jesse Barnes wrote:
> > > > From: Kristen Carlson Accardi <kristen@linux.intel.com>
> > > > 
> > > > This matches the runtime suspend paths and allows the system to enter
> > > > the lowest power mode at freeze time.
> > > > 
> > > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > 
> > > pc8 is fully subsumed into runtime pm by now. Do we _really_ still need
> > > this?
> > 
> > Yes, since the system suspend/resume handlers are called with an RPM ref
> > held and thus PC8 disabled.
> 
> But doesn't patch 1 try to fix that?

That only disables the display side, but we won't disable PC8 until the
RPM suspend handler is called. And that won't happen because the last
RPM ref is held by the DPM framework for the duration of system
suspend/resume handlers.

> Imo we should have this here but instead go through highl-level the runtime
> pm functions to shut off the chip on all platforms.

After the planned refactoring we could have a low-level function that we
can call both from here and the runtime PM path, but until that happens
we need to do this here directly.

--Imre


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: make sure PC8 is enabled on suspend and disabled on resume

[Daniel Vetter]

On Mon, Jun 02, 2014 at 06:57:18PM +0300, Imre Deak wrote:
> On Mon, 2014-06-02 at 17:32 +0200, Daniel Vetter wrote:
> > On Mon, Jun 02, 2014 at 02:37:35PM +0300, Imre Deak wrote:
> > > On Mon, 2014-06-02 at 10:45 +0200, Daniel Vetter wrote:
> > > > On Thu, May 29, 2014 at 02:11:37PM -0700, Jesse Barnes wrote:
> > > > > From: Kristen Carlson Accardi <kristen@linux.intel.com>
> > > > > 
> > > > > This matches the runtime suspend paths and allows the system to enter
> > > > > the lowest power mode at freeze time.
> > > > > 
> > > > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > > 
> > > > pc8 is fully subsumed into runtime pm by now. Do we _really_ still need
> > > > this?
> > > 
> > > Yes, since the system suspend/resume handlers are called with an RPM ref
> > > held and thus PC8 disabled.
> > 
> > But doesn't patch 1 try to fix that?
> 
> That only disables the display side, but we won't disable PC8 until the
> RPM suspend handler is called. And that won't happen because the last
> RPM ref is held by the DPM framework for the duration of system
> suspend/resume handlers.

Yeah, there's discussion going on to make system suspend be optionally
based upon runtime pm in the core. Atm that's not possible since the
system wakes up everyone to suspend them.

> > Imo we should have this here but instead go through highl-level the runtime
> > pm functions to shut off the chip on all platforms.
> 
> After the planned refactoring we could have a low-level function that we
> can call both from here and the runtime PM path, but until that happens
> we need to do this here directly.

Yeah, that's what I'm actually aiming for - we should be able to call the
runtime pm suspend code from the system suspend code and share pretty much
all the code. The sequence I'm thinking of would be for system suspend:

1. Stop everything we need to stop (gpu, display, rps, ...) to be able to
enter runtime pm.

2. Check for leaked references. This might be tricky because audio.

3. Call runtime pm suspend hooks.

Resume would be the same, but inverted.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/i915: leave rc6 enabled at suspend time v3

[Jesse Barnes]

This allows the system to enter the lowest power mode during system freeze.

v2: delete force wake timer at suspend (Imre)
v3: add GT work suspend function (Imre)

Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_drv.c  |  4 ++--
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c  | 20 ++++++++++++++++++++
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 66c6ffb..4774299 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -521,7 +521,7 @@ static int i915_drm_freeze(struct drm_device *dev)
 		drm_irq_uninstall(dev);
 		dev_priv->enable_hotplug_processing = false;
 
-		intel_disable_gt_powersave(dev);
+		intel_suspend_gt_powersave(dev);
 
 		/*
 		 * Disable CRTCs directly since we want to preserve sw state
@@ -542,8 +542,8 @@ static int i915_drm_freeze(struct drm_device *dev)
 
 	i915_save_state(dev);
 
+	del_timer_sync(&dev_priv->uncore.force_wake_timer);
 	intel_opregion_fini(dev);
-	intel_uncore_fini(dev);
 
 	console_lock();
 	intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c597b0d..74fbe4d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -957,6 +957,7 @@ void intel_init_gt_powersave(struct drm_device *dev);
 void intel_cleanup_gt_powersave(struct drm_device *dev);
 void intel_enable_gt_powersave(struct drm_device *dev);
 void intel_disable_gt_powersave(struct drm_device *dev);
+void intel_suspend_gt_powersave(struct drm_device *dev);
 void intel_reset_gt_powersave(struct drm_device *dev);
 void ironlake_teardown_rc6(struct drm_device *dev);
 void gen6_update_ring_freq(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 1840d15..139eebe 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4864,6 +4864,26 @@ void intel_cleanup_gt_powersave(struct drm_device *dev)
 		valleyview_cleanup_gt_powersave(dev);
 }
 
+/**
+ * intel_suspend_gt_powersave - suspend PM work and helper threads
+ * @dev: drm device
+ *
+ * We don't want to disable RC6 or other features here, we just want
+ * to make sure any work we've queued has finished and won't bother
+ * us while we're suspended.
+ */
+void intel_suspend_gt_powersave(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	/* Interrupts should be disabled already to avoid re-arming. */
+	WARN_ON(dev->irq_enabled);
+
+	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
+
+	cancel_work_sync(&dev_priv->rps.work);
+}
+
 void intel_disable_gt_powersave(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-- 
1.9.1

Re: [PATCH] drm/i915: make sure PC8 is enabled on suspend and disabled on resume v4

[Imre Deak]

On Fri, 2014-05-30 at 11:48 -0700, Jesse Barnes wrote:
> From: Kristen Carlson Accardi <kristen@linux.intel.com>
> 
> This matches the runtime suspend paths and allows the system to enter
> the lowest power mode at freeze time.
> 
> v2: move disable_pc8 call to thaw_early (Imre)
>     move enable_pc8 to freeze_late (Imre/Jesse)
> v3: drop spurious hunk from _freeze now that we have freeze_late (Jesse)
> v4: move back to suspend_late (Imre was right)
> 
> Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index a573f5a..2583442 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -608,6 +608,9 @@ static int i915_drm_thaw_early(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> +	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> +		hsw_disable_pc8(dev_priv);
> +
>  	intel_uncore_early_sanitize(dev);
>  	intel_uncore_sanitize(dev);
>  	intel_power_domains_init_hw(dev_priv);
> @@ -891,6 +894,7 @@ static int i915_pm_suspend_late(struct device *dev)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
>  	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> +	struct drm_i915_private *dev_priv = drm_dev->dev_private;
>  
>  	/*
>  	 * We have a suspedn ordering issue with the snd-hda driver also
> @@ -904,6 +908,9 @@ static int i915_pm_suspend_late(struct device *dev)
>  	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
>  		return 0;
>  
> +	if (IS_HASWELL(drm_dev) || IS_BROADWELL(drm_dev))
> +		hsw_enable_pc8(dev_priv);
> +
>  	pci_disable_device(pdev);
>  	pci_set_power_state(pdev, PCI_D3hot);
>  

[PATCH] drm/i915: leave rc6 enabled at suspend time v4

[Jesse Barnes]

This allows the system to enter the lowest power mode during system freeze.

v2: delete force wake timer at suspend (Imre)
v3: add GT work suspend function (Imre)
v4: use uncore forcewake reset (Daniel)

Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_drv.c     |  4 ++--
 drivers/gpu/drm/i915/i915_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_drv.h    |  1 +
 drivers/gpu/drm/i915/intel_pm.c     | 20 ++++++++++++++++++++
 drivers/gpu/drm/i915/intel_uncore.c |  2 +-
 5 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 66c6ffb..7148eac 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -521,7 +521,7 @@ static int i915_drm_freeze(struct drm_device *dev)
 		drm_irq_uninstall(dev);
 		dev_priv->enable_hotplug_processing = false;
 
-		intel_disable_gt_powersave(dev);
+		intel_suspend_gt_powersave(dev);
 
 		/*
 		 * Disable CRTCs directly since we want to preserve sw state
@@ -542,8 +542,8 @@ static int i915_drm_freeze(struct drm_device *dev)
 
 	i915_save_state(dev);
 
+	intel_uncore_forcewake_reset(dev, false);
 	intel_opregion_fini(dev);
-	intel_uncore_fini(dev);
 
 	console_lock();
 	intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bea9ab40..89d6b47 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2084,6 +2084,7 @@ extern void intel_uncore_early_sanitize(struct drm_device *dev);
 extern void intel_uncore_init(struct drm_device *dev);
 extern void intel_uncore_check_errors(struct drm_device *dev);
 extern void intel_uncore_fini(struct drm_device *dev);
+extern void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore);
 
 void
 i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c597b0d..74fbe4d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -957,6 +957,7 @@ void intel_init_gt_powersave(struct drm_device *dev);
 void intel_cleanup_gt_powersave(struct drm_device *dev);
 void intel_enable_gt_powersave(struct drm_device *dev);
 void intel_disable_gt_powersave(struct drm_device *dev);
+void intel_suspend_gt_powersave(struct drm_device *dev);
 void intel_reset_gt_powersave(struct drm_device *dev);
 void ironlake_teardown_rc6(struct drm_device *dev);
 void gen6_update_ring_freq(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 1840d15..139eebe 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4864,6 +4864,26 @@ void intel_cleanup_gt_powersave(struct drm_device *dev)
 		valleyview_cleanup_gt_powersave(dev);
 }
 
+/**
+ * intel_suspend_gt_powersave - suspend PM work and helper threads
+ * @dev: drm device
+ *
+ * We don't want to disable RC6 or other features here, we just want
+ * to make sure any work we've queued has finished and won't bother
+ * us while we're suspended.
+ */
+void intel_suspend_gt_powersave(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	/* Interrupts should be disabled already to avoid re-arming. */
+	WARN_ON(dev->irq_enabled);
+
+	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
+
+	cancel_work_sync(&dev_priv->rps.work);
+}
+
 void intel_disable_gt_powersave(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 871c284..741a4e3 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -316,7 +316,7 @@ static void gen6_force_wake_timer(unsigned long arg)
 	intel_runtime_pm_put(dev_priv);
 }
 
-static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
+void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long irqflags;
-- 
1.9.1

Re: [PATCH] drm/i915: leave rc6 enabled at suspend time v4

[Daniel Vetter]

On Wed, Jun 04, 2014 at 01:45:22PM -0700, Jesse Barnes wrote:
> This allows the system to enter the lowest power mode during system freeze.
> 
> v2: delete force wake timer at suspend (Imre)
> v3: add GT work suspend function (Imre)
> v4: use uncore forcewake reset (Daniel)
> 
> Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Someone please look at

https://bugs.freedesktop.org/show_bug.cgi?id=78059

Yes, I'm starting to block semi-related feature work on regressions
because we suck that much.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.c     |  4 ++--
>  drivers/gpu/drm/i915/i915_drv.h     |  1 +
>  drivers/gpu/drm/i915/intel_drv.h    |  1 +
>  drivers/gpu/drm/i915/intel_pm.c     | 20 ++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_uncore.c |  2 +-
>  5 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 66c6ffb..7148eac 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -521,7 +521,7 @@ static int i915_drm_freeze(struct drm_device *dev)
>  		drm_irq_uninstall(dev);
>  		dev_priv->enable_hotplug_processing = false;
>  
> -		intel_disable_gt_powersave(dev);
> +		intel_suspend_gt_powersave(dev);
>  
>  		/*
>  		 * Disable CRTCs directly since we want to preserve sw state
> @@ -542,8 +542,8 @@ static int i915_drm_freeze(struct drm_device *dev)
>  
>  	i915_save_state(dev);
>  
> +	intel_uncore_forcewake_reset(dev, false);
>  	intel_opregion_fini(dev);
> -	intel_uncore_fini(dev);
>  
>  	console_lock();
>  	intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index bea9ab40..89d6b47 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2084,6 +2084,7 @@ extern void intel_uncore_early_sanitize(struct drm_device *dev);
>  extern void intel_uncore_init(struct drm_device *dev);
>  extern void intel_uncore_check_errors(struct drm_device *dev);
>  extern void intel_uncore_fini(struct drm_device *dev);
> +extern void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore);
>  
>  void
>  i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index c597b0d..74fbe4d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -957,6 +957,7 @@ void intel_init_gt_powersave(struct drm_device *dev);
>  void intel_cleanup_gt_powersave(struct drm_device *dev);
>  void intel_enable_gt_powersave(struct drm_device *dev);
>  void intel_disable_gt_powersave(struct drm_device *dev);
> +void intel_suspend_gt_powersave(struct drm_device *dev);
>  void intel_reset_gt_powersave(struct drm_device *dev);
>  void ironlake_teardown_rc6(struct drm_device *dev);
>  void gen6_update_ring_freq(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 1840d15..139eebe 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4864,6 +4864,26 @@ void intel_cleanup_gt_powersave(struct drm_device *dev)
>  		valleyview_cleanup_gt_powersave(dev);
>  }
>  
> +/**
> + * intel_suspend_gt_powersave - suspend PM work and helper threads
> + * @dev: drm device
> + *
> + * We don't want to disable RC6 or other features here, we just want
> + * to make sure any work we've queued has finished and won't bother
> + * us while we're suspended.
> + */
> +void intel_suspend_gt_powersave(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	/* Interrupts should be disabled already to avoid re-arming. */
> +	WARN_ON(dev->irq_enabled);
> +
> +	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> +
> +	cancel_work_sync(&dev_priv->rps.work);
> +}
> +
>  void intel_disable_gt_powersave(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 871c284..741a4e3 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -316,7 +316,7 @@ static void gen6_force_wake_timer(unsigned long arg)
>  	intel_runtime_pm_put(dev_priv);
>  }
>  
> -static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
> +void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	unsigned long irqflags;
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: leave rc6 enabled at suspend time v4

[Jesse Barnes]

On Thu, 5 Jun 2014 11:21:02 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Wed, Jun 04, 2014 at 01:45:22PM -0700, Jesse Barnes wrote:
> > This allows the system to enter the lowest power mode during system freeze.
> > 
> > v2: delete force wake timer at suspend (Imre)
> > v3: add GT work suspend function (Imre)
> > v4: use uncore forcewake reset (Daniel)
> > 
> > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> Someone please look at
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=78059
> 
> Yes, I'm starting to block semi-related feature work on regressions
> because we suck that much.

Chris said a fix was already available...

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: leave rc6 enabled at suspend time v4

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 4797 bytes --]

On Wed, 2014-06-04 at 13:45 -0700, Jesse Barnes wrote:
> This allows the system to enter the lowest power mode during system freeze.
> 
> v2: delete force wake timer at suspend (Imre)
> v3: add GT work suspend function (Imre)
> v4: use uncore forcewake reset (Daniel)
> 
> Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.c     |  4 ++--
>  drivers/gpu/drm/i915/i915_drv.h     |  1 +
>  drivers/gpu/drm/i915/intel_drv.h    |  1 +
>  drivers/gpu/drm/i915/intel_pm.c     | 20 ++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_uncore.c |  2 +-
>  5 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 66c6ffb..7148eac 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -521,7 +521,7 @@ static int i915_drm_freeze(struct drm_device *dev)
>  		drm_irq_uninstall(dev);
>  		dev_priv->enable_hotplug_processing = false;
>  
> -		intel_disable_gt_powersave(dev);
> +		intel_suspend_gt_powersave(dev);

I realized now that we actually do need to enable RC6 explicitly. If we
get here right after runtime resume, the deferred RC6 enabling might be
still pending and so RC6 might not be enabled.

--Imre

>  
>  		/*
>  		 * Disable CRTCs directly since we want to preserve sw state
> @@ -542,8 +542,8 @@ static int i915_drm_freeze(struct drm_device *dev)
>  
>  	i915_save_state(dev);
>  
> +	intel_uncore_forcewake_reset(dev, false);
>  	intel_opregion_fini(dev);
> -	intel_uncore_fini(dev);
>  
>  	console_lock();
>  	intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index bea9ab40..89d6b47 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2084,6 +2084,7 @@ extern void intel_uncore_early_sanitize(struct drm_device *dev);
>  extern void intel_uncore_init(struct drm_device *dev);
>  extern void intel_uncore_check_errors(struct drm_device *dev);
>  extern void intel_uncore_fini(struct drm_device *dev);
> +extern void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore);
>  
>  void
>  i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index c597b0d..74fbe4d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -957,6 +957,7 @@ void intel_init_gt_powersave(struct drm_device *dev);
>  void intel_cleanup_gt_powersave(struct drm_device *dev);
>  void intel_enable_gt_powersave(struct drm_device *dev);
>  void intel_disable_gt_powersave(struct drm_device *dev);
> +void intel_suspend_gt_powersave(struct drm_device *dev);
>  void intel_reset_gt_powersave(struct drm_device *dev);
>  void ironlake_teardown_rc6(struct drm_device *dev);
>  void gen6_update_ring_freq(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 1840d15..139eebe 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4864,6 +4864,26 @@ void intel_cleanup_gt_powersave(struct drm_device *dev)
>  		valleyview_cleanup_gt_powersave(dev);
>  }
>  
> +/**
> + * intel_suspend_gt_powersave - suspend PM work and helper threads
> + * @dev: drm device
> + *
> + * We don't want to disable RC6 or other features here, we just want
> + * to make sure any work we've queued has finished and won't bother
> + * us while we're suspended.
> + */
> +void intel_suspend_gt_powersave(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	/* Interrupts should be disabled already to avoid re-arming. */
> +	WARN_ON(dev->irq_enabled);
> +
> +	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> +
> +	cancel_work_sync(&dev_priv->rps.work);
> +}
> +
>  void intel_disable_gt_powersave(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 871c284..741a4e3 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -316,7 +316,7 @@ static void gen6_force_wake_timer(unsigned long arg)
>  	intel_runtime_pm_put(dev_priv);
>  }
>  
> -static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
> +void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	unsigned long irqflags;


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: leave rc6 enabled at suspend time v4

[Daniel Vetter]

On Tue, Jun 10, 2014 at 04:42:50PM +0300, Imre Deak wrote:
> On Wed, 2014-06-04 at 13:45 -0700, Jesse Barnes wrote:
> > This allows the system to enter the lowest power mode during system freeze.
> > 
> > v2: delete force wake timer at suspend (Imre)
> > v3: add GT work suspend function (Imre)
> > v4: use uncore forcewake reset (Daniel)
> > 
> > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c     |  4 ++--
> >  drivers/gpu/drm/i915/i915_drv.h     |  1 +
> >  drivers/gpu/drm/i915/intel_drv.h    |  1 +
> >  drivers/gpu/drm/i915/intel_pm.c     | 20 ++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_uncore.c |  2 +-
> >  5 files changed, 25 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 66c6ffb..7148eac 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -521,7 +521,7 @@ static int i915_drm_freeze(struct drm_device *dev)
> >  		drm_irq_uninstall(dev);
> >  		dev_priv->enable_hotplug_processing = false;
> >  
> > -		intel_disable_gt_powersave(dev);
> > +		intel_suspend_gt_powersave(dev);
> 
> I realized now that we actually do need to enable RC6 explicitly. If we
> get here right after runtime resume, the deferred RC6 enabling might be
> still pending and so RC6 might not be enabled.

Hm, for runtime pm we might schedule the delayed rps work with a 0 delay,
just to get the gpu going faster. Just an aside.

Also some runtime pm vs system suspend tests in igt would be good if we
don't have them yet. And if we have them some analysis why this didn't
blow up in testing (and something to address that gap if feasible).
-Daniel

> 
> --Imre
> 
> >  
> >  		/*
> >  		 * Disable CRTCs directly since we want to preserve sw state
> > @@ -542,8 +542,8 @@ static int i915_drm_freeze(struct drm_device *dev)
> >  
> >  	i915_save_state(dev);
> >  
> > +	intel_uncore_forcewake_reset(dev, false);
> >  	intel_opregion_fini(dev);
> > -	intel_uncore_fini(dev);
> >  
> >  	console_lock();
> >  	intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED);
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index bea9ab40..89d6b47 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2084,6 +2084,7 @@ extern void intel_uncore_early_sanitize(struct drm_device *dev);
> >  extern void intel_uncore_init(struct drm_device *dev);
> >  extern void intel_uncore_check_errors(struct drm_device *dev);
> >  extern void intel_uncore_fini(struct drm_device *dev);
> > +extern void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore);
> >  
> >  void
> >  i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index c597b0d..74fbe4d 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -957,6 +957,7 @@ void intel_init_gt_powersave(struct drm_device *dev);
> >  void intel_cleanup_gt_powersave(struct drm_device *dev);
> >  void intel_enable_gt_powersave(struct drm_device *dev);
> >  void intel_disable_gt_powersave(struct drm_device *dev);
> > +void intel_suspend_gt_powersave(struct drm_device *dev);
> >  void intel_reset_gt_powersave(struct drm_device *dev);
> >  void ironlake_teardown_rc6(struct drm_device *dev);
> >  void gen6_update_ring_freq(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 1840d15..139eebe 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4864,6 +4864,26 @@ void intel_cleanup_gt_powersave(struct drm_device *dev)
> >  		valleyview_cleanup_gt_powersave(dev);
> >  }
> >  
> > +/**
> > + * intel_suspend_gt_powersave - suspend PM work and helper threads
> > + * @dev: drm device
> > + *
> > + * We don't want to disable RC6 or other features here, we just want
> > + * to make sure any work we've queued has finished and won't bother
> > + * us while we're suspended.
> > + */
> > +void intel_suspend_gt_powersave(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +	/* Interrupts should be disabled already to avoid re-arming. */
> > +	WARN_ON(dev->irq_enabled);
> > +
> > +	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> > +
> > +	cancel_work_sync(&dev_priv->rps.work);
> > +}
> > +
> >  void intel_disable_gt_powersave(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index 871c284..741a4e3 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -316,7 +316,7 @@ static void gen6_force_wake_timer(unsigned long arg)
> >  	intel_runtime_pm_put(dev_priv);
> >  }
> >  
> > -static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
> > +void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	unsigned long irqflags;
> 



> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: leave rc6 enabled at suspend time v4

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 2313 bytes --]

On Tue, 2014-06-10 at 15:57 +0200, Daniel Vetter wrote:
> On Tue, Jun 10, 2014 at 04:42:50PM +0300, Imre Deak wrote:
> > On Wed, 2014-06-04 at 13:45 -0700, Jesse Barnes wrote:
> > > This allows the system to enter the lowest power mode during system freeze.
> > > 
> > > v2: delete force wake timer at suspend (Imre)
> > > v3: add GT work suspend function (Imre)
> > > v4: use uncore forcewake reset (Daniel)
> > > 
> > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.c     |  4 ++--
> > >  drivers/gpu/drm/i915/i915_drv.h     |  1 +
> > >  drivers/gpu/drm/i915/intel_drv.h    |  1 +
> > >  drivers/gpu/drm/i915/intel_pm.c     | 20 ++++++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_uncore.c |  2 +-
> > >  5 files changed, 25 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > index 66c6ffb..7148eac 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -521,7 +521,7 @@ static int i915_drm_freeze(struct drm_device *dev)
> > >  		drm_irq_uninstall(dev);
> > >  		dev_priv->enable_hotplug_processing = false;
> > >  
> > > -		intel_disable_gt_powersave(dev);
> > > +		intel_suspend_gt_powersave(dev);
> > 
> > I realized now that we actually do need to enable RC6 explicitly. If we
> > get here right after runtime resume, the deferred RC6 enabling might be
> > still pending and so RC6 might not be enabled.
> 
> Hm, for runtime pm we might schedule the delayed rps work with a 0 delay,
> just to get the gpu going faster. Just an aside.
>
> Also some runtime pm vs system suspend tests in igt would be good if we
> don't have them yet. And if we have them some analysis why this didn't
> blow up in testing (and something to address that gap if feasible).

To clarify the above is about the new functionality to enable deeper
system wide power states. Atm, we don't have a bug related to the above
runtime resume->system suspend sequence, since we explicitly disable gt
power saving/RC6. For deeper power states we have to do the opposite and
leave RC6 enabled and that's what I commented on.

--Imre 


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: leave rc6 enabled at suspend time v4

[Daniel Vetter]

On Tue, Jun 10, 2014 at 05:41:49PM +0300, Imre Deak wrote:
> On Tue, 2014-06-10 at 15:57 +0200, Daniel Vetter wrote:
> > On Tue, Jun 10, 2014 at 04:42:50PM +0300, Imre Deak wrote:
> > > On Wed, 2014-06-04 at 13:45 -0700, Jesse Barnes wrote:
> > > > This allows the system to enter the lowest power mode during system freeze.
> > > > 
> > > > v2: delete force wake timer at suspend (Imre)
> > > > v3: add GT work suspend function (Imre)
> > > > v4: use uncore forcewake reset (Daniel)
> > > > 
> > > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.c     |  4 ++--
> > > >  drivers/gpu/drm/i915/i915_drv.h     |  1 +
> > > >  drivers/gpu/drm/i915/intel_drv.h    |  1 +
> > > >  drivers/gpu/drm/i915/intel_pm.c     | 20 ++++++++++++++++++++
> > > >  drivers/gpu/drm/i915/intel_uncore.c |  2 +-
> > > >  5 files changed, 25 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > > index 66c6ffb..7148eac 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > @@ -521,7 +521,7 @@ static int i915_drm_freeze(struct drm_device *dev)
> > > >  		drm_irq_uninstall(dev);
> > > >  		dev_priv->enable_hotplug_processing = false;
> > > >  
> > > > -		intel_disable_gt_powersave(dev);
> > > > +		intel_suspend_gt_powersave(dev);
> > > 
> > > I realized now that we actually do need to enable RC6 explicitly. If we
> > > get here right after runtime resume, the deferred RC6 enabling might be
> > > still pending and so RC6 might not be enabled.
> > 
> > Hm, for runtime pm we might schedule the delayed rps work with a 0 delay,
> > just to get the gpu going faster. Just an aside.
> >
> > Also some runtime pm vs system suspend tests in igt would be good if we
> > don't have them yet. And if we have them some analysis why this didn't
> > blow up in testing (and something to address that gap if feasible).
> 
> To clarify the above is about the new functionality to enable deeper
> system wide power states. Atm, we don't have a bug related to the above
> runtime resume->system suspend sequence, since we explicitly disable gt
> power saving/RC6. For deeper power states we have to do the opposite and
> leave RC6 enabled and that's what I commented on.

Well I just want to make sure that we have test coverage. If Jesse has run
all the rpm tests with piglit run -t rpm and it didn't blow up we need to
fix this gap.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: leave rc6 enabled at suspend time v4

[Jesse Barnes]

On Tue, 10 Jun 2014 17:26:45 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Jun 10, 2014 at 05:41:49PM +0300, Imre Deak wrote:
> > On Tue, 2014-06-10 at 15:57 +0200, Daniel Vetter wrote:
> > > On Tue, Jun 10, 2014 at 04:42:50PM +0300, Imre Deak wrote:
> > > > On Wed, 2014-06-04 at 13:45 -0700, Jesse Barnes wrote:
> > > > > This allows the system to enter the lowest power mode during system freeze.
> > > > > 
> > > > > v2: delete force wake timer at suspend (Imre)
> > > > > v3: add GT work suspend function (Imre)
> > > > > v4: use uncore forcewake reset (Daniel)
> > > > > 
> > > > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_drv.c     |  4 ++--
> > > > >  drivers/gpu/drm/i915/i915_drv.h     |  1 +
> > > > >  drivers/gpu/drm/i915/intel_drv.h    |  1 +
> > > > >  drivers/gpu/drm/i915/intel_pm.c     | 20 ++++++++++++++++++++
> > > > >  drivers/gpu/drm/i915/intel_uncore.c |  2 +-
> > > > >  5 files changed, 25 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > > > index 66c6ffb..7148eac 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > > @@ -521,7 +521,7 @@ static int i915_drm_freeze(struct drm_device *dev)
> > > > >  		drm_irq_uninstall(dev);
> > > > >  		dev_priv->enable_hotplug_processing = false;
> > > > >  
> > > > > -		intel_disable_gt_powersave(dev);
> > > > > +		intel_suspend_gt_powersave(dev);
> > > > 
> > > > I realized now that we actually do need to enable RC6 explicitly. If we
> > > > get here right after runtime resume, the deferred RC6 enabling might be
> > > > still pending and so RC6 might not be enabled.

Seems like we could just flush the enable if it was outstanding?

> > > Hm, for runtime pm we might schedule the delayed rps work with a 0 delay,
> > > just to get the gpu going faster. Just an aside.
> > >
> > > Also some runtime pm vs system suspend tests in igt would be good if we
> > > don't have them yet. And if we have them some analysis why this didn't
> > > blow up in testing (and something to address that gap if feasible).
> > 
> > To clarify the above is about the new functionality to enable deeper
> > system wide power states. Atm, we don't have a bug related to the above
> > runtime resume->system suspend sequence, since we explicitly disable gt
> > power saving/RC6. For deeper power states we have to do the opposite and
> > leave RC6 enabled and that's what I commented on.
> 
> Well I just want to make sure that we have test coverage. If Jesse has run
> all the rpm tests with piglit run -t rpm and it didn't blow up we need to
> fix this gap.

I have not...  I'm not sure how to test this though, as I think we'd
need to measure c states or power when we do a suspend.  If we checked
for RC6 enable at resume, we'd probably race with the enable work queue.

Either way, we should add some asserts to the suspend path to make sure
all the conditions we need for minimum power are met.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: leave rc6 enabled at suspend time v4

[Jesse Barnes]

On Wed, 11 Jun 2014 15:21:16 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Tue, 10 Jun 2014 17:26:45 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Tue, Jun 10, 2014 at 05:41:49PM +0300, Imre Deak wrote:
> > > On Tue, 2014-06-10 at 15:57 +0200, Daniel Vetter wrote:
> > > > On Tue, Jun 10, 2014 at 04:42:50PM +0300, Imre Deak wrote:
> > > > > On Wed, 2014-06-04 at 13:45 -0700, Jesse Barnes wrote:
> > > > > > This allows the system to enter the lowest power mode during system freeze.
> > > > > > 
> > > > > > v2: delete force wake timer at suspend (Imre)
> > > > > > v3: add GT work suspend function (Imre)
> > > > > > v4: use uncore forcewake reset (Daniel)
> > > > > > 
> > > > > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> > > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/i915_drv.c     |  4 ++--
> > > > > >  drivers/gpu/drm/i915/i915_drv.h     |  1 +
> > > > > >  drivers/gpu/drm/i915/intel_drv.h    |  1 +
> > > > > >  drivers/gpu/drm/i915/intel_pm.c     | 20 ++++++++++++++++++++
> > > > > >  drivers/gpu/drm/i915/intel_uncore.c |  2 +-
> > > > > >  5 files changed, 25 insertions(+), 3 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > > > > index 66c6ffb..7148eac 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > > > @@ -521,7 +521,7 @@ static int i915_drm_freeze(struct drm_device *dev)
> > > > > >  		drm_irq_uninstall(dev);
> > > > > >  		dev_priv->enable_hotplug_processing = false;
> > > > > >  
> > > > > > -		intel_disable_gt_powersave(dev);
> > > > > > +		intel_suspend_gt_powersave(dev);
> > > > > 
> > > > > I realized now that we actually do need to enable RC6 explicitly. If we
> > > > > get here right after runtime resume, the deferred RC6 enabling might be
> > > > > still pending and so RC6 might not be enabled.
> 
> Seems like we could just flush the enable if it was outstanding?

Oh and I see we already have the flush in v4... do you think we might
not actually arm the deferred enable work before we get here?  I think
the runtime_resume and suspend calls should be serialized, but if not
we'd need some locking I guess...

Jesse

Re: [PATCH] drm/i915: leave rc6 enabled at suspend time v4

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 3362 bytes --]

On Wed, 2014-06-11 at 15:24 -0700, Jesse Barnes wrote:
> On Wed, 11 Jun 2014 15:21:16 -0700
> Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> 
> > On Tue, 10 Jun 2014 17:26:45 +0200
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> > 
> > > On Tue, Jun 10, 2014 at 05:41:49PM +0300, Imre Deak wrote:
> > > > On Tue, 2014-06-10 at 15:57 +0200, Daniel Vetter wrote:
> > > > > On Tue, Jun 10, 2014 at 04:42:50PM +0300, Imre Deak wrote:
> > > > > > On Wed, 2014-06-04 at 13:45 -0700, Jesse Barnes wrote:
> > > > > > > This allows the system to enter the lowest power mode during system freeze.
> > > > > > > 
> > > > > > > v2: delete force wake timer at suspend (Imre)
> > > > > > > v3: add GT work suspend function (Imre)
> > > > > > > v4: use uncore forcewake reset (Daniel)
> > > > > > > 
> > > > > > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> > > > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/i915/i915_drv.c     |  4 ++--
> > > > > > >  drivers/gpu/drm/i915/i915_drv.h     |  1 +
> > > > > > >  drivers/gpu/drm/i915/intel_drv.h    |  1 +
> > > > > > >  drivers/gpu/drm/i915/intel_pm.c     | 20 ++++++++++++++++++++
> > > > > > >  drivers/gpu/drm/i915/intel_uncore.c |  2 +-
> > > > > > >  5 files changed, 25 insertions(+), 3 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > > > > > index 66c6ffb..7148eac 100644
> > > > > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > > > > @@ -521,7 +521,7 @@ static int i915_drm_freeze(struct drm_device *dev)
> > > > > > >  		drm_irq_uninstall(dev);
> > > > > > >  		dev_priv->enable_hotplug_processing = false;
> > > > > > >  
> > > > > > > -		intel_disable_gt_powersave(dev);
> > > > > > > +		intel_suspend_gt_powersave(dev);
> > > > > > 
> > > > > > I realized now that we actually do need to enable RC6 explicitly. If we
> > > > > > get here right after runtime resume, the deferred RC6 enabling might be
> > > > > > still pending and so RC6 might not be enabled.
> > 
> > Seems like we could just flush the enable if it was outstanding?
> 
> Oh and I see we already have the flush in v4...

Right, I was just blind and I guess also thinking in terms of timers
that don't have a similar flush API. Anyway this is fine and my comment
about it above can be ignored.

> do you think we might not actually arm the deferred enable work before
> we get here? 

I haven't thought about this. But now that you mentioned I looked into
the device and dpm core and can't see what would prevent our system
suspend handler to be called while we are still running our driver load
function. But this is a separate issue (if true at all) that exists
regardless of this patch and also not a really real life scenario. So I
suggest we investigate this as a follow-up.

> I think the runtime_resume and suspend calls should be serialized, but
> if not we'd need some locking I guess...

Yes, this is taken care of the RPM framework. Similarly system
suspend/resume handlers are serialized wrt. the runtime suspend/resume
handlers.

This patch is:
Reviewed-by: Imre Deak <imre.deak@intel.com>

Btw, now all 4 patches in the patchset are reviewed.

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx