[PATCH 00/16] Merge PC8 with runtime PM, v3

[PATCH 00/16] Merge PC8 with runtime PM, v3

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Hi

So this series is obviously the follow up of "Merge PC8 with runtime PM, v2". It
contains the changes requested by our reviewers, a rebase, and the latest
Reviewed-by tags. Daniel asked me to send it on a new thread to avoid confusion,
and I agree this is the best way to proceed.

I based this series on top of "[PATCH 0/6] More runtime PM fixes". So please
grab that before applying these patches.

This series is mostly reviewed. Patches that still don't have Reviewed-by tags
are just: 2, 4 and 9.

Notes to Daniel and other possible people applying these patches:
 - Please merge patches in the exact order I sent! The order I used here is
   strategically designed to avoid regressions in the middle.
 - After applying patch 3, please read the notice on its commit message and
   check if the result is correct.

Thanks,
Paulo

Paulo Zanoni (16):
  drm/i915: extract __hsw_do_{en,dis}able_package_c8
  drm/i915: make PC8 be part of runtime PM suspend/resume
  drm/i915: get/put runtime PM when we get/put a power domain
  drm/i915: remove dev_priv->pc8.requirements_met
  drm/i915: get runtime PM references when the GPU is idle/busy
  drm/i915: kill pc8.disable_count
  drm/i915: remove an indirection level on PC8 functions
  drm/i915: don't get/put PC8 reference on freeze/thaw
  drm/i915: make intel_aux_display_runtime_get get runtime PM, not PC8
  drm/i915: don't get/put PC8 when getting/putting power wells
  drm/i915: remove dev_priv->pc8.enabled
  drm/i915: move pc8.irqs_disabled to pm.irqs_disabled
  drm/i915: kill struct i915_package_c8
  drm/i915: rename __hsw_do_{en,dis}able_pc8
  drm/i915: update the PC8 and runtime PM documentation
  drm/i915: init pm.suspended earlier

 drivers/gpu/drm/i915/i915_debugfs.c  |   8 +-
 drivers/gpu/drm/i915/i915_dma.c      |   2 -
 drivers/gpu/drm/i915/i915_drv.c      |  13 ++-
 drivers/gpu/drm/i915/i915_drv.h      |  65 +++----------
 drivers/gpu/drm/i915/i915_gem.c      |   2 +-
 drivers/gpu/drm/i915/i915_irq.c      |  58 ++++++------
 drivers/gpu/drm/i915/i915_params.c   |  10 --
 drivers/gpu/drm/i915/intel_display.c | 177 ++++++++++-------------------------
 drivers/gpu/drm/i915/intel_drv.h     |   9 +-
 drivers/gpu/drm/i915/intel_pm.c      |  26 +++--
 10 files changed, 119 insertions(+), 251 deletions(-)

-- 
1.8.5.3

[PATCH 01/16] drm/i915: extract __hsw_do_{en, dis}able_package_c8

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

When we merge PC8 and runtime PM, these new functions are going to be
called by the runtime suspend/resume functions, and their callers are
going to be removed.

v2: - Rebase

Reviewed-by: Imre Deak <imre.deak@intel.com> (v1)
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 64 +++++++++++++++++++++---------------
 1 file changed, 38 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index dc2b377..9c3e2c5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6772,19 +6772,11 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
 	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
 }
 
-void hsw_enable_pc8_work(struct work_struct *__work)
+static void __hsw_do_enable_pc8(struct drm_i915_private *dev_priv)
 {
-	struct drm_i915_private *dev_priv =
-		container_of(to_delayed_work(__work), struct drm_i915_private,
-			     pc8.enable_work);
 	struct drm_device *dev = dev_priv->dev;
 	uint32_t val;
 
-	WARN_ON(!HAS_PC8(dev));
-
-	if (dev_priv->pc8.enabled)
-		return;
-
 	DRM_DEBUG_KMS("Enabling package C8+\n");
 
 	dev_priv->pc8.enabled = true;
@@ -6798,7 +6790,21 @@ void hsw_enable_pc8_work(struct work_struct *__work)
 	lpt_disable_clkout_dp(dev);
 	hsw_pc8_disable_interrupts(dev);
 	hsw_disable_lcpll(dev_priv, true, true);
+}
+
+void hsw_enable_pc8_work(struct work_struct *__work)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(to_delayed_work(__work), struct drm_i915_private,
+			     pc8.enable_work);
+	struct drm_device *dev = dev_priv->dev;
+
+	WARN_ON(!HAS_PC8(dev));
 
+	if (dev_priv->pc8.enabled)
+		return;
+
+	__hsw_do_enable_pc8(dev_priv);
 	intel_runtime_pm_put(dev_priv);
 }
 
@@ -6816,29 +6822,13 @@ static void __hsw_enable_package_c8(struct drm_i915_private *dev_priv)
 			      msecs_to_jiffies(i915.pc8_timeout));
 }
 
-static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv)
+static void __hsw_do_disable_package_c8(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
 	uint32_t val;
 
-	WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
-	WARN(dev_priv->pc8.disable_count < 0,
-	     "pc8.disable_count: %d\n", dev_priv->pc8.disable_count);
-
-	dev_priv->pc8.disable_count++;
-	if (dev_priv->pc8.disable_count != 1)
-		return;
-
-	WARN_ON(!HAS_PC8(dev));
-
-	cancel_delayed_work_sync(&dev_priv->pc8.enable_work);
-	if (!dev_priv->pc8.enabled)
-		return;
-
 	DRM_DEBUG_KMS("Disabling package C8+\n");
 
-	intel_runtime_pm_get(dev_priv);
-
 	hsw_restore_lcpll(dev_priv);
 	hsw_pc8_restore_interrupts(dev);
 	lpt_init_pch_refclk(dev);
@@ -6857,6 +6847,28 @@ static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv)
 	dev_priv->pc8.enabled = false;
 }
 
