[PATCH 00/11] Runtime PM fixes

[PATCH 00/11] Runtime PM fixes

[Paulo Zanoni]

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

Hi

This series contains a collection of fixes for problems I found on the runtime
PM code. All these problems either result in failure of some pm_pc8 test, or
result in a Kernel WARN/DRM_ERROR when running pm_pc8, or there is a patch in
this series that adds a WARN that will be triggered if we ever unfix the
problem. Notice that nothing comes from bugzilla reports (see below).

So the real fixes of this series are patches 1-5. Patches 6+ either only fix
debugfs files, or add more WARNs to make sure the problems we solve here remain
solved, or do small cleanups.

I did _not_ add Cc: stable@vger.kernel.org tags to the patches for the following
reasons reasons: (i) I simply don't know if we want them; (ii) I have other 44
patches that should be applied on top of these, so having part of the series in
-fixes and another in -next can be a mess; and (iii) runtime PM is still
disabled by default (although a simple sysfs write can enable it, and powertop
makes this even easier) and its autosuspend delay is set to 10s, plus 5ms of PC8
timeout, so the problems are either very hard or impossible to reproduce without
i915.pc8_timeout=0 (and that's why, I believe, we don't have bug reports for
these things). But if we wan to Cc stable, it's fine too: we can do this with
all the patches, or just with 1-5, or more.

Thanks,
Paulo

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

Paulo Zanoni (10):
  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: get runtime PM at intel_set_mode
  drm/i915: get runtime PM while trying to detect CRT
  drm/i915: get/put runtime PM in more places at i915_debugfs.c
  drm/i915: kill dev_priv->pc8.gpu_idle
  drm/i915: call assert_device_not_suspended at gen6_force_wake_work
  drm/i915: assert force wake is disabled when we runtime suspend
  drm/i915: don't get/put runtime PM at the debugfs forcewake file
  drm/i915: assert we're not runtime suspended when writing registers

 drivers/gpu/drm/i915/i915_debugfs.c  | 21 +++++++++++++---
 drivers/gpu/drm/i915/i915_drv.c      |  1 +
 drivers/gpu/drm/i915/i915_drv.h      | 19 ++++++++++-----
 drivers/gpu/drm/i915/intel_crt.c     | 26 ++++++++++++++------
 drivers/gpu/drm/i915/intel_display.c | 47 ++++++++++++++----------------------
 drivers/gpu/drm/i915/intel_pm.c      |  3 +--
 drivers/gpu/drm/i915/intel_uncore.c  | 34 ++++++++++++++++++--------
 7 files changed, 94 insertions(+), 57 deletions(-)

-- 
1.8.5.3

[PATCH 01/11] 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: From Paulo
  - Make it compile
  - Drop the __i915_add_request chunk

Reported-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>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      | 8 ++++++++
 drivers/gpu/drm/i915/intel_display.c | 9 +++++++++
 2 files changed, 17 insertions(+)


Chris did not reply to my review comments yet, so I just went and implemented
them. We need at least an ACK form him here before merging.


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/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/11] 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
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/11] 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!).

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index c628414..1f7226f 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,6 +395,7 @@ 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;
@@ -405,13 +408,15 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
 	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);
+	if (!delayed)
+		intel_runtime_pm_put(dev_priv);
 }
 
 /* We give fast paths for the really cool registers */
-- 
1.8.5.3

[PATCH 04/11] drm/i915: get runtime PM at intel_set_mode

[Paulo Zanoni]

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

Otherwise, when we run intel_modeset_check_state we may already be
runtime suspended, and our state checking code will read registers
while the device is suspended. This can only happen if your
autosuspend_delay_ms is low (not the default 10s).

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 10ec401..c64fb7f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9746,13 +9746,18 @@ static int intel_set_mode(struct drm_crtc *crtc,
 			  struct drm_display_mode *mode,
 			  int x, int y, struct drm_framebuffer *fb)
 {
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
+	intel_runtime_pm_get(dev_priv);
+
 	ret = __intel_set_mode(crtc, mode, x, y, fb);
 
 	if (ret == 0)
 		intel_modeset_check_state(crtc->dev);
 
+	intel_runtime_pm_put(dev_priv);
 	return ret;
 }
 
-- 
1.8.5.3

[PATCH 05/11] drm/i915: get runtime PM while trying to detect CRT

[Paulo Zanoni]

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

Otherwise we'll read registers that return 0xffffffff, trigger some
WARNs, think CRT is actually connected (because certain bits are 1),
and fail the drm-resources-equal testcase!

Tested on a SNB machine with runtime PM support (which is not upstream
yet, but is already on my public tree at freedesktop.org, and will
hopefully eventually become upstream).

Testcase: igt/pm_pc8/drm-resources-equal
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_crt.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 9864aa1..4c1230c 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -630,10 +630,13 @@ static enum drm_connector_status
 intel_crt_detect(struct drm_connector *connector, bool force)
 {
 	struct drm_device *dev = connector->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crt *crt = intel_attached_crt(connector);
 	enum drm_connector_status status;
 	struct intel_load_detect_pipe tmp;
 
+	intel_runtime_pm_get(dev_priv);
+
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force=%d\n",
 		      connector->base.id, drm_get_connector_name(connector),
 		      force);
@@ -645,23 +648,30 @@ intel_crt_detect(struct drm_connector *connector, bool force)
 		 */
 		if (intel_crt_detect_hotplug(connector)) {
 			DRM_DEBUG_KMS("CRT detected via hotplug\n");
-			return connector_status_connected;
+			status = connector_status_connected;
+			goto out;
 		} else
 			DRM_DEBUG_KMS("CRT not detected via hotplug\n");
 	}
 
-	if (intel_crt_detect_ddc(connector))
-		return connector_status_connected;
+	if (intel_crt_detect_ddc(connector)) {
+		status = connector_status_connected;
+		goto out;
+	}
 
 	/* Load detection is broken on HPD capable machines. Whoever wants a
 	 * broken monitor (without edid) to work behind a broken kvm (that fails
 	 * to have the right resistors for HP detection) needs to fix this up.
 	 * For now just bail out. */
-	if (I915_HAS_HOTPLUG(dev))
-		return connector_status_disconnected;
+	if (I915_HAS_HOTPLUG(dev)) {
+		status = connector_status_disconnected;
+		goto out;
+	}
 
-	if (!force)
-		return connector->status;
+	if (!force) {
+		status = connector->status;
+		goto out;
+	}
 
 	/* for pre-945g platforms use load detect */
 	if (intel_get_load_detect_pipe(connector, NULL, &tmp)) {
@@ -673,6 +683,8 @@ intel_crt_detect(struct drm_connector *connector, bool force)
 	} else
 		status = connector_status_unknown;
 
+out:
+	intel_runtime_pm_put(dev_priv);
 	return status;
 }
 
-- 
1.8.5.3

[PATCH 06/11] drm/i915: get/put runtime PM in more places at i915_debugfs.c

[Paulo Zanoni]

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

These are places where we read (not write) registers while we're
runtime suspended.

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index d90a707..34e347f 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1348,6 +1348,8 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
 		return 0;
 	}
 
+	intel_runtime_pm_get(dev_priv);
+
 	if (intel_fbc_enabled(dev)) {
 		seq_puts(m, "FBC enabled\n");
 	} else {
@@ -1391,6 +1393,9 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
 		}
 		seq_putc(m, '\n');
 	}
+
+	intel_runtime_pm_put(dev_priv);
+
 	return 0;
 }
 
@@ -1405,11 +1410,15 @@ static int i915_ips_status(struct seq_file *m, void *unused)
 		return 0;
 	}
 
+	intel_runtime_pm_get(dev_priv);
+
 	if (IS_BROADWELL(dev) || I915_READ(IPS_CTL) & IPS_ENABLE)
 		seq_puts(m, "enabled\n");
 	else
 		seq_puts(m, "disabled\n");
 
+	intel_runtime_pm_put(dev_priv);
+
 	return 0;
 }
 
@@ -1420,6 +1429,8 @@ static int i915_sr_status(struct seq_file *m, void *unused)
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	bool sr_enabled = false;
 
+	intel_runtime_pm_get(dev_priv);
+
 	if (HAS_PCH_SPLIT(dev))
 		sr_enabled = I915_READ(WM1_LP_ILK) & WM1_LP_SR_EN;
 	else if (IS_CRESTLINE(dev) || IS_I945G(dev) || IS_I945GM(dev))
@@ -1429,6 +1440,8 @@ static int i915_sr_status(struct seq_file *m, void *unused)
 	else if (IS_PINEVIEW(dev))
 		sr_enabled = I915_READ(DSPFW3) & PINEVIEW_SELF_REFRESH_EN;
 
+	intel_runtime_pm_put(dev_priv);
+
 	seq_printf(m, "self-refresh: %s\n",
 		   sr_enabled ? "enabled" : "disabled");
 
@@ -1972,12 +1985,16 @@ static int i915_energy_uJ(struct seq_file *m, void *data)
 	if (INTEL_INFO(dev)->gen < 6)
 		return -ENODEV;
 
+	intel_runtime_pm_get(dev_priv);
+
 	rdmsrl(MSR_RAPL_POWER_UNIT, power);
 	power = (power & 0x1f00) >> 8;
 	units = 1000000 / (1 << power); /* convert to uJ */
 	power = I915_READ(MCH_SECP_NRG_STTS);
 	power *= units;
 
