[PATCH 00/23] Merge PC8 with runtime PM, v2

[PATCH 00/23] Merge PC8 with runtime PM, v2

[Paulo Zanoni]

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

Hi

This is the second time I send this series to the mailing list. Please read the
first cover letter:
   http://lists.freedesktop.org/archives/intel-gfx/2013-December/037721.html

What's new?

Two main differences:
  - Added a patch from Chris, which resulted in a change on how we handle
    dev_priv->pc8.gpu_busy later.
  - Fixed a bug on the forcewake handling.

There is some discussion if we want to merge this first, or the VLV power wells
patches first. Independently of the decision, I think we should try to at least
discuss these patches and review what can be reviewed. I really think this
series should make it easier to add runtime PM support to other platforms, and I
even added BDW and SNB support on top of these patches.

Thanks,
Paulo

Chris Wilson (1):
  drm/i915: Accurately track when we mark the hardware as idle/busy

Paulo Zanoni (22):
  drm/i915: put runtime PM only at the end of intel_mark_idle
  drm/i915: put runtime PM only when we actually release force_wake
  drm/i915: kill dev_priv->pc8.gpu_idle
  drm/i915: rename modeset_update_power_wells
  drm/i915: get/put runtime PM without holding rps.hw_lock
  drm/i915: add forcewake functions that don't touch runtime PM
  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: get/put runtime PM references for GMBUS and DP AUX
  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  |  22 ++--
 drivers/gpu/drm/i915/i915_dma.c      |   2 -
 drivers/gpu/drm/i915/i915_drv.c      |  13 ++-
 drivers/gpu/drm/i915/i915_drv.h      |  79 +++++---------
 drivers/gpu/drm/i915/i915_gem.c      |  16 ++-
 drivers/gpu/drm/i915/i915_irq.c      |  58 +++++------
 drivers/gpu/drm/i915/i915_params.c   |  10 --
 drivers/gpu/drm/i915/intel_display.c | 197 ++++++++---------------------------
 drivers/gpu/drm/i915/intel_dp.c      |   4 +-
 drivers/gpu/drm/i915/intel_drv.h     |  11 +-
 drivers/gpu/drm/i915/intel_i2c.c     |   4 +-
 drivers/gpu/drm/i915/intel_pm.c      |  43 ++------
 drivers/gpu/drm/i915/intel_uncore.c  |  54 +++++++++-
 13 files changed, 192 insertions(+), 321 deletions(-)

-- 
1.8.5.3

[PATCH 01/23] drm/i915: Accurately track when we mark the hardware as idle/busy

[Paulo Zanoni]

From: Chris Wilson <chris@chris-wilson.co.uk>

We currently call intel_mark_idle() too often, as we do so as a
side-effect of processing the request queue. However, we the calls to
intel_mark_idle() are expected to be paired with a call to
intel_mark_busy() (or else we try to idle the hardware by accessing
registers that are already disabled). Make the idle/busy tracking
explicit to prevent the multiple calls.

v2: We can drop some of the complexity in __i915_add_request() as
queue_delayed_work() already behaves as we want (not requeuing the item
if it is already in the queue) and mark_busy/mark_idle imply that the
idle task is inactive.

v3: We do still need to cancel the pending idle task so that it is sent
again after the current busy load completes (not in the middle of it).

Reported-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  8 ++++++++
 drivers/gpu/drm/i915/i915_gem.c      | 14 +++++---------
 drivers/gpu/drm/i915/intel_display.c |  9 +++++++++
 3 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8c64831..a5caa7e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1124,6 +1124,14 @@ struct i915_gem_mm {
 	 */
 	bool interruptible;
 
+	/**
+	 * Is the GPU currently considered idle, or busy executing userspace
+	 * requests?  Whilst idle, we attempt to power down the hardware and
+	 * display clocks. In order to reduce the effect on performance, there
+	 * is a slight delay before we do so.
+	 */
+	bool busy;
+
 	/** Bit 6 swizzling required for X tiling */
 	uint32_t bit_6_swizzle_x;
 	/** Bit 6 swizzling required for Y tiling */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 3618bb0..6978e69 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2148,7 +2148,6 @@ int __i915_add_request(struct intel_ring_buffer *ring,
 	drm_i915_private_t *dev_priv = ring->dev->dev_private;
 	struct drm_i915_gem_request *request;
 	u32 request_ring_position, request_start;
-	int was_empty;
 	int ret;
 
 	request_start = intel_ring_get_tail(ring);
@@ -2199,7 +2198,6 @@ int __i915_add_request(struct intel_ring_buffer *ring,
 		i915_gem_context_reference(request->ctx);
 
 	request->emitted_jiffies = jiffies;
-	was_empty = list_empty(&ring->request_list);
 	list_add_tail(&request->list, &ring->request_list);
 	request->file_priv = NULL;
 
@@ -2220,13 +2218,11 @@ int __i915_add_request(struct intel_ring_buffer *ring,
 	if (!dev_priv->ums.mm_suspended) {
 		i915_queue_hangcheck(ring->dev);
 
-		if (was_empty) {
-			cancel_delayed_work_sync(&dev_priv->mm.idle_work);
-			queue_delayed_work(dev_priv->wq,
-					   &dev_priv->mm.retire_work,
-					   round_jiffies_up_relative(HZ));
-			intel_mark_busy(dev_priv->dev);
-		}
+		cancel_delayed_work_sync(&dev_priv->mm.idle_work);
+		queue_delayed_work(dev_priv->wq,
+				   &dev_priv->mm.retire_work,
+				   round_jiffies_up_relative(HZ));
+		intel_mark_busy(dev_priv->dev);
 	}
 
 	if (out_seqno)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f19e6ea..dd416f2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8192,8 +8192,12 @@ void intel_mark_busy(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	if (dev_priv->mm.busy)
+		return;
+
 	hsw_package_c8_gpu_busy(dev_priv);
 	i915_update_gfx_val(dev_priv);
+	dev_priv->mm.busy = true;
 }
 
 void intel_mark_idle(struct drm_device *dev)
@@ -8201,6 +8205,11 @@ void intel_mark_idle(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
 
+	if (!dev_priv->mm.busy)
+		return;
+
+	dev_priv->mm.busy = false;
+
 	hsw_package_c8_gpu_idle(dev_priv);
 
 	if (!i915.powersave)
-- 
1.8.5.3

[PATCH 02/23] drm/i915: put runtime PM only at the end of intel_mark_idle

[Paulo Zanoni]

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

Because intel_mark_idle still touches some registers: it needs the
machine to be awake. If you set both the autosuspend and PC8 delays to
zero, you can get a "Device suspended" WARN when gen6_rps_idle touches
registers.

This is not easy to reproduce, but happens once in a while when
running pm_pc8.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index dd416f2..10ec401 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8210,10 +8210,8 @@ void intel_mark_idle(struct drm_device *dev)
 
 	dev_priv->mm.busy = false;
 
-	hsw_package_c8_gpu_idle(dev_priv);
-
 	if (!i915.powersave)
-		return;
+		goto out;
 
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
 		if (!crtc->fb)
@@ -8224,6 +8222,9 @@ void intel_mark_idle(struct drm_device *dev)
 
 	if (INTEL_INFO(dev)->gen >= 6)
 		gen6_rps_idle(dev->dev_private);
+
+out:
+	hsw_package_c8_gpu_idle(dev_priv);
 }
 
 void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
-- 
1.8.5.3

[PATCH 03/23] drm/i915: put runtime PM only when we actually release force_wake

[Paulo Zanoni]

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

When we call gen6_gt_force_wake_put we don't actually put force_wake,
we just schedule gen6_force_wake_work through mod_delayed_work, and
that will eventually release force_wake.

The problem is that we call intel_runtime_pm_put directly at
gen6_gt_force_wake_put, so most of the times we put our runtime PM
reference before the delayed work happens, so we may runtime suspend
while force_wake is still supposed to be enabled if the graphics
autosuspend_delay_ms is too small.

Now the nice thing about the current code is that after it triggers
the delayed work function it gets a refcount, and it only triggers the
delayed work function if refcount is zero. This guarantees that when
we schedule the funciton, it will run before we try to schedule it
again, which simplifies the problem and allows for the current
solution to work properly (hopefully!).

v2: - Keep the VLV refcounts balanced (Jesse)

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

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index c628414..6a21f43 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -299,6 +299,8 @@ static void gen6_force_wake_work(struct work_struct *work)
 	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);
+
+	intel_runtime_pm_put(dev_priv);
 }
 
 static void intel_uncore_forcewake_reset(struct drm_device *dev)
@@ -393,25 +395,31 @@ void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
 void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
 {
 	unsigned long irqflags;
+	bool delayed = false;
 
 	if (!dev_priv->uncore.funcs.force_wake_put)
 		return;
 
 	/* Redirect to VLV specific routine */
-	if (IS_VALLEYVIEW(dev_priv->dev))
-		return vlv_force_wake_put(dev_priv, fw_engine);
+	if (IS_VALLEYVIEW(dev_priv->dev)) {
+		vlv_force_wake_put(dev_priv, fw_engine);
+		goto out;
+	}
 
 
 	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 	if (--dev_priv->uncore.forcewake_count == 0) {
 		dev_priv->uncore.forcewake_count++;
+		delayed = true;
 		mod_delayed_work(dev_priv->wq,
 				 &dev_priv->uncore.force_wake_work,
 				 1);
 	}
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 
-	intel_runtime_pm_put(dev_priv);
+out:
+	if (!delayed)
+		intel_runtime_pm_put(dev_priv);
 }
 
 /* We give fast paths for the really cool registers */
-- 
1.8.5.3

[PATCH 04/23] drm/i915: kill dev_priv->pc8.gpu_idle

[Paulo Zanoni]

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

Since the addition of dev_priv->mm.busy, there's no more need for
dev_priv->pc8.gpu_idle, so kill it.

Notice that when you remove gpu_idle, hsw_package_c8_gpu_idle and
hsw_package_c8_gpu_busy become identical to hsw_enable_package_c8 and
hsw_disable_package_c8, so just use them.

Also, when we boot the machine, dev_priv->mm.busy initially considers
the machine as idle. This is opposed to dev_priv->pc8.gpu_idle, which
considered it busy. So dev_priv->pc8.disable_count has to be
initalized to 1 now.

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/intel_display.c | 30 ++----------------------------
 drivers/gpu/drm/i915/intel_pm.c      |  3 +--
 4 files changed, 8 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index d90a707..72cc6d0 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1997,7 +1997,7 @@ 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->pc8.gpu_idle));