+static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+
+	WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
+	WARN(dev_priv->pc8.disable_count < 0,
+	     "pc8.disable_count: %d\n", dev_priv->pc8.disable_count);
+
+	dev_priv->pc8.disable_count++;
+	if (dev_priv->pc8.disable_count != 1)
+		return;
+
+	WARN_ON(!HAS_PC8(dev));
+
+	cancel_delayed_work_sync(&dev_priv->pc8.enable_work);
+	if (!dev_priv->pc8.enabled)
+		return;
+
+	intel_runtime_pm_get(dev_priv);
+	__hsw_do_disable_package_c8(dev_priv);
+}
+
 void hsw_enable_package_c8(struct drm_i915_private *dev_priv)
 {
 	if (!HAS_PC8(dev_priv->dev))
-- 
1.8.5.3

[PATCH 02/16] drm/i915: make PC8 be part of runtime PM suspend/resume

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Currently, when our driver becomes idle for i915.pc8_timeout (default:
5s) we enable PC8, so we save some power, but not everything we can.
Then, while PC8 is enabled, if we stay idle for more
autosuspend_delay_ms (default: 10s) we'll enter runtime PM and put the
graphics device in D3 state, saving even more power. The two features
are separate things with increasing levels of power savings, but if we
disable PC8 we'll never get into D3.

While from the modularity point of view it would be nice to keep these
features as separate, we have reasons to merge them:
 - We are not aware of anybody wanting a "PC8 without D3" environment.
 - If we keep both features as separate, we'll have to to test both
   PC8 and PC8+D3 code paths. We're already having a major pain to
   make QA do automated testing of just one thing, testing both paths
   will cost even more.
 - Only Haswell+ supports PC8, so if we want to add runtime PM support
   to, for example, IVB, we'll have to copy some code from the PC8
   feature to runtime PM, so merging both features as a single thing
   will make it easier for enabling runtime PM on other platforms.

This patch only does the very basic steps required to have PC8 and
runtime PM merged on a single feature: the next patches will take care
of cleaning up everything.

v2: - Rebase.
v3: - Rebase.
    - Fully remove the deprecated i915 params since Daniel doesn't
      consider them as part of the ABI.
v4: - Rebase.
    - Fix typo in the commit message.
v5: - Rebase, again.
    - Add a huge comment explaining the different forcewake usage
      (Chris, Daniel).
    - Use open-coded forcewake functions (Daniel).

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c      |  2 --
 drivers/gpu/drm/i915/i915_drv.c      |  8 +++++
 drivers/gpu/drm/i915/i915_drv.h      |  7 ++--
 drivers/gpu/drm/i915/i915_params.c   | 10 ------
 drivers/gpu/drm/i915/intel_display.c | 65 ++++++++++++++++--------------------
 drivers/gpu/drm/i915/intel_drv.h     |  3 +-
 drivers/gpu/drm/i915/intel_pm.c      |  1 -
 7 files changed, 43 insertions(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index e4d2b9f..6aa23af 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1822,8 +1822,6 @@ int i915_driver_unload(struct drm_device *dev)
 	cancel_work_sync(&dev_priv->gpu_error.work);
 	i915_destroy_error_state(dev);
 
-	cancel_delayed_work_sync(&dev_priv->pc8.enable_work);
-
 	if (dev->pdev->msi_enabled)
 		pci_disable_msi(dev->pdev);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 658fe24..6178c95 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -848,6 +848,9 @@ static int i915_runtime_suspend(struct device *device)
 
 	DRM_DEBUG_KMS("Suspending device\n");
 
+	if (HAS_PC8(dev))
+		__hsw_do_enable_pc8(dev_priv);
+
 	i915_gem_release_all_mmaps(dev_priv);
 
 	del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
@@ -862,6 +865,7 @@ static int i915_runtime_suspend(struct device *device)
 	 */
 	intel_opregion_notify_adapter(dev, PCI_D1);
 
+	DRM_DEBUG_KMS("Device suspended\n");
 	return 0;
 }
 
@@ -878,6 +882,10 @@ static int i915_runtime_resume(struct device *device)
 	intel_opregion_notify_adapter(dev, PCI_D0);
 	dev_priv->pm.suspended = false;
 
+	if (HAS_PC8(dev))
+		__hsw_do_disable_pc8(dev_priv);
+
+	DRM_DEBUG_KMS("Device resumed\n");
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2a319ba..a5f1780 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1335,6 +1335,10 @@ struct ilk_wm_values {
 /*
  * This struct tracks the state needed for the Package C8+ feature.
  *
+ * TODO: we're merging the Package C8+ feature with the runtime PM support. To
+ * avoid having to update the documentation at each patch of the series, we'll
+ * do a final update at the end.
+ *
  * Package states C8 and deeper are really deep PC states that can only be
  * reached when all the devices on the system allow it, so even if the graphics
  * device allows PC8+, it doesn't mean the system will actually get to these
@@ -1388,7 +1392,6 @@ struct i915_package_c8 {
 	bool enabled;
 	int disable_count;
 	struct mutex lock;
-	struct delayed_work enable_work;
 
 	struct {
 		uint32_t deimr;
@@ -2092,8 +2095,6 @@ struct i915_params {
 	unsigned int preliminary_hw_support;
 	int disable_power_well;
 	int enable_ips;
-	int enable_pc8;
-	int pc8_timeout;
 	int invert_brightness;
 	int enable_cmd_parser;
 	/* leave bools at the end to not create holes */
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index a66ffb6..d1d7980 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -42,8 +42,6 @@ struct i915_params i915 __read_mostly = {
 	.disable_power_well = 1,
 	.enable_ips = 1,
 	.fastboot = 0,
-	.enable_pc8 = 1,
-	.pc8_timeout = 5000,
 	.prefault_disable = 0,
 	.reset = true,
 	.invert_brightness = 0,
@@ -135,14 +133,6 @@ module_param_named(fastboot, i915.fastboot, bool, 0600);
 MODULE_PARM_DESC(fastboot,
 	"Try to skip unnecessary mode sets at boot time (default: false)");
 
-module_param_named(enable_pc8, i915.enable_pc8, int, 0600);
-MODULE_PARM_DESC(enable_pc8,
-	"Enable support for low power package C states (PC8+) (default: true)");
-
-module_param_named(pc8_timeout, i915.pc8_timeout, int, 0600);
-MODULE_PARM_DESC(pc8_timeout,
-	"Number of msecs of idleness required to enter PC8+ (default: 5000)");
-
 module_param_named(prefault_disable, i915.prefault_disable, bool, 0600);
 MODULE_PARM_DESC(prefault_disable,
 	"Disable page prefaulting for pread/pwrite/reloc (default:false). "
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9c3e2c5..ab02848 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6726,6 +6726,7 @@ static void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
 static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
 {
 	uint32_t val;
+	unsigned long irqflags;
 
 	val = I915_READ(LCPLL_CTL);
 
@@ -6733,9 +6734,22 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
 		    LCPLL_POWER_DOWN_ALLOW)) == LCPLL_PLL_LOCK)
 		return;
 
-	/* Make sure we're not on PC8 state before disabling PC8, otherwise
-	 * we'll hang the machine! */
-	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
+	/*
+	 * Make sure we're not on PC8 state before disabling PC8, otherwise
+	 * we'll hang the machine. To prevent PC8 state, just enable force_wake.
+	 *
+	 * The other problem is that hsw_restore_lcpll() is called as part of
+	 * the runtime PM resume sequence, so we can't just call
+	 * gen6_gt_force_wake_get() because that function calls
+	 * intel_runtime_pm_get(), and we can't change the runtime PM refcount
+	 * while we are on the resume sequence. So to solve this problem we have
+	 * to call special forcewake code that doesn't touch runtime PM and
+	 * doesn't enable the forcewake delayed work.
+	 */
+	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
+	if (dev_priv->uncore.forcewake_count++ == 0)
+		dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
+	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 
 	if (val & LCPLL_POWER_DOWN_ALLOW) {
 		val &= ~LCPLL_POWER_DOWN_ALLOW;
@@ -6769,14 +6783,20 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
 			DRM_ERROR("Switching back to LCPLL failed\n");
 	}
 
-	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
+	/* See the big comment above. */
+	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
+	if (--dev_priv->uncore.forcewake_count == 0)
+		dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
+	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
-static void __hsw_do_enable_pc8(struct drm_i915_private *dev_priv)
+void __hsw_do_enable_pc8(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
 	uint32_t val;
 
+	WARN_ON(!HAS_PC8(dev));
+
 	DRM_DEBUG_KMS("Enabling package C8+\n");
 
 	dev_priv->pc8.enabled = true;
@@ -6792,22 +6812,6 @@ static void __hsw_do_enable_pc8(struct drm_i915_private *dev_priv)
 	hsw_disable_lcpll(dev_priv, true, true);
 }
 
-void hsw_enable_pc8_work(struct work_struct *__work)
-{
-	struct drm_i915_private *dev_priv =
-		container_of(to_delayed_work(__work), struct drm_i915_private,
-			     pc8.enable_work);
-	struct drm_device *dev = dev_priv->dev;
-
-	WARN_ON(!HAS_PC8(dev));
-
-	if (dev_priv->pc8.enabled)
-		return;
-
-	__hsw_do_enable_pc8(dev_priv);
-	intel_runtime_pm_put(dev_priv);
-}
-
 static void __hsw_enable_package_c8(struct drm_i915_private *dev_priv)
 {
 	WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
@@ -6818,15 +6822,16 @@ static void __hsw_enable_package_c8(struct drm_i915_private *dev_priv)
 	if (dev_priv->pc8.disable_count != 0)
 		return;
 
-	schedule_delayed_work(&dev_priv->pc8.enable_work,
-			      msecs_to_jiffies(i915.pc8_timeout));
+	intel_runtime_pm_put(dev_priv);
 }
 
-static void __hsw_do_disable_package_c8(struct drm_i915_private *dev_priv)
+void __hsw_do_disable_pc8(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
 	uint32_t val;
 
+	WARN_ON(!HAS_PC8(dev));
+
 	DRM_DEBUG_KMS("Disabling package C8+\n");
 
 	hsw_restore_lcpll(dev_priv);
@@ -6849,8 +6854,6 @@ static void __hsw_do_disable_package_c8(struct drm_i915_private *dev_priv)
 
 static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv)
 {
-	struct drm_device *dev = dev_priv->dev;
-
 	WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
 	WARN(dev_priv->pc8.disable_count < 0,
 	     "pc8.disable_count: %d\n", dev_priv->pc8.disable_count);
@@ -6859,14 +6862,7 @@ static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv)
 	if (dev_priv->pc8.disable_count != 1)
 		return;
 
-	WARN_ON(!HAS_PC8(dev));
-
-	cancel_delayed_work_sync(&dev_priv->pc8.enable_work);
-	if (!dev_priv->pc8.enabled)
-		return;
-
 	intel_runtime_pm_get(dev_priv);
-	__hsw_do_disable_package_c8(dev_priv);
 }
 
 void hsw_enable_package_c8(struct drm_i915_private *dev_priv)
@@ -6924,9 +6920,6 @@ static void hsw_update_package_c8(struct drm_device *dev)
 	if (!HAS_PC8(dev_priv->dev))
 		return;
 
-	if (!i915.enable_pc8)
-		return;
-
 	mutex_lock(&dev_priv->pc8.lock);
 
 	allow = hsw_can_enable_package_c8(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 805d207..3f7ed0f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -722,7 +722,8 @@ unsigned long intel_gen4_compute_page_offset(int *x, int *y,
 					     unsigned int bpp,
 					     unsigned int pitch);
 void intel_display_handle_reset(struct drm_device *dev);
-void hsw_enable_pc8_work(struct work_struct *__work);
+void __hsw_do_enable_pc8(struct drm_i915_private *dev_priv);
+void __hsw_do_disable_pc8(struct drm_i915_private *dev_priv);
 void hsw_enable_package_c8(struct drm_i915_private *dev_priv);
 void hsw_disable_package_c8(struct drm_i915_private *dev_priv);
 void intel_dp_get_m_n(struct intel_crtc *crtc,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ad58ce3..c5c36dc 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -6161,7 +6161,6 @@ void intel_pm_setup(struct drm_device *dev)
 	dev_priv->pc8.irqs_disabled = false;
 	dev_priv->pc8.enabled = false;
 	dev_priv->pc8.disable_count = 1; /* requirements_met */
-	INIT_DELAYED_WORK(&dev_priv->pc8.enable_work, hsw_enable_pc8_work);
 	INIT_DELAYED_WORK(&dev_priv->rps.delayed_resume_work,
 			  intel_gen6_powersave_work);
 }
-- 
1.8.5.3

Re: [PATCH 02/16] drm/i915: make PC8 be part of runtime PM suspend/resume

[Imre Deak]

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

On Fri, 2014-03-07 at 20:08 -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Currently, when our driver becomes idle for i915.pc8_timeout (default:
> 5s) we enable PC8, so we save some power, but not everything we can.
> Then, while PC8 is enabled, if we stay idle for more
> autosuspend_delay_ms (default: 10s) we'll enter runtime PM and put the
> graphics device in D3 state, saving even more power. The two features
> are separate things with increasing levels of power savings, but if we
> disable PC8 we'll never get into D3.
> 
> While from the modularity point of view it would be nice to keep these
> features as separate, we have reasons to merge them:
>  - We are not aware of anybody wanting a "PC8 without D3" environment.
>  - If we keep both features as separate, we'll have to to test both
>    PC8 and PC8+D3 code paths. We're already having a major pain to
>    make QA do automated testing of just one thing, testing both paths
>    will cost even more.
>  - Only Haswell+ supports PC8, so if we want to add runtime PM support
>    to, for example, IVB, we'll have to copy some code from the PC8
>    feature to runtime PM, so merging both features as a single thing
>    will make it easier for enabling runtime PM on other platforms.
> 
> This patch only does the very basic steps required to have PC8 and
> runtime PM merged on a single feature: the next patches will take care
> of cleaning up everything.
> 
> v2: - Rebase.
> v3: - Rebase.
>     - Fully remove the deprecated i915 params since Daniel doesn't
>       consider them as part of the ABI.
> v4: - Rebase.
>     - Fix typo in the commit message.
> v5: - Rebase, again.
>     - Add a huge comment explaining the different forcewake usage
>       (Chris, Daniel).
>     - Use open-coded forcewake functions (Daniel).
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c      |  2 --
>  drivers/gpu/drm/i915/i915_drv.c      |  8 +++++
>  drivers/gpu/drm/i915/i915_drv.h      |  7 ++--
>  drivers/gpu/drm/i915/i915_params.c   | 10 ------
>  drivers/gpu/drm/i915/intel_display.c | 65 ++++++++++++++++--------------------
>  drivers/gpu/drm/i915/intel_drv.h     |  3 +-
>  drivers/gpu/drm/i915/intel_pm.c      |  1 -
>  7 files changed, 43 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index e4d2b9f..6aa23af 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1822,8 +1822,6 @@ int i915_driver_unload(struct drm_device *dev)
>  	cancel_work_sync(&dev_priv->gpu_error.work);
>  	i915_destroy_error_state(dev);
>  
> -	cancel_delayed_work_sync(&dev_priv->pc8.enable_work);
> -
>  	if (dev->pdev->msi_enabled)
>  		pci_disable_msi(dev->pdev);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 658fe24..6178c95 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -848,6 +848,9 @@ static int i915_runtime_suspend(struct device *device)
>  
>  	DRM_DEBUG_KMS("Suspending device\n");
>  
> +	if (HAS_PC8(dev))
> +		__hsw_do_enable_pc8(dev_priv);
> +
>  	i915_gem_release_all_mmaps(dev_priv);
>  
>  	del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
> @@ -862,6 +865,7 @@ static int i915_runtime_suspend(struct device *device)
>  	 */
>  	intel_opregion_notify_adapter(dev, PCI_D1);
>  
> +	DRM_DEBUG_KMS("Device suspended\n");
>  	return 0;
>  }
>  
> @@ -878,6 +882,10 @@ static int i915_runtime_resume(struct device *device)
>  	intel_opregion_notify_adapter(dev, PCI_D0);
>  	dev_priv->pm.suspended = false;
>  
> +	if (HAS_PC8(dev))
> +		__hsw_do_disable_pc8(dev_priv);
> +
> +	DRM_DEBUG_KMS("Device resumed\n");
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2a319ba..a5f1780 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1335,6 +1335,10 @@ struct ilk_wm_values {
>  /*
>   * This struct tracks the state needed for the Package C8+ feature.
>   *
> + * TODO: we're merging the Package C8+ feature with the runtime PM support. To
> + * avoid having to update the documentation at each patch of the series, we'll
> + * do a final update at the end.
> + *
>   * Package states C8 and deeper are really deep PC states that can only be
>   * reached when all the devices on the system allow it, so even if the graphics
>   * device allows PC8+, it doesn't mean the system will actually get to these
> @@ -1388,7 +1392,6 @@ struct i915_package_c8 {
>  	bool enabled;
>  	int disable_count;
>  	struct mutex lock;
> -	struct delayed_work enable_work;
>  
>  	struct {
>  		uint32_t deimr;
> @@ -2092,8 +2095,6 @@ struct i915_params {
>  	unsigned int preliminary_hw_support;
>  	int disable_power_well;
>  	int enable_ips;
> -	int enable_pc8;
> -	int pc8_timeout;
>  	int invert_brightness;
>  	int enable_cmd_parser;
>  	/* leave bools at the end to not create holes */
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index a66ffb6..d1d7980 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -42,8 +42,6 @@ struct i915_params i915 __read_mostly = {
>  	.disable_power_well = 1,
>  	.enable_ips = 1,
>  	.fastboot = 0,
> -	.enable_pc8 = 1,
> -	.pc8_timeout = 5000,
>  	.prefault_disable = 0,
>  	.reset = true,
>  	.invert_brightness = 0,
> @@ -135,14 +133,6 @@ module_param_named(fastboot, i915.fastboot, bool, 0600);
>  MODULE_PARM_DESC(fastboot,
>  	"Try to skip unnecessary mode sets at boot time (default: false)");
>  
> -module_param_named(enable_pc8, i915.enable_pc8, int, 0600);
> -MODULE_PARM_DESC(enable_pc8,
> -	"Enable support for low power package C states (PC8+) (default: true)");
> -
> -module_param_named(pc8_timeout, i915.pc8_timeout, int, 0600);
> -MODULE_PARM_DESC(pc8_timeout,
> -	"Number of msecs of idleness required to enter PC8+ (default: 5000)");
> -
>  module_param_named(prefault_disable, i915.prefault_disable, bool, 0600);
>  MODULE_PARM_DESC(prefault_disable,
>  	"Disable page prefaulting for pread/pwrite/reloc (default:false). "
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9c3e2c5..ab02848 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6726,6 +6726,7 @@ static void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
>  static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
>  {
>  	uint32_t val;
> +	unsigned long irqflags;
>  
>  	val = I915_READ(LCPLL_CTL);
>  
> @@ -6733,9 +6734,22 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
>  		    LCPLL_POWER_DOWN_ALLOW)) == LCPLL_PLL_LOCK)
>  		return;
>  
> -	/* Make sure we're not on PC8 state before disabling PC8, otherwise
> -	 * we'll hang the machine! */
> -	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
> +	/*
> +	 * Make sure we're not on PC8 state before disabling PC8, otherwise

While at it the above comment could be clarified. For me 'before
enabling PLL, which also disables PC8' would be clearer, if that's what
was meant.

> +	 * we'll hang the machine. To prevent PC8 state, just enable force_wake.
> +	 *
> +	 * The other problem is that hsw_restore_lcpll() is called as part of
> +	 * the runtime PM resume sequence, so we can't just call
> +	 * gen6_gt_force_wake_get() because that function calls
> +	 * intel_runtime_pm_get(), and we can't change the runtime PM refcount
> +	 * while we are on the resume sequence. So to solve this problem we have
> +	 * to call special forcewake code that doesn't touch runtime PM and
> +	 * doesn't enable the forcewake delayed work.
> +	 */
> +	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> +	if (dev_priv->uncore.forcewake_count++ == 0)
> +		dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
> +	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  
>  	if (val & LCPLL_POWER_DOWN_ALLOW) {
>  		val &= ~LCPLL_POWER_DOWN_ALLOW;
> @@ -6769,14 +6783,20 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
>  			DRM_ERROR("Switching back to LCPLL failed\n");
>  	}
>  
> -	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
> +	/* See the big comment above. */
> +	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> +	if (--dev_priv->uncore.forcewake_count == 0)
> +		dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
> +	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);

For clarity I would group these in separate functions with the rest of
force wake helpers in intel_uncore.c.

Otherwise looks good, so with the above fixed:
Reviewed-by: Imre Deak <imre.deak@intel.com>

>  }
>  
> -static void __hsw_do_enable_pc8(struct drm_i915_private *dev_priv)
> +void __hsw_do_enable_pc8(struct drm_i915_private *dev_priv)
>  {
>  	struct drm_device *dev = dev_priv->dev;
>  	uint32_t val;
>  
> +	WARN_ON(!HAS_PC8(dev));
> +
>  	DRM_DEBUG_KMS("Enabling package C8+\n");
>  
>  	dev_priv->pc8.enabled = true;
> @@ -6792,22 +6812,6 @@ static void __hsw_do_enable_pc8(struct drm_i915_private *dev_priv)
>  	hsw_disable_lcpll(dev_priv, true, true);
>  }
>  
> -void hsw_enable_pc8_work(struct work_struct *__work)
> -{
> -	struct drm_i915_private *dev_priv =
> -		container_of(to_delayed_work(__work), struct drm_i915_private,
> -			     pc8.enable_work);
> -	struct drm_device *dev = dev_priv->dev;
> -
> -	WARN_ON(!HAS_PC8(dev));
> -
> -	if (dev_priv->pc8.enabled)
> -		return;
> -
> -	__hsw_do_enable_pc8(dev_priv);
> -	intel_runtime_pm_put(dev_priv);
> -}
> -
>  static void __hsw_enable_package_c8(struct drm_i915_private *dev_priv)
>  {
>  	WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
> @@ -6818,15 +6822,16 @@ static void __hsw_enable_package_c8(struct drm_i915_private *dev_priv)
>  	if (dev_priv->pc8.disable_count != 0)
>  		return;
>  
> -	schedule_delayed_work(&dev_priv->pc8.enable_work,
> -			      msecs_to_jiffies(i915.pc8_timeout));
> +	intel_runtime_pm_put(dev_priv);
>  }
>  
> -static void __hsw_do_disable_package_c8(struct drm_i915_private *dev_priv)
> +void __hsw_do_disable_pc8(struct drm_i915_private *dev_priv)
>  {
>  	struct drm_device *dev = dev_priv->dev;
>  	uint32_t val;
>  
> +	WARN_ON(!HAS_PC8(dev));
> +
>  	DRM_DEBUG_KMS("Disabling package C8+\n");
>  
>  	hsw_restore_lcpll(dev_priv);
> @@ -6849,8 +6854,6 @@ static void __hsw_do_disable_package_c8(struct drm_i915_private *dev_priv)
>  
>  static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv)
>  {
> -	struct drm_device *dev = dev_priv->dev;
> -
>  	WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
>  	WARN(dev_priv->pc8.disable_count < 0,
>  	     "pc8.disable_count: %d\n", dev_priv->pc8.disable_count);
> @@ -6859,14 +6862,7 @@ static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv)
>  	if (dev_priv->pc8.disable_count != 1)
>  		return;
>  
> -	WARN_ON(!HAS_PC8(dev));
> -
> -	cancel_delayed_work_sync(&dev_priv->pc8.enable_work);
> -	if (!dev_priv->pc8.enabled)
> -		return;
> -
>  	intel_runtime_pm_get(dev_priv);
> -	__hsw_do_disable_package_c8(dev_priv);
>  }
>  
>  void hsw_enable_package_c8(struct drm_i915_private *dev_priv)
> @@ -6924,9 +6920,6 @@ static void hsw_update_package_c8(struct drm_device *dev)
>  	if (!HAS_PC8(dev_priv->dev))
>  		return;
>  
> -	if (!i915.enable_pc8)
> -		return;
> -
>  	mutex_lock(&dev_priv->pc8.lock);
>  
>  	allow = hsw_can_enable_package_c8(dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 805d207..3f7ed0f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -722,7 +722,8 @@ unsigned long intel_gen4_compute_page_offset(int *x, int *y,
>  					     unsigned int bpp,
>  					     unsigned int pitch);
>  void intel_display_handle_reset(struct drm_device *dev);
> -void hsw_enable_pc8_work(struct work_struct *__work);
> +void __hsw_do_enable_pc8(struct drm_i915_private *dev_priv);
> +void __hsw_do_disable_pc8(struct drm_i915_private *dev_priv);
>  void hsw_enable_package_c8(struct drm_i915_private *dev_priv);
>  void hsw_disable_package_c8(struct drm_i915_private *dev_priv);
>  void intel_dp_get_m_n(struct intel_crtc *crtc,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index ad58ce3..c5c36dc 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -6161,7 +6161,6 @@ void intel_pm_setup(struct drm_device *dev)
>  	dev_priv->pc8.irqs_disabled = false;
>  	dev_priv->pc8.enabled = false;
>  	dev_priv->pc8.disable_count = 1; /* requirements_met */
> -	INIT_DELAYED_WORK(&dev_priv->pc8.enable_work, hsw_enable_pc8_work);
>  	INIT_DELAYED_WORK(&dev_priv->rps.delayed_resume_work,
>  			  intel_gen6_powersave_work);
>  }


[-- 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 02/16] drm/i915: make PC8 be part of runtime PM suspend/resume

[Daniel Vetter]

On Wed, Mar 19, 2014 at 05:07:53PM +0200, Imre Deak wrote:
> On Fri, 2014-03-07 at 20:08 -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > Currently, when our driver becomes idle for i915.pc8_timeout (default:
> > 5s) we enable PC8, so we save some power, but not everything we can.
> > Then, while PC8 is enabled, if we stay idle for more
> > autosuspend_delay_ms (default: 10s) we'll enter runtime PM and put the
> > graphics device in D3 state, saving even more power. The two features
> > are separate things with increasing levels of power savings, but if we
> > disable PC8 we'll never get into D3.
> > 
> > While from the modularity point of view it would be nice to keep these
> > features as separate, we have reasons to merge them:
> >  - We are not aware of anybody wanting a "PC8 without D3" environment.
> >  - If we keep both features as separate, we'll have to to test both
> >    PC8 and PC8+D3 code paths. We're already having a major pain to
> >    make QA do automated testing of just one thing, testing both paths
> >    will cost even more.
> >  - Only Haswell+ supports PC8, so if we want to add runtime PM support
> >    to, for example, IVB, we'll have to copy some code from the PC8
> >    feature to runtime PM, so merging both features as a single thing
> >    will make it easier for enabling runtime PM on other platforms.
> > 
> > This patch only does the very basic steps required to have PC8 and
> > runtime PM merged on a single feature: the next patches will take care
> > of cleaning up everything.
> > 
> > v2: - Rebase.
> > v3: - Rebase.
> >     - Fully remove the deprecated i915 params since Daniel doesn't
> >       consider them as part of the ABI.
> > v4: - Rebase.
> >     - Fix typo in the commit message.
> > v5: - Rebase, again.
> >     - Add a huge comment explaining the different forcewake usage
> >       (Chris, Daniel).
> >     - Use open-coded forcewake functions (Daniel).
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c      |  2 --
> >  drivers/gpu/drm/i915/i915_drv.c      |  8 +++++
> >  drivers/gpu/drm/i915/i915_drv.h      |  7 ++--
> >  drivers/gpu/drm/i915/i915_params.c   | 10 ------
> >  drivers/gpu/drm/i915/intel_display.c | 65 ++++++++++++++++--------------------
> >  drivers/gpu/drm/i915/intel_drv.h     |  3 +-
> >  drivers/gpu/drm/i915/intel_pm.c      |  1 -
> >  7 files changed, 43 insertions(+), 53 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index e4d2b9f..6aa23af 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1822,8 +1822,6 @@ int i915_driver_unload(struct drm_device *dev)
> >  	cancel_work_sync(&dev_priv->gpu_error.work);
> >  	i915_destroy_error_state(dev);
> >  
> > -	cancel_delayed_work_sync(&dev_priv->pc8.enable_work);
> > -
> >  	if (dev->pdev->msi_enabled)
> >  		pci_disable_msi(dev->pdev);
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 658fe24..6178c95 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -848,6 +848,9 @@ static int i915_runtime_suspend(struct device *device)
> >  
> >  	DRM_DEBUG_KMS("Suspending device\n");
> >  
> > +	if (HAS_PC8(dev))
> > +		__hsw_do_enable_pc8(dev_priv);
> > +
> >  	i915_gem_release_all_mmaps(dev_priv);
> >  
> >  	del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
> > @@ -862,6 +865,7 @@ static int i915_runtime_suspend(struct device *device)
> >  	 */
> >  	intel_opregion_notify_adapter(dev, PCI_D1);
> >  
> > +	DRM_DEBUG_KMS("Device suspended\n");
> >  	return 0;
> >  }
> >  
> > @@ -878,6 +882,10 @@ static int i915_runtime_resume(struct device *device)
> >  	intel_opregion_notify_adapter(dev, PCI_D0);
> >  	dev_priv->pm.suspended = false;
> >  
> > +	if (HAS_PC8(dev))
> > +		__hsw_do_disable_pc8(dev_priv);
> > +
> > +	DRM_DEBUG_KMS("Device resumed\n");
> >  	return 0;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 2a319ba..a5f1780 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1335,6 +1335,10 @@ struct ilk_wm_values {
> >  /*
> >   * This struct tracks the state needed for the Package C8+ feature.
> >   *
> > + * TODO: we're merging the Package C8+ feature with the runtime PM support. To
> > + * avoid having to update the documentation at each patch of the series, we'll
> > + * do a final update at the end.
> > + *
> >   * Package states C8 and deeper are really deep PC states that can only be
> >   * reached when all the devices on the system allow it, so even if the graphics
> >   * device allows PC8+, it doesn't mean the system will actually get to these
> > @@ -1388,7 +1392,6 @@ struct i915_package_c8 {
> >  	bool enabled;
> >  	int disable_count;
> >  	struct mutex lock;
> > -	struct delayed_work enable_work;
> >  
> >  	struct {
> >  		uint32_t deimr;
> > @@ -2092,8 +2095,6 @@ struct i915_params {
> >  	unsigned int preliminary_hw_support;
> >  	int disable_power_well;
> >  	int enable_ips;
> > -	int enable_pc8;
> > -	int pc8_timeout;
> >  	int invert_brightness;
> >  	int enable_cmd_parser;
> >  	/* leave bools at the end to not create holes */
> > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> > index a66ffb6..d1d7980 100644
> > --- a/drivers/gpu/drm/i915/i915_params.c
> > +++ b/drivers/gpu/drm/i915/i915_params.c
> > @@ -42,8 +42,6 @@ struct i915_params i915 __read_mostly = {
> >  	.disable_power_well = 1,
> >  	.enable_ips = 1,
> >  	.fastboot = 0,
> > -	.enable_pc8 = 1,
> > -	.pc8_timeout = 5000,
> >  	.prefault_disable = 0,
> >  	.reset = true,
> >  	.invert_brightness = 0,
> > @@ -135,14 +133,6 @@ module_param_named(fastboot, i915.fastboot, bool, 0600);
> >  MODULE_PARM_DESC(fastboot,
> >  	"Try to skip unnecessary mode sets at boot time (default: false)");
> >  
> > -module_param_named(enable_pc8, i915.enable_pc8, int, 0600);
> > -MODULE_PARM_DESC(enable_pc8,
> > -	"Enable support for low power package C states (PC8+) (default: true)");
> > -
> > -module_param_named(pc8_timeout, i915.pc8_timeout, int, 0600);
> > -MODULE_PARM_DESC(pc8_timeout,
> > -	"Number of msecs of idleness required to enter PC8+ (default: 5000)");
> > -
> >  module_param_named(prefault_disable, i915.prefault_disable, bool, 0600);
> >  MODULE_PARM_DESC(prefault_disable,
> >  	"Disable page prefaulting for pread/pwrite/reloc (default:false). "
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 9c3e2c5..ab02848 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6726,6 +6726,7 @@ static void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
> >  static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
> >  {
> >  	uint32_t val;
> > +	unsigned long irqflags;
> >  
> >  	val = I915_READ(LCPLL_CTL);
> >  
> > @@ -6733,9 +6734,22 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
> >  		    LCPLL_POWER_DOWN_ALLOW)) == LCPLL_PLL_LOCK)
> >  		return;
> >  
> > -	/* Make sure we're not on PC8 state before disabling PC8, otherwise
> > -	 * we'll hang the machine! */
> > -	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
> > +	/*
> > +	 * Make sure we're not on PC8 state before disabling PC8, otherwise
> 
> While at it the above comment could be clarified. For me 'before
> enabling PLL, which also disables PC8' would be clearer, if that's what
> was meant.

I think we can do this comment polishing later on.

> > +	 * we'll hang the machine. To prevent PC8 state, just enable force_wake.
> > +	 *
> > +	 * The other problem is that hsw_restore_lcpll() is called as part of
> > +	 * the runtime PM resume sequence, so we can't just call
> > +	 * gen6_gt_force_wake_get() because that function calls
> > +	 * intel_runtime_pm_get(), and we can't change the runtime PM refcount
> > +	 * while we are on the resume sequence. So to solve this problem we have
> > +	 * to call special forcewake code that doesn't touch runtime PM and
> > +	 * doesn't enable the forcewake delayed work.
> > +	 */
> > +	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> > +	if (dev_priv->uncore.forcewake_count++ == 0)
> > +		dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
> > +	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> >  
> >  	if (val & LCPLL_POWER_DOWN_ALLOW) {
> >  		val &= ~LCPLL_POWER_DOWN_ALLOW;
> > @@ -6769,14 +6783,20 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
> >  			DRM_ERROR("Switching back to LCPLL failed\n");
> >  	}
> >  
> > -	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
> > +	/* See the big comment above. */
> > +	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> > +	if (--dev_priv->uncore.forcewake_count == 0)
> > +		dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
> > +	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> 
> For clarity I would group these in separate functions with the rest of
> force wake helpers in intel_uncore.c.

tbh we currently have a rather big mess with all our forcewake functions
and the setup/init code. Before we encapsulate things like crazy I think
we should work out all the existing little bugs and races we have :(
> 
> Otherwise looks good, so with the above fixed:
> Reviewed-by: Imre Deak <imre.deak@intel.com>

Thanks for patches&review, merged it all to dinq.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 03/16] drm/i915: get/put runtime PM when we get/put a power domain

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Any power domain will require the HW to be in PCI D0 state, so just do
the simple thing.

Dear maintainer: since intel_display_power_put() and
intel_display_power_get() are almost identical, git-am has failed
apply the patch on my local machine once: it added both chunks to
put(), instead of one chunk to get() and another to put(). When you
apply this patch to your tree, please check if it is correct.

v2: - Add the warning above.
v3: - Rebase.

Reviewed-by: Imre Deak <imre.deak@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index c5c36dc..53ac8d0 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5577,6 +5577,8 @@ void intel_display_power_get(struct drm_i915_private *dev_priv,
 	struct i915_power_well *power_well;
 	int i;
 
+	intel_runtime_pm_get(dev_priv);
+
 	power_domains = &dev_priv->power_domains;
 
 	mutex_lock(&power_domains->lock);
@@ -5621,6 +5623,8 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
 	}
 
 	mutex_unlock(&power_domains->lock);
+
+	intel_runtime_pm_put(dev_priv);
 }
 
 static struct i915_power_domains *hsw_pwr;