+	intel_runtime_pm_put(dev_priv);
+
 	seq_printf(m, "%llu", (long long unsigned)power);
 
 	return 0;
-- 
1.8.5.3

[PATCH 07/11] 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 34e347f..62d0c0915 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2014,7 +2014,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 c64fb7f..796a116 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 08/11] drm/i915: call assert_device_not_suspended at gen6_force_wake_work

[Paulo Zanoni]

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

Because we shouldn't be runtime suspended when forcewake is supposed
to be enabled.

This commit will trigger the WARNs because we currently have a bug
with this. The next patches should fix the bug.

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

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 1f7226f..212de36 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -40,6 +40,12 @@
 
 #define __raw_posting_read(dev_priv__, reg__) (void)__raw_i915_read32(dev_priv__, reg__)
 
+static void
+assert_device_not_suspended(struct drm_i915_private *dev_priv)
+{
+	WARN(HAS_RUNTIME_PM(dev_priv->dev) && dev_priv->pm.suspended,
+	     "Device suspended\n");
+}
 
 static void __gen6_gt_wait_for_thread_c0(struct drm_i915_private *dev_priv)
 {
@@ -295,6 +301,8 @@ static void gen6_force_wake_work(struct work_struct *work)
 		container_of(work, typeof(*dev_priv), uncore.force_wake_work.work);
 	unsigned long irqflags;
 
+	assert_device_not_suspended(dev_priv);
+
 	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);
@@ -451,13 +459,6 @@ hsw_unclaimed_reg_check(struct drm_i915_private *dev_priv, u32 reg)
 	}
 }
 
-static void
-assert_device_not_suspended(struct drm_i915_private *dev_priv)
-{
-	WARN(HAS_RUNTIME_PM(dev_priv->dev) && dev_priv->pm.suspended,
-	     "Device suspended\n");
-}
-
 #define REG_READ_HEADER(x) \
 	unsigned long irqflags; \
 	u##x val = 0; \
-- 
1.8.5.3

[PATCH 09/11] drm/i915: assert force wake is disabled when we runtime suspend

[Paulo Zanoni]

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

Just to be sure...

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

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 2d05d7c..0c1e9e4 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -847,6 +847,7 @@ static int i915_runtime_suspend(struct device *device)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	WARN_ON(!HAS_RUNTIME_PM(dev));
+	assert_force_wake_inactive(dev_priv);
 
 	DRM_DEBUG_KMS("Suspending device\n");
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2a2a3a9..bc81c86 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2604,6 +2604,7 @@ 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 assert_force_wake_inactive(struct drm_i915_private *dev_priv);
 
 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 212de36..c3a4d6f 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -427,6 +427,14 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
 		intel_runtime_pm_put(dev_priv);
 }
 
+void assert_force_wake_inactive(struct drm_i915_private *dev_priv)
+{
+	if (!dev_priv->uncore.funcs.force_wake_get)
+		return;
+
+	WARN_ON(dev_priv->uncore.forcewake_count > 0);
+}
+
 /* 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 10/11] drm/i915: don't get/put runtime PM at the debugfs forcewake file

[Paulo Zanoni]

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

Because gen6_gt_force_wake_{get,put} should already be responsible for
getting/putting runtime PM.

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 62d0c0915..2ec7b05 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3597,7 +3597,6 @@ static int i915_forcewake_open(struct inode *inode, struct file *file)
 	if (INTEL_INFO(dev)->gen < 6)
 		return 0;
 
-	intel_runtime_pm_get(dev_priv);
 	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
 
 	return 0;
@@ -3612,7 +3611,6 @@ static int i915_forcewake_release(struct inode *inode, struct file *file)
 		return 0;
 
 	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
-	intel_runtime_pm_put(dev_priv);
 
 	return 0;
 }
-- 
1.8.5.3

[PATCH 11/11] drm/i915: assert we're not runtime suspended when writing registers

[Paulo Zanoni]

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

I could swear this was already happening in the current code...

Also, put the reads and writes in a generic place, so we don't forget
it again when we add runtime PM support to new platforms.

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

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index c3a4d6f..acce5e8 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -470,6 +470,7 @@ hsw_unclaimed_reg_check(struct drm_i915_private *dev_priv, u32 reg)
 #define REG_READ_HEADER(x) \
 	unsigned long irqflags; \
 	u##x val = 0; \
+	assert_device_not_suspended(dev_priv); \
 	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags)
 
 #define REG_READ_FOOTER \
@@ -568,6 +569,7 @@ __gen4_read(64)
 #define REG_WRITE_HEADER \
 	unsigned long irqflags; \
 	trace_i915_reg_rw(true, reg, val, sizeof(val), trace); \
+	assert_device_not_suspended(dev_priv); \
 	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags)
 
 #define REG_WRITE_FOOTER \
@@ -598,7 +600,6 @@ gen6_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace
 	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
 		__fifo_ret = __gen6_gt_wait_for_fifo(dev_priv); \
 	} \
-	assert_device_not_suspended(dev_priv); \
 	__raw_i915_write##x(dev_priv, reg, val); \
 	if (unlikely(__fifo_ret)) { \
 		gen6_gt_check_fifodbg(dev_priv); \
@@ -614,7 +615,6 @@ hsw_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace)
 	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
 		__fifo_ret = __gen6_gt_wait_for_fifo(dev_priv); \
 	} \
-	assert_device_not_suspended(dev_priv); \
 	hsw_unclaimed_reg_clear(dev_priv, reg); \
 	__raw_i915_write##x(dev_priv, reg, val); \
 	if (unlikely(__fifo_ret)) { \
-- 
1.8.5.3

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

[Chris Wilson]

On Fri, Feb 21, 2014 at 01:52:18PM -0300, Paulo Zanoni wrote:
> 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: From Paulo
>   - Make it compile
>   - Drop the __i915_add_request chunk
> 
> Reported-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>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      | 8 ++++++++
>  drivers/gpu/drm/i915/intel_display.c | 9 +++++++++
>  2 files changed, 17 insertions(+)
> 
> 
> Chris did not reply to my review comments yet, so I just went and implemented
> them. We need at least an ACK form him here before merging.

Didn't see them... Why have you altered the logic?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Paulo Zanoni]

2014-02-21 13:55 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Fri, Feb 21, 2014 at 01:52:18PM -0300, Paulo Zanoni wrote:
>> 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: From Paulo
>>   - Make it compile
>>   - Drop the __i915_add_request chunk
>>
>> Reported-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>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h      | 8 ++++++++
>>  drivers/gpu/drm/i915/intel_display.c | 9 +++++++++
>>  2 files changed, 17 insertions(+)
>>
>>
>> Chris did not reply to my review comments yet, so I just went and implemented
>> them. We need at least an ACK form him here before merging.
>
> Didn't see them... Why have you altered the logic?

See the comment at the __i915_add_request chunk:

http://lists.freedesktop.org/archives/intel-gfx/2014-February/040334.html

Maybe I just broke your patch :)
If my review doesn't make sense, we can stick to your version, it
should do the job, and I can retest everything easily.

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



-- 
Paulo Zanoni

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

[Chris Wilson]

On Fri, Feb 21, 2014 at 02:04:32PM -0300, Paulo Zanoni wrote:
> 2014-02-21 13:55 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Fri, Feb 21, 2014 at 01:52:18PM -0300, Paulo Zanoni wrote:
> >> 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: From Paulo
> >>   - Make it compile
> >>   - Drop the __i915_add_request chunk
> >>
> >> Reported-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>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_drv.h      | 8 ++++++++
> >>  drivers/gpu/drm/i915/intel_display.c | 9 +++++++++
> >>  2 files changed, 17 insertions(+)
> >>
> >>
> >> Chris did not reply to my review comments yet, so I just went and implemented
> >> them. We need at least an ACK form him here before merging.
> >
> > Didn't see them... Why have you altered the logic?
> 
> See the comment at the __i915_add_request chunk:
> 
> http://lists.freedesktop.org/archives/intel-gfx/2014-February/040334.html

Oh, I didn't look for comments inline.
> 
> Maybe I just broke your patch :)
> If my review doesn't make sense, we can stick to your version, it
> should do the job, and I can retest everything easily.

If there was a pending work item, the call to intel_mark_busy() would
return false. So we can revamp the logic around there a little bit. The
reason for the change should be self-evident - the previous code lost its
way in the transition to multiple rings arguing over a global property.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Jesse Barnes]

On Fri, 21 Feb 2014 13:52:19 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

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

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

-- 
Jesse Barnes, Intel Open Source Technology Center

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

[Jesse Barnes]

On Fri, 21 Feb 2014 13:52:20 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> 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!).
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index c628414..1f7226f 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,6 +395,7 @@ 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;
> @@ -405,13 +408,15 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
>  	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);
> +	if (!delayed)
> +		intel_runtime_pm_put(dev_priv);
>  }
>  
>  /* We give fast paths for the really cool registers */

Do we need this for the VLV path too?

It's a little confusing that we do this delayed thing, incrementing the
count and then decrementing again in the work queue, but what you have
looks correct for both cases.

So with the VLV thing addressed:

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

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 04/11] drm/i915: get runtime PM at intel_set_mode

[Jesse Barnes]