+	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));
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a5caa7e..2a2a3a9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1321,11 +1321,10 @@ struct ilk_wm_values {
  * 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 and gpu_idle variables. When we
- * switch requirements_met or gpu_idle 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. The gpu_idle
- * variable is true when the GPU is idle.
+ * 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
@@ -1348,7 +1347,6 @@ struct ilk_wm_values {
  */
 struct i915_package_c8 {
 	bool requirements_met;
-	bool gpu_idle;
 	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 10ec401..0183a34 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6812,32 +6812,6 @@ done:
 	mutex_unlock(&dev_priv->pc8.lock);
 }
 
-static void hsw_package_c8_gpu_idle(struct drm_i915_private *dev_priv)
-{
-	if (!HAS_PC8(dev_priv->dev))
-		return;
-
-	mutex_lock(&dev_priv->pc8.lock);
-	if (!dev_priv->pc8.gpu_idle) {
-		dev_priv->pc8.gpu_idle = true;
-		__hsw_enable_package_c8(dev_priv);
-	}
-	mutex_unlock(&dev_priv->pc8.lock);
-}
-
-static void hsw_package_c8_gpu_busy(struct drm_i915_private *dev_priv)
-{
-	if (!HAS_PC8(dev_priv->dev))
-		return;
-
-	mutex_lock(&dev_priv->pc8.lock);
-	if (dev_priv->pc8.gpu_idle) {
-		dev_priv->pc8.gpu_idle = false;
-		__hsw_disable_package_c8(dev_priv);
-	}
-	mutex_unlock(&dev_priv->pc8.lock);
-}
-
 #define for_each_power_domain(domain, mask)				\
 	for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++)	\
 		if ((1 << (domain)) & (mask))
@@ -8195,7 +8169,7 @@ void intel_mark_busy(struct drm_device *dev)
 	if (dev_priv->mm.busy)
 		return;
 
-	hsw_package_c8_gpu_busy(dev_priv);
+	hsw_disable_package_c8(dev_priv);
 	i915_update_gfx_val(dev_priv);
 	dev_priv->mm.busy = true;
 }
@@ -8224,7 +8198,7 @@ void intel_mark_idle(struct drm_device *dev)
 		gen6_rps_idle(dev->dev_private);
 
 out:
-	hsw_package_c8_gpu_idle(dev_priv);
+	hsw_enable_package_c8(dev_priv);
 }
 
 void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a6b877a..50b80bb 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5786,10 +5786,9 @@ void intel_pm_setup(struct drm_device *dev)
 
 	mutex_init(&dev_priv->pc8.lock);
 	dev_priv->pc8.requirements_met = false;
-	dev_priv->pc8.gpu_idle = false;
 	dev_priv->pc8.irqs_disabled = false;
 	dev_priv->pc8.enabled = false;
-	dev_priv->pc8.disable_count = 2; /* requirements_met + gpu_idle */
+	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

[PATCH 05/23] drm/i915: rename modeset_update_power_wells

[Paulo Zanoni]

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

To modeset_update_crtc_power_domains, since this function is
responsible for updating all the power domains of all CRTCs after a
modeset. In the future we should also run this function on all
platforms, not just Haswell.

Reviewed-by: Imre Deak <imre.deak@intel.com>
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 0183a34..cbe3487 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6847,7 +6847,7 @@ void intel_display_set_init_power(struct drm_device *dev, bool enable)
 	dev_priv->power_domains.init_power_on = enable;
 }
 
-static void modeset_update_power_wells(struct drm_device *dev)
+static void modeset_update_crtc_power_domains(struct drm_device *dev)
 {
 	unsigned long pipe_domains[I915_MAX_PIPES] = { 0, };
 	struct intel_crtc *crtc;
@@ -6884,7 +6884,7 @@ static void modeset_update_power_wells(struct drm_device *dev)
 
 static void haswell_modeset_global_resources(struct drm_device *dev)
 {
-	modeset_update_power_wells(dev);
+	modeset_update_crtc_power_domains(dev);
 	hsw_update_package_c8(dev);
 }
 
-- 
1.8.5.3

[PATCH 06/23] drm/i915: get/put runtime PM without holding rps.hw_lock

[Paulo Zanoni]

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

We'll need this when we merge PC8 and Runtime PM: the PC8
enable/disable functions need that lock.

Also, it's good practice to not hold a lock for longer than strictly
needed.

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 72cc6d0..c89d30a 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1468,7 +1468,7 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
 	struct drm_info_node *node = (struct drm_info_node *) m->private;
 	struct drm_device *dev = node->minor->dev;
 	drm_i915_private_t *dev_priv = dev->dev_private;
-	int ret;
+	int ret = 0;
 	int gpu_freq, ia_freq;
 
 	if (!(IS_GEN6(dev) || IS_GEN7(dev))) {
@@ -1476,12 +1476,13 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
 		return 0;
 	}
 
+	intel_runtime_pm_get(dev_priv);
+
 	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
 
 	ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
 	if (ret)
-		return ret;
-	intel_runtime_pm_get(dev_priv);
+		goto out;
 
 	seq_puts(m, "GPU freq (MHz)\tEffective CPU freq (MHz)\tEffective Ring freq (MHz)\n");
 
@@ -1498,10 +1499,11 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
 			   ((ia_freq >> 8) & 0xff) * 100);
 	}
 
-	intel_runtime_pm_put(dev_priv);
 	mutex_unlock(&dev_priv->rps.hw_lock);
 
-	return 0;
+out:
+	intel_runtime_pm_put(dev_priv);
+	return ret;
 }
 
 static int i915_gfxec(struct seq_file *m, void *unused)
-- 
1.8.5.3

[PATCH 07/23] drm/i915: add forcewake functions that don't touch runtime PM

[Paulo Zanoni]

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

To solve a chicken-and-egg problem. Currently when we get/put
forcewake we also get/put runtime PM and this works fine because the
runtime PM code doesn't need forcewake. But we're going to merge PC8
and runtime PM into a single feature, and the PC8 code (the LCPLL
code) does need the forcewake, so that specific piece of code needs to
call the _no_rpm version so it doesn't try to get runtime PM in the
code that gets runtime PM.

For now the new functions are unused, we'll use them on the patch that
merges PC8 with runtime PM.

Also notice that, so simplify things, the put() function doesn't use
the workqueue, since the workqueue also puts runtime PM.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h     |  4 ++++
 drivers/gpu/drm/i915/intel_uncore.c | 40 +++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2a2a3a9..dcaa130 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2604,6 +2604,10 @@ extern void intel_display_print_error_state(struct drm_i915_error_state_buf *e,
  */
 void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine);
 void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine);
+void gen6_gt_force_wake_get_no_rpm(struct drm_i915_private *dev_priv,
+				   int fw_engine);
+void gen6_gt_force_wake_put_no_rpm(struct drm_i915_private *dev_priv,
+				   int fw_engine);
 
 int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val);
 int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val);
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 6a21f43..d0ec32a 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -422,6 +422,46 @@ out:
 		intel_runtime_pm_put(dev_priv);
 }
 
+/*
+ * These two _no_rpm functions below should only be used by the code that runs
+ * while we are enabling/disabling runtiem PM. See gen6_gt_force_wake_get().
+ */
+void gen6_gt_force_wake_get_no_rpm(struct drm_i915_private *dev_priv,
+				   int fw_engine)
+{
+	unsigned long irqflags;
+
+	if (!dev_priv->uncore.funcs.force_wake_get)
+		return;
+
+	/* Redirect to VLV specific routine */
+	if (IS_VALLEYVIEW(dev_priv->dev))
+		return vlv_force_wake_get(dev_priv, fw_engine);
+
+	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);
+}
+
+void gen6_gt_force_wake_put_no_rpm(struct drm_i915_private *dev_priv,
+				   int fw_engine)
+{
+	unsigned long irqflags;
+
+	if (!dev_priv->uncore.funcs.force_wake_put)
+		return;
+
+	/* Redirect to VLV specific routine */
+	if (IS_VALLEYVIEW(dev_priv->dev))
+		return vlv_force_wake_put(dev_priv, fw_engine);
+
+	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);
+}
+
 /* We give fast paths for the really cool registers */
 #define NEEDS_FORCE_WAKE(dev_priv, reg) \
 	 ((reg) < 0x40000 && (reg) != FORCEWAKE)
-- 
1.8.5.3

[PATCH 08/23] 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 cbe3487..ce582eb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6651,19 +6651,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;
@@ -6677,7 +6669,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);
 }
 
@@ -6695,29 +6701,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);
@@ -6736,6 +6726,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 09/23] 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.

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 | 43 ++++++++----------------------------
 drivers/gpu/drm/i915/intel_drv.h     |  3 ++-
 drivers/gpu/drm/i915/intel_pm.c      |  1 -
 7 files changed, 23 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 7688abc..da0b2ee 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1818,8 +1818,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 2d05d7c..983ab56 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -850,6 +850,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);
@@ -864,6 +867,7 @@ static int i915_runtime_suspend(struct device *device)
 	 */
 	intel_opregion_notify_adapter(dev, PCI_D1);
 
+	DRM_DEBUG_KMS("Device suspended\n");
 	return 0;
 }
 