-- 
1.8.5.3

[PATCH 04/16] drm/i915: remove dev_priv->pc8.requirements_met

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

The requirements_met variable was used to track two things: enabled
CRTCs and the power well. After the latest chagnes, we get a runtime
PM reference whenever we get any of the power domains, and we get
power domains when we enable CRTCs or the power well, so we should
already be covered, not needing this specific tracking.

v2: - Rebase.
v3: - Rebase.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |  2 --
 drivers/gpu/drm/i915/i915_drv.h      |  1 -
 drivers/gpu/drm/i915/intel_display.c | 54 ------------------------------------
 drivers/gpu/drm/i915/intel_pm.c      |  5 ++--
 4 files changed, 3 insertions(+), 59 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 6ee529e..6f96b8f 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2013,8 +2013,6 @@ static int i915_pc8_status(struct seq_file *m, void *unused)
 	}
 
 	mutex_lock(&dev_priv->pc8.lock);
-	seq_printf(m, "Requirements met: %s\n",
-		   yesno(dev_priv->pc8.requirements_met));
 	seq_printf(m, "GPU idle: %s\n", yesno(!dev_priv->mm.busy));
 	seq_printf(m, "Disable count: %d\n", dev_priv->pc8.disable_count);
 	seq_printf(m, "IRQs disabled: %s\n",
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a5f1780..75faa51 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1386,7 +1386,6 @@ struct ilk_wm_values {
  * For more, read "Display Sequences for Package C8" on our documentation.
  */
 struct i915_package_c8 {
-	bool requirements_met;
 	bool irqs_disabled;
 	/* Only true after the delayed work task actually enables it. */
 	bool enabled;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ab02848..ea9165b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6885,63 +6885,9 @@ void hsw_disable_package_c8(struct drm_i915_private *dev_priv)
 	mutex_unlock(&dev_priv->pc8.lock);
 }
 
-static bool hsw_can_enable_package_c8(struct drm_i915_private *dev_priv)
-{
-	struct drm_device *dev = dev_priv->dev;
-	struct intel_crtc *crtc;
-	uint32_t val;
-
-	list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head)
-		if (crtc->base.enabled)
-			return false;
-
-	/* This case is still possible since we have the i915.disable_power_well
-	 * parameter and also the KVMr or something else might be requesting the
-	 * power well. */
-	val = I915_READ(HSW_PWR_WELL_DRIVER);
-	if (val != 0) {
-		DRM_DEBUG_KMS("Not enabling PC8: power well on\n");
-		return false;
-	}
-
-	return true;
-}
-
-/* Since we're called from modeset_global_resources there's no way to
- * symmetrically increase and decrease the refcount, so we use
- * dev_priv->pc8.requirements_met to track whether we already have the refcount
- * or not.
- */
-static void hsw_update_package_c8(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	bool allow;
-
-	if (!HAS_PC8(dev_priv->dev))
-		return;
-
-	mutex_lock(&dev_priv->pc8.lock);
-
-	allow = hsw_can_enable_package_c8(dev_priv);
-
-	if (allow == dev_priv->pc8.requirements_met)
-		goto done;
-
-	dev_priv->pc8.requirements_met = allow;
-
-	if (allow)
-		__hsw_enable_package_c8(dev_priv);
-	else
-		__hsw_disable_package_c8(dev_priv);
-
-done:
-	mutex_unlock(&dev_priv->pc8.lock);
-}
-
 static void haswell_modeset_global_resources(struct drm_device *dev)
 {
 	modeset_update_crtc_power_domains(dev);
-	hsw_update_package_c8(dev);
 }
 
 static int haswell_crtc_mode_set(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 53ac8d0..0c69ee2 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5938,6 +5938,8 @@ void intel_init_runtime_pm(struct drm_i915_private *dev_priv)
 	pm_runtime_set_autosuspend_delay(device, 10000); /* 10s */
 	pm_runtime_mark_last_busy(device);
 	pm_runtime_use_autosuspend(device);
+
+	pm_runtime_put_autosuspend(device);
 }
 
 void intel_fini_runtime_pm(struct drm_i915_private *dev_priv)
@@ -6161,10 +6163,9 @@ void intel_pm_setup(struct drm_device *dev)
 	mutex_init(&dev_priv->rps.hw_lock);
 
 	mutex_init(&dev_priv->pc8.lock);
-	dev_priv->pc8.requirements_met = false;
 	dev_priv->pc8.irqs_disabled = false;
 	dev_priv->pc8.enabled = false;
-	dev_priv->pc8.disable_count = 1; /* requirements_met */
+	dev_priv->pc8.disable_count = 0;
 	INIT_DELAYED_WORK(&dev_priv->rps.delayed_resume_work,
 			  intel_gen6_powersave_work);
 }