On Fri, 21 Feb 2014 13:52:21 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Otherwise, when we run intel_modeset_check_state we may already be
> runtime suspended, and our state checking code will read registers
> while the device is suspended. This can only happen if your
> autosuspend_delay_ms is low (not the default 10s).
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 10ec401..c64fb7f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9746,13 +9746,18 @@ static int intel_set_mode(struct drm_crtc *crtc,
>  			  struct drm_display_mode *mode,
>  			  int x, int y, struct drm_framebuffer *fb)
>  {
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int ret;
>  
> +	intel_runtime_pm_get(dev_priv);
> +
>  	ret = __intel_set_mode(crtc, mode, x, y, fb);
>  
>  	if (ret == 0)
>  		intel_modeset_check_state(crtc->dev);
>  
> +	intel_runtime_pm_put(dev_priv);
>  	return ret;
>  }
>  

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

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 05/11] drm/i915: get runtime PM while trying to detect CRT

[Jesse Barnes]

On Fri, 21 Feb 2014 13:52:22 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Otherwise we'll read registers that return 0xffffffff, trigger some
> WARNs, think CRT is actually connected (because certain bits are 1),
> and fail the drm-resources-equal testcase!
> 
> Tested on a SNB machine with runtime PM support (which is not upstream
> yet, but is already on my public tree at freedesktop.org, and will
> hopefully eventually become upstream).
> 
> Testcase: igt/pm_pc8/drm-resources-equal
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_crt.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index 9864aa1..4c1230c 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -630,10 +630,13 @@ static enum drm_connector_status
>  intel_crt_detect(struct drm_connector *connector, bool force)
>  {
>  	struct drm_device *dev = connector->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crt *crt = intel_attached_crt(connector);
>  	enum drm_connector_status status;
>  	struct intel_load_detect_pipe tmp;
>  
> +	intel_runtime_pm_get(dev_priv);
> +
>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force=%d\n",
>  		      connector->base.id, drm_get_connector_name(connector),
>  		      force);
> @@ -645,23 +648,30 @@ intel_crt_detect(struct drm_connector *connector, bool force)
>  		 */
>  		if (intel_crt_detect_hotplug(connector)) {
>  			DRM_DEBUG_KMS("CRT detected via hotplug\n");
> -			return connector_status_connected;
> +			status = connector_status_connected;
> +			goto out;
>  		} else
>  			DRM_DEBUG_KMS("CRT not detected via hotplug\n");
>  	}
>  
> -	if (intel_crt_detect_ddc(connector))
> -		return connector_status_connected;
> +	if (intel_crt_detect_ddc(connector)) {
> +		status = connector_status_connected;
> +		goto out;
> +	}
>  
>  	/* Load detection is broken on HPD capable machines. Whoever wants a
>  	 * broken monitor (without edid) to work behind a broken kvm (that fails
>  	 * to have the right resistors for HP detection) needs to fix this up.
>  	 * For now just bail out. */
> -	if (I915_HAS_HOTPLUG(dev))
> -		return connector_status_disconnected;
> +	if (I915_HAS_HOTPLUG(dev)) {
> +		status = connector_status_disconnected;
> +		goto out;
> +	}
>  
> -	if (!force)
> -		return connector->status;
> +	if (!force) {
> +		status = connector->status;
> +		goto out;
> +	}
>  
>  	/* for pre-945g platforms use load detect */
>  	if (intel_get_load_detect_pipe(connector, NULL, &tmp)) {
> @@ -673,6 +683,8 @@ intel_crt_detect(struct drm_connector *connector, bool force)
>  	} else
>  		status = connector_status_unknown;
>  
> +out:
> +	intel_runtime_pm_put(dev_priv);
>  	return status;
>  }
>  

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

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 06/11] drm/i915: get/put runtime PM in more places at i915_debugfs.c

[Jesse Barnes]

On Fri, 21 Feb 2014 13:52:23 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> These are places where we read (not write) registers while we're
> runtime suspended.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index d90a707..34e347f 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1348,6 +1348,8 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
>  		return 0;
>  	}
>  
> +	intel_runtime_pm_get(dev_priv);
> +
>  	if (intel_fbc_enabled(dev)) {
>  		seq_puts(m, "FBC enabled\n");
>  	} else {
> @@ -1391,6 +1393,9 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
>  		}
>  		seq_putc(m, '\n');
>  	}
> +
> +	intel_runtime_pm_put(dev_priv);
> +
>  	return 0;
>  }
>  
> @@ -1405,11 +1410,15 @@ static int i915_ips_status(struct seq_file *m, void *unused)
>  		return 0;
>  	}
>  
> +	intel_runtime_pm_get(dev_priv);
> +
>  	if (IS_BROADWELL(dev) || I915_READ(IPS_CTL) & IPS_ENABLE)
>  		seq_puts(m, "enabled\n");
>  	else
>  		seq_puts(m, "disabled\n");
>  
> +	intel_runtime_pm_put(dev_priv);
> +
>  	return 0;
>  }
>  
> @@ -1420,6 +1429,8 @@ static int i915_sr_status(struct seq_file *m, void *unused)
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  	bool sr_enabled = false;
>  
> +	intel_runtime_pm_get(dev_priv);
> +
>  	if (HAS_PCH_SPLIT(dev))
>  		sr_enabled = I915_READ(WM1_LP_ILK) & WM1_LP_SR_EN;
>  	else if (IS_CRESTLINE(dev) || IS_I945G(dev) || IS_I945GM(dev))
> @@ -1429,6 +1440,8 @@ static int i915_sr_status(struct seq_file *m, void *unused)
>  	else if (IS_PINEVIEW(dev))
>  		sr_enabled = I915_READ(DSPFW3) & PINEVIEW_SELF_REFRESH_EN;
>  
> +	intel_runtime_pm_put(dev_priv);
> +
>  	seq_printf(m, "self-refresh: %s\n",
>  		   sr_enabled ? "enabled" : "disabled");
>  
> @@ -1972,12 +1985,16 @@ static int i915_energy_uJ(struct seq_file *m, void *data)
>  	if (INTEL_INFO(dev)->gen < 6)
>  		return -ENODEV;
>  
> +	intel_runtime_pm_get(dev_priv);
> +
>  	rdmsrl(MSR_RAPL_POWER_UNIT, power);
>  	power = (power & 0x1f00) >> 8;
>  	units = 1000000 / (1 << power); /* convert to uJ */
>  	power = I915_READ(MCH_SECP_NRG_STTS);
>  	power *= units;
>  
> +	intel_runtime_pm_put(dev_priv);
> +
>  	seq_printf(m, "%llu", (long long unsigned)power);
>  
>  	return 0;

Looks like there are more places we need this too.. wonder if it would
be better to put the get into some wrapper around our sysfs files...

But these bits look correct, if not sufficient, so:

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

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 06/11] drm/i915: get/put runtime PM in more places at i915_debugfs.c

[Paulo Zanoni]

2014-02-21 14:41 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
> On Fri, 21 Feb 2014 13:52:23 -0300
> Paulo Zanoni <przanoni@gmail.com> wrote:
>
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> These are places where we read (not write) registers while we're
>> runtime suspended.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index d90a707..34e347f 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -1348,6 +1348,8 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
>>               return 0;
>>       }
>>
>> +     intel_runtime_pm_get(dev_priv);
>> +
>>       if (intel_fbc_enabled(dev)) {
>>               seq_puts(m, "FBC enabled\n");
>>       } else {
>> @@ -1391,6 +1393,9 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
>>               }
>>               seq_putc(m, '\n');
>>       }
>> +
>> +     intel_runtime_pm_put(dev_priv);
>> +
>>       return 0;
>>  }
>>
>> @@ -1405,11 +1410,15 @@ static int i915_ips_status(struct seq_file *m, void *unused)
>>               return 0;
>>       }
>>
>> +     intel_runtime_pm_get(dev_priv);
>> +
>>       if (IS_BROADWELL(dev) || I915_READ(IPS_CTL) & IPS_ENABLE)
>>               seq_puts(m, "enabled\n");
>>       else
>>               seq_puts(m, "disabled\n");
>>
>> +     intel_runtime_pm_put(dev_priv);
>> +
>>       return 0;
>>  }
>>
>> @@ -1420,6 +1429,8 @@ static int i915_sr_status(struct seq_file *m, void *unused)
>>       drm_i915_private_t *dev_priv = dev->dev_private;
>>       bool sr_enabled = false;
>>
>> +     intel_runtime_pm_get(dev_priv);
>> +
>>       if (HAS_PCH_SPLIT(dev))
>>               sr_enabled = I915_READ(WM1_LP_ILK) & WM1_LP_SR_EN;
>>       else if (IS_CRESTLINE(dev) || IS_I945G(dev) || IS_I945GM(dev))
>> @@ -1429,6 +1440,8 @@ static int i915_sr_status(struct seq_file *m, void *unused)
>>       else if (IS_PINEVIEW(dev))
>>               sr_enabled = I915_READ(DSPFW3) & PINEVIEW_SELF_REFRESH_EN;
>>
>> +     intel_runtime_pm_put(dev_priv);
>> +
>>       seq_printf(m, "self-refresh: %s\n",
>>                  sr_enabled ? "enabled" : "disabled");
>>
>> @@ -1972,12 +1985,16 @@ static int i915_energy_uJ(struct seq_file *m, void *data)
>>       if (INTEL_INFO(dev)->gen < 6)
>>               return -ENODEV;
>>
>> +     intel_runtime_pm_get(dev_priv);
>> +
>>       rdmsrl(MSR_RAPL_POWER_UNIT, power);
>>       power = (power & 0x1f00) >> 8;
>>       units = 1000000 / (1 << power); /* convert to uJ */
>>       power = I915_READ(MCH_SECP_NRG_STTS);
>>       power *= units;
>>
>> +     intel_runtime_pm_put(dev_priv);
>> +
>>       seq_printf(m, "%llu", (long long unsigned)power);
>>
>>       return 0;
>
> Looks like there are more places we need this too.. wonder if it would
> be better to put the get into some wrapper around our sysfs files...