@@ -880,6 +884,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 dcaa130..f53e77d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1299,6 +1299,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
@@ -1352,7 +1356,6 @@ struct i915_package_c8 {
 	bool enabled;
 	int disable_count;
 	struct mutex lock;
-	struct delayed_work enable_work;
 
 	struct {
 		uint32_t deimr;
@@ -1968,8 +1971,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;
 	/* leave bools at the end to not create holes */
 	bool enable_hangcheck;
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 3b48258..5bf38f9 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,
@@ -134,14 +132,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 ce582eb..788b165 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6614,7 +6614,7 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
 
 	/* 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);
+	gen6_gt_force_wake_get_no_rpm(dev_priv, FORCEWAKE_ALL);
 
 	if (val & LCPLL_POWER_DOWN_ALLOW) {
 		val &= ~LCPLL_POWER_DOWN_ALLOW;
@@ -6648,14 +6648,16 @@ 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);
+	gen6_gt_force_wake_put_no_rpm(dev_priv, FORCEWAKE_ALL);
 }
 
-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;
@@ -6671,22 +6673,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));
@@ -6697,15 +6683,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);
@@ -6728,8 +6715,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);
@@ -6738,14 +6723,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)
@@ -6803,9 +6781,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 a4ffc02..ea24eae 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -720,7 +720,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 50b80bb..d68fee2 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5789,7 +5789,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

[PATCH 10/23] 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 to
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.

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 d68fee2..772aa678 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5341,6 +5341,8 @@ void intel_display_power_get(struct drm_device *dev,
 	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);
@@ -5372,6 +5374,8 @@ void intel_display_power_put(struct drm_device *dev,
 		__intel_power_well_put(dev, power_well);
 
 	mutex_unlock(&power_domains->lock);
+
+	intel_runtime_pm_put(dev_priv);
 }
 
 static struct i915_power_domains *hsw_pwr;
-- 
1.8.5.3

[PATCH 11/23] 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.

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 c89d30a..36daaa8 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1997,8 +1997,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 f53e77d..6ea5c4b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1350,7 +1350,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 788b165..0d82241 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6746,59 +6746,6 @@ 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);
-}
-
 #define for_each_power_domain(domain, mask)				\
 	for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++)	\
 		if ((1 << (domain)) & (mask))
@@ -6872,7 +6819,6 @@ static void modeset_update_crtc_power_domains(struct drm_device *dev)
 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 772aa678..962e0d1 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5566,6 +5566,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)
@@ -5789,10 +5791,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

[PATCH 12/23] 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.

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 0d82241..0de1aa7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8102,7 +8102,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;
 }
@@ -8131,7 +8131,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 13/23] 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.

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 36daaa8..ad6c209 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1998,7 +1998,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 6ea5c4b..8b66c14 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1353,7 +1353,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 0de1aa7..3b57257 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6676,13 +6676,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);
 }
 
@@ -6716,13 +6709,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 962e0d1..86e6835 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5793,7 +5793,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 14/23] 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.

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 3b57257..ef0bcf8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6673,12 +6673,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;
@@ -6706,19 +6700,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);
 }
 
@@ -6728,7 +6716,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

[PATCH 15/23] 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.

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 983ab56..7a8b86e 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -433,7 +433,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, true);
 
 	drm_kms_helper_poll_disable(dev);
@@ -606,10 +605,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 16/23] drm/i915: get/put runtime PM references for GMBUS and DP AUX

[Paulo Zanoni]

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

We had these intel_aux_display_runtime_{get,put} abstractions that
would just get/put PC8 references, but now that runtime PM and PC8
are the same stuff, we just need the runtime PM references, so just
get/put runtime PM directly, because that's what the rest of our code
does.

Another way to solve this problem would be to add DP_AUX and GMBUS
power domains, and then use intel_display_power_{get,put}, but every
power domain already gets/puts runtime PM references, so this would
just make things more complex.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  |  4 ++--
 drivers/gpu/drm/i915/intel_drv.h |  2 --
 drivers/gpu/drm/i915/intel_i2c.c |  4 ++--
 drivers/gpu/drm/i915/intel_pm.c  | 11 -----------
 4 files changed, 4 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index c512d78..79d4844 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -469,7 +469,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 
 	intel_dp_check_edp(intel_dp);
 
-	intel_aux_display_runtime_get(dev_priv);
+	intel_runtime_pm_get(dev_priv);
 
 	/* Try to wait for any previous AUX channel activity */
 	for (try = 0; try < 3; try++) {
@@ -563,7 +563,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 	ret = recv_bytes;
 out:
 	pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
-	intel_aux_display_runtime_put(dev_priv);
+	intel_runtime_pm_put(dev_priv);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ea24eae..a2e0cd7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -890,8 +890,6 @@ void ironlake_teardown_rc6(struct drm_device *dev);
 void gen6_update_ring_freq(struct drm_device *dev);
 void gen6_rps_idle(struct drm_i915_private *dev_priv);
 void gen6_rps_boost(struct drm_i915_private *dev_priv);
-void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv);
-void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_get(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
 void intel_init_runtime_pm(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index d33b61d..3d403ce 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -446,7 +446,7 @@ gmbus_xfer(struct i2c_adapter *adapter,
 	int i, reg_offset;
 	int ret = 0;
 
-	intel_aux_display_runtime_get(dev_priv);
+	intel_runtime_pm_get(dev_priv);
 	mutex_lock(&dev_priv->gmbus_mutex);
 
 	if (bus->force_bit) {
@@ -546,7 +546,7 @@ timeout:
 
 out:
 	mutex_unlock(&dev_priv->gmbus_mutex);
-	intel_aux_display_runtime_put(dev_priv);
+	intel_runtime_pm_put(dev_priv);
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 86e6835..1e3580f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5516,17 +5516,6 @@ void intel_power_domains_init_hw(struct drm_device *dev)
 		I915_WRITE(HSW_PWR_WELL_BIOS, 0);
 }
 
-/* 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);
-}
-
-void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv)
-{
-	hsw_enable_package_c8(dev_priv);
-}
-
 void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
-- 
1.8.5.3

[PATCH 17/23] 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.

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      | 12 ++----------
 3 files changed, 2 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ef0bcf8..d9ea9b89 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6700,26 +6700,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);
-}
-
 #define for_each_power_domain(domain, mask)				\
 	for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++)	\
 		if ((1 << (domain)) & (mask))
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a2e0cd7..2c02d68 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -722,8 +722,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 1e3580f..78f56e8 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5311,26 +5311,18 @@ static void hsw_set_power_well(struct drm_device *dev,
 static void __intel_power_well_get(struct drm_device *dev,
 				   struct i915_power_well *power_well)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	if (!power_well->count++ && power_well->set) {
-		hsw_disable_package_c8(dev_priv);
+	if (!power_well->count++ && power_well->set)
 		power_well->set(dev, power_well, true);
-	}
 }
 
 static void __intel_power_well_put(struct drm_device *dev,
 				   struct i915_power_well *power_well)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
 	WARN_ON(!power_well->count);
 
 	if (!--power_well->count && power_well->set &&
-	    i915.disable_power_well) {
+	    i915.disable_power_well)
 		power_well->set(dev, power_well, false);
-		hsw_enable_package_c8(dev_priv);
-	}
 }
 
 void intel_display_power_get(struct drm_device *dev,
-- 
1.8.5.3

[PATCH 18/23] 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.

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 ad6c209..93d6065 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2000,7 +2000,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 8b66c14..16e344e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1351,8 +1351,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 d9ea9b89..271dd66 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6660,8 +6660,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;
@@ -6697,7 +6695,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;
 }
 
 #define for_each_power_domain(domain, mask)				\
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 78f56e8..c85507a 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5278,8 +5278,6 @@ static void hsw_set_power_well(struct drm_device *dev,
 	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;
@@ -5773,7 +5771,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 19/23] 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.

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      | 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 93d6065..213b093 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1999,7 +1999,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 16e344e..74fba1c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1350,8 +1350,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;
@@ -1362,10 +1366,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 6978e69..998dabe 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1017,7 +1017,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 f68aee3..14b26f2 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;
 	}
@@ -4016,32 +4016,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;
@@ -4061,13 +4061,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 271dd66..420fad5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6667,7 +6667,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);
 }
 
@@ -6681,7 +6681,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 2c02d68..d0434e8 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -616,8 +616,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 c85507a..5ff4b59 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5770,7 +5770,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 20/23] 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.

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 213b093..ff81b90 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1996,11 +1996,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 74fba1c..08b22e3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1349,10 +1349,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;
@@ -1590,8 +1586,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 5ff4b59..3bd6e8f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5769,7 +5769,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 21/23] 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.

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 7a8b86e..88ea6ce 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -846,7 +846,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);
 
@@ -880,7 +880,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 420fad5..c981e63 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6651,7 +6651,7 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
 	gen6_gt_force_wake_put_no_rpm(dev_priv, FORCEWAKE_ALL);
 }
 
-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;
@@ -6671,7 +6671,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 d0434e8..288f531 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -720,8 +720,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 22/23] 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.

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 08b22e3..a7aeb38 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1297,47 +1297,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 frmo 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
@@ -1347,7 +1319,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 c981e63..4354946 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6651,6 +6651,29 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
 	gen6_gt_force_wake_put_no_rpm(dev_priv, FORCEWAKE_ALL);
 }
 
+/*
+ * 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 23/23] 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.

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 3bd6e8f..88b434b 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5535,8 +5535,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;
 
@@ -5772,5 +5770,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

Re: [PATCH 07/23] drm/i915: add forcewake functions that don't touch runtime PM

[Chris Wilson]

On Thu, Feb 27, 2014 at 07:26:34PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> To solve a chicken-and-egg problem. Currently when we get/put
> forcewake we also get/put runtime PM and this works fine because the
> runtime PM code doesn't need forcewake. But we're going to merge PC8
> and runtime PM into a single feature, and the PC8 code (the LCPLL
> code) does need the forcewake, so that specific piece of code needs to
> call the _no_rpm version so it doesn't try to get runtime PM in the
> code that gets runtime PM.
> 
> For now the new functions are unused, we'll use them on the patch that
> merges PC8 with runtime PM.
> 
> Also notice that, so simplify things, the put() function doesn't use
> the workqueue, since the workqueue also puts runtime PM.

Where are these routines called? The names are awful but not quite awful
enough to avoid confusion. Is it possible to hide these to a single .c?

The workqueue doesn't touch rpm here, so there routines could be
simplified if they remain in intel_uncore.c.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Chris Wilson]

On Thu, Feb 27, 2014 at 07:26:36PM -0300, Paulo Zanoni wrote:
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ce582eb..788b165 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6614,7 +6614,7 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
>  
>  	/* 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);
> +	gen6_gt_force_wake_get_no_rpm(dev_priv, FORCEWAKE_ALL);

I want more indications that this is legal - i.e. that the function is
only called from a special context handling rpm. As it is, it is not
immediately clear that the rearrangements in disable_package_c8 is
complete. If you moved this forcwake dance higher and comment why the
intel_runtime_pm_get() handling is so special, you might not scare so
many causal readers.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 04/23] drm/i915: kill dev_priv->pc8.gpu_idle

[Imre Deak]

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

On Thu, 2014-02-27 at 19:26 -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Since the addition of dev_priv->mm.busy, there's no more need for
> dev_priv->pc8.gpu_idle, so kill it.
> 
> Notice that when you remove gpu_idle, hsw_package_c8_gpu_idle and
> hsw_package_c8_gpu_busy become identical to hsw_enable_package_c8 and
> hsw_disable_package_c8, so just use them.
> 
> Also, when we boot the machine, dev_priv->mm.busy initially considers
> the machine as idle. This is opposed to dev_priv->pc8.gpu_idle, which
> considered it busy. So dev_priv->pc8.disable_count has to be
> initalized to 1 now.
> 
> 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      | 10 ++++------
>  drivers/gpu/drm/i915/intel_display.c | 30 ++----------------------------
>  drivers/gpu/drm/i915/intel_pm.c      |  3 +--
>  4 files changed, 8 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index d90a707..72cc6d0 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1997,7 +1997,7 @@ 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->pc8.gpu_idle));
> +	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));
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a5caa7e..2a2a3a9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1321,11 +1321,10 @@ struct ilk_wm_values {
>   * 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 and gpu_idle variables. When we
> - * switch requirements_met or gpu_idle 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. The gpu_idle
> - * variable is true when the GPU is idle.
> + * 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
> @@ -1348,7 +1347,6 @@ struct ilk_wm_values {
>   */
>  struct i915_package_c8 {
>  	bool requirements_met;
> -	bool gpu_idle;
>  	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 10ec401..0183a34 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6812,32 +6812,6 @@ done:
>  	mutex_unlock(&dev_priv->pc8.lock);
>  }
>  
> -static void hsw_package_c8_gpu_idle(struct drm_i915_private *dev_priv)
> -{
> -	if (!HAS_PC8(dev_priv->dev))
> -		return;
> -
> -	mutex_lock(&dev_priv->pc8.lock);
> -	if (!dev_priv->pc8.gpu_idle) {
> -		dev_priv->pc8.gpu_idle = true;
> -		__hsw_enable_package_c8(dev_priv);
> -	}
> -	mutex_unlock(&dev_priv->pc8.lock);
> -}
> -
> -static void hsw_package_c8_gpu_busy(struct drm_i915_private *dev_priv)
> -{
> -	if (!HAS_PC8(dev_priv->dev))
> -		return;
> -
> -	mutex_lock(&dev_priv->pc8.lock);
> -	if (dev_priv->pc8.gpu_idle) {
> -		dev_priv->pc8.gpu_idle = false;
> -		__hsw_disable_package_c8(dev_priv);
> -	}
> -	mutex_unlock(&dev_priv->pc8.lock);
> -}
> -
>  #define for_each_power_domain(domain, mask)				\
>  	for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++)	\
>  		if ((1 << (domain)) & (mask))
> @@ -8195,7 +8169,7 @@ void intel_mark_busy(struct drm_device *dev)
>  	if (dev_priv->mm.busy)
>  		return;
>  
> -	hsw_package_c8_gpu_busy(dev_priv);
> +	hsw_disable_package_c8(dev_priv);
>  	i915_update_gfx_val(dev_priv);
>  	dev_priv->mm.busy = true;
>  }
> @@ -8224,7 +8198,7 @@ void intel_mark_idle(struct drm_device *dev)
>  		gen6_rps_idle(dev->dev_private);
>  
>  out:
> -	hsw_package_c8_gpu_idle(dev_priv);
> +	hsw_enable_package_c8(dev_priv);
>  }
>  
>  void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a6b877a..50b80bb 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5786,10 +5786,9 @@ void intel_pm_setup(struct drm_device *dev)
>  
>  	mutex_init(&dev_priv->pc8.lock);
>  	dev_priv->pc8.requirements_met = false;
> -	dev_priv->pc8.gpu_idle = false;
>  	dev_priv->pc8.irqs_disabled = false;
>  	dev_priv->pc8.enabled = false;
> -	dev_priv->pc8.disable_count = 2; /* requirements_met + gpu_idle */
> +	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 10/23] drm/i915: get/put runtime PM when we get/put a power domain

[Imre Deak]

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

On Thu, 2014-02-27 at 19:26 -0300, Paulo Zanoni wrote:
> 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 to
> 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.
> 
> 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 d68fee2..772aa678 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5341,6 +5341,8 @@ void intel_display_power_get(struct drm_device *dev,
>  	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);
> @@ -5372,6 +5374,8 @@ void intel_display_power_put(struct drm_device *dev,
>  		__intel_power_well_put(dev, power_well);
>  
>  	mutex_unlock(&power_domains->lock);
> +
> +	intel_runtime_pm_put(dev_priv);
>  }

I'd prefer to have these in the power_well->enable/disable handlers
after applying the VLV power domains patchset. That way we would have
the whole power well enable/disable sequence laid out in the same
function and avoid going towards a middle-ware like approach.

But this is ok, if we apply this patchset first. In that case:
Reviewed-by: Imre Deak <imre.deak@intel.com>

>  
>  static struct i915_power_domains *hsw_pwr;


[-- 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 12/23] drm/i915: get runtime PM references when the GPU is idle/busy

[Jesse Barnes]

On Thu, 27 Feb 2014 19:26:39 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> 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.
> 
> 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 0d82241..0de1aa7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8102,7 +8102,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;
>  }
> @@ -8131,7 +8131,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,

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 13/23] drm/i915: kill pc8.disable_count