-- 
1.8.5.3

Re: [PATCH 04/16] drm/i915: remove dev_priv->pc8.requirements_met

[Imre Deak]

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

On Fri, 2014-03-07 at 20:08 -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> The requirements_met variable was used to track two things: enabled
> CRTCs and the power well. After the latest chagnes, we get a runtime
> PM reference whenever we get any of the power domains, and we get
> power domains when we enable CRTCs or the power well, so we should
> already be covered, not needing this specific tracking.
> 
> v2: - Rebase.
> v3: - Rebase.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

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

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  |  2 --
>  drivers/gpu/drm/i915/i915_drv.h      |  1 -
>  drivers/gpu/drm/i915/intel_display.c | 54 ------------------------------------
>  drivers/gpu/drm/i915/intel_pm.c      |  5 ++--
>  4 files changed, 3 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 6ee529e..6f96b8f 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2013,8 +2013,6 @@ static int i915_pc8_status(struct seq_file *m, void *unused)
>  	}
>  
>  	mutex_lock(&dev_priv->pc8.lock);
> -	seq_printf(m, "Requirements met: %s\n",
> -		   yesno(dev_priv->pc8.requirements_met));
>  	seq_printf(m, "GPU idle: %s\n", yesno(!dev_priv->mm.busy));
>  	seq_printf(m, "Disable count: %d\n", dev_priv->pc8.disable_count);
>  	seq_printf(m, "IRQs disabled: %s\n",
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a5f1780..75faa51 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1386,7 +1386,6 @@ struct ilk_wm_values {
>   * For more, read "Display Sequences for Package C8" on our documentation.
>   */
>  struct i915_package_c8 {
> -	bool requirements_met;
>  	bool irqs_disabled;
>  	/* Only true after the delayed work task actually enables it. */
>  	bool enabled;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ab02848..ea9165b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6885,63 +6885,9 @@ void hsw_disable_package_c8(struct drm_i915_private *dev_priv)
>  	mutex_unlock(&dev_priv->pc8.lock);
>  }
>  
> -static bool hsw_can_enable_package_c8(struct drm_i915_private *dev_priv)
> -{
> -	struct drm_device *dev = dev_priv->dev;
> -	struct intel_crtc *crtc;
> -	uint32_t val;
> -
> -	list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head)
> -		if (crtc->base.enabled)
> -			return false;
> -
> -	/* This case is still possible since we have the i915.disable_power_well
> -	 * parameter and also the KVMr or something else might be requesting the
> -	 * power well. */
> -	val = I915_READ(HSW_PWR_WELL_DRIVER);
> -	if (val != 0) {
> -		DRM_DEBUG_KMS("Not enabling PC8: power well on\n");
> -		return false;
> -	}
> -
> -	return true;
> -}
> -
> -/* Since we're called from modeset_global_resources there's no way to
> - * symmetrically increase and decrease the refcount, so we use
> - * dev_priv->pc8.requirements_met to track whether we already have the refcount
> - * or not.
> - */
> -static void hsw_update_package_c8(struct drm_device *dev)
> -{
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	bool allow;
> -
> -	if (!HAS_PC8(dev_priv->dev))
> -		return;
> -
> -	mutex_lock(&dev_priv->pc8.lock);
> -
> -	allow = hsw_can_enable_package_c8(dev_priv);
> -
> -	if (allow == dev_priv->pc8.requirements_met)
> -		goto done;
> -
> -	dev_priv->pc8.requirements_met = allow;
> -
> -	if (allow)
> -		__hsw_enable_package_c8(dev_priv);
> -	else
> -		__hsw_disable_package_c8(dev_priv);
> -
> -done:
> -	mutex_unlock(&dev_priv->pc8.lock);
> -}
> -
>  static void haswell_modeset_global_resources(struct drm_device *dev)
>  {
>  	modeset_update_crtc_power_domains(dev);
> -	hsw_update_package_c8(dev);
>  }
>  
>  static int haswell_crtc_mode_set(struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 53ac8d0..0c69ee2 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5938,6 +5938,8 @@ void intel_init_runtime_pm(struct drm_i915_private *dev_priv)
>  	pm_runtime_set_autosuspend_delay(device, 10000); /* 10s */
>  	pm_runtime_mark_last_busy(device);
>  	pm_runtime_use_autosuspend(device);
> +
> +	pm_runtime_put_autosuspend(device);
>  }
>  
>  void intel_fini_runtime_pm(struct drm_i915_private *dev_priv)
> @@ -6161,10 +6163,9 @@ void intel_pm_setup(struct drm_device *dev)
>  	mutex_init(&dev_priv->rps.hw_lock);
>  
>  	mutex_init(&dev_priv->pc8.lock);
> -	dev_priv->pc8.requirements_met = false;
>  	dev_priv->pc8.irqs_disabled = false;
>  	dev_priv->pc8.enabled = false;
> -	dev_priv->pc8.disable_count = 1; /* requirements_met */
> +	dev_priv->pc8.disable_count = 0;
>  	INIT_DELAYED_WORK(&dev_priv->rps.delayed_resume_work,
>  			  intel_gen6_powersave_work);
>  }


[-- 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

[PATCH 05/16] drm/i915: get runtime PM references when the GPU is idle/busy

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

... instead of PC8 references. Now that both are the same thing and we
are killing PC8, just get the runtime PM reference.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ea9165b..2a3f8f9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8175,7 +8175,7 @@ void intel_mark_busy(struct drm_device *dev)
 	if (dev_priv->mm.busy)
 		return;
 
-	hsw_disable_package_c8(dev_priv);
+	intel_runtime_pm_get(dev_priv);
 	i915_update_gfx_val(dev_priv);
 	dev_priv->mm.busy = true;
 }
@@ -8204,7 +8204,7 @@ void intel_mark_idle(struct drm_device *dev)
 		gen6_rps_idle(dev->dev_private);
 
 out:
-	hsw_enable_package_c8(dev_priv);
+	intel_runtime_pm_put(dev_priv);
 }
 
 void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
-- 
1.8.5.3

[PATCH 06/16] drm/i915: kill pc8.disable_count

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Since after the latest patches it's only being used to prevent
getting/putting the runtime PM refcount.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |  1 -
 drivers/gpu/drm/i915/i915_drv.h      |  1 -
 drivers/gpu/drm/i915/intel_display.c | 14 --------------
 drivers/gpu/drm/i915/intel_pm.c      |  1 -
 4 files changed, 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 6f96b8f..9508eb0 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2014,7 +2014,6 @@ static int i915_pc8_status(struct seq_file *m, void *unused)
 
 	mutex_lock(&dev_priv->pc8.lock);
 	seq_printf(m, "GPU idle: %s\n", yesno(!dev_priv->mm.busy));
-	seq_printf(m, "Disable count: %d\n", dev_priv->pc8.disable_count);
 	seq_printf(m, "IRQs disabled: %s\n",
 		   yesno(dev_priv->pc8.irqs_disabled));
 	seq_printf(m, "Enabled: %s\n", yesno(dev_priv->pc8.enabled));
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 75faa51..c4e999c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1389,7 +1389,6 @@ struct i915_package_c8 {
 	bool irqs_disabled;
 	/* Only true after the delayed work task actually enables it. */
 	bool enabled;
-	int disable_count;
 	struct mutex lock;
 
 	struct {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2a3f8f9..88348d6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6815,13 +6815,6 @@ void __hsw_do_enable_pc8(struct drm_i915_private *dev_priv)
 static void __hsw_enable_package_c8(struct drm_i915_private *dev_priv)
 {
 	WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
-	WARN(dev_priv->pc8.disable_count < 1,
-	     "pc8.disable_count: %d\n", dev_priv->pc8.disable_count);
-
-	dev_priv->pc8.disable_count--;
-	if (dev_priv->pc8.disable_count != 0)
-		return;
-
 	intel_runtime_pm_put(dev_priv);
 }
 
@@ -6855,13 +6848,6 @@ void __hsw_do_disable_pc8(struct drm_i915_private *dev_priv)
 static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv)
 {
 	WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
-	WARN(dev_priv->pc8.disable_count < 0,
-	     "pc8.disable_count: %d\n", dev_priv->pc8.disable_count);
-
-	dev_priv->pc8.disable_count++;
-	if (dev_priv->pc8.disable_count != 1)
-		return;
-
 	intel_runtime_pm_get(dev_priv);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 0c69ee2..550491e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -6165,7 +6165,6 @@ void intel_pm_setup(struct drm_device *dev)
 	mutex_init(&dev_priv->pc8.lock);
 	dev_priv->pc8.irqs_disabled = false;
 	dev_priv->pc8.enabled = false;
-	dev_priv->pc8.disable_count = 0;
 	INIT_DELAYED_WORK(&dev_priv->rps.delayed_resume_work,
 			  intel_gen6_powersave_work);
 }
-- 
1.8.5.3

[PATCH 07/16] drm/i915: remove an indirection level on PC8 functions

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

After the latest changes, the indirection is useless.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 88348d6..e49c217 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6812,12 +6812,6 @@ void __hsw_do_enable_pc8(struct drm_i915_private *dev_priv)
 	hsw_disable_lcpll(dev_priv, true, true);
 }
 
-static void __hsw_enable_package_c8(struct drm_i915_private *dev_priv)
-{
-	WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
-	intel_runtime_pm_put(dev_priv);
-}
-
 void __hsw_do_disable_pc8(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
@@ -6845,19 +6839,13 @@ void __hsw_do_disable_pc8(struct drm_i915_private *dev_priv)
 	dev_priv->pc8.enabled = false;
 }
 
-static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv)
-{
-	WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
-	intel_runtime_pm_get(dev_priv);
-}
-
 void hsw_enable_package_c8(struct drm_i915_private *dev_priv)
 {
 	if (!HAS_PC8(dev_priv->dev))
 		return;
 
 	mutex_lock(&dev_priv->pc8.lock);
-	__hsw_enable_package_c8(dev_priv);
+	intel_runtime_pm_put(dev_priv);
 	mutex_unlock(&dev_priv->pc8.lock);
 }
 
@@ -6867,7 +6855,7 @@ void hsw_disable_package_c8(struct drm_i915_private *dev_priv)
 		return;
 
 	mutex_lock(&dev_priv->pc8.lock);
-	__hsw_disable_package_c8(dev_priv);
+	intel_runtime_pm_get(dev_priv);
 	mutex_unlock(&dev_priv->pc8.lock);
 }
 
-- 
1.8.5.3

Re: [PATCH 07/16] drm/i915: remove an indirection level on PC8 functions

[Chris Wilson]

On Fri, Mar 07, 2014 at 08:08:10PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> After the latest changes, the indirection is useless.
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 16 ++--------------
>  1 file changed, 2 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 88348d6..e49c217 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6812,12 +6812,6 @@ void __hsw_do_enable_pc8(struct drm_i915_private *dev_priv)
>  	hsw_disable_lcpll(dev_priv, true, true);
>  }
>  
> -static void __hsw_enable_package_c8(struct drm_i915_private *dev_priv)
> -{
> -	WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
> -	intel_runtime_pm_put(dev_priv);
> -}
> -
>  void __hsw_do_disable_pc8(struct drm_i915_private *dev_priv)
>  {
>  	struct drm_device *dev = dev_priv->dev;
> @@ -6845,19 +6839,13 @@ void __hsw_do_disable_pc8(struct drm_i915_private *dev_priv)
>  	dev_priv->pc8.enabled = false;
>  }
>  
> -static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv)
> -{
> -	WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
> -	intel_runtime_pm_get(dev_priv);
> -}
> -
>  void hsw_enable_package_c8(struct drm_i915_private *dev_priv)
>  {
>  	if (!HAS_PC8(dev_priv->dev))
>  		return;
>  
>  	mutex_lock(&dev_priv->pc8.lock);
> -	__hsw_enable_package_c8(dev_priv);
> +	intel_runtime_pm_put(dev_priv);
>  	mutex_unlock(&dev_priv->pc8.lock);
>  }
>  
> @@ -6867,7 +6855,7 @@ void hsw_disable_package_c8(struct drm_i915_private *dev_priv)
>  		return;
>  
>  	mutex_lock(&dev_priv->pc8.lock);
> -	__hsw_disable_package_c8(dev_priv);
> +	intel_runtime_pm_get(dev_priv);
>  	mutex_unlock(&dev_priv->pc8.lock);

Now I am very confused about the locking.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 07/16] drm/i915: remove an indirection level on PC8 functions

[Daniel Vetter]

On Tue, Mar 11, 2014 at 08:20:24AM +0000, Chris Wilson wrote:
> On Fri, Mar 07, 2014 at 08:08:10PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > After the latest changes, the indirection is useless.
> > 
> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 16 ++--------------
> >  1 file changed, 2 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 88348d6..e49c217 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6812,12 +6812,6 @@ void __hsw_do_enable_pc8(struct drm_i915_private *dev_priv)
> >  	hsw_disable_lcpll(dev_priv, true, true);
> >  }
> >  
> > -static void __hsw_enable_package_c8(struct drm_i915_private *dev_priv)
> > -{
> > -	WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
> > -	intel_runtime_pm_put(dev_priv);
> > -}
> > -
> >  void __hsw_do_disable_pc8(struct drm_i915_private *dev_priv)
> >  {
> >  	struct drm_device *dev = dev_priv->dev;
> > @@ -6845,19 +6839,13 @@ void __hsw_do_disable_pc8(struct drm_i915_private *dev_priv)
> >  	dev_priv->pc8.enabled = false;
> >  }
> >  
> > -static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv)
> > -{
> > -	WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
> > -	intel_runtime_pm_get(dev_priv);
> > -}
> > -
> >  void hsw_enable_package_c8(struct drm_i915_private *dev_priv)
> >  {
> >  	if (!HAS_PC8(dev_priv->dev))
> >  		return;
> >  
> >  	mutex_lock(&dev_priv->pc8.lock);
> > -	__hsw_enable_package_c8(dev_priv);
> > +	intel_runtime_pm_put(dev_priv);
> >  	mutex_unlock(&dev_priv->pc8.lock);
> >  }
> >  
> > @@ -6867,7 +6855,7 @@ void hsw_disable_package_c8(struct drm_i915_private *dev_priv)
> >  		return;
> >  
> >  	mutex_lock(&dev_priv->pc8.lock);
> > -	__hsw_disable_package_c8(dev_priv);
> > +	intel_runtime_pm_get(dev_priv);
> >  	mutex_unlock(&dev_priv->pc8.lock);
> 
> Now I am very confused about the locking.

Afacs this confusion all goes away a few patches later on. Otherwise I
think this would indeed be ... strange.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 08/16] drm/i915: don't get/put PC8 reference on freeze/thaw

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

We already get runtime PM references, and PC8 is now part of runtime
PM, so this is enough.

v2: - Rebase.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 6178c95..381a225 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -428,7 +428,6 @@ static int i915_drm_freeze(struct drm_device *dev)
 
 	/* We do a lot of poking in a lot of registers, make sure they work
 	 * properly. */
-	hsw_disable_package_c8(dev_priv);
 	intel_display_set_init_power(dev_priv, true);
 
 	drm_kms_helper_poll_disable(dev);
@@ -603,10 +602,6 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
 		schedule_work(&dev_priv->console_resume_work);
 	}
 
-	/* Undo what we did at i915_drm_freeze so the refcount goes back to the
-	 * expected level. */
-	hsw_enable_package_c8(dev_priv);
-
 	mutex_lock(&dev_priv->modeset_restore_lock);
 	dev_priv->modeset_restore = MODESET_DONE;
 	mutex_unlock(&dev_priv->modeset_restore_lock);
-- 
1.8.5.3

[PATCH 09/16] drm/i915: make intel_aux_display_runtime_get get runtime PM, not PC8

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Because we merged the PC8 and runtime PM features, so calling
intel_runtime_pm_get now has the same meaning, and we plan to just
remove hsw_disable_package_c8 for this exact reason.

My first patch tried to completely kill
intel_aux_display_runtime_get/put, because I was assuming that whoever
needed more than just runtime PM would have to get the appropriate
power domain instead of that, but it seems some people still want the
intel_aux_display_runtime_get abstraction, so keep it until someone
else tries to replace it with the more-standard power domain calls.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 550491e..8df5f8f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5888,15 +5888,14 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv)
 	intel_power_domains_resume(dev_priv);
 }
 
-/* Disables PC8 so we can use the GMBUS and DP AUX interrupts. */
 void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv)
 {
-	hsw_disable_package_c8(dev_priv);
+	intel_runtime_pm_get(dev_priv);
 }
 
 void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv)
 {
-	hsw_enable_package_c8(dev_priv);
+	intel_runtime_pm_put(dev_priv);
 }
 
 void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
-- 
1.8.5.3

Re: [PATCH 09/16] drm/i915: make intel_aux_display_runtime_get get runtime PM, not PC8

[Imre Deak]

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

On Fri, 2014-03-07 at 20:08 -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Because we merged the PC8 and runtime PM features, so calling
> intel_runtime_pm_get now has the same meaning, and we plan to just
> remove hsw_disable_package_c8 for this exact reason.
> 
> My first patch tried to completely kill
> intel_aux_display_runtime_get/put, because I was assuming that whoever
> needed more than just runtime PM would have to get the appropriate
> power domain instead of that, but it seems some people still want the
> intel_aux_display_runtime_get abstraction, so keep it until someone
> else tries to replace it with the more-standard power domain calls.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

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

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 550491e..8df5f8f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5888,15 +5888,14 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv)
>  	intel_power_domains_resume(dev_priv);
>  }
>  
> -/* Disables PC8 so we can use the GMBUS and DP AUX interrupts. */
>  void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv)
>  {
> -	hsw_disable_package_c8(dev_priv);
> +	intel_runtime_pm_get(dev_priv);
>  }
>  
>  void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv)
>  {
> -	hsw_enable_package_c8(dev_priv);
> +	intel_runtime_pm_put(dev_priv);
>  }
>  
>  void intel_runtime_pm_get(struct drm_i915_private *dev_priv)


[-- 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

[PATCH 10/16] drm/i915: don't get/put PC8 when getting/putting power wells

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Because we already get/put runtime PM every time we get/put any power
domain, and now PC8 and runtime PM are the same thing.

With this, we can also now kill the hsw_{en,dis}able_package_c8
functions.

v2: - Rebase.
v3: - Rebase.
v4: - Rebase.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 20 --------------------
 drivers/gpu/drm/i915/intel_drv.h     |  2 --
 drivers/gpu/drm/i915/intel_pm.c      |  2 --
 3 files changed, 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e49c217..caecbc7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6839,26 +6839,6 @@ void __hsw_do_disable_pc8(struct drm_i915_private *dev_priv)
 	dev_priv->pc8.enabled = false;
 }
 