Well, at least the deubgfs-read test from pm_pc8 doesn't complain on
my HSW and SNB. But yeah, I thought about the wrapper too. I imagine
people adding new debugfs files will always forget about the runtime
PM refcounts, so maybe the wrapper is safer. But I could do this in a
separate patch.

I also thought about wrapping the connector->detect functions.

Thanks for the reviews!

>
> But these bits look correct, if not sufficient, so:
>
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>
> --
> Jesse Barnes, Intel Open Source Technology Center



-- 
Paulo Zanoni

Re: [PATCH 08/11] drm/i915: call assert_device_not_suspended at gen6_force_wake_work

[Paulo Zanoni]

2014-02-21 13:52 GMT-03:00 Paulo Zanoni <przanoni@gmail.com>:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Because we shouldn't be runtime suspended when forcewake is supposed
> to be enabled.
>
> This commit will trigger the WARNs because we currently have a bug
> with this. The next patches should fix the bug.

Actually I changed the patch order and forgot to remove the sentence
above. Patch 03/11 should fix it.

>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 1f7226f..212de36 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -40,6 +40,12 @@
>
>  #define __raw_posting_read(dev_priv__, reg__) (void)__raw_i915_read32(dev_priv__, reg__)
>
> +static void
> +assert_device_not_suspended(struct drm_i915_private *dev_priv)
> +{
> +       WARN(HAS_RUNTIME_PM(dev_priv->dev) && dev_priv->pm.suspended,
> +            "Device suspended\n");
> +}
>
>  static void __gen6_gt_wait_for_thread_c0(struct drm_i915_private *dev_priv)
>  {
> @@ -295,6 +301,8 @@ static void gen6_force_wake_work(struct work_struct *work)
>                 container_of(work, typeof(*dev_priv), uncore.force_wake_work.work);
>         unsigned long irqflags;
>
> +       assert_device_not_suspended(dev_priv);
> +
>         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);
> @@ -451,13 +459,6 @@ hsw_unclaimed_reg_check(struct drm_i915_private *dev_priv, u32 reg)
>         }
>  }
>
> -static void
> -assert_device_not_suspended(struct drm_i915_private *dev_priv)
> -{
> -       WARN(HAS_RUNTIME_PM(dev_priv->dev) && dev_priv->pm.suspended,
> -            "Device suspended\n");
> -}
> -
>  #define REG_READ_HEADER(x) \
>         unsigned long irqflags; \
>         u##x val = 0; \
> --
> 1.8.5.3
>



-- 
Paulo Zanoni

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

[Paulo Zanoni]

2014-02-21 14:27 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Fri, Feb 21, 2014 at 02:04:32PM -0300, Paulo Zanoni wrote:
>> 2014-02-21 13:55 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
>> > On Fri, Feb 21, 2014 at 01:52:18PM -0300, Paulo Zanoni wrote:
>> >> 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: From Paulo
>> >>   - Make it compile
>> >>   - Drop the __i915_add_request chunk
>> >>
>> >> Reported-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>
>> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >> ---
>> >>  drivers/gpu/drm/i915/i915_drv.h      | 8 ++++++++
>> >>  drivers/gpu/drm/i915/intel_display.c | 9 +++++++++
>> >>  2 files changed, 17 insertions(+)
>> >>
>> >>
>> >> Chris did not reply to my review comments yet, so I just went and implemented
>> >> them. We need at least an ACK form him here before merging.
>> >
>> > Didn't see them... Why have you altered the logic?
>>
>> See the comment at the __i915_add_request chunk:
>>
>> http://lists.freedesktop.org/archives/intel-gfx/2014-February/040334.html
>
> Oh, I didn't look for comments inline.
>>
>> Maybe I just broke your patch :)
>> If my review doesn't make sense, we can stick to your version, it
>> should do the job, and I can retest everything easily.
>
> If there was a pending work item, the call to intel_mark_busy() would
> return false. So we can revamp the logic around there a little bit. The
> reason for the change should be self-evident - the previous code lost its
> way in the transition to multiple rings arguing over a global property

Just to avoid any possible confusions when/if we merge this series:
Chris sent a new version of this patch on the original mail thread.

Thanks,
Paulo
.
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni

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

[Paulo Zanoni]

2014-02-21 14:34 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
> On Fri, 21 Feb 2014 13:52:20 -0300
> Paulo Zanoni <przanoni@gmail.com> wrote:
>
>> 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!).
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_uncore.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> index c628414..1f7226f 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,6 +395,7 @@ 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;
>> @@ -405,13 +408,15 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
>>       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);
>> +     if (!delayed)
>> +             intel_runtime_pm_put(dev_priv);
>>  }
>>
>>  /* We give fast paths for the really cool registers */
>
> Do we need this for the VLV path too?

Yeah, my patch is wrong for VLV due to that "return". I'll send a new version.

By the way, why doesn't VLV use the delayed work queue? I would assume
the work queue is there to improve performance somehow, so it could be
a good idea to use it... And maybe try to avoid special-casing VLV
would be good too :)

>
> It's a little confusing that we do this delayed thing, incrementing the
> count and then decrementing again in the work queue, but what you have
> looks correct for both cases.
>
> So with the VLV thing addressed:
>
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>
> --
> Jesse Barnes, Intel Open Source Technology Center



-- 
Paulo Zanoni

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

[Jesse Barnes]

On Fri, 21 Feb 2014 17:08:50 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> 2014-02-21 14:34 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
> > On Fri, 21 Feb 2014 13:52:20 -0300
> > Paulo Zanoni <przanoni@gmail.com> wrote:
> >
> >> 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!).
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_uncore.c | 7 ++++++-
> >>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> >> index c628414..1f7226f 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,6 +395,7 @@ 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;
> >> @@ -405,13 +408,15 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
> >>       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);
> >> +     if (!delayed)
> >> +             intel_runtime_pm_put(dev_priv);
> >>  }
> >>
> >>  /* We give fast paths for the really cool registers */
> >
> > Do we need this for the VLV path too?
> 
> Yeah, my patch is wrong for VLV due to that "return". I'll send a new version.
> 
> By the way, why doesn't VLV use the delayed work queue? I would assume
> the work queue is there to improve performance somehow, so it could be
> a good idea to use it... And maybe try to avoid special-casing VLV
> would be good too :)

I don't know why VLV has an early return there rather than just using a
function pointer like everything else.  ISTR reviewing that patch from
Deepak and suggesting something else, but I guess Daniel merged it
anyway.

But yes, it should be fixed up as well and should probably use the
delayed mechanism.

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 03/11] 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)

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

Re: [PATCH 04/11] drm/i915: get runtime PM at intel_set_mode

[Imre Deak]

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

On Fri, 2014-02-21 at 13:52 -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Otherwise, when we run intel_modeset_check_state we may already be
> runtime suspended, and our state checking code will read registers
> while the device is suspended. This can only happen if your
> autosuspend_delay_ms is low (not the default 10s).
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 10ec401..c64fb7f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9746,13 +9746,18 @@ static int intel_set_mode(struct drm_crtc *crtc,
>  			  struct drm_display_mode *mode,
>  			  int x, int y, struct drm_framebuffer *fb)
>  {
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int ret;
>  
> +	intel_runtime_pm_get(dev_priv);
> +
>  	ret = __intel_set_mode(crtc, mode, x, y, fb);
>  
>  	if (ret == 0)
>  		intel_modeset_check_state(crtc->dev);
>  
> +	intel_runtime_pm_put(dev_priv);
>  	return ret;
>  }

Ideally these should be done as part of a power domain get/put as some
platforms will need to turn on some power wells too and on that path we
do anyway a runtime PM get/put.

In the latest VLV power domain support patchset [1] I added the power
domain get/put and state check to places I thought necessary. I haven't
tested it on HSW but afaics the ones added for the HW state readout code
would solve the issue you describe here.

--Imre

[1] http://www.spinics.net/lists/intel-gfx/msg40344.html

[-- 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 05/11] drm/i915: get runtime PM while trying to detect CRT

[Imre Deak]

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

On Fri, 2014-02-21 at 13:52 -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Otherwise we'll read registers that return 0xffffffff, trigger some
> WARNs, think CRT is actually connected (because certain bits are 1),
> and fail the drm-resources-equal testcase!
> 
> Tested on a SNB machine with runtime PM support (which is not upstream
> yet, but is already on my public tree at freedesktop.org, and will
> hopefully eventually become upstream).
> 
> Testcase: igt/pm_pc8/drm-resources-equal
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_crt.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index 9864aa1..4c1230c 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -630,10 +630,13 @@ static enum drm_connector_status
>  intel_crt_detect(struct drm_connector *connector, bool force)
>  {
>  	struct drm_device *dev = connector->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crt *crt = intel_attached_crt(connector);
>  	enum drm_connector_status status;
>  	struct intel_load_detect_pipe tmp;
>  
> +	intel_runtime_pm_get(dev_priv);
> +
>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force=%d\n",
>  		      connector->base.id, drm_get_connector_name(connector),
>  		      force);
> @@ -645,23 +648,30 @@ intel_crt_detect(struct drm_connector *connector, bool force)
>  		 */
>  		if (intel_crt_detect_hotplug(connector)) {
>  			DRM_DEBUG_KMS("CRT detected via hotplug\n");
> -			return connector_status_connected;
> +			status = connector_status_connected;
> +			goto out;
>  		} else
>  			DRM_DEBUG_KMS("CRT not detected via hotplug\n");
>  	}
>  
> -	if (intel_crt_detect_ddc(connector))
> -		return connector_status_connected;
> +	if (intel_crt_detect_ddc(connector)) {
> +		status = connector_status_connected;
> +		goto out;
> +	}
>  
>  	/* Load detection is broken on HPD capable machines. Whoever wants a
>  	 * broken monitor (without edid) to work behind a broken kvm (that fails
>  	 * to have the right resistors for HP detection) needs to fix this up.
>  	 * For now just bail out. */
> -	if (I915_HAS_HOTPLUG(dev))
> -		return connector_status_disconnected;
> +	if (I915_HAS_HOTPLUG(dev)) {
> +		status = connector_status_disconnected;
> +		goto out;
> +	}
>  
> -	if (!force)
> -		return connector->status;
> +	if (!force) {
> +		status = connector->status;
> +		goto out;
> +	}
>  
>  	/* for pre-945g platforms use load detect */
>  	if (intel_get_load_detect_pipe(connector, NULL, &tmp)) {
> @@ -673,6 +683,8 @@ intel_crt_detect(struct drm_connector *connector, bool force)
>  	} else
>  		status = connector_status_unknown;
>  
> +out:
> +	intel_runtime_pm_put(dev_priv);
>  	return status;
>  }

Similarly to 04/11, this should be part of a power domain get/put for a
new CRT power domain I added in the VLV power domain support patches. On
top of those to solve this issue we'd also need to add an enable/disable
handler for the HSW always-on power well, which would only do a
hsw_disable_package_c8()/hsw_enable_package_c8().

--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 04/11] drm/i915: get runtime PM at intel_set_mode

[Paulo Zanoni]

2014-02-24 8:23 GMT-03:00 Imre Deak <imre.deak@intel.com>:
> On Fri, 2014-02-21 at 13:52 -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Otherwise, when we run intel_modeset_check_state we may already be
>> runtime suspended, and our state checking code will read registers
>> while the device is suspended. This can only happen if your
>> autosuspend_delay_ms is low (not the default 10s).
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 10ec401..c64fb7f 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -9746,13 +9746,18 @@ static int intel_set_mode(struct drm_crtc *crtc,
>>                         struct drm_display_mode *mode,
>>                         int x, int y, struct drm_framebuffer *fb)
>>  {
>> +     struct drm_device *dev = crtc->dev;
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>>       int ret;
>>
>> +     intel_runtime_pm_get(dev_priv);
>> +
>>       ret = __intel_set_mode(crtc, mode, x, y, fb);
>>
>>       if (ret == 0)
>>               intel_modeset_check_state(crtc->dev);
>>
>> +     intel_runtime_pm_put(dev_priv);
>>       return ret;
>>  }
>
> Ideally these should be done as part of a power domain get/put as some
> platforms will need to turn on some power wells too and on that path we
> do anyway a runtime PM get/put.
>
> In the latest VLV power domain support patchset [1] I added the power
> domain get/put and state check to places I thought necessary. I haven't
> tested it on HSW but afaics the ones added for the HW state readout code
> would solve the issue you describe here.

Yes. I just quickly read the patches, and they seem to try to solve
this problem. Due to the reasons you wrote on the first paragraph, I
think in the long term we want the power domains solution. But as I
mentioned in the cover letter, this series contains bug fixes and
maybe we want them on -fixes and even stable Kernels, so maybe we want
to merge this patch, then later merge the code that uses power
domains, then remove the runitme_pm_get calls and leave just the power
domain calls? I'm not sure.

>
> --Imre
>
> [1] http://www.spinics.net/lists/intel-gfx/msg40344.html



-- 
Paulo Zanoni

Re: [PATCH 05/11] drm/i915: get runtime PM while trying to detect CRT

[Paulo Zanoni]

2014-02-24 8:33 GMT-03:00 Imre Deak <imre.deak@intel.com>:
> On Fri, 2014-02-21 at 13:52 -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Otherwise we'll read registers that return 0xffffffff, trigger some
>> WARNs, think CRT is actually connected (because certain bits are 1),
>> and fail the drm-resources-equal testcase!
>>
>> Tested on a SNB machine with runtime PM support (which is not upstream
>> yet, but is already on my public tree at freedesktop.org, and will
>> hopefully eventually become upstream).
>>
>> Testcase: igt/pm_pc8/drm-resources-equal
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_crt.c | 26 +++++++++++++++++++-------
>>  1 file changed, 19 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
>> index 9864aa1..4c1230c 100644
>> --- a/drivers/gpu/drm/i915/intel_crt.c
>> +++ b/drivers/gpu/drm/i915/intel_crt.c
>> @@ -630,10 +630,13 @@ static enum drm_connector_status
>>  intel_crt_detect(struct drm_connector *connector, bool force)
>>  {
>>       struct drm_device *dev = connector->dev;
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>>       struct intel_crt *crt = intel_attached_crt(connector);
>>       enum drm_connector_status status;
>>       struct intel_load_detect_pipe tmp;
>>
>> +     intel_runtime_pm_get(dev_priv);
>> +
>>       DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force=%d\n",
>>                     connector->base.id, drm_get_connector_name(connector),
>>                     force);
>> @@ -645,23 +648,30 @@ intel_crt_detect(struct drm_connector *connector, bool force)
>>                */
>>               if (intel_crt_detect_hotplug(connector)) {
>>                       DRM_DEBUG_KMS("CRT detected via hotplug\n");
>> -                     return connector_status_connected;
>> +                     status = connector_status_connected;
>> +                     goto out;
>>               } else
>>                       DRM_DEBUG_KMS("CRT not detected via hotplug\n");
>>       }
>>
>> -     if (intel_crt_detect_ddc(connector))
>> -             return connector_status_connected;
>> +     if (intel_crt_detect_ddc(connector)) {
>> +             status = connector_status_connected;
>> +             goto out;
>> +     }
>>
>>       /* Load detection is broken on HPD capable machines. Whoever wants a
>>        * broken monitor (without edid) to work behind a broken kvm (that fails
>>        * to have the right resistors for HP detection) needs to fix this up.
>>        * For now just bail out. */
>> -     if (I915_HAS_HOTPLUG(dev))
>> -             return connector_status_disconnected;
>> +     if (I915_HAS_HOTPLUG(dev)) {
>> +             status = connector_status_disconnected;
>> +             goto out;
>> +     }
>>
>> -     if (!force)
>> -             return connector->status;
>> +     if (!force) {
>> +             status = connector->status;
>> +             goto out;
>> +     }
>>
>>       /* for pre-945g platforms use load detect */
>>       if (intel_get_load_detect_pipe(connector, NULL, &tmp)) {
>> @@ -673,6 +683,8 @@ intel_crt_detect(struct drm_connector *connector, bool force)
>>       } else
>>               status = connector_status_unknown;
>>
>> +out:
>> +     intel_runtime_pm_put(dev_priv);
>>       return status;
>>  }
>
> Similarly to 04/11, this should be part of a power domain get/put for a
> new CRT power domain I added in the VLV power domain support patches. On
> top of those to solve this issue we'd also need to add an enable/disable
> handler for the HSW always-on power well, which would only do a
> hsw_disable_package_c8()/hsw_enable_package_c8().

Yes, I guess your series solves the same problem. But then, again,
this is a bug fix, so the same discussion of patch 4/11 applies here.

>
> --Imre



-- 
Paulo Zanoni

Re: [PATCH 04/11] drm/i915: get runtime PM at intel_set_mode

[Imre Deak]

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

On Mon, 2014-02-24 at 11:34 -0300, Paulo Zanoni wrote:
> 2014-02-24 8:23 GMT-03:00 Imre Deak <imre.deak@intel.com>:
> > On Fri, 2014-02-21 at 13:52 -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> Otherwise, when we run intel_modeset_check_state we may already be
> >> runtime suspended, and our state checking code will read registers
> >> while the device is suspended. This can only happen if your
> >> autosuspend_delay_ms is low (not the default 10s).
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_display.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 10ec401..c64fb7f 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -9746,13 +9746,18 @@ static int intel_set_mode(struct drm_crtc *crtc,
> >>                         struct drm_display_mode *mode,
> >>                         int x, int y, struct drm_framebuffer *fb)
> >>  {
> >> +     struct drm_device *dev = crtc->dev;
> >> +     struct drm_i915_private *dev_priv = dev->dev_private;
> >>       int ret;
> >>
> >> +     intel_runtime_pm_get(dev_priv);
> >> +
> >>       ret = __intel_set_mode(crtc, mode, x, y, fb);
> >>
> >>       if (ret == 0)
> >>               intel_modeset_check_state(crtc->dev);
> >>
> >> +     intel_runtime_pm_put(dev_priv);
> >>       return ret;
> >>  }
> >
> > Ideally these should be done as part of a power domain get/put as some
> > platforms will need to turn on some power wells too and on that path we
> > do anyway a runtime PM get/put.
> >
> > In the latest VLV power domain support patchset [1] I added the power
> > domain get/put and state check to places I thought necessary. I haven't
> > tested it on HSW but afaics the ones added for the HW state readout code
> > would solve the issue you describe here.
> 
> Yes. I just quickly read the patches, and they seem to try to solve
> this problem. Due to the reasons you wrote on the first paragraph, I
> think in the long term we want the power domains solution. But as I
> mentioned in the cover letter, this series contains bug fixes and
> maybe we want them on -fixes and even stable Kernels, so maybe we want
> to merge this patch, then later merge the code that uses power
> domains, then remove the runitme_pm_get calls and leave just the power
> domain calls? I'm not sure.

Ok, I can rebase my patchset based on the above.

Note that runtime PM was enabled post 3.13, so this fix is needed for
-fixes, but not for stable kernels.

The same goes for patch 5/11.

--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 07/11] drm/i915: kill dev_priv->pc8.gpu_idle

[Imre Deak]

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

On Fri, 2014-02-21 at 13:52 -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>
> ---
>  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 34e347f..62d0c0915 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2014,7 +2014,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 c64fb7f..796a116 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 */

This looks ok, but it's part of "Merge PC8 with runtime PM, v2" along
with patch 1/11, so they can be skipped from this patchset.

--Imre


>  	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 08/11] drm/i915: call assert_device_not_suspended at gen6_force_wake_work

[Imre Deak]

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

On Fri, 2014-02-21 at 15:05 -0300, Paulo Zanoni wrote:
> 2014-02-21 13:52 GMT-03:00 Paulo Zanoni <przanoni@gmail.com>:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > Because we shouldn't be runtime suspended when forcewake is supposed
> > to be enabled.
> >
> > This commit will trigger the WARNs because we currently have a bug
> > with this. The next patches should fix the bug.
> 
> Actually I changed the patch order and forgot to remove the sentence
> above. Patch 03/11 should fix it.
> 
> >
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Patch 3/11 is now part of "Merge PC8 with runtime PM, v2" and this
should come after that. With the order fixed:

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

> > ---
> >  drivers/gpu/drm/i915/intel_uncore.c | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index 1f7226f..212de36 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -40,6 +40,12 @@
> >
> >  #define __raw_posting_read(dev_priv__, reg__) (void)__raw_i915_read32(dev_priv__, reg__)
> >
> > +static void
> > +assert_device_not_suspended(struct drm_i915_private *dev_priv)
> > +{
> > +       WARN(HAS_RUNTIME_PM(dev_priv->dev) && dev_priv->pm.suspended,
> > +            "Device suspended\n");
> > +}
> >
> >  static void __gen6_gt_wait_for_thread_c0(struct drm_i915_private *dev_priv)
> >  {
> > @@ -295,6 +301,8 @@ static void gen6_force_wake_work(struct work_struct *work)
> >                 container_of(work, typeof(*dev_priv), uncore.force_wake_work.work);
> >         unsigned long irqflags;
> >
> > +       assert_device_not_suspended(dev_priv);
> > +
> >         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);
> > @@ -451,13 +459,6 @@ hsw_unclaimed_reg_check(struct drm_i915_private *dev_priv, u32 reg)
> >         }
> >  }
> >
> > -static void
> > -assert_device_not_suspended(struct drm_i915_private *dev_priv)
> > -{
> > -       WARN(HAS_RUNTIME_PM(dev_priv->dev) && dev_priv->pm.suspended,
> > -            "Device suspended\n");
> > -}
> > -
> >  #define REG_READ_HEADER(x) \
> >         unsigned long irqflags; \
> >         u##x val = 0; \
> > --
> > 1.8.5.3
> >
> 
> 
> 


[-- 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 09/11] drm/i915: assert force wake is disabled when we runtime suspend

[Imre Deak]

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

On Fri, 2014-02-21 at 13:52 -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Just to be sure...
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c     | 1 +
>  drivers/gpu/drm/i915/i915_drv.h     | 1 +
>  drivers/gpu/drm/i915/intel_uncore.c | 8 ++++++++
>  3 files changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 2d05d7c..0c1e9e4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -847,6 +847,7 @@ static int i915_runtime_suspend(struct device *device)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
>  	WARN_ON(!HAS_RUNTIME_PM(dev));
> +	assert_force_wake_inactive(dev_priv);
>  
>  	DRM_DEBUG_KMS("Suspending device\n");
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2a2a3a9..bc81c86 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2604,6 +2604,7 @@ 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 assert_force_wake_inactive(struct drm_i915_private *dev_priv);
>  
>  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 212de36..c3a4d6f 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -427,6 +427,14 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
>  		intel_runtime_pm_put(dev_priv);
>  }
>  
> +void assert_force_wake_inactive(struct drm_i915_private *dev_priv)
> +{
> +	if (!dev_priv->uncore.funcs.force_wake_get)
> +		return;
> +
> +	WARN_ON(dev_priv->uncore.forcewake_count > 0);
> +}
> +

As patch 8/11, this one would also trigger without 3/11, so it should be
applied after that.

You could also check the VLV forcewake refcounts, but I'm ok if you add
that later.

With the patch order fixed:
Reviewed-by: Imre Deak <imre.deak@intel.com>


>  /* We give fast paths for the really cool registers */
>  #define NEEDS_FORCE_WAKE(dev_priv, reg) \
>  	 ((reg) < 0x40000 && (reg) != FORCEWAKE)


[-- 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/11] drm/i915: don't get/put runtime PM at the debugfs forcewake file

[Imre Deak]

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

On Fri, 2014-02-21 at 13:52 -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Because gen6_gt_force_wake_{get,put} should already be responsible for
> getting/putting runtime PM.
> 
> 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 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 62d0c0915..2ec7b05 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3597,7 +3597,6 @@ static int i915_forcewake_open(struct inode *inode, struct file *file)
>  	if (INTEL_INFO(dev)->gen < 6)
>  		return 0;
>  
> -	intel_runtime_pm_get(dev_priv);
>  	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
>  
>  	return 0;
> @@ -3612,7 +3611,6 @@ static int i915_forcewake_release(struct inode *inode, struct file *file)
>  		return 0;
>  
>  	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
> -	intel_runtime_pm_put(dev_priv);
>  
>  	return 0;
>  }


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

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

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

Re: [PATCH 11/11] drm/i915: assert we're not runtime suspended when writing registers

[Imre Deak]

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

On Fri, 2014-02-21 at 13:52 -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> I could swear this was already happening in the current code...
> 
> Also, put the reads and writes in a generic place, so we don't forget
> it again when we add runtime PM support to new platforms.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

In subject: s/writing/reading/ . Otherwise:
Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index c3a4d6f..acce5e8 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -470,6 +470,7 @@ hsw_unclaimed_reg_check(struct drm_i915_private *dev_priv, u32 reg)
>  #define REG_READ_HEADER(x) \
>  	unsigned long irqflags; \
>  	u##x val = 0; \
> +	assert_device_not_suspended(dev_priv); \
>  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags)
>  
>  #define REG_READ_FOOTER \
> @@ -568,6 +569,7 @@ __gen4_read(64)
>  #define REG_WRITE_HEADER \
>  	unsigned long irqflags; \
>  	trace_i915_reg_rw(true, reg, val, sizeof(val), trace); \
> +	assert_device_not_suspended(dev_priv); \
>  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags)
>  
>  #define REG_WRITE_FOOTER \
> @@ -598,7 +600,6 @@ gen6_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace
>  	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
>  		__fifo_ret = __gen6_gt_wait_for_fifo(dev_priv); \
>  	} \
> -	assert_device_not_suspended(dev_priv); \
>  	__raw_i915_write##x(dev_priv, reg, val); \
>  	if (unlikely(__fifo_ret)) { \
>  		gen6_gt_check_fifodbg(dev_priv); \
> @@ -614,7 +615,6 @@ hsw_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace)
>  	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
>  		__fifo_ret = __gen6_gt_wait_for_fifo(dev_priv); \
>  	} \
> -	assert_device_not_suspended(dev_priv); \
>  	hsw_unclaimed_reg_clear(dev_priv, reg); \
>  	__raw_i915_write##x(dev_priv, reg, val); \
>  	if (unlikely(__fifo_ret)) { \


[-- 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 07/11] drm/i915: kill dev_priv->pc8.gpu_idle

[Paulo Zanoni]

2014-02-28 10:50 GMT-03:00 Imre Deak <imre.deak@intel.com>:
> On Fri, 2014-02-21 at 13:52 -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>
>> ---
>>  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 34e347f..62d0c0915 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -2014,7 +2014,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 c64fb7f..796a116 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 */
>
> This looks ok, but it's part of "Merge PC8 with runtime PM, v2" along
> with patch 1/11, so they can be skipped from this patchset.

Yes. Since you spotted some potential conflicts between this series
and yours, I decided to send "Merge PC8 with runtime PM v2" in a way
that it's independent form this series: it just contains the fixes
that are necessary, avoiding some of the conflicts you detected. So we
can just merge that series instead of this, if wanted.

>
> --Imre
>
>
>>       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);
>



-- 
Paulo Zanoni

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

[Daniel Vetter]

On Fri, Feb 21, 2014 at 04:34:29PM -0300, Paulo Zanoni wrote:
> 2014-02-21 14:27 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Fri, Feb 21, 2014 at 02:04:32PM -0300, Paulo Zanoni wrote:
> >> 2014-02-21 13:55 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> >> > On Fri, Feb 21, 2014 at 01:52:18PM -0300, Paulo Zanoni wrote:
> >> >> 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: From Paulo
> >> >>   - Make it compile
> >> >>   - Drop the __i915_add_request chunk
> >> >>
> >> >> Reported-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>
> >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> >> ---
> >> >>  drivers/gpu/drm/i915/i915_drv.h      | 8 ++++++++
> >> >>  drivers/gpu/drm/i915/intel_display.c | 9 +++++++++
> >> >>  2 files changed, 17 insertions(+)
> >> >>
> >> >>
> >> >> Chris did not reply to my review comments yet, so I just went and implemented
> >> >> them. We need at least an ACK form him here before merging.
> >> >
> >> > Didn't see them... Why have you altered the logic?
> >>
> >> See the comment at the __i915_add_request chunk:
> >>
> >> http://lists.freedesktop.org/archives/intel-gfx/2014-February/040334.html
> >
> > Oh, I didn't look for comments inline.
> >>
> >> Maybe I just broke your patch :)
> >> If my review doesn't make sense, we can stick to your version, it
> >> should do the job, and I can retest everything easily.
> >
> > If there was a pending work item, the call to intel_mark_busy() would
> > return false. So we can revamp the logic around there a little bit. The
> > reason for the change should be self-evident - the previous code lost its
> > way in the transition to multiple rings arguing over a global property
> 
> Just to avoid any possible confusions when/if we merge this series:
> Chris sent a new version of this patch on the original mail thread.

Just to double check: Have I merged the right version?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Daniel Vetter]

On Fri, Feb 21, 2014 at 09:28:25AM -0800, Jesse Barnes wrote:
> On Fri, 21 Feb 2014 13:52:19 -0300
> Paulo Zanoni <przanoni@gmail.com> wrote:
> 
> > 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
> > 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,
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Smells like another good reason for Jesse's idea with a register range
based power domain checker. Especially with suspend delays we'll have a
really hard time catching these kinds of bugs otherwise ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Chris Wilson]

On Wed, Mar 05, 2014 at 02:13:15PM +0100, Daniel Vetter wrote:
> Just to double check: Have I merged the right version?

No conflicts or residual with my tree, so yes.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Daniel Vetter]

On Fri, Feb 21, 2014 at 12:16:58PM -0800, Jesse Barnes wrote:
> On Fri, 21 Feb 2014 17:08:50 -0300
> Paulo Zanoni <przanoni@gmail.com> wrote:
> 
> > 2014-02-21 14:34 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
> > > On Fri, 21 Feb 2014 13:52:20 -0300
> > > Paulo Zanoni <przanoni@gmail.com> wrote:
> > >
> > >> 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!).
> > >>
> > >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > >> ---
> > >>  drivers/gpu/drm/i915/intel_uncore.c | 7 ++++++-
> > >>  1 file changed, 6 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > >> index c628414..1f7226f 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,6 +395,7 @@ 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;
> > >> @@ -405,13 +408,15 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
> > >>       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);
> > >> +     if (!delayed)
> > >> +             intel_runtime_pm_put(dev_priv);
> > >>  }
> > >>
> > >>  /* We give fast paths for the really cool registers */
> > >
> > > Do we need this for the VLV path too?
> > 
> > Yeah, my patch is wrong for VLV due to that "return". I'll send a new version.
> > 
> > By the way, why doesn't VLV use the delayed work queue? I would assume
> > the work queue is there to improve performance somehow, so it could be
> > a good idea to use it... And maybe try to avoid special-casing VLV
> > would be good too :)
> 
> I don't know why VLV has an early return there rather than just using a
> function pointer like everything else.  ISTR reviewing that patch from
> Deepak and suggesting something else, but I guess Daniel merged it
> anyway.
> 
> But yes, it should be fixed up as well and should probably use the
> delayed mechanism.

The delayed work (well timer since Chris' latest patch) can only handle
one forcewake domain. vlv has two ...

That reminds me: This delayed suspend stuff is kinda implemented in the
linux pm domain code already. Just needed to drop this since I've
predicted that in the end we'll invent all these wheels, too ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 04/11] drm/i915: get runtime PM at intel_set_mode

[Daniel Vetter]

On Fri, Feb 21, 2014 at 01:52:21PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Otherwise, when we run intel_modeset_check_state we may already be
> runtime suspended, and our state checking code will read registers
> while the device is suspended. This can only happen if your
> autosuspend_delay_ms is low (not the default 10s).
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 10ec401..c64fb7f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9746,13 +9746,18 @@ static int intel_set_mode(struct drm_crtc *crtc,
>  			  struct drm_display_mode *mode,
>  			  int x, int y, struct drm_framebuffer *fb)
>  {
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int ret;
>  
> +	intel_runtime_pm_get(dev_priv);
> +
>  	ret = __intel_set_mode(crtc, mode, x, y, fb);
>  
>  	if (ret == 0)
>  		intel_modeset_check_state(crtc->dev);
>  
> +	intel_runtime_pm_put(dev_priv);

I've thought our code has the relevant power domain checks sprinkled all
over the get_config/state functions already? If that's not the case I
prefer we fix that, similar to my suggestion in reply to Imre's patches of
moving the power domain checking into the callbacks.

Wrt -fixes: Since the default autosuspend delay will make it impossible to
hit this I think we can punt. Rules for late -rc and cc: stable is that it
needs to be a real-world problem with a real bug report.

And one more: To make pm_pc8 more useful, could we just set the
autosuspend delay to 0 while the test is running? Then restore it again
with an igt atexit handler. That should help with our coverage and hitting
this tiny little issues you've fixed in this series.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 06/11] drm/i915: get/put runtime PM in more places at i915_debugfs.c

[Daniel Vetter]

On Fri, Feb 21, 2014 at 02:46:55PM -0300, Paulo Zanoni wrote:
> 2014-02-21 14:41 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
> > On Fri, 21 Feb 2014 13:52:23 -0300
> > Paulo Zanoni <przanoni@gmail.com> wrote:
> >
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> These are places where we read (not write) registers while we're
> >> runtime suspended.
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_debugfs.c | 17 +++++++++++++++++
> >>  1 file changed, 17 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> >> index d90a707..34e347f 100644
> >> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> >> @@ -1348,6 +1348,8 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
> >>               return 0;
> >>       }
> >>
> >> +     intel_runtime_pm_get(dev_priv);
> >> +
> >>       if (intel_fbc_enabled(dev)) {
> >>               seq_puts(m, "FBC enabled\n");
> >>       } else {
> >> @@ -1391,6 +1393,9 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
> >>               }
> >>               seq_putc(m, '\n');
> >>       }
> >> +
> >> +     intel_runtime_pm_put(dev_priv);
> >> +
> >>       return 0;
> >>  }
> >>
> >> @@ -1405,11 +1410,15 @@ static int i915_ips_status(struct seq_file *m, void *unused)
> >>               return 0;
> >>       }
> >>
> >> +     intel_runtime_pm_get(dev_priv);
> >> +
> >>       if (IS_BROADWELL(dev) || I915_READ(IPS_CTL) & IPS_ENABLE)
> >>               seq_puts(m, "enabled\n");
> >>       else
> >>               seq_puts(m, "disabled\n");
> >>
> >> +     intel_runtime_pm_put(dev_priv);
> >> +
> >>       return 0;
> >>  }
> >>
> >> @@ -1420,6 +1429,8 @@ static int i915_sr_status(struct seq_file *m, void *unused)
> >>       drm_i915_private_t *dev_priv = dev->dev_private;
> >>       bool sr_enabled = false;
> >>
> >> +     intel_runtime_pm_get(dev_priv);
> >> +
> >>       if (HAS_PCH_SPLIT(dev))
> >>               sr_enabled = I915_READ(WM1_LP_ILK) & WM1_LP_SR_EN;
> >>       else if (IS_CRESTLINE(dev) || IS_I945G(dev) || IS_I945GM(dev))
> >> @@ -1429,6 +1440,8 @@ static int i915_sr_status(struct seq_file *m, void *unused)
> >>       else if (IS_PINEVIEW(dev))
> >>               sr_enabled = I915_READ(DSPFW3) & PINEVIEW_SELF_REFRESH_EN;
> >>
> >> +     intel_runtime_pm_put(dev_priv);
> >> +
> >>       seq_printf(m, "self-refresh: %s\n",
> >>                  sr_enabled ? "enabled" : "disabled");
> >>
> >> @@ -1972,12 +1985,16 @@ static int i915_energy_uJ(struct seq_file *m, void *data)
> >>       if (INTEL_INFO(dev)->gen < 6)
> >>               return -ENODEV;
> >>
> >> +     intel_runtime_pm_get(dev_priv);
> >> +
> >>       rdmsrl(MSR_RAPL_POWER_UNIT, power);
> >>       power = (power & 0x1f00) >> 8;
> >>       units = 1000000 / (1 << power); /* convert to uJ */
> >>       power = I915_READ(MCH_SECP_NRG_STTS);
> >>       power *= units;
> >>
> >> +     intel_runtime_pm_put(dev_priv);
> >> +
> >>       seq_printf(m, "%llu", (long long unsigned)power);
> >>
> >>       return 0;
> >
> > Looks like there are more places we need this too.. wonder if it would
> > be better to put the get into some wrapper around our sysfs files...
> 
> Well, at least the deubgfs-read test from pm_pc8 doesn't complain on
> my HSW and SNB. But yeah, I thought about the wrapper too. I imagine
> people adding new debugfs files will always forget about the runtime
> PM refcounts, so maybe the wrapper is safer. But I could do this in a
> separate patch.
> 
> I also thought about wrapping the connector->detect functions.