[Jesse Barnes]

On Thu, 27 Feb 2014 19:26:40 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> 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.
> 
> 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 36daaa8..ad6c209 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1998,7 +1998,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 6ea5c4b..8b66c14 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1353,7 +1353,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 0de1aa7..3b57257 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6676,13 +6676,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);
>  }
>  
> @@ -6716,13 +6709,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 962e0d1..86e6835 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5793,7 +5793,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);
>  }

So now it looks like these functions are just documentation around the
runtime PM bits.  I don't see them get remove totally in favor of the
runtime_pm_get|put calls later on, is that possible or desirable?

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 14/23] drm/i915: remove an indirection level on PC8 functions

[Jesse Barnes]

On Thu, 27 Feb 2014 19:26:41 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> After the latest changes, the indirection is useless.
> 
> 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 3b57257..ef0bcf8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6673,12 +6673,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;
> @@ -6706,19 +6700,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);
>  }
>  
> @@ -6728,7 +6716,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);
>  }
>  

Oh here it is, the next patch in the series. :)

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 15/23] drm/i915: don't get/put PC8 reference on freeze/thaw

[Jesse Barnes]

On Thu, 27 Feb 2014 19:26:42 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> 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.
> 
> 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 983ab56..7a8b86e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -433,7 +433,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, true);
>  
>  	drm_kms_helper_poll_disable(dev);
> @@ -606,10 +605,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);

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 16/23] drm/i915: get/put runtime PM references for GMBUS and DP AUX

[Jesse Barnes]