-void hsw_enable_package_c8(struct drm_i915_private *dev_priv)
-{
-	if (!HAS_PC8(dev_priv->dev))
-		return;
-
-	mutex_lock(&dev_priv->pc8.lock);
-	intel_runtime_pm_put(dev_priv);
-	mutex_unlock(&dev_priv->pc8.lock);
-}
-
-void hsw_disable_package_c8(struct drm_i915_private *dev_priv)
-{
-	if (!HAS_PC8(dev_priv->dev))
-		return;
-
-	mutex_lock(&dev_priv->pc8.lock);
-	intel_runtime_pm_get(dev_priv);
-	mutex_unlock(&dev_priv->pc8.lock);
-}
-
 static void haswell_modeset_global_resources(struct drm_device *dev)
 {
 	modeset_update_crtc_power_domains(dev);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3f7ed0f..b26cfc6 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -724,8 +724,6 @@ unsigned long intel_gen4_compute_page_offset(int *x, int *y,
 void intel_display_handle_reset(struct drm_device *dev);
 void __hsw_do_enable_pc8(struct drm_i915_private *dev_priv);
 void __hsw_do_disable_pc8(struct drm_i915_private *dev_priv);
-void hsw_enable_package_c8(struct drm_i915_private *dev_priv);
-void hsw_disable_package_c8(struct drm_i915_private *dev_priv);
 void intel_dp_get_m_n(struct intel_crtc *crtc,
 		      struct intel_crtc_config *pipe_config);
 int intel_dotclock_calculate(int link_freq, const struct intel_link_m_n *m_n);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 8df5f8f..6068497 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5391,7 +5391,6 @@ static void hsw_power_well_sync_hw(struct drm_i915_private *dev_priv,
 static void hsw_power_well_enable(struct drm_i915_private *dev_priv,
 				  struct i915_power_well *power_well)
 {
-	hsw_disable_package_c8(dev_priv);
 	hsw_set_power_well(dev_priv, power_well, true);
 }
 
@@ -5399,7 +5398,6 @@ static void hsw_power_well_disable(struct drm_i915_private *dev_priv,
 				   struct i915_power_well *power_well)
 {
 	hsw_set_power_well(dev_priv, power_well, false);
-	hsw_enable_package_c8(dev_priv);
 }
 
 static void i9xx_always_on_power_well_noop(struct drm_i915_private *dev_priv,
-- 
1.8.5.3

[PATCH 11/16] drm/i915: remove dev_priv->pc8.enabled

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

It was just being used on debugfs and on a WARN inside
hsw_set_power_well. But now that we PC8 is part of runtime PM and we
get/put runtime PM when we get/put any power domain, we shouldn't need
the WARN anymore.

v2: - Rebase.
v3: - Rebase.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  | 1 -
 drivers/gpu/drm/i915/i915_drv.h      | 2 --
 drivers/gpu/drm/i915/intel_display.c | 3 ---
 drivers/gpu/drm/i915/intel_pm.c      | 3 ---
 4 files changed, 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 9508eb0..920b7dc 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2016,7 +2016,6 @@ static int i915_pc8_status(struct seq_file *m, void *unused)
 	seq_printf(m, "GPU idle: %s\n", yesno(!dev_priv->mm.busy));
 	seq_printf(m, "IRQs disabled: %s\n",
 		   yesno(dev_priv->pc8.irqs_disabled));
-	seq_printf(m, "Enabled: %s\n", yesno(dev_priv->pc8.enabled));
 	mutex_unlock(&dev_priv->pc8.lock);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c4e999c..9190707 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1387,8 +1387,6 @@ struct ilk_wm_values {
  */
 struct i915_package_c8 {
 	bool irqs_disabled;
-	/* Only true after the delayed work task actually enables it. */
-	bool enabled;
 	struct mutex lock;
 
 	struct {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index caecbc7..2fc543b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6799,8 +6799,6 @@ void __hsw_do_enable_pc8(struct drm_i915_private *dev_priv)
 
 	DRM_DEBUG_KMS("Enabling package C8+\n");
 
-	dev_priv->pc8.enabled = true;
-
 	if (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
 		val = I915_READ(SOUTH_DSPCLK_GATE_D);
 		val &= ~PCH_LP_PARTITION_LEVEL_DISABLE;
@@ -6836,7 +6834,6 @@ void __hsw_do_disable_pc8(struct drm_i915_private *dev_priv)
 	mutex_lock(&dev_priv->rps.hw_lock);
 	gen6_update_ring_freq(dev);
 	mutex_unlock(&dev_priv->rps.hw_lock);
-	dev_priv->pc8.enabled = false;
 }
 
 static void haswell_modeset_global_resources(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 6068497..3b76f6e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5345,8 +5345,6 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
 	bool is_enabled, enable_requested;
 	uint32_t tmp;
 
-	WARN_ON(dev_priv->pc8.enabled);
-
 	tmp = I915_READ(HSW_PWR_WELL_DRIVER);
 	is_enabled = tmp & HSW_PWR_WELL_STATE_ENABLED;
 	enable_requested = tmp & HSW_PWR_WELL_ENABLE_REQUEST;
@@ -6161,7 +6159,6 @@ void intel_pm_setup(struct drm_device *dev)
 
 	mutex_init(&dev_priv->pc8.lock);
 	dev_priv->pc8.irqs_disabled = false;
-	dev_priv->pc8.enabled = false;
 	INIT_DELAYED_WORK(&dev_priv->rps.delayed_resume_work,
 			  intel_gen6_powersave_work);
 }
-- 
1.8.5.3

[PATCH 12/16] drm/i915: move pc8.irqs_disabled to pm.irqs_disabled

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

When other platforms add runtime PM support they will also need to
disable interrupts, so move the variable to the runtime PM struct.

Also notice that the longer-term goal is to completely kill the
regsave struct, and I even have patches for that.

v2: - Rebase.
v3: - Rebase.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |  2 +-
 drivers/gpu/drm/i915/i915_drv.h      | 10 +++----
 drivers/gpu/drm/i915/i915_gem.c      |  2 +-
 drivers/gpu/drm/i915/i915_irq.c      | 58 ++++++++++++++++++------------------
 drivers/gpu/drm/i915/intel_display.c |  4 +--
 drivers/gpu/drm/i915/intel_drv.h     |  4 +--
 drivers/gpu/drm/i915/intel_pm.c      |  3 +-
 7 files changed, 42 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 920b7dc..9d276c0 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2015,7 +2015,7 @@ static int i915_pc8_status(struct seq_file *m, void *unused)
 	mutex_lock(&dev_priv->pc8.lock);
 	seq_printf(m, "GPU idle: %s\n", yesno(!dev_priv->mm.busy));
 	seq_printf(m, "IRQs disabled: %s\n",
-		   yesno(dev_priv->pc8.irqs_disabled));
+		   yesno(dev_priv->pm.irqs_disabled));
 	mutex_unlock(&dev_priv->pc8.lock);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9190707..f399472 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1386,8 +1386,12 @@ struct ilk_wm_values {
  * For more, read "Display Sequences for Package C8" on our documentation.
  */
 struct i915_package_c8 {
-	bool irqs_disabled;
 	struct mutex lock;
+};
+
+struct i915_runtime_pm {
+	bool suspended;
+	bool irqs_disabled;
 
 	struct {
 		uint32_t deimr;
@@ -1398,10 +1402,6 @@ struct i915_package_c8 {
 	} regsave;
 };
 
-struct i915_runtime_pm {
-	bool suspended;
-};
-
 enum intel_pipe_crc_source {
 	INTEL_PIPE_CRC_SOURCE_NONE,
 	INTEL_PIPE_CRC_SOURCE_PLANE1,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 177c207..07d6a79 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1041,7 +1041,7 @@ static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
 	unsigned long timeout_expire;
 	int ret;
 
-	WARN(dev_priv->pc8.irqs_disabled, "IRQs disabled\n");
+	WARN(dev_priv->pm.irqs_disabled, "IRQs disabled\n");
 
 	if (i915_seqno_passed(ring->get_seqno(ring, true), seqno))
 		return 0;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index be2713f..a5df448 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -86,9 +86,9 @@ ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
 {
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	if (dev_priv->pc8.irqs_disabled) {
+	if (dev_priv->pm.irqs_disabled) {
 		WARN(1, "IRQs disabled\n");
-		dev_priv->pc8.regsave.deimr &= ~mask;
+		dev_priv->pm.regsave.deimr &= ~mask;
 		return;
 	}
 
@@ -104,9 +104,9 @@ ironlake_disable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
 {
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	if (dev_priv->pc8.irqs_disabled) {
+	if (dev_priv->pm.irqs_disabled) {
 		WARN(1, "IRQs disabled\n");
-		dev_priv->pc8.regsave.deimr |= mask;
+		dev_priv->pm.regsave.deimr |= mask;
 		return;
 	}
 
@@ -129,10 +129,10 @@ static void ilk_update_gt_irq(struct drm_i915_private *dev_priv,
 {
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	if (dev_priv->pc8.irqs_disabled) {
+	if (dev_priv->pm.irqs_disabled) {
 		WARN(1, "IRQs disabled\n");
-		dev_priv->pc8.regsave.gtimr &= ~interrupt_mask;
-		dev_priv->pc8.regsave.gtimr |= (~enabled_irq_mask &
+		dev_priv->pm.regsave.gtimr &= ~interrupt_mask;
+		dev_priv->pm.regsave.gtimr |= (~enabled_irq_mask &
 						interrupt_mask);
 		return;
 	}
@@ -167,10 +167,10 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
 
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	if (dev_priv->pc8.irqs_disabled) {
+	if (dev_priv->pm.irqs_disabled) {
 		WARN(1, "IRQs disabled\n");
-		dev_priv->pc8.regsave.gen6_pmimr &= ~interrupt_mask;
-		dev_priv->pc8.regsave.gen6_pmimr |= (~enabled_irq_mask &
+		dev_priv->pm.regsave.gen6_pmimr &= ~interrupt_mask;
+		dev_priv->pm.regsave.gen6_pmimr |= (~enabled_irq_mask &
 						     interrupt_mask);
 		return;
 	}
@@ -313,11 +313,11 @@ static void ibx_display_interrupt_update(struct drm_i915_private *dev_priv,
 
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	if (dev_priv->pc8.irqs_disabled &&
+	if (dev_priv->pm.irqs_disabled &&
 	    (interrupt_mask & SDE_HOTPLUG_MASK_CPT)) {
 		WARN(1, "IRQs disabled\n");
-		dev_priv->pc8.regsave.sdeimr &= ~interrupt_mask;
-		dev_priv->pc8.regsave.sdeimr |= (~enabled_irq_mask &
+		dev_priv->pm.regsave.sdeimr &= ~interrupt_mask;
+		dev_priv->pm.regsave.sdeimr |= (~enabled_irq_mask &
 						 interrupt_mask);
 		return;
 	}
@@ -4118,32 +4118,32 @@ void intel_hpd_init(struct drm_device *dev)
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 }
 
-/* Disable interrupts so we can allow Package C8+. */
-void hsw_pc8_disable_interrupts(struct drm_device *dev)
+/* Disable interrupts so we can allow runtime PM. */
+void hsw_runtime_pm_disable_interrupts(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long irqflags;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 
-	dev_priv->pc8.regsave.deimr = I915_READ(DEIMR);
-	dev_priv->pc8.regsave.sdeimr = I915_READ(SDEIMR);
-	dev_priv->pc8.regsave.gtimr = I915_READ(GTIMR);
-	dev_priv->pc8.regsave.gtier = I915_READ(GTIER);
-	dev_priv->pc8.regsave.gen6_pmimr = I915_READ(GEN6_PMIMR);
+	dev_priv->pm.regsave.deimr = I915_READ(DEIMR);
+	dev_priv->pm.regsave.sdeimr = I915_READ(SDEIMR);
+	dev_priv->pm.regsave.gtimr = I915_READ(GTIMR);
+	dev_priv->pm.regsave.gtier = I915_READ(GTIER);
+	dev_priv->pm.regsave.gen6_pmimr = I915_READ(GEN6_PMIMR);
 
 	ironlake_disable_display_irq(dev_priv, 0xffffffff);
 	ibx_disable_display_interrupt(dev_priv, 0xffffffff);
 	ilk_disable_gt_irq(dev_priv, 0xffffffff);
 	snb_disable_pm_irq(dev_priv, 0xffffffff);
 
-	dev_priv->pc8.irqs_disabled = true;
+	dev_priv->pm.irqs_disabled = true;
 
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 }
 
-/* Restore interrupts so we can recover from Package C8+. */
-void hsw_pc8_restore_interrupts(struct drm_device *dev)
+/* Restore interrupts so we can recover from runtime PM. */
+void hsw_runtime_pm_restore_interrupts(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long irqflags;
@@ -4163,13 +4163,13 @@ void hsw_pc8_restore_interrupts(struct drm_device *dev)
 	val = I915_READ(GEN6_PMIMR);
 	WARN(val != 0xffffffff, "GEN6_PMIMR is 0x%08x\n", val);
 
-	dev_priv->pc8.irqs_disabled = false;
+	dev_priv->pm.irqs_disabled = false;
 
-	ironlake_enable_display_irq(dev_priv, ~dev_priv->pc8.regsave.deimr);
-	ibx_enable_display_interrupt(dev_priv, ~dev_priv->pc8.regsave.sdeimr);
-	ilk_enable_gt_irq(dev_priv, ~dev_priv->pc8.regsave.gtimr);
-	snb_enable_pm_irq(dev_priv, ~dev_priv->pc8.regsave.gen6_pmimr);
-	I915_WRITE(GTIER, dev_priv->pc8.regsave.gtier);
+	ironlake_enable_display_irq(dev_priv, ~dev_priv->pm.regsave.deimr);
+	ibx_enable_display_interrupt(dev_priv, ~dev_priv->pm.regsave.sdeimr);
+	ilk_enable_gt_irq(dev_priv, ~dev_priv->pm.regsave.gtimr);
+	snb_enable_pm_irq(dev_priv, ~dev_priv->pm.regsave.gen6_pmimr);
+	I915_WRITE(GTIER, dev_priv->pm.regsave.gtier);
 
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2fc543b..e869c97 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6806,7 +6806,7 @@ void __hsw_do_enable_pc8(struct drm_i915_private *dev_priv)
 	}
 
 	lpt_disable_clkout_dp(dev);
-	hsw_pc8_disable_interrupts(dev);
+	hsw_runtime_pm_disable_interrupts(dev);
 	hsw_disable_lcpll(dev_priv, true, true);
 }
 
@@ -6820,7 +6820,7 @@ void __hsw_do_disable_pc8(struct drm_i915_private *dev_priv)
 	DRM_DEBUG_KMS("Disabling package C8+\n");
 
 	hsw_restore_lcpll(dev_priv);
-	hsw_pc8_restore_interrupts(dev);
+	hsw_runtime_pm_restore_interrupts(dev);
 	lpt_init_pch_refclk(dev);
 
 	if (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index b26cfc6..f572ddb 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -618,8 +618,8 @@ void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 void ilk_disable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 void snb_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 void snb_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
-void hsw_pc8_disable_interrupts(struct drm_device *dev);
-void hsw_pc8_restore_interrupts(struct drm_device *dev);
+void hsw_runtime_pm_disable_interrupts(struct drm_device *dev);
+void hsw_runtime_pm_restore_interrupts(struct drm_device *dev);
 
 
 /* intel_crt.c */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 3b76f6e..8fcdf28 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -6158,7 +6158,8 @@ void intel_pm_setup(struct drm_device *dev)
 	mutex_init(&dev_priv->rps.hw_lock);
 
 	mutex_init(&dev_priv->pc8.lock);
-	dev_priv->pc8.irqs_disabled = false;
 	INIT_DELAYED_WORK(&dev_priv->rps.delayed_resume_work,
 			  intel_gen6_powersave_work);
+
+	dev_priv->pm.irqs_disabled = false;
 }
-- 
1.8.5.3

[PATCH 13/16] drm/i915: kill struct i915_package_c8

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

The only remaining field of the struct was the lock, which was
useless.

v2: - Rebase.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 2 --
 drivers/gpu/drm/i915/i915_drv.h     | 6 ------
 drivers/gpu/drm/i915/intel_pm.c     | 1 -
 3 files changed, 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 9d276c0..ba12ca9 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2012,11 +2012,9 @@ static int i915_pc8_status(struct seq_file *m, void *unused)
 		return 0;
 	}
 
-	mutex_lock(&dev_priv->pc8.lock);
 	seq_printf(m, "GPU idle: %s\n", yesno(!dev_priv->mm.busy));
 	seq_printf(m, "IRQs disabled: %s\n",
 		   yesno(dev_priv->pm.irqs_disabled));
-	mutex_unlock(&dev_priv->pc8.lock);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f399472..b741298 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1385,10 +1385,6 @@ struct ilk_wm_values {
  *
  * For more, read "Display Sequences for Package C8" on our documentation.
  */
-struct i915_package_c8 {
-	struct mutex lock;
-};
-
 struct i915_runtime_pm {
 	bool suspended;
 	bool irqs_disabled;
@@ -1628,8 +1624,6 @@ typedef struct drm_i915_private {
 		struct ilk_wm_values hw;
 	} wm;
 
-	struct i915_package_c8 pc8;
-
 	struct i915_runtime_pm pm;
 
 	/* Old dri1 support infrastructure, beware the dragons ya fools entering
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 8fcdf28..ba80fad 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -6157,7 +6157,6 @@ void intel_pm_setup(struct drm_device *dev)
 
 	mutex_init(&dev_priv->rps.hw_lock);
 
-	mutex_init(&dev_priv->pc8.lock);
 	INIT_DELAYED_WORK(&dev_priv->rps.delayed_resume_work,
 			  intel_gen6_powersave_work);
 
-- 
1.8.5.3

[PATCH 14/16] drm/i915: rename __hsw_do_{en, dis}able_pc8

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

After we removed all the intermediate abstractions, we can rename
these functions to just hsw_{en,dis}able_pc8.

v2: - Rebase.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c      | 4 ++--
 drivers/gpu/drm/i915/intel_display.c | 4 ++--
 drivers/gpu/drm/i915/intel_drv.h     | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 381a225..74f7a91 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -844,7 +844,7 @@ static int i915_runtime_suspend(struct device *device)
 	DRM_DEBUG_KMS("Suspending device\n");
 
 	if (HAS_PC8(dev))
-		__hsw_do_enable_pc8(dev_priv);
+		hsw_enable_pc8(dev_priv);
 
 	i915_gem_release_all_mmaps(dev_priv);
 
@@ -878,7 +878,7 @@ static int i915_runtime_resume(struct device *device)
 	dev_priv->pm.suspended = false;
 
 	if (HAS_PC8(dev))
-		__hsw_do_disable_pc8(dev_priv);
+		hsw_disable_pc8(dev_priv);
 
 	DRM_DEBUG_KMS("Device resumed\n");
 	return 0;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e869c97..53e480a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6790,7 +6790,7 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
-void __hsw_do_enable_pc8(struct drm_i915_private *dev_priv)
+void hsw_enable_pc8(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
 	uint32_t val;
@@ -6810,7 +6810,7 @@ void __hsw_do_enable_pc8(struct drm_i915_private *dev_priv)
 	hsw_disable_lcpll(dev_priv, true, true);
 }
 
-void __hsw_do_disable_pc8(struct drm_i915_private *dev_priv)
+void hsw_disable_pc8(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
 	uint32_t val;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f572ddb..9a7db84 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -722,8 +722,8 @@ unsigned long intel_gen4_compute_page_offset(int *x, int *y,
 					     unsigned int bpp,
 					     unsigned int pitch);
 void intel_display_handle_reset(struct drm_device *dev);
-void __hsw_do_enable_pc8(struct drm_i915_private *dev_priv);
-void __hsw_do_disable_pc8(struct drm_i915_private *dev_priv);
+void hsw_enable_pc8(struct drm_i915_private *dev_priv);
+void hsw_disable_pc8(struct drm_i915_private *dev_priv);
 void intel_dp_get_m_n(struct intel_crtc *crtc,
 		      struct intel_crtc_config *pipe_config);
 int intel_dotclock_calculate(int link_freq, const struct intel_link_m_n *m_n);
-- 
1.8.5.3

[PATCH 15/16] drm/i915: update the PC8 and runtime PM documentation

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Now that PC8 got much simpler, there are less things to document.
Also, runtime PM already has a nice documentation, so we don't need to
re-explain it on our driver.

v2: - Rebase.
    - Fix typo (Jesse).

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      | 52 +++++++++---------------------------
 drivers/gpu/drm/i915/intel_display.c | 23 ++++++++++++++++
 2 files changed, 35 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b741298..70feb61 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1333,47 +1333,19 @@ struct ilk_wm_values {
 };
 
 /*
- * This struct tracks the state needed for the Package C8+ feature.
+ * This struct helps tracking the state needed for runtime PM, which puts the
+ * device in PCI D3 state. Notice that when this happens, nothing on the
+ * graphics device works, even register access, so we don't get interrupts nor
+ * anything else.
  *
- * TODO: we're merging the Package C8+ feature with the runtime PM support. To
- * avoid having to update the documentation at each patch of the series, we'll
- * do a final update at the end.
+ * Every piece of our code that needs to actually touch the hardware needs to
+ * either call intel_runtime_pm_get or call intel_display_power_get with the
+ * appropriate power domain.
  *
- * Package states C8 and deeper are really deep PC states that can only be
- * reached when all the devices on the system allow it, so even if the graphics
- * device allows PC8+, it doesn't mean the system will actually get to these
- * states.
- *
- * Our driver only allows PC8+ when all the outputs are disabled, the power well
- * is disabled and the GPU is idle. When these conditions are met, we manually
- * do the other conditions: disable the interrupts, clocks and switch LCPLL
- * refclk to Fclk.
- *
- * When we really reach PC8 or deeper states (not just when we allow it) we lose
- * the state of some registers, so when we come back from PC8+ we need to
- * restore this state. We don't get into PC8+ if we're not in RC6, so we don't
- * need to take care of the registers kept by RC6.
- *
- * The interrupt disabling is part of the requirements. We can only leave the
- * PCH HPD interrupts enabled. If we're in PC8+ and we get another interrupt we
- * can lock the machine.
- *
- * Ideally every piece of our code that needs PC8+ disabled would call
- * hsw_disable_package_c8, which would increment disable_count and prevent the
- * system from reaching PC8+. But we don't have a symmetric way to do this for
- * everything, so we have the requirements_met variable. When we switch
- * requirements_met to true we decrease disable_count, and increase it in the
- * opposite case. The requirements_met variable is true when all the CRTCs,
- * encoders and the power well are disabled.
- *
- * In addition to everything, we only actually enable PC8+ if disable_count
- * stays at zero for at least some seconds. This is implemented with the
- * enable_work variable. We do this so we don't enable/disable PC8 dozens of
- * consecutive times when all screens are disabled and some background app
- * queries the state of our connectors, or we have some application constantly
- * waking up to use the GPU. Only after the enable_work function actually
- * enables PC8+ the "enable" variable will become true, which means that it can
- * be false even if disable_count is 0.
+ * Our driver uses the autosuspend delay feature, which means we'll only really
+ * suspend if we stay with zero refcount for a certain amount of time. The
+ * default value is currently very conservative (see intel_init_runtime_pm), but
+ * it can be changed with the standard runtime PM files from sysfs.
  *
  * The irqs_disabled variable becomes true exactly after we disable the IRQs and
  * goes back to false exactly before we reenable the IRQs. We use this variable
@@ -1383,7 +1355,7 @@ struct ilk_wm_values {
  * inside struct regsave so when we restore the IRQs they will contain the
  * latest expected values.
  *
- * For more, read "Display Sequences for Package C8" on our documentation.
+ * For more, read the Documentation/power/runtime_pm.txt.
  */
 struct i915_runtime_pm {
 	bool suspended;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 53e480a..ed9233e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6790,6 +6790,29 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
+/*
+ * Package states C8 and deeper are really deep PC states that can only be
+ * reached when all the devices on the system allow it, so even if the graphics
+ * device allows PC8+, it doesn't mean the system will actually get to these
+ * states. Our driver only allows PC8+ when going into runtime PM.
+ *
+ * The requirements for PC8+ are that all the outputs are disabled, the power
+ * well is disabled and most interrupts are disabled, and these are also
+ * requirements for runtime PM. When these conditions are met, we manually do
+ * the other conditions: disable the interrupts, clocks and switch LCPLL refclk
+ * to Fclk. If we're in PC8+ and we get an non-hotplug interrupt, we can hard
+ * hang the machine.
+ *
+ * When we really reach PC8 or deeper states (not just when we allow it) we lose
+ * the state of some registers, so when we come back from PC8+ we need to
+ * restore this state. We don't get into PC8+ if we're not in RC6, so we don't
+ * need to take care of the registers kept by RC6. Notice that this happens even
+ * if we don't put the device in PCI D3 state (which is what currently happens
+ * because of the runtime PM support).
+ *
+ * For more, read "Display Sequences for Package C8" on the hardware
+ * documentation.
+ */
 void hsw_enable_pc8(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
-- 
1.8.5.3

[PATCH 16/16] drm/i915: init pm.suspended earlier

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Function intel_init_runtime_pm is supposed to start allowing runtime
PM from that point, but it's called very late on the driver
initialization code, to prevent the driver from trying to suspend
while still initializing. The problem is that variables are accessed
earlier than that, so initalize them at intel_pm_setup, which is
supposed to be the correct place.

Notice that this shouldn't fix any specific bugs because dev_priv is
zeroed when allocated, so the value is already correct right from the
start.

v2: - Rebase.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ba80fad..0181012 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5923,8 +5923,6 @@ void intel_init_runtime_pm(struct drm_i915_private *dev_priv)
 	struct drm_device *dev = dev_priv->dev;
 	struct device *device = &dev->pdev->dev;
 
-	dev_priv->pm.suspended = false;
-
 	if (!HAS_RUNTIME_PM(dev))
 		return;
 
@@ -6160,5 +6158,6 @@ void intel_pm_setup(struct drm_device *dev)
 	INIT_DELAYED_WORK(&dev_priv->rps.delayed_resume_work,
 			  intel_gen6_powersave_work);
 
+	dev_priv->pm.suspended = false;
 	dev_priv->pm.irqs_disabled = false;
 }
-- 
1.8.5.3