I prefer explicit get/put calls instead of magic wrappers. runtime pm
get/put calls can be rather heavy-weight functions when we need to
actually bring up a domain again and restore piles of registers. So being
explicit is imo a feature.

Even more so with our explosion of power domains - I expect that some of
the debugfs files need to grab multiple different domains all over the
place.
-Daniel

> 
> Thanks for the reviews!
> 
> >
> > But these bits look correct, if not sufficient, so:
> >
> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> >
> > --
> > Jesse Barnes, Intel Open Source Technology Center
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 07/11] drm/i915: kill dev_priv->pc8.gpu_idle

[Daniel Vetter]

On Fri, Feb 28, 2014 at 05:11:46PM -0300, Paulo Zanoni wrote:
> 2014-02-28 10:50 GMT-03:00 Imre Deak <imre.deak@intel.com>:
> > On Fri, 2014-02-21 at 13:52 -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>
> >> ---
> >>  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 34e347f..62d0c0915 100644
> >> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> >> @@ -2014,7 +2014,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 c64fb7f..796a116 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 */
> >
> > This looks ok, but it's part of "Merge PC8 with runtime PM, v2" along
> > with patch 1/11, so they can be skipped from this patchset.
> 
> Yes. Since you spotted some potential conflicts between this series
> and yours, I decided to send "Merge PC8 with runtime PM v2" in a way
> that it's independent form this series: it just contains the fixes
> that are necessary, avoiding some of the conflicts you detected. So we
> can just merge that series instead of this, if wanted.

I'm voting for potential conflicts be damned - this entire runtime pm
enabling will keep on being ugly for a while, so I'm just trying to pull
in as much as possible ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 10/11] drm/i915: don't get/put runtime PM at the debugfs forcewake file

[Daniel Vetter]

On Fri, Feb 21, 2014 at 01:52:27PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Because gen6_gt_force_wake_{get,put} should already be responsible for
> getting/putting runtime PM.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 62d0c0915..2ec7b05 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3597,7 +3597,6 @@ static int i915_forcewake_open(struct inode *inode, struct file *file)
>  	if (INTEL_INFO(dev)->gen < 6)
>  		return 0;
>  
> -	intel_runtime_pm_get(dev_priv);
>  	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);

I agree that doing the runtime get/put from within the forcewake get/put
gives us nice nesting. The with that is that we do the forcewake get/put
from within register I/O functions, which means it's not simple at all to
figure out where all runtime_pm_get is called, which can lead to some big
surprises around locking (since our resume functions do a _lot_ of work).

This is the reason why I've asked for the exception for the forcewake ->
runtime pm nesting. We could fix this properly by moving the forcewake
get/put out of the register read/write functions and sprinkle it
explicitly over the code, but that's a _lot_ more work.

For now I'll punt on this patch until convinced otherwise.
-Daniel

>  
>  	return 0;
> @@ -3612,7 +3611,6 @@ static int i915_forcewake_release(struct inode *inode, struct file *file)
>  		return 0;
>  
>  	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
> -	intel_runtime_pm_put(dev_priv);
>  
>  	return 0;
>  }
> -- 
> 1.8.5.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 11/11] drm/i915: assert we're not runtime suspended when writing registers

[Daniel Vetter]

On Fri, Feb 28, 2014 at 05:16:12PM +0200, Imre Deak wrote:
> On Fri, 2014-02-21 at 13:52 -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > I could swear this was already happening in the current code...
> > 
> > Also, put the reads and writes in a generic place, so we don't forget
> > it again when we add runtime PM support to new platforms.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> In subject: s/writing/reading/ . Otherwise:
> Reviewed-by: Imre Deak <imre.deak@intel.com>
> 
> > ---
> >  drivers/gpu/drm/i915/intel_uncore.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index c3a4d6f..acce5e8 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -470,6 +470,7 @@ hsw_unclaimed_reg_check(struct drm_i915_private *dev_priv, u32 reg)
> >  #define REG_READ_HEADER(x) \
> >  	unsigned long irqflags; \
> >  	u##x val = 0; \
> > +	assert_device_not_suspended(dev_priv); \
> >  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags)

Hm, shouldn't we do this in the generic part of the register r/w
functions? Anyway, I've merged all patches in this series to dinq with the
exception of the two I've complained about.

Thanks, Daniel
> >  
> >  #define REG_READ_FOOTER \
> > @@ -568,6 +569,7 @@ __gen4_read(64)
> >  #define REG_WRITE_HEADER \
> >  	unsigned long irqflags; \
> >  	trace_i915_reg_rw(true, reg, val, sizeof(val), trace); \
> > +	assert_device_not_suspended(dev_priv); \
> >  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags)
> >  
> >  #define REG_WRITE_FOOTER \
> > @@ -598,7 +600,6 @@ gen6_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace
> >  	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
> >  		__fifo_ret = __gen6_gt_wait_for_fifo(dev_priv); \
> >  	} \
> > -	assert_device_not_suspended(dev_priv); \
> >  	__raw_i915_write##x(dev_priv, reg, val); \
> >  	if (unlikely(__fifo_ret)) { \
> >  		gen6_gt_check_fifodbg(dev_priv); \
> > @@ -614,7 +615,6 @@ hsw_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace)
> >  	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
> >  		__fifo_ret = __gen6_gt_wait_for_fifo(dev_priv); \
> >  	} \
> > -	assert_device_not_suspended(dev_priv); \
> >  	hsw_unclaimed_reg_clear(dev_priv, reg); \
> >  	__raw_i915_write##x(dev_priv, reg, val); \
> >  	if (unlikely(__fifo_ret)) { \
> 



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


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

Re: [PATCH 11/11] drm/i915: assert we're not runtime suspended when writing registers

[Daniel Vetter]

On Wed, Mar 05, 2014 at 02:44:18PM +0100, Daniel Vetter wrote:
> On Fri, Feb 28, 2014 at 05:16:12PM +0200, Imre Deak wrote:
> > On Fri, 2014-02-21 at 13:52 -0300, Paulo Zanoni wrote:
> > > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > 
> > > I could swear this was already happening in the current code...
> > > 
> > > Also, put the reads and writes in a generic place, so we don't forget
> > > it again when we add runtime PM support to new platforms.
> > > 
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > In subject: s/writing/reading/ . Otherwise:
> > Reviewed-by: Imre Deak <imre.deak@intel.com>
> > 
> > > ---
> > >  drivers/gpu/drm/i915/intel_uncore.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > > index c3a4d6f..acce5e8 100644
> > > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > > @@ -470,6 +470,7 @@ hsw_unclaimed_reg_check(struct drm_i915_private *dev_priv, u32 reg)
> > >  #define REG_READ_HEADER(x) \
> > >  	unsigned long irqflags; \
> > >  	u##x val = 0; \
> > > +	assert_device_not_suspended(dev_priv); \
> > >  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags)
> 
> Hm, shouldn't we do this in the generic part of the register r/w
> functions? Anyway, I've merged all patches in this series to dinq with the
> exception of the two I've complained about.

Argh, failure at reading diffs. Your patch Does The Right Thing ;-)

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

Re: [PATCH 04/11] drm/i915: get runtime PM at intel_set_mode

[Paulo Zanoni]

2014-03-05 10:25 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> On Fri, Feb 21, 2014 at 01:52:21PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Otherwise, when we run intel_modeset_check_state we may already be
>> runtime suspended, and our state checking code will read registers
>> while the device is suspended. This can only happen if your
>> autosuspend_delay_ms is low (not the default 10s).
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 10ec401..c64fb7f 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -9746,13 +9746,18 @@ static int intel_set_mode(struct drm_crtc *crtc,
>>                         struct drm_display_mode *mode,
>>                         int x, int y, struct drm_framebuffer *fb)
>>  {
>> +     struct drm_device *dev = crtc->dev;
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>>       int ret;
>>
>> +     intel_runtime_pm_get(dev_priv);
>> +
>>       ret = __intel_set_mode(crtc, mode, x, y, fb);
>>
>>       if (ret == 0)
>>               intel_modeset_check_state(crtc->dev);
>>
>> +     intel_runtime_pm_put(dev_priv);
>
> I've thought our code has the relevant power domain checks sprinkled all
> over the get_config/state functions already? If that's not the case I
> prefer we fix that, similar to my suggestion in reply to Imre's patches of
> moving the power domain checking into the callbacks.
>
> Wrt -fixes: Since the default autosuspend delay will make it impossible to
> hit this I think we can punt. Rules for late -rc and cc: stable is that it
> needs to be a real-world problem with a real bug report.
>
> And one more: To make pm_pc8 more useful, could we just set the
> autosuspend delay to 0 while the test is running? Then restore it again
> with an igt atexit handler. That should help with our coverage and hitting
> this tiny little issues you've fixed in this series.

We already set autosuspend_delay_msg to zero when the test is running,
we just don't restore the original value later. The problem is that we
currently have autosuspend_delay_ms and i915.pc8_timeout as non-zero
default, and we don't have support to change i915.pc8_timeout at
runtime. If we merge the series that's currently under review (00/23
Merge PC8 with runtime PM, v2), we'll just kill i915.pc8_timeout, so
we should be fine, and QA will automatically start running everything
with zero timeouts.

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



-- 
Paulo Zanoni