On Thu, 27 Feb 2014 19:26:43 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We had these intel_aux_display_runtime_{get,put} abstractions that
> would just get/put PC8 references, but now that runtime PM and PC8
> are the same stuff, we just need the runtime PM references, so just
> get/put runtime PM directly, because that's what the rest of our code
> does.
> 
> Another way to solve this problem would be to add DP_AUX and GMBUS
> power domains, and then use intel_display_power_{get,put}, but every
> power domain already gets/puts runtime PM references, so this would
> just make things more complex.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  |  4 ++--
>  drivers/gpu/drm/i915/intel_drv.h |  2 --
>  drivers/gpu/drm/i915/intel_i2c.c |  4 ++--
>  drivers/gpu/drm/i915/intel_pm.c  | 11 -----------
>  4 files changed, 4 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index c512d78..79d4844 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -469,7 +469,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  
>  	intel_dp_check_edp(intel_dp);
>  
> -	intel_aux_display_runtime_get(dev_priv);
> +	intel_runtime_pm_get(dev_priv);
>  
>  	/* Try to wait for any previous AUX channel activity */
>  	for (try = 0; try < 3; try++) {
> @@ -563,7 +563,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  	ret = recv_bytes;
>  out:
>  	pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
> -	intel_aux_display_runtime_put(dev_priv);
> +	intel_runtime_pm_put(dev_priv);
>  
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index ea24eae..a2e0cd7 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -890,8 +890,6 @@ void ironlake_teardown_rc6(struct drm_device *dev);
>  void gen6_update_ring_freq(struct drm_device *dev);
>  void gen6_rps_idle(struct drm_i915_private *dev_priv);
>  void gen6_rps_boost(struct drm_i915_private *dev_priv);
> -void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv);
> -void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv);
>  void intel_runtime_pm_get(struct drm_i915_private *dev_priv);
>  void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
>  void intel_init_runtime_pm(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index d33b61d..3d403ce 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -446,7 +446,7 @@ gmbus_xfer(struct i2c_adapter *adapter,
>  	int i, reg_offset;
>  	int ret = 0;
>  
> -	intel_aux_display_runtime_get(dev_priv);
> +	intel_runtime_pm_get(dev_priv);
>  	mutex_lock(&dev_priv->gmbus_mutex);
>  
>  	if (bus->force_bit) {
> @@ -546,7 +546,7 @@ timeout:
>  
>  out:
>  	mutex_unlock(&dev_priv->gmbus_mutex);
> -	intel_aux_display_runtime_put(dev_priv);
> +	intel_runtime_pm_put(dev_priv);
>  	return ret;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 86e6835..1e3580f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5516,17 +5516,6 @@ void intel_power_domains_init_hw(struct drm_device *dev)
>  		I915_WRITE(HSW_PWR_WELL_BIOS, 0);
>  }
>  
> -/* 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);
> -}
> -
> -void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv)
> -{
> -	hsw_enable_package_c8(dev_priv);
> -}
> -
>  void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
>  {
>  	struct drm_device *dev = dev_priv->dev;

But OTOH, in cases where we have a separate, explicit power well for
display, doesn't the aux_display_runtime_get|put make sense?  We don't
want just global runtime get/put everywhere since we can be finer
grained in may cases, right?

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 17/23] drm/i915: don't get/put PC8 when getting/putting power wells

[Jesse Barnes]

On Thu, 27 Feb 2014 19:26:44 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> 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.
> 
> 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      | 12 ++----------
>  3 files changed, 2 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ef0bcf8..d9ea9b89 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6700,26 +6700,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);
> -}
> -
>  #define for_each_power_domain(domain, mask)				\
>  	for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++)	\
>  		if ((1 << (domain)) & (mask))
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a2e0cd7..2c02d68 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -722,8 +722,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 1e3580f..78f56e8 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5311,26 +5311,18 @@ static void hsw_set_power_well(struct drm_device *dev,
>  static void __intel_power_well_get(struct drm_device *dev,
>  				   struct i915_power_well *power_well)
>  {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -
> -	if (!power_well->count++ && power_well->set) {
> -		hsw_disable_package_c8(dev_priv);
> +	if (!power_well->count++ && power_well->set)
>  		power_well->set(dev, power_well, true);
> -	}
>  }
>  
>  static void __intel_power_well_put(struct drm_device *dev,
>  				   struct i915_power_well *power_well)
>  {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -
>  	WARN_ON(!power_well->count);
>  
>  	if (!--power_well->count && power_well->set &&
> -	    i915.disable_power_well) {
> +	    i915.disable_power_well)
>  		power_well->set(dev, power_well, false);
> -		hsw_enable_package_c8(dev_priv);
> -	}
>  }
>  
>  void intel_display_power_get(struct drm_device *dev,

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 18/23] drm/i915: remove dev_priv->pc8.enabled

[Jesse Barnes]

On Thu, 27 Feb 2014 19:26:45 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> 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.
> 
> 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 ad6c209..93d6065 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2000,7 +2000,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 8b66c14..16e344e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1351,8 +1351,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 d9ea9b89..271dd66 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6660,8 +6660,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;
> @@ -6697,7 +6695,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;
>  }
>  
>  #define for_each_power_domain(domain, mask)				\
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 78f56e8..c85507a 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5278,8 +5278,6 @@ static void hsw_set_power_well(struct drm_device *dev,
>  	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;
> @@ -5773,7 +5771,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);
>  }

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 19/23] drm/i915: move pc8.irqs_disabled to pm.irqs_disabled

[Jesse Barnes]

On Thu, 27 Feb 2014 19:26:46 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> 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.
> 
> 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      | 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 93d6065..213b093 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1999,7 +1999,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 16e344e..74fba1c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1350,8 +1350,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;
> @@ -1362,10 +1366,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 6978e69..998dabe 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1017,7 +1017,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 f68aee3..14b26f2 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;
>  	}
> @@ -4016,32 +4016,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;
> @@ -4061,13 +4061,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 271dd66..420fad5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6667,7 +6667,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);
>  }
>  
> @@ -6681,7 +6681,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 2c02d68..d0434e8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -616,8 +616,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 c85507a..5ff4b59 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5770,7 +5770,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;
>  }

I wonder if we should eventually not bother with saving/restoring the
interrupt state and do a full re-init like we'll have to in the display
power well case.  But that's a separate issue really...

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 20/23] drm/i915: kill struct i915_package_c8

[Jesse Barnes]

On Thu, 27 Feb 2014 19:26:47 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> The only remaining field of the struct was the lock, which was
> useless.
> 
> v2: - 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     | 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 213b093..ff81b90 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1996,11 +1996,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 74fba1c..08b22e3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1349,10 +1349,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;
> @@ -1590,8 +1586,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 5ff4b59..3bd6e8f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5769,7 +5769,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);
>  

Yay, this is a nice simplification overall.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 21/23] drm/i915: rename __hsw_do_{en, dis}able_pc8

[Jesse Barnes]

On Thu, 27 Feb 2014 19:26:48 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> 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.
> 
> 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 7a8b86e..88ea6ce 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -846,7 +846,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);
>  
> @@ -880,7 +880,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 420fad5..c981e63 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6651,7 +6651,7 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
>  	gen6_gt_force_wake_put_no_rpm(dev_priv, FORCEWAKE_ALL);
>  }
>  
> -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;
> @@ -6671,7 +6671,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 d0434e8..288f531 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -720,8 +720,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);

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 22/23] drm/i915: update the PC8 and runtime PM documentation

[Jesse Barnes]

On Thu, 27 Feb 2014 19:26:49 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> + * 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 frmo sysfs.

typo "from"

Otherwise, Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

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

[Jesse Barnes]

On Thu, 27 Feb 2014 19:26:50 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> 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.
> 
> 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 3bd6e8f..88b434b 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5535,8 +5535,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;
>  
> @@ -5772,5 +5770,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;
>  }

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Though my earlier comments about getting rid of the init special case
still apply... I think it would be a little easier to understand in
that case (though maybe not, I guess we'd have to see the patches and
resulting code).

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 16/23] drm/i915: get/put runtime PM references for GMBUS and DP AUX

[Imre Deak]

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

On Fri, 2014-02-28 at 09:13 -0800, Jesse Barnes wrote:
> On Thu, 27 Feb 2014 19:26:43 -0300
> Paulo Zanoni <przanoni@gmail.com> wrote:
> 
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > We had these intel_aux_display_runtime_{get,put} abstractions that
> > would just get/put PC8 references, but now that runtime PM and PC8
> > are the same stuff, we just need the runtime PM references, so just
> > get/put runtime PM directly, because that's what the rest of our code
> > does.
> > 
> > Another way to solve this problem would be to add DP_AUX and GMBUS
> > power domains, and then use intel_display_power_{get,put}, but every
> > power domain already gets/puts runtime PM references, so this would
> > just make things more complex.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c  |  4 ++--
> >  drivers/gpu/drm/i915/intel_drv.h |  2 --
> >  drivers/gpu/drm/i915/intel_i2c.c |  4 ++--
> >  drivers/gpu/drm/i915/intel_pm.c  | 11 -----------
> >  4 files changed, 4 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index c512d78..79d4844 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -469,7 +469,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> >  
> >  	intel_dp_check_edp(intel_dp);
> >  
> > -	intel_aux_display_runtime_get(dev_priv);
> > +	intel_runtime_pm_get(dev_priv);
> >  
> >  	/* Try to wait for any previous AUX channel activity */
> >  	for (try = 0; try < 3; try++) {
> > @@ -563,7 +563,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> >  	ret = recv_bytes;
> >  out:
> >  	pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
> > -	intel_aux_display_runtime_put(dev_priv);
> > +	intel_runtime_pm_put(dev_priv);
> >  
> >  	return ret;
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index ea24eae..a2e0cd7 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -890,8 +890,6 @@ void ironlake_teardown_rc6(struct drm_device *dev);
> >  void gen6_update_ring_freq(struct drm_device *dev);
> >  void gen6_rps_idle(struct drm_i915_private *dev_priv);
> >  void gen6_rps_boost(struct drm_i915_private *dev_priv);
> > -void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv);
> > -void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv);
> >  void intel_runtime_pm_get(struct drm_i915_private *dev_priv);
> >  void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
> >  void intel_init_runtime_pm(struct drm_i915_private *dev_priv);
> > diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> > index d33b61d..3d403ce 100644
> > --- a/drivers/gpu/drm/i915/intel_i2c.c
> > +++ b/drivers/gpu/drm/i915/intel_i2c.c
> > @@ -446,7 +446,7 @@ gmbus_xfer(struct i2c_adapter *adapter,
> >  	int i, reg_offset;
> >  	int ret = 0;
> >  
> > -	intel_aux_display_runtime_get(dev_priv);
> > +	intel_runtime_pm_get(dev_priv);
> >  	mutex_lock(&dev_priv->gmbus_mutex);
> >  
> >  	if (bus->force_bit) {
> > @@ -546,7 +546,7 @@ timeout:
> >  
> >  out:
> >  	mutex_unlock(&dev_priv->gmbus_mutex);
> > -	intel_aux_display_runtime_put(dev_priv);
> > +	intel_runtime_pm_put(dev_priv);
> >  	return ret;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 86e6835..1e3580f 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5516,17 +5516,6 @@ void intel_power_domains_init_hw(struct drm_device *dev)
> >  		I915_WRITE(HSW_PWR_WELL_BIOS, 0);
> >  }
> >  
> > -/* 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);
> > -}
> > -
> > -void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv)
> > -{
> > -	hsw_enable_package_c8(dev_priv);
> > -}
> > -
> >  void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
> >  {
> >  	struct drm_device *dev = dev_priv->dev;
> 
> But OTOH, in cases where we have a separate, explicit power well for
> display, doesn't the aux_display_runtime_get|put make sense?  We don't
> want just global runtime get/put everywhere since we can be finer
> grained in may cases, right?

I think here we should just depend on connector->detect and ->get_modes
getting the needed power domains, which will also adjust the RPM
refcount accordingly.

--Imre

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

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

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

Re: [PATCH 16/23] drm/i915: get/put runtime PM references for GMBUS and DP AUX

[Jesse Barnes]

On Fri, 28 Feb 2014 19:38:17 +0200
Imre Deak <imre.deak@intel.com> wrote:

> On Fri, 2014-02-28 at 09:13 -0800, Jesse Barnes wrote:
> > On Thu, 27 Feb 2014 19:26:43 -0300
> > Paulo Zanoni <przanoni@gmail.com> wrote:
> > 
> > > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > 
> > > We had these intel_aux_display_runtime_{get,put} abstractions that
> > > would just get/put PC8 references, but now that runtime PM and PC8
> > > are the same stuff, we just need the runtime PM references, so just
> > > get/put runtime PM directly, because that's what the rest of our code
> > > does.
> > > 
> > > Another way to solve this problem would be to add DP_AUX and GMBUS
> > > power domains, and then use intel_display_power_{get,put}, but every
> > > power domain already gets/puts runtime PM references, so this would
> > > just make things more complex.
> > > 
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c  |  4 ++--
> > >  drivers/gpu/drm/i915/intel_drv.h |  2 --
> > >  drivers/gpu/drm/i915/intel_i2c.c |  4 ++--
> > >  drivers/gpu/drm/i915/intel_pm.c  | 11 -----------
> > >  4 files changed, 4 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index c512d78..79d4844 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -469,7 +469,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > >  
> > >  	intel_dp_check_edp(intel_dp);
> > >  
> > > -	intel_aux_display_runtime_get(dev_priv);
> > > +	intel_runtime_pm_get(dev_priv);
> > >  
> > >  	/* Try to wait for any previous AUX channel activity */
> > >  	for (try = 0; try < 3; try++) {
> > > @@ -563,7 +563,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > >  	ret = recv_bytes;
> > >  out:
> > >  	pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
> > > -	intel_aux_display_runtime_put(dev_priv);
> > > +	intel_runtime_pm_put(dev_priv);
> > >  
> > >  	return ret;
> > >  }
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index ea24eae..a2e0cd7 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -890,8 +890,6 @@ void ironlake_teardown_rc6(struct drm_device *dev);
> > >  void gen6_update_ring_freq(struct drm_device *dev);
> > >  void gen6_rps_idle(struct drm_i915_private *dev_priv);
> > >  void gen6_rps_boost(struct drm_i915_private *dev_priv);
> > > -void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv);
> > > -void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv);
> > >  void intel_runtime_pm_get(struct drm_i915_private *dev_priv);
> > >  void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
> > >  void intel_init_runtime_pm(struct drm_i915_private *dev_priv);
> > > diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> > > index d33b61d..3d403ce 100644
> > > --- a/drivers/gpu/drm/i915/intel_i2c.c
> > > +++ b/drivers/gpu/drm/i915/intel_i2c.c
> > > @@ -446,7 +446,7 @@ gmbus_xfer(struct i2c_adapter *adapter,
> > >  	int i, reg_offset;
> > >  	int ret = 0;
> > >  
> > > -	intel_aux_display_runtime_get(dev_priv);
> > > +	intel_runtime_pm_get(dev_priv);
> > >  	mutex_lock(&dev_priv->gmbus_mutex);
> > >  
> > >  	if (bus->force_bit) {
> > > @@ -546,7 +546,7 @@ timeout:
> > >  
> > >  out:
> > >  	mutex_unlock(&dev_priv->gmbus_mutex);
> > > -	intel_aux_display_runtime_put(dev_priv);
> > > +	intel_runtime_pm_put(dev_priv);
> > >  	return ret;
> > >  }
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index 86e6835..1e3580f 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -5516,17 +5516,6 @@ void intel_power_domains_init_hw(struct drm_device *dev)
> > >  		I915_WRITE(HSW_PWR_WELL_BIOS, 0);
> > >  }
> > >  
> > > -/* 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);
> > > -}
> > > -
> > > -void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv)
> > > -{
> > > -	hsw_enable_package_c8(dev_priv);
> > > -}
> > > -
> > >  void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
> > >  {
> > >  	struct drm_device *dev = dev_priv->dev;
> > 
> > But OTOH, in cases where we have a separate, explicit power well for
> > display, doesn't the aux_display_runtime_get|put make sense?  We don't
> > want just global runtime get/put everywhere since we can be finer
> > grained in may cases, right?
> 
> I think here we should just depend on connector->detect and ->get_modes
> getting the needed power domains, which will also adjust the RPM
> refcount accordingly.

That should work too I think, do we have any paths outside those where
we would do aux poking?  Maybe audio or DDC brightness controls in the
future?  I think audio is ok today, and we can worry about new stuff as
it comes along...

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 19/23] drm/i915: move pc8.irqs_disabled to pm.irqs_disabled

[Paulo Zanoni]

2014-02-28 14:15 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
> On Thu, 27 Feb 2014 19:26:46 -0300
> Paulo Zanoni <przanoni@gmail.com> wrote:
>
>> 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.
>>
>> 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      | 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 93d6065..213b093 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -1999,7 +1999,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 16e344e..74fba1c 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1350,8 +1350,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;
>> @@ -1362,10 +1366,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 6978e69..998dabe 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -1017,7 +1017,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 f68aee3..14b26f2 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;
>>       }
>> @@ -4016,32 +4016,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;
>> @@ -4061,13 +4061,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 271dd66..420fad5 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -6667,7 +6667,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);
>>  }
>>
>> @@ -6681,7 +6681,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 2c02d68..d0434e8 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -616,8 +616,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 c85507a..5ff4b59 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -5770,7 +5770,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;
>>  }
>
> I wonder if we should eventually not bother with saving/restoring the
> interrupt state and do a full re-init like we'll have to in the display
> power well case.  But that's a separate issue really...

That's the plan. See this:
http://cgit.freedesktop.org/~pzanoni/linux/commit/?h=bdw-runtime-pm-2014-02-26&id=843563a642b5471053cb86f631872e02b27de45d

>
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>
> --
> Jesse Barnes, Intel Open Source Technology Center



-- 
Paulo Zanoni

Re: [PATCH 16/23] drm/i915: get/put runtime PM references for GMBUS and DP AUX

[Imre Deak]

On Fri, 2014-02-28 at 09:55 -0800, Jesse Barnes wrote:
> On Fri, 28 Feb 2014 19:38:17 +0200
> Imre Deak <imre.deak@intel.com> wrote:
> 
> > On Fri, 2014-02-28 at 09:13 -0800, Jesse Barnes wrote:
> > > On Thu, 27 Feb 2014 19:26:43 -0300
> > > Paulo Zanoni <przanoni@gmail.com> wrote:
> > > 
> > > > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > 
> > > > We had these intel_aux_display_runtime_{get,put} abstractions that
> > > > would just get/put PC8 references, but now that runtime PM and PC8
> > > > are the same stuff, we just need the runtime PM references, so just
> > > > get/put runtime PM directly, because that's what the rest of our code
> > > > does.
> > > > 
> > > > Another way to solve this problem would be to add DP_AUX and GMBUS
> > > > power domains, and then use intel_display_power_{get,put}, but every
> > > > power domain already gets/puts runtime PM references, so this would
> > > > just make things more complex.
> > > > 
> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp.c  |  4 ++--
> > > >  drivers/gpu/drm/i915/intel_drv.h |  2 --
> > > >  drivers/gpu/drm/i915/intel_i2c.c |  4 ++--
> > > >  drivers/gpu/drm/i915/intel_pm.c  | 11 -----------
> > > >  4 files changed, 4 insertions(+), 17 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > index c512d78..79d4844 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -469,7 +469,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > >  
> > > >  	intel_dp_check_edp(intel_dp);
> > > >  
> > > > -	intel_aux_display_runtime_get(dev_priv);
> > > > +	intel_runtime_pm_get(dev_priv);
> > > >  
> > > >  	/* Try to wait for any previous AUX channel activity */
> > > >  	for (try = 0; try < 3; try++) {
> > > > @@ -563,7 +563,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > >  	ret = recv_bytes;
> > > >  out:
> > > >  	pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
> > > > -	intel_aux_display_runtime_put(dev_priv);
> > > > +	intel_runtime_pm_put(dev_priv);
> > > >  
> > > >  	return ret;
> > > >  }
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > index ea24eae..a2e0cd7 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -890,8 +890,6 @@ void ironlake_teardown_rc6(struct drm_device *dev);
> > > >  void gen6_update_ring_freq(struct drm_device *dev);
> > > >  void gen6_rps_idle(struct drm_i915_private *dev_priv);
> > > >  void gen6_rps_boost(struct drm_i915_private *dev_priv);
> > > > -void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv);
> > > > -void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv);
> > > >  void intel_runtime_pm_get(struct drm_i915_private *dev_priv);
> > > >  void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
> > > >  void intel_init_runtime_pm(struct drm_i915_private *dev_priv);
> > > > diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> > > > index d33b61d..3d403ce 100644
> > > > --- a/drivers/gpu/drm/i915/intel_i2c.c
> > > > +++ b/drivers/gpu/drm/i915/intel_i2c.c
> > > > @@ -446,7 +446,7 @@ gmbus_xfer(struct i2c_adapter *adapter,
> > > >  	int i, reg_offset;
> > > >  	int ret = 0;
> > > >  
> > > > -	intel_aux_display_runtime_get(dev_priv);
> > > > +	intel_runtime_pm_get(dev_priv);
> > > >  	mutex_lock(&dev_priv->gmbus_mutex);
> > > >  
> > > >  	if (bus->force_bit) {
> > > > @@ -546,7 +546,7 @@ timeout:
> > > >  
> > > >  out:
> > > >  	mutex_unlock(&dev_priv->gmbus_mutex);
> > > > -	intel_aux_display_runtime_put(dev_priv);
> > > > +	intel_runtime_pm_put(dev_priv);
> > > >  	return ret;
> > > >  }
> > > >  
> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > index 86e6835..1e3580f 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > @@ -5516,17 +5516,6 @@ void intel_power_domains_init_hw(struct drm_device *dev)
> > > >  		I915_WRITE(HSW_PWR_WELL_BIOS, 0);
> > > >  }
> > > >  
> > > > -/* 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);
> > > > -}
> > > > -
> > > > -void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv)
> > > > -{
> > > > -	hsw_enable_package_c8(dev_priv);
> > > > -}
> > > > -
> > > >  void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
> > > >  {
> > > >  	struct drm_device *dev = dev_priv->dev;
> > > 
> > > But OTOH, in cases where we have a separate, explicit power well for
> > > display, doesn't the aux_display_runtime_get|put make sense?  We don't
> > > want just global runtime get/put everywhere since we can be finer
> > > grained in may cases, right?
> > 
> > I think here we should just depend on connector->detect and ->get_modes
> > getting the needed power domains, which will also adjust the RPM
> > refcount accordingly.
> 
> That should work too I think, do we have any paths outside those where
> we would do aux poking?

Yea, actually link (re)training for example, but for that we already now
take the needed power domains (and hence adjust RPM) during modeset.

> Maybe audio or DDC brightness controls in the future?  I think audio is
> ok today, and we can worry about new stuff as it comes along...

I assume all these need a non-NULL modeset, so we are covered based on
the above. Also as a side-note, we have to get/put the power domain on
these paths not RPM, because some platforms may need some power well to
be on in addition to the D0 state RPM guarantees.

--Imre

Re: [PATCH 16/23] drm/i915: get/put runtime PM references for GMBUS and DP AUX

[Paulo Zanoni]

2014-02-28 15:20 GMT-03:00 Imre Deak <imre.deak@intel.com>:
> On Fri, 2014-02-28 at 09:55 -0800, Jesse Barnes wrote:
>> On Fri, 28 Feb 2014 19:38:17 +0200
>> Imre Deak <imre.deak@intel.com> wrote:
>>
>> > On Fri, 2014-02-28 at 09:13 -0800, Jesse Barnes wrote:
>> > > On Thu, 27 Feb 2014 19:26:43 -0300
>> > > Paulo Zanoni <przanoni@gmail.com> wrote:
>> > >
>> > > > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> > > >
>> > > > We had these intel_aux_display_runtime_{get,put} abstractions that
>> > > > would just get/put PC8 references, but now that runtime PM and PC8
>> > > > are the same stuff, we just need the runtime PM references, so just
>> > > > get/put runtime PM directly, because that's what the rest of our code
>> > > > does.
>> > > >
>> > > > Another way to solve this problem would be to add DP_AUX and GMBUS
>> > > > power domains, and then use intel_display_power_{get,put}, but every
>> > > > power domain already gets/puts runtime PM references, so this would
>> > > > just make things more complex.
>> > > >
>> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> > > > ---
>> > > >  drivers/gpu/drm/i915/intel_dp.c  |  4 ++--
>> > > >  drivers/gpu/drm/i915/intel_drv.h |  2 --
>> > > >  drivers/gpu/drm/i915/intel_i2c.c |  4 ++--
>> > > >  drivers/gpu/drm/i915/intel_pm.c  | 11 -----------
>> > > >  4 files changed, 4 insertions(+), 17 deletions(-)
>> > > >
>> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> > > > index c512d78..79d4844 100644
>> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > > > @@ -469,7 +469,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>> > > >
>> > > >         intel_dp_check_edp(intel_dp);
>> > > >
>> > > > -       intel_aux_display_runtime_get(dev_priv);
>> > > > +       intel_runtime_pm_get(dev_priv);
>> > > >
>> > > >         /* Try to wait for any previous AUX channel activity */
>> > > >         for (try = 0; try < 3; try++) {
>> > > > @@ -563,7 +563,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>> > > >         ret = recv_bytes;
>> > > >  out:
>> > > >         pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
>> > > > -       intel_aux_display_runtime_put(dev_priv);
>> > > > +       intel_runtime_pm_put(dev_priv);
>> > > >
>> > > >         return ret;
>> > > >  }
>> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> > > > index ea24eae..a2e0cd7 100644
>> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
>> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
>> > > > @@ -890,8 +890,6 @@ void ironlake_teardown_rc6(struct drm_device *dev);
>> > > >  void gen6_update_ring_freq(struct drm_device *dev);
>> > > >  void gen6_rps_idle(struct drm_i915_private *dev_priv);
>> > > >  void gen6_rps_boost(struct drm_i915_private *dev_priv);
>> > > > -void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv);
>> > > > -void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv);
>> > > >  void intel_runtime_pm_get(struct drm_i915_private *dev_priv);
>> > > >  void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
>> > > >  void intel_init_runtime_pm(struct drm_i915_private *dev_priv);
>> > > > diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
>> > > > index d33b61d..3d403ce 100644
>> > > > --- a/drivers/gpu/drm/i915/intel_i2c.c
>> > > > +++ b/drivers/gpu/drm/i915/intel_i2c.c
>> > > > @@ -446,7 +446,7 @@ gmbus_xfer(struct i2c_adapter *adapter,
>> > > >         int i, reg_offset;
>> > > >         int ret = 0;
>> > > >
>> > > > -       intel_aux_display_runtime_get(dev_priv);
>> > > > +       intel_runtime_pm_get(dev_priv);
>> > > >         mutex_lock(&dev_priv->gmbus_mutex);
>> > > >
>> > > >         if (bus->force_bit) {
>> > > > @@ -546,7 +546,7 @@ timeout:
>> > > >
>> > > >  out:
>> > > >         mutex_unlock(&dev_priv->gmbus_mutex);
>> > > > -       intel_aux_display_runtime_put(dev_priv);
>> > > > +       intel_runtime_pm_put(dev_priv);
>> > > >         return ret;
>> > > >  }
>> > > >
>> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> > > > index 86e6835..1e3580f 100644
>> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
>> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
>> > > > @@ -5516,17 +5516,6 @@ void intel_power_domains_init_hw(struct drm_device *dev)
>> > > >                 I915_WRITE(HSW_PWR_WELL_BIOS, 0);
>> > > >  }
>> > > >
>> > > > -/* 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);
>> > > > -}
>> > > > -
>> > > > -void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv)
>> > > > -{
>> > > > -       hsw_enable_package_c8(dev_priv);
>> > > > -}
>> > > > -
>> > > >  void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
>> > > >  {
>> > > >         struct drm_device *dev = dev_priv->dev;
>> > >
>> > > But OTOH, in cases where we have a separate, explicit power well for
>> > > display, doesn't the aux_display_runtime_get|put make sense?  We don't
>> > > want just global runtime get/put everywhere since we can be finer
>> > > grained in may cases, right?
>> >
>> > I think here we should just depend on connector->detect and ->get_modes
>> > getting the needed power domains, which will also adjust the RPM
>> > refcount accordingly.
>>
>> That should work too I think, do we have any paths outside those where
>> we would do aux poking?
>
> Yea, actually link (re)training for example, but for that we already now
> take the needed power domains (and hence adjust RPM) during modeset.
>
>> Maybe audio or DDC brightness controls in the future?  I think audio is
>> ok today, and we can worry about new stuff as it comes along...
>
> I assume all these need a non-NULL modeset, so we are covered based on
> the above. Also as a side-note, we have to get/put the power domain on
> these paths not RPM, because some platforms may need some power well to
> be on in addition to the D0 state RPM guarantees.

I agree with Imre. For now, while the current code just needs runtime
PM, we just get runtime PM. But as soon as we add support to some
power well that also needs to be enabled, then we just get the
appropriate power domain instead of runtime PM (remember: on this
series, whenever we get a power domain reference, we also get a
runtime PM reference).

>
> --Imre
>



-- 
Paulo Zanoni

Re: [PATCH 07/23] drm/i915: add forcewake functions that don't touch runtime PM

[Paulo Zanoni]

2014-02-28 5:44 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Thu, Feb 27, 2014 at 07:26:34PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> To solve a chicken-and-egg problem. Currently when we get/put
>> forcewake we also get/put runtime PM and this works fine because the
>> runtime PM code doesn't need forcewake. But we're going to merge PC8
>> and runtime PM into a single feature, and the PC8 code (the LCPLL
>> code) does need the forcewake, so that specific piece of code needs to
>> call the _no_rpm version so it doesn't try to get runtime PM in the
>> code that gets runtime PM.
>>
>> For now the new functions are unused, we'll use them on the patch that
>> merges PC8 with runtime PM.
>>
>> Also notice that, so simplify things, the put() function doesn't use
>> the workqueue, since the workqueue also puts runtime PM.
>
> Where are these routines called? The names are awful but not quite awful
> enough to avoid confusion. Is it possible to hide these to a single .c?

They are called on patch 09/23, inside function hsw_restore_lcpll(),
from intel_display.c.

I'm always open to naming suggestions. I could try
gen6_gt_force_wake_get_no_runtime_pm.

I could also just try to put all this code inline in the caller, but
IMHO that wouldn't be an improvement over this.

Another thing worth mentioning, is that at hsw_restore_lcpll we expect
the forcewake count to be always zero. So we could do some other kind
of trick relying on that, but I don't think it's very future-proof.

>
> The workqueue doesn't touch rpm here, so there routines could be
> simplified if they remain in intel_uncore.c.

I don't understand what you mean with the sentence above. Could you
please clarify?

Thanks for the review!
Paulo

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni

Re: [PATCH 07/23] drm/i915: add forcewake functions that don't touch runtime PM

[Chris Wilson]

On Fri, Feb 28, 2014 at 04:38:28PM -0300, Paulo Zanoni wrote:
> Another thing worth mentioning, is that at hsw_restore_lcpll we expect
> the forcewake count to be always zero. So we could do some other kind
> of trick relying on that, but I don't think it's very future-proof.

Actually, having read how these new functions were used, I have further
doubts that they are being used correctly. I think adding comments to
the callsites should answer most of my questions, or help show the
weakness in the current code.
 
> 2014-02-28 5:44 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > The workqueue doesn't touch rpm here, so there routines could be
> > simplified if they remain in intel_uncore.c.
> 
> I don't understand what you mean with the sentence above. Could you
> please clarify?

I was reading the patches out of order, so hadn't seen the addition of
the rpm into the workqueue.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 10/23] drm/i915: get/put runtime PM when we get/put a power domain

[Paulo Zanoni]

2014-02-28 12:45 GMT-03:00 Imre Deak <imre.deak@intel.com>:
> On Thu, 2014-02-27 at 19:26 -0300, Paulo Zanoni wrote:
>> 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 to
>> 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.
>>
>> 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 d68fee2..772aa678 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -5341,6 +5341,8 @@ void intel_display_power_get(struct drm_device *dev,
>>       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);
>> @@ -5372,6 +5374,8 @@ void intel_display_power_put(struct drm_device *dev,
>>               __intel_power_well_put(dev, power_well);
>>
>>       mutex_unlock(&power_domains->lock);
>> +
>> +     intel_runtime_pm_put(dev_priv);
>>  }
>
> I'd prefer to have these in the power_well->enable/disable handlers
> after applying the VLV power domains patchset. That way we would have
> the whole power well enable/disable sequence laid out in the same
> function and avoid going towards a middle-ware like approach.

As long as we get/put runtime PM for every single power domain,
including the ones that don't have a corresponding power well, I'm
fine your suggestion. But IMHO the current approach is the simplest,
and I don't see why we would want do change the order here (notice
that I didn't read the VLV docs).

>
> But this is ok, if we apply this patchset first. In that case:
> Reviewed-by: Imre Deak <imre.deak@intel.com>

Thanks for the reviews!

>
>>
>>  static struct i915_power_domains *hsw_pwr;
>



-- 
Paulo Zanoni

Re: [PATCH 07/23] drm/i915: add forcewake functions that don't touch runtime PM

[Daniel Vetter]

On Thu, Feb 27, 2014 at 07:26:34PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> To solve a chicken-and-egg problem. Currently when we get/put
> forcewake we also get/put runtime PM and this works fine because the
> runtime PM code doesn't need forcewake. But we're going to merge PC8
> and runtime PM into a single feature, and the PC8 code (the LCPLL
> code) does need the forcewake, so that specific piece of code needs to
> call the _no_rpm version so it doesn't try to get runtime PM in the
> code that gets runtime PM.
> 
> For now the new functions are unused, we'll use them on the patch that
> merges PC8 with runtime PM.
> 
> Also notice that, so simplify things, the put() function doesn't use
> the workqueue, since the workqueue also puts runtime PM.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

See my previous mail, but I'm still freaked out about this. Doing runtime
pm hidden deep down in our register I/O functions is imo rather deeply
troubling ... I'll punt for now on this.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 16/23] drm/i915: get/put runtime PM references for GMBUS and DP AUX

[Daniel Vetter]

On Fri, Feb 28, 2014 at 09:55:12AM -0800, Jesse Barnes wrote:
> On Fri, 28 Feb 2014 19:38:17 +0200
> Imre Deak <imre.deak@intel.com> wrote:
> 
> > On Fri, 2014-02-28 at 09:13 -0800, Jesse Barnes wrote:
> > > On Thu, 27 Feb 2014 19:26:43 -0300
> > > Paulo Zanoni <przanoni@gmail.com> wrote:
> > > 
> > > > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > 
> > > > We had these intel_aux_display_runtime_{get,put} abstractions that
> > > > would just get/put PC8 references, but now that runtime PM and PC8
> > > > are the same stuff, we just need the runtime PM references, so just
> > > > get/put runtime PM directly, because that's what the rest of our code
> > > > does.
> > > > 
> > > > Another way to solve this problem would be to add DP_AUX and GMBUS
> > > > power domains, and then use intel_display_power_{get,put}, but every
> > > > power domain already gets/puts runtime PM references, so this would
> > > > just make things more complex.
> > > > 
> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp.c  |  4 ++--
> > > >  drivers/gpu/drm/i915/intel_drv.h |  2 --
> > > >  drivers/gpu/drm/i915/intel_i2c.c |  4 ++--
> > > >  drivers/gpu/drm/i915/intel_pm.c  | 11 -----------
> > > >  4 files changed, 4 insertions(+), 17 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > index c512d78..79d4844 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -469,7 +469,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > >  
> > > >  	intel_dp_check_edp(intel_dp);
> > > >  
> > > > -	intel_aux_display_runtime_get(dev_priv);
> > > > +	intel_runtime_pm_get(dev_priv);
> > > >  
> > > >  	/* Try to wait for any previous AUX channel activity */
> > > >  	for (try = 0; try < 3; try++) {
> > > > @@ -563,7 +563,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > >  	ret = recv_bytes;
> > > >  out:
> > > >  	pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
> > > > -	intel_aux_display_runtime_put(dev_priv);
> > > > +	intel_runtime_pm_put(dev_priv);
> > > >  
> > > >  	return ret;
> > > >  }
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > index ea24eae..a2e0cd7 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -890,8 +890,6 @@ void ironlake_teardown_rc6(struct drm_device *dev);
> > > >  void gen6_update_ring_freq(struct drm_device *dev);
> > > >  void gen6_rps_idle(struct drm_i915_private *dev_priv);
> > > >  void gen6_rps_boost(struct drm_i915_private *dev_priv);
> > > > -void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv);
> > > > -void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv);
> > > >  void intel_runtime_pm_get(struct drm_i915_private *dev_priv);
> > > >  void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
> > > >  void intel_init_runtime_pm(struct drm_i915_private *dev_priv);
> > > > diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> > > > index d33b61d..3d403ce 100644
> > > > --- a/drivers/gpu/drm/i915/intel_i2c.c
> > > > +++ b/drivers/gpu/drm/i915/intel_i2c.c
> > > > @@ -446,7 +446,7 @@ gmbus_xfer(struct i2c_adapter *adapter,
> > > >  	int i, reg_offset;
> > > >  	int ret = 0;
> > > >  
> > > > -	intel_aux_display_runtime_get(dev_priv);
> > > > +	intel_runtime_pm_get(dev_priv);
> > > >  	mutex_lock(&dev_priv->gmbus_mutex);
> > > >  
> > > >  	if (bus->force_bit) {
> > > > @@ -546,7 +546,7 @@ timeout:
> > > >  
> > > >  out:
> > > >  	mutex_unlock(&dev_priv->gmbus_mutex);
> > > > -	intel_aux_display_runtime_put(dev_priv);
> > > > +	intel_runtime_pm_put(dev_priv);
> > > >  	return ret;
> > > >  }
> > > >  
> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > index 86e6835..1e3580f 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > @@ -5516,17 +5516,6 @@ void intel_power_domains_init_hw(struct drm_device *dev)
> > > >  		I915_WRITE(HSW_PWR_WELL_BIOS, 0);
> > > >  }
> > > >  
> > > > -/* 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);
> > > > -}
> > > > -
> > > > -void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv)
> > > > -{
> > > > -	hsw_enable_package_c8(dev_priv);
> > > > -}
> > > > -
> > > >  void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
> > > >  {
> > > >  	struct drm_device *dev = dev_priv->dev;
> > > 
> > > But OTOH, in cases where we have a separate, explicit power well for
> > > display, doesn't the aux_display_runtime_get|put make sense?  We don't
> > > want just global runtime get/put everywhere since we can be finer
> > > grained in may cases, right?
> > 
> > I think here we should just depend on connector->detect and ->get_modes
> > getting the needed power domains, which will also adjust the RPM
> > refcount accordingly.
> 
> That should work too I think, do we have any paths outside those where
> we would do aux poking?  Maybe audio or DDC brightness controls in the
> future?  I think audio is ok today, and we can worry about new stuff as
> it comes along...

Yes, userspace can directly do i2c transactions on gmbus and through i2c
over dp aux also there. And I guess eventually our dp aux helpers in drm
core will grow their own native userspace interface.

I'm not on top of all the changes in our future platforms, iirc there's
been some more fine-grained changes just for the dp aux/gmbus stuff.
Damien/Ville/Rodrigo?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 00/23] Merge PC8 with runtime PM, v2

[Daniel Vetter]

On Thu, Feb 27, 2014 at 07:26:27PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Hi
> 
> This is the second time I send this series to the mailing list. Please read the
> first cover letter:
>    http://lists.freedesktop.org/archives/intel-gfx/2013-December/037721.html
> 
> What's new?
> 
> Two main differences:
>   - Added a patch from Chris, which resulted in a change on how we handle
>     dev_priv->pc8.gpu_busy later.
>   - Fixed a bug on the forcewake handling.
> 
> There is some discussion if we want to merge this first, or the VLV power wells
> patches first. Independently of the decision, I think we should try to at least
> discuss these patches and review what can be reviewed. I really think this
> series should make it easier to add runtime PM support to other platforms, and I
> even added BDW and SNB support on top of these patches.

Looks good. Only too imo minor things:
- Drop the runtime pm get/put from the forcewake get/put functions and
  adjust callers accordingly to grab the runtime pm ref themselves outside
  of the register access. That should fully address Chris'&mine concern in
  that regard. I only expect a minor impact here, if that turns out wrong
  please complain and we need to take another look.

- The aux display power domain patch. Needs acks from platform owners imo,
  I'd just keep it for now.

Also I've merged quite a few patches already, hopefully the most recent
versions. When rebasing pls collect all the r-b tags for speedy merging.

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

Re: [PATCH 00/23] Merge PC8 with runtime PM, v2

[Paulo Zanoni]

2014-03-05 14:09 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> On Thu, Feb 27, 2014 at 07:26:27PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Hi
>>
>> This is the second time I send this series to the mailing list. Please read the
>> first cover letter:
>>    http://lists.freedesktop.org/archives/intel-gfx/2013-December/037721.html
>>
>> What's new?
>>
>> Two main differences:
>>   - Added a patch from Chris, which resulted in a change on how we handle
>>     dev_priv->pc8.gpu_busy later.
>>   - Fixed a bug on the forcewake handling.
>>
>> There is some discussion if we want to merge this first, or the VLV power wells
>> patches first. Independently of the decision, I think we should try to at least
>> discuss these patches and review what can be reviewed. I really think this
>> series should make it easier to add runtime PM support to other platforms, and I
>> even added BDW and SNB support on top of these patches.
>
> Looks good. Only too imo minor things:
> - Drop the runtime pm get/put from the forcewake get/put functions and
>   adjust callers accordingly to grab the runtime pm ref themselves outside
>   of the register access. That should fully address Chris'&mine concern in
>   that regard. I only expect a minor impact here, if that turns out wrong
>   please complain and we need to take another look.

I am confused, because you're asking me this, but you merged
"drm/i915: put runtime PM only when we actually release force_wake"
without commenting anything. Maybe it's not clear to me what you
really want. Let me explain the problem so you can give your opinion.

First of all, the thing is that we're getting/putting runtime PM only
at gen6_gt_force_wake_get/put, and that is *not* called when we're
reading/writing generic registers, it's only called explicitly at
certain points of our code (mostly intel_pm.c, and a few other
places). So keep in mind that we're not getting/putting any runtime PM
references at I915_READ/WRITE, and we *do* have a check to see if we
do any reads/writes while we're runtime suspended.

I can't do what you're requesting because of the delayed forcewake
put. Imagine the following code:

1. intel_runtime_pm_get()
2. gen6_gt_force_wake_get()
3. do_stuff()
4. gen6_gt_force_wake_put()
5. intel_runtime_pm_put()

The problem is that in step 4 we won't actually be putting forcewake,
we will just be launching the dealyed function that will, eventually,
disable forcewake. So at step 5 we will put runtime PM, allowing the
device to be suspended while it is still in "forcewake" mode. With
this, we may runtime suspend the driver, then later
gen6_gt_force_wake_work() will be called, which will trigger
__gen6_gt_force_wake_put() - notice the underlines on the name - which
will try to read/write registers while the device is runtime
suspended. And this is not what we want.

So, IMHO, it doesn't make sense to runtime suspend the driver while
we're in forcewake mode. And that's why we can't make the callers of
gen6_gt_force_wake_get() get their own runtime PM refcounts. It also
doesn't make sense to drop the forcewake refcount and just cancel the
forcewake task when we runtime suspend, because if we do this we'll
just be unsolving the exact problem that the delayed forcewake queue
solves.

So, considering the problem above, what exactly do you need and why?

>
> - The aux display power domain patch. Needs acks from platform owners imo,
>   I'd just keep it for now.

I can write a new patch that doesn't kill
intel_aux_display_runtime_get. The implementation will just contain a
call to intel_runtime_pm_get... Then you can merge the color you
prefer :)


>
> Also I've merged quite a few patches already, hopefully the most recent
> versions. When rebasing pls collect all the r-b tags for speedy merging.
>
> Thanks, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

Re: [PATCH 00/23] Merge PC8 with runtime PM, v2

[Daniel Vetter]

On Thu, Mar 6, 2014 at 11:23 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2014-03-05 14:09 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
>> On Thu, Feb 27, 2014 at 07:26:27PM -0300, Paulo Zanoni wrote:
>>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>>
>>> Hi
>>>
>>> This is the second time I send this series to the mailing list. Please read the
>>> first cover letter:
>>>    http://lists.freedesktop.org/archives/intel-gfx/2013-December/037721.html
>>>
>>> What's new?
>>>
>>> Two main differences:
>>>   - Added a patch from Chris, which resulted in a change on how we handle
>>>     dev_priv->pc8.gpu_busy later.
>>>   - Fixed a bug on the forcewake handling.
>>>
>>> There is some discussion if we want to merge this first, or the VLV power wells
>>> patches first. Independently of the decision, I think we should try to at least
>>> discuss these patches and review what can be reviewed. I really think this
>>> series should make it easier to add runtime PM support to other platforms, and I
>>> even added BDW and SNB support on top of these patches.
>>
>> Looks good. Only too imo minor things:
>> - Drop the runtime pm get/put from the forcewake get/put functions and
>>   adjust callers accordingly to grab the runtime pm ref themselves outside
>>   of the register access. That should fully address Chris'&mine concern in
>>   that regard. I only expect a minor impact here, if that turns out wrong
>>   please complain and we need to take another look.
>
> I am confused, because you're asking me this, but you merged
> "drm/i915: put runtime PM only when we actually release force_wake"
> without commenting anything. Maybe it's not clear to me what you
> really want. Let me explain the problem so you can give your opinion.
>
> First of all, the thing is that we're getting/putting runtime PM only
> at gen6_gt_force_wake_get/put, and that is *not* called when we're
> reading/writing generic registers, it's only called explicitly at
> certain points of our code (mostly intel_pm.c, and a few other
> places). So keep in mind that we're not getting/putting any runtime PM
> references at I915_READ/WRITE, and we *do* have a check to see if we
> do any reads/writes while we're runtime suspended.
>
> I can't do what you're requesting because of the delayed forcewake
> put. Imagine the following code:
>
> 1. intel_runtime_pm_get()
> 2. gen6_gt_force_wake_get()
> 3. do_stuff()
> 4. gen6_gt_force_wake_put()
> 5. intel_runtime_pm_put()
>
> The problem is that in step 4 we won't actually be putting forcewake,
> we will just be launching the dealyed function that will, eventually,
> disable forcewake. So at step 5 we will put runtime PM, allowing the
> device to be suspended while it is still in "forcewake" mode. With
> this, we may runtime suspend the driver, then later
> gen6_gt_force_wake_work() will be called, which will trigger
> __gen6_gt_force_wake_put() - notice the underlines on the name - which
> will try to read/write registers while the device is runtime
> suspended. And this is not what we want.
>
> So, IMHO, it doesn't make sense to runtime suspend the driver while
> we're in forcewake mode. And that's why we can't make the callers of
> gen6_gt_force_wake_get() get their own runtime PM refcounts. It also
> doesn't make sense to drop the forcewake refcount and just cancel the
> forcewake task when we runtime suspend, because if we do this we'll
> just be unsolving the exact problem that the delayed forcewake queue
> solves.
>
> So, considering the problem above, what exactly do you need and why?

I agree that I didn't understand the issue at hand at all, and that
your approach is sound. That leaves us with Chris' concern, I'll reply
directly to Chris' mail to address that.

My apologies for the massive confusion I've caused here.

>> - The aux display power domain patch. Needs acks from platform owners imo,
>>   I'd just keep it for now.
>
> I can write a new patch that doesn't kill
> intel_aux_display_runtime_get. The implementation will just contain a
> call to intel_runtime_pm_get... Then you can merge the color you
> prefer :)

Yeah, I think for now I prefer to keep the aux_display_runtime_get/put
color, at least as long as the other platform owners don't wake up ;-)

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

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

[Daniel Vetter]

On Fri, Feb 28, 2014 at 09:42:02AM +0000, Chris Wilson wrote:
> On Thu, Feb 27, 2014 at 07:26:36PM -0300, Paulo Zanoni wrote:
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index ce582eb..788b165 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6614,7 +6614,7 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
> >  
> >  	/* 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);
> > +	gen6_gt_force_wake_get_no_rpm(dev_priv, FORCEWAKE_ALL);
> 
> I want more indications that this is legal - i.e. that the function is
> only called from a special context handling rpm. As it is, it is not
> immediately clear that the rearrangements in disable_package_c8 is
> complete. If you moved this forcwake dance higher and comment why the
> intel_runtime_pm_get() handling is so special, you might not scare so
> many causal readers.

I agree with Chris, this is tricky and should be obvious and stand out.

What about open-coding the _no_rpm function here (like we have an
open-coded fw get/put around register acccess) and putting a big comment
above this explaining what exactly is going on, why we need it and why
this is safe?

For this case here being explicit and verbose is imo more important than
the little common code extraction the _no_rpm function allows us to do.
Especially since we have open-coded forcewake dances at other places
already, too.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 00/23] Merge PC8 with runtime PM, v2

[Daniel Vetter]

On Thu, Mar 6, 2014 at 11:45 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> I agree that I didn't understand the issue at hand at all, and that
> your approach is sound. That leaves us with Chris' concern, I'll reply
> directly to Chris' mail to address that.
>
> My apologies for the massive confusion I've caused here.

I've forgotten to mention: Please add your excellent explanation from
above to the commit message which adds the no-runtime-pm forcewake
get/put calls.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch