[PATCH v4 00/10] drm/i915: improve the RPM device suspended assert

[PATCH v4 00/10] drm/i915: improve the RPM device suspended assert

[Imre Deak]

This is v4 of [1]. It has the following changes:
- fix module reload that got broken in v3 due to removal of HAS_RUNTIME_PM 
  (added patch 1-3 + revised patch 4)
- disable the wakeref asserts in the IRQ handlers and RPS work too
  (revised patch 7)

Imre Deak (10):
  drm/i915: clarify comment about mandatory RPM put/get during driver
    load/unload
  drm/i915: disable power well support on platforms without runtime PM
    support
  drm/i915: refactor RPM disabling due to RC6 being disabled
  drm/i915: remove HAS_RUNTIME_PM check from RPM get/put/assert helpers
  drm/i915: add assert_rpm_wakelock_held helper
  drm/i915: use assert_rpm_wakelock_held instead of opencoding it
  drm/i915: add support for checking if we hold an RPM reference
  drm/i915: check that we hold an RPM wakelock ref before we put it
  drm/i915: add support for checking RPM atomic sections
  drm/i915: check that we are in an RPM atomic section in GGTT PTE
    updaters

 drivers/gpu/drm/i915/i915_dma.c         |  7 ++++
 drivers/gpu/drm/i915/i915_drv.c         | 39 ++++++++++++++++--
 drivers/gpu/drm/i915/i915_drv.h         |  2 +
 drivers/gpu/drm/i915/i915_gem_gtt.c     | 33 +++++++++++++++
 drivers/gpu/drm/i915/i915_irq.c         | 73 ++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_drv.h        | 72 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_pm.c         | 17 ++++++++
 drivers/gpu/drm/i915/intel_runtime_pm.c | 57 ++++++++++++-------------
 drivers/gpu/drm/i915/intel_uncore.c     | 23 ++++-------
 9 files changed, 270 insertions(+), 53 deletions(-)

-- 
2.5.0

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

[PATCH 01/10] drm/i915: clarify comment about mandatory RPM put/get during driver load/unload

[Imre Deak]

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 2c2151f..9945040 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1975,9 +1975,15 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv)
  */
 void intel_power_domains_fini(struct drm_i915_private *dev_priv)
 {
-	/* The i915.ko module is still not prepared to be loaded when
+	/*
+	 * The i915.ko module is still not prepared to be loaded when
 	 * the power well is not enabled, so just enable it in case
-	 * we're going to unload/reload. */
+	 * we're going to unload/reload.
+	 * The following also reacquires the RPM reference the core passed
+	 * to the driver during loading, which is dropped in
+	 * intel_runtime_pm_enable(). We have to hand back the control of the
+	 * device to the core with this reference held.
+	 */
 	intel_display_set_init_power(dev_priv, true);
 
 	/* Remove the refcount we took to keep power well support disabled. */
@@ -2313,6 +2319,11 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
 	pm_runtime_mark_last_busy(device);
 	pm_runtime_use_autosuspend(device);
 
+	/*
+	 * The core calls the driver load handler with an RPM reference held.
+	 * We drop that here and will reacquire it during unloading in
+	 * intel_power_domains_fini().
+	 */
 	pm_runtime_put_autosuspend(device);
 }
 
-- 
2.5.0

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

[PATCH 02/10] drm/i915: disable power well support on platforms without runtime PM support

[Imre Deak]

All platforms with power well support have runtime PM support, so
simplify things by explicitly disabling power well support on platforms
without runtime PM support. This results in holding the init power domain
reference whenever the driver is loaded in addition to an RPM reference,
which reflects the reality better and makes it possible to simplify
things by removing the HAS_RUNTIME_PM special casing from more places in
a follow-up patch.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 9945040..f4ff5f5 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1908,6 +1908,11 @@ static int
 sanitize_disable_power_well_option(const struct drm_i915_private *dev_priv,
 				   int disable_power_well)
 {
+	if (!HAS_RUNTIME_PM(dev_priv)) {
+		DRM_DEBUG_KMS("No runtime PM support, disabling display power well support\n");
+		return 0;
+	}
+
 	if (disable_power_well >= 0)
 		return !!disable_power_well;
 
-- 
2.5.0

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

[PATCH 03/10] drm/i915: refactor RPM disabling due to RC6 being disabled

[Imre Deak]

We can make the RPM dependency on RC6 explciit in the code by taking an
actual RPM reference, instead of avoiding to drop the initial one. This
will also enable us to remove the HAS_RUNTIME_PM special casing from
more places in the next patch.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c         | 15 +++++++++++++++
 drivers/gpu/drm/i915/intel_runtime_pm.c |  9 ---------
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 99f2642..f93c4b9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -6015,7 +6015,17 @@ static void intel_init_emon(struct drm_device *dev)
 
 void intel_init_gt_powersave(struct drm_device *dev)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
 	i915.enable_rc6 = sanitize_rc6_option(dev, i915.enable_rc6);
+	/*
+	 * RPM depends on RC6 to save restore the GT HW context, so make RC6 a
+	 * requirement.
+	 */
+	if (!i915.enable_rc6) {
+		DRM_INFO("RC6 disabled, disabling runtime PM support\n");
+		intel_runtime_pm_get(dev_priv);
+	}
 
 	if (IS_CHERRYVIEW(dev))
 		cherryview_init_gt_powersave(dev);
@@ -6025,10 +6035,15 @@ void intel_init_gt_powersave(struct drm_device *dev)
 
 void intel_cleanup_gt_powersave(struct drm_device *dev)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
 	if (IS_CHERRYVIEW(dev))
 		return;
 	else if (IS_VALLEYVIEW(dev))
 		valleyview_cleanup_gt_powersave(dev);
+
+	if (!i915.enable_rc6)
+		intel_runtime_pm_put(dev_priv);
 }
 
 static void gen6_suspend_rps(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index f4ff5f5..342baa9 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -2311,15 +2311,6 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
 	if (!HAS_RUNTIME_PM(dev))
 		return;
 
-	/*
-	 * RPM depends on RC6 to save restore the GT HW context, so make RC6 a
-	 * requirement.
-	 */
-	if (!intel_enable_rc6(dev)) {
-		DRM_INFO("RC6 disabled, disabling runtime PM support\n");
-		return;
-	}
-
 	pm_runtime_set_autosuspend_delay(device, 10000); /* 10s */
 	pm_runtime_mark_last_busy(device);
 	pm_runtime_use_autosuspend(device);
-- 
2.5.0

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

[PATCH 04/10] drm/i915: remove HAS_RUNTIME_PM check from RPM get/put/assert helpers

[Imre Deak]

We don't really need to check this flag in the get/put/assert helpers,
as on platforms without RPM support we won't ever enable RPM. That means
pm.suspend will be always false and the assert will be always true.

Do this to simplify the code and to let us extend the RPM asserts to all
platforms for a better coverage.

Motivated by Ville.

v2-v3:
- unchanged
v4:
- remove the HAS_RUNTIME_PM check from intel_runtime_pm_enable() too
  made possible by the previous two patches

Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v3)
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 12 ------------
 drivers/gpu/drm/i915/intel_uncore.c     |  3 +--
 2 files changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 342baa9..bca7ca7 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -2237,9 +2237,6 @@ void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
 	struct drm_device *dev = dev_priv->dev;
 	struct device *device = &dev->pdev->dev;
 
-	if (!HAS_RUNTIME_PM(dev))
-		return;
-
 	pm_runtime_get_sync(device);
 	WARN(dev_priv->pm.suspended, "Device still suspended.\n");
 }
@@ -2266,9 +2263,6 @@ void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv)
 	struct drm_device *dev = dev_priv->dev;
 	struct device *device = &dev->pdev->dev;
 
-	if (!HAS_RUNTIME_PM(dev))
-		return;
-
 	WARN(dev_priv->pm.suspended, "Getting nosync-ref while suspended.\n");
 	pm_runtime_get_noresume(device);
 }
@@ -2286,9 +2280,6 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv)
 	struct drm_device *dev = dev_priv->dev;
 	struct device *device = &dev->pdev->dev;
 
-	if (!HAS_RUNTIME_PM(dev))
-		return;
-
 	pm_runtime_mark_last_busy(device);
 	pm_runtime_put_autosuspend(device);
 }
@@ -2308,9 +2299,6 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
 	struct drm_device *dev = dev_priv->dev;
 	struct device *device = &dev->pdev->dev;
 
-	if (!HAS_RUNTIME_PM(dev))
-		return;
-
 	pm_runtime_set_autosuspend_delay(device, 10000); /* 10s */
 	pm_runtime_mark_last_busy(device);
 	pm_runtime_use_autosuspend(device);
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index fcf04fe..0226776 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -53,8 +53,7 @@ intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id)
 static void
 assert_device_not_suspended(struct drm_i915_private *dev_priv)
 {
-	WARN_ONCE(HAS_RUNTIME_PM(dev_priv->dev) && dev_priv->pm.suspended,
-		  "Device suspended\n");
+	WARN_ONCE(dev_priv->pm.suspended, "Device suspended\n");
 }
 
 static inline void
-- 
2.5.0

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

[PATCH 05/10] drm/i915: add assert_rpm_wakelock_held helper

[Imre Deak]

As a preparation for follow-up patches add a new helper that checks
whether we hold an RPM reference, since this is what we want most of
the cases. Atm this helper will only check for the HW suspended state, a
follow-up patch will do the actual change to check the refcount instead.
One exception is the forcewake release timer function, where it's
guaranteed that the HW is on even though the RPM refcount drops to zero.
This guarantee is provided by flushing the timer in the runtime suspend
handler. So leave the assert_device_not_suspended check in place there.

Also rename assert_device_suspended for consistency and export these
helpers as a preparation for the follow-up patches.

No functional change.

v3:
- change the assert warning message to be more meaningful (Chris)

Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_drv.h    | 14 ++++++++++++++
 drivers/gpu/drm/i915/intel_uncore.c | 20 +++++++-------------
 2 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 798463e..9837a25 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1430,6 +1430,20 @@ void intel_display_power_get(struct drm_i915_private *dev_priv,
 			     enum intel_display_power_domain domain);
 void intel_display_power_put(struct drm_i915_private *dev_priv,
 			     enum intel_display_power_domain domain);
+
+static inline void
+assert_rpm_device_not_suspended(struct drm_i915_private *dev_priv)
+{
+	WARN_ONCE(dev_priv->pm.suspended,
+		  "Device suspended during HW access\n");
+}
+
+static inline void
+assert_rpm_wakelock_held(struct drm_i915_private *dev_priv)
+{
+	assert_rpm_device_not_suspended(dev_priv);
+}
+
 void intel_runtime_pm_get(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 0226776..3c63d94 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -50,12 +50,6 @@ intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id)
 	return "unknown";
 }
 
-static void
-assert_device_not_suspended(struct drm_i915_private *dev_priv)
-{
-	WARN_ONCE(dev_priv->pm.suspended, "Device suspended\n");
-}
-
 static inline void
 fw_domain_reset(const struct intel_uncore_forcewake_domain *d)
 {
@@ -235,7 +229,7 @@ static void intel_uncore_fw_release_timer(unsigned long arg)
 	struct intel_uncore_forcewake_domain *domain = (void *)arg;
 	unsigned long irqflags;
 
-	assert_device_not_suspended(domain->i915);
+	assert_rpm_device_not_suspended(domain->i915);
 
 	spin_lock_irqsave(&domain->i915->uncore.lock, irqflags);
 	if (WARN_ON(domain->wake_count == 0))
@@ -627,7 +621,7 @@ hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
 
 #define GEN2_READ_HEADER(x) \
 	u##x val = 0; \
-	assert_device_not_suspended(dev_priv);
+	assert_rpm_wakelock_held(dev_priv);
 
 #define GEN2_READ_FOOTER \
 	trace_i915_reg_rw(false, reg, val, sizeof(val), trace); \
@@ -669,7 +663,7 @@ __gen2_read(64)
 	u32 offset = i915_mmio_reg_offset(reg); \
 	unsigned long irqflags; \
 	u##x val = 0; \
-	assert_device_not_suspended(dev_priv); \
+	assert_rpm_wakelock_held(dev_priv); \
 	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags)
 
 #define GEN6_READ_FOOTER \
@@ -802,7 +796,7 @@ __gen6_read(64)
 #define VGPU_READ_HEADER(x) \
 	unsigned long irqflags; \
 	u##x val = 0; \
-	assert_device_not_suspended(dev_priv); \
+	assert_rpm_device_not_suspended(dev_priv); \
 	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags)
 
 #define VGPU_READ_FOOTER \
@@ -829,7 +823,7 @@ __vgpu_read(64)
 
 #define GEN2_WRITE_HEADER \
 	trace_i915_reg_rw(true, reg, val, sizeof(val), trace); \
-	assert_device_not_suspended(dev_priv); \
+	assert_rpm_wakelock_held(dev_priv); \
 
 #define GEN2_WRITE_FOOTER
 
@@ -869,7 +863,7 @@ __gen2_write(64)
 	u32 offset = i915_mmio_reg_offset(reg); \
 	unsigned long irqflags; \
 	trace_i915_reg_rw(true, reg, val, sizeof(val), trace); \
-	assert_device_not_suspended(dev_priv); \
+	assert_rpm_wakelock_held(dev_priv); \
 	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags)
 
 #define GEN6_WRITE_FOOTER \
@@ -1045,7 +1039,7 @@ __gen6_write(64)
 #define VGPU_WRITE_HEADER \
 	unsigned long irqflags; \
 	trace_i915_reg_rw(true, reg, val, sizeof(val), trace); \
-	assert_device_not_suspended(dev_priv); \
+	assert_rpm_device_not_suspended(dev_priv); \
 	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags)
 
 #define VGPU_WRITE_FOOTER \
-- 
2.5.0

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

[PATCH 06/10] drm/i915: use assert_rpm_wakelock_held instead of opencoding it

[Imre Deak]

Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 10 ++++------
 drivers/gpu/drm/i915/intel_uncore.c     |  2 +-
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index bca7ca7..603d410 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -538,8 +538,7 @@ static void assert_can_enable_dc5(struct drm_i915_private *dev_priv)
 
 	WARN_ONCE((I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5),
 		  "DC5 already programmed to be enabled.\n");
-	WARN_ONCE(dev_priv->pm.suspended,
-		  "DC5 cannot be enabled, if platform is runtime-suspended.\n");
+	assert_rpm_wakelock_held(dev_priv);
 
 	assert_csr_loaded(dev_priv);
 }
@@ -553,8 +552,7 @@ static void assert_can_disable_dc5(struct drm_i915_private *dev_priv)
 	if (dev_priv->power_domains.initializing)
 		return;
 
-	WARN_ONCE(dev_priv->pm.suspended,
-		"Disabling of DC5 while platform is runtime-suspended should never happen.\n");
+	assert_rpm_wakelock_held(dev_priv);
 }
 
 static void gen9_enable_dc5(struct drm_i915_private *dev_priv)
@@ -2238,7 +2236,7 @@ void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
 	struct device *device = &dev->pdev->dev;
 
 	pm_runtime_get_sync(device);
-	WARN(dev_priv->pm.suspended, "Device still suspended.\n");
+	assert_rpm_wakelock_held(dev_priv);
 }
 
 /**
@@ -2263,7 +2261,7 @@ void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv)
 	struct drm_device *dev = dev_priv->dev;
 	struct device *device = &dev->pdev->dev;
 
-	WARN(dev_priv->pm.suspended, "Getting nosync-ref while suspended.\n");
+	assert_rpm_wakelock_held(dev_priv);
 	pm_runtime_get_noresume(device);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 3c63d94..277e60a 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -404,7 +404,7 @@ void intel_uncore_forcewake_get(struct drm_i915_private *dev_priv,
 	if (!dev_priv->uncore.funcs.force_wake_get)
 		return;
 
-	WARN_ON(dev_priv->pm.suspended);
+	assert_rpm_wakelock_held(dev_priv);
 
 	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 	__intel_uncore_forcewake_get(dev_priv, fw_domains);
-- 
2.5.0

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

[PATCH 07/10] drm/i915: add support for checking if we hold an RPM reference

[Imre Deak]

Atm, we assert that the device is not suspended until the point when the
device is truly put to a suspended state. This is fine, but we can catch
more problems if we check that RPM refcount is non-zero. After that one
drops to zero we shouldn't access the device any more, even if the actual
device suspend may be delayed. Change assert_rpm_wakelock_held()
accordingly to check for a non-zero RPM refcount in addition to the
current device-not-suspended check.

For the new asserts to work we need to annotate every place explicitly in
the code where we expect that the device is powered. The places where we
only assume this, but may not hold an RPM reference:
- driver load
  We assume the device to be powered until we enable RPM. Make this
  explicit by taking an RPM reference around the load function.
- system and runtime sudpend/resume handlers
  These handlers are called when the RPM reference becomes 0 and know the
  exact point after which the device can get powered off. Disable the
  RPM-reference-held check for their duration.
- the IRQ, hangcheck and RPS work handlers
  These handlers are flushed in the system/runtime suspend handler
  before the device is powered off, so it's guaranteed that they won't
  run while the device is powered off even though they don't hold any
  RPM reference. Disable the RPM-reference-held check for their duration.

In all these cases we still check that the device is not suspended.
These explicit annotations also have the positive side effect of
documenting our assumptions better.

This caught additional WARNs from the atomic modeset path, those should
be fixed separately.

v2:
- remove the redundant HAS_RUNTIME_PM check (moved to patch 1) (Ville)
v3:
- use a new dedicated RPM wakelock refcount to also catch cases where
  our own RPM get/put functions were not called (Chris)
- assert also that the new RPM wakelock refcount is 0 in the RPM
  suspend handler (Chris)
- change the assert error message to be more meaningful (Chris)
- prevent false assert errors and check that the RPM wakelock is 0 in
  the RPM resume handler too
- prevent false assert errors in the hangcheck work too
- add a device not suspended assert check to the hangcheck work
v4:
- rename disable/enable_rpm_asserts to disable/enable_rpm_wakeref_asserts
  and wakelock_count to wakeref_count
- disable the wakeref asserts in the IRQ handlers and RPS work too
- update/clarify commit message

Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v3)
---
 drivers/gpu/drm/i915/i915_dma.c         |  7 ++++
 drivers/gpu/drm/i915/i915_drv.c         | 39 ++++++++++++++++--
 drivers/gpu/drm/i915/i915_drv.h         |  1 +
 drivers/gpu/drm/i915/i915_irq.c         | 73 ++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_drv.h        | 41 ++++++++++++++++++
 drivers/gpu/drm/i915/intel_pm.c         |  1 +
 drivers/gpu/drm/i915/intel_runtime_pm.c |  6 +++
 7 files changed, 158 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 84e2b20..077d64c 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -896,6 +896,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 
 	intel_pm_setup(dev);
 
+	intel_runtime_pm_get(dev_priv);
+
 	intel_display_crc_init(dev);
 
 	i915_dump_device_info(dev_priv);
@@ -1085,6 +1087,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 
 	i915_audio_component_init(dev_priv);
 
+	intel_runtime_pm_put(dev_priv);
+
 	return 0;
 
 out_power_well:
@@ -1120,6 +1124,9 @@ free_priv:
 	kmem_cache_destroy(dev_priv->requests);
 	kmem_cache_destroy(dev_priv->vmas);
 	kmem_cache_destroy(dev_priv->objects);
+
+	intel_runtime_pm_put(dev_priv);
+
 	kfree(dev_priv);
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 8ddfcce..61267e7 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -580,6 +580,8 @@ static int i915_drm_suspend(struct drm_device *dev)
 	dev_priv->modeset_restore = MODESET_SUSPENDED;
 	mutex_unlock(&dev_priv->modeset_restore_lock);
 
+	disable_rpm_wakeref_asserts(dev_priv);
+
 	/* We do a lot of poking in a lot of registers, make sure they work
 	 * properly. */
 	intel_display_set_init_power(dev_priv, true);
@@ -592,7 +594,7 @@ static int i915_drm_suspend(struct drm_device *dev)
 	if (error) {
 		dev_err(&dev->pdev->dev,
 			"GEM idle failed, resume might fail\n");
-		return error;
+		goto out;
 	}
 
 	intel_guc_suspend(dev);
@@ -635,7 +637,10 @@ static int i915_drm_suspend(struct drm_device *dev)
 	if (HAS_CSR(dev_priv))
 		flush_work(&dev_priv->csr.work);
 
-	return 0;
+out:
+	enable_rpm_wakeref_asserts(dev_priv);
+
+	return error;
 }
 
 static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation)
@@ -644,6 +649,8 @@ static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation)
 	bool fw_csr;
 	int ret;
 
+	disable_rpm_wakeref_asserts(dev_priv);
+
 	fw_csr = suspend_to_idle(dev_priv) && dev_priv->csr.dmc_payload;
 	/*
 	 * In case of firmware assisted context save/restore don't manually
@@ -662,7 +669,7 @@ static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation)
 		if (!fw_csr)
 			intel_power_domains_init_hw(dev_priv, true);
 
-		return ret;
+		goto out;
 	}
 
 	pci_disable_device(drm_dev->pdev);
@@ -683,7 +690,10 @@ static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation)
 
 	dev_priv->suspended_to_idle = suspend_to_idle(dev_priv);
 
-	return 0;
+out:
+	enable_rpm_wakeref_asserts(dev_priv);
+
+	return ret;
 }
 
 int i915_suspend_switcheroo(struct drm_device *dev, pm_message_t state)
@@ -714,6 +724,8 @@ static int i915_drm_resume(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	disable_rpm_wakeref_asserts(dev_priv);
+
 	mutex_lock(&dev->struct_mutex);
 	i915_gem_restore_gtt_mappings(dev);
 	mutex_unlock(&dev->struct_mutex);
@@ -778,6 +790,8 @@ static int i915_drm_resume(struct drm_device *dev)
 
 	drm_kms_helper_poll_enable(dev);
 
+	enable_rpm_wakeref_asserts(dev_priv);
+
 	return 0;
 }
 
@@ -802,6 +816,8 @@ static int i915_drm_resume_early(struct drm_device *dev)
 
 	pci_set_master(dev->pdev);
 
+	disable_rpm_wakeref_asserts(dev_priv);
+
 	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 		ret = vlv_resume_prepare(dev_priv, false);
 	if (ret)
@@ -823,6 +839,8 @@ static int i915_drm_resume_early(struct drm_device *dev)
 out:
 	dev_priv->suspended_to_idle = false;
 
+	enable_rpm_wakeref_asserts(dev_priv);
+
 	return ret;
 }
 
@@ -1455,6 +1473,9 @@ static int intel_runtime_suspend(struct device *device)
 
 		return -EAGAIN;
 	}
+
+	disable_rpm_wakeref_asserts(dev_priv);
+
 	/*
 	 * We are safe here against re-faults, since the fault handler takes
 	 * an RPM reference.
@@ -1474,10 +1495,15 @@ static int intel_runtime_suspend(struct device *device)
 		DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret);
 		intel_runtime_pm_enable_interrupts(dev_priv);
 
+		enable_rpm_wakeref_asserts(dev_priv);
+
 		return ret;
 	}
 
 	intel_uncore_forcewake_reset(dev, false);
+
+	enable_rpm_wakeref_asserts(dev_priv);
+	WARN_ON_ONCE(atomic_read(&dev_priv->pm.wakeref_count));
 	dev_priv->pm.suspended = true;
 
 	/*
@@ -1521,6 +1547,9 @@ static int intel_runtime_resume(struct device *device)
 
 	DRM_DEBUG_KMS("Resuming device\n");
 
+	WARN_ON_ONCE(atomic_read(&dev_priv->pm.wakeref_count));
+	disable_rpm_wakeref_asserts(dev_priv);
+
 	intel_opregion_notify_adapter(dev, PCI_D0);
 	dev_priv->pm.suspended = false;
 
@@ -1555,6 +1584,8 @@ static int intel_runtime_resume(struct device *device)
 
 	intel_enable_gt_powersave(dev);
 
+	enable_rpm_wakeref_asserts(dev_priv);
+
 	if (ret)
 		DRM_ERROR("Runtime resume failed, disabling it (%d)\n", ret);
 	else
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9124085..2894716 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1602,6 +1602,7 @@ struct skl_wm_level {
  * For more, read the Documentation/power/runtime_pm.txt.
  */
 struct i915_runtime_pm {
+	atomic_t wakeref_count;
 	bool suspended;
 	bool irqs_enabled;
 };
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 86664d1..50a46ad 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1103,6 +1103,13 @@ static void gen6_pm_rps_work(struct work_struct *work)
 		spin_unlock_irq(&dev_priv->irq_lock);
 		return;
 	}
+
+	/*
+	 * The RPS work is synced during runtime suspend, we don't require a
+	 * wakeref.
+	 */
+	disable_rpm_wakeref_asserts(dev_priv);
+
 	pm_iir = dev_priv->rps.pm_iir;
 	dev_priv->rps.pm_iir = 0;
 	/* Make sure not to corrupt PMIMR state used by ringbuffer on GEN6 */
@@ -1115,7 +1122,7 @@ static void gen6_pm_rps_work(struct work_struct *work)
 	WARN_ON(pm_iir & ~dev_priv->pm_rps_events);
 
 	if ((pm_iir & dev_priv->pm_rps_events) == 0 && !client_boost)
-		return;
+		goto out;
 
 	mutex_lock(&dev_priv->rps.hw_lock);
 
@@ -1170,6 +1177,8 @@ static void gen6_pm_rps_work(struct work_struct *work)
 	intel_set_rps(dev_priv->dev, new_delay);
 
 	mutex_unlock(&dev_priv->rps.hw_lock);
+out:
+	enable_rpm_wakeref_asserts(dev_priv);
 }
 
 
@@ -1758,6 +1767,9 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
 	if (!intel_irqs_enabled(dev_priv))
 		return IRQ_NONE;
 
+	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
+	disable_rpm_wakeref_asserts(dev_priv);
+
 	while (true) {
 		/* Find, clear, then process each source of interrupt */
 
@@ -1792,6 +1804,8 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
 	}
 
 out:
+	enable_rpm_wakeref_asserts(dev_priv);
+
 	return ret;
 }
 
@@ -1805,6 +1819,9 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
 	if (!intel_irqs_enabled(dev_priv))
 		return IRQ_NONE;
 
+	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
+	disable_rpm_wakeref_asserts(dev_priv);
+
 	for (;;) {
 		master_ctl = I915_READ(GEN8_MASTER_IRQ) & ~GEN8_MASTER_IRQ_CONTROL;
 		iir = I915_READ(VLV_IIR);
@@ -1835,6 +1852,8 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
 		POSTING_READ(GEN8_MASTER_IRQ);
 	}
 
+	enable_rpm_wakeref_asserts(dev_priv);
+
 	return ret;
 }
 
@@ -2165,6 +2184,9 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 	if (!intel_irqs_enabled(dev_priv))
 		return IRQ_NONE;
 
+	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
+	disable_rpm_wakeref_asserts(dev_priv);
+
 	/* We get interrupts on unclaimed registers, so check for this before we
 	 * do any I915_{READ,WRITE}. */
 	intel_uncore_check_errors(dev);
@@ -2223,6 +2245,9 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 		POSTING_READ(SDEIER);
 	}
 
+	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
+	enable_rpm_wakeref_asserts(dev_priv);
+
 	return ret;
 }
 
@@ -2255,6 +2280,9 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 	if (!intel_irqs_enabled(dev_priv))
 		return IRQ_NONE;
 
+	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
+	disable_rpm_wakeref_asserts(dev_priv);
+
 	if (INTEL_INFO(dev_priv)->gen >= 9)
 		aux_mask |=  GEN9_AUX_CHANNEL_B | GEN9_AUX_CHANNEL_C |
 			GEN9_AUX_CHANNEL_D;
@@ -2262,7 +2290,7 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 	master_ctl = I915_READ_FW(GEN8_MASTER_IRQ);
 	master_ctl &= ~GEN8_MASTER_IRQ_CONTROL;
 	if (!master_ctl)
-		return IRQ_NONE;
+		goto out;
 
 	I915_WRITE_FW(GEN8_MASTER_IRQ, 0);
 
@@ -2393,6 +2421,9 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 	I915_WRITE_FW(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
 	POSTING_READ_FW(GEN8_MASTER_IRQ);
 
+out:
+	enable_rpm_wakeref_asserts(dev_priv);
+
 	return ret;
 }
 
@@ -2989,6 +3020,12 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
 	if (!i915.enable_hangcheck)
 		return;
 
+	/*
+	 * The hangcheck work is synced during runtime suspend, we don't
+	 * require a wakeref.
+	 */
+	disable_rpm_wakeref_asserts(dev_priv);
+
 	for_each_ring(ring, dev_priv, i) {
 		u64 acthd;
 		u32 seqno;
@@ -3080,13 +3117,18 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
 		}
 	}
 
-	if (rings_hung)
-		return i915_handle_error(dev, true, "Ring hung");
+	if (rings_hung) {
+		i915_handle_error(dev, true, "Ring hung");
+		goto out;
+	}
 
 	if (busy_count)
 		/* Reset timer case chip hangs without another request
 		 * being added */
 		i915_queue_hangcheck(dev);
+
+out:
+	enable_rpm_wakeref_asserts(dev_priv);
 }
 
 void i915_queue_hangcheck(struct drm_device *dev)
@@ -3878,13 +3920,18 @@ static irqreturn_t i8xx_irq_handler(int irq, void *arg)
 	u16 flip_mask =
 		I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT |
 		I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT;
+	irqreturn_t ret;
 
 	if (!intel_irqs_enabled(dev_priv))
 		return IRQ_NONE;
 
+	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
+	disable_rpm_wakeref_asserts(dev_priv);
+
+	ret = IRQ_NONE;
 	iir = I915_READ16(IIR);
 	if (iir == 0)
-		return IRQ_NONE;
+		goto out;
 
 	while (iir & ~flip_mask) {
 		/* Can't rely on pipestat interrupt bit in iir as it might
@@ -3933,8 +3980,12 @@ static irqreturn_t i8xx_irq_handler(int irq, void *arg)
 
 		iir = new_iir;
 	}
+	ret = IRQ_HANDLED;
 
-	return IRQ_HANDLED;
+out:
+	enable_rpm_wakeref_asserts(dev_priv);
+
+	return ret;
 }
 
 static void i8xx_irq_uninstall(struct drm_device * dev)
@@ -4063,6 +4114,9 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)
 	if (!intel_irqs_enabled(dev_priv))
 		return IRQ_NONE;
 
+	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
+	disable_rpm_wakeref_asserts(dev_priv);
+
 	iir = I915_READ(IIR);
 	do {
 		bool irq_received = (iir & ~flip_mask) != 0;
@@ -4145,6 +4199,8 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)
 		iir = new_iir;
 	} while (iir & ~flip_mask);
 
+	enable_rpm_wakeref_asserts(dev_priv);
+
 	return ret;
 }
 
@@ -4284,6 +4340,9 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
 	if (!intel_irqs_enabled(dev_priv))
 		return IRQ_NONE;
 
+	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
+	disable_rpm_wakeref_asserts(dev_priv);
+
 	iir = I915_READ(IIR);
 
 	for (;;) {
@@ -4369,6 +4428,8 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
 		iir = new_iir;
 	}
 
+	enable_rpm_wakeref_asserts(dev_priv);
+
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9837a25..d8e4aca 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1442,6 +1442,47 @@ static inline void
 assert_rpm_wakelock_held(struct drm_i915_private *dev_priv)
 {
 	assert_rpm_device_not_suspended(dev_priv);
+	WARN_ONCE(!atomic_read(&dev_priv->pm.wakeref_count),
+		  "RPM wakelock ref not held during HW access");
+}
+
+/**
+ * disable_rpm_wakeref_asserts - disable the RPM assert checks
+ * @dev_priv: i915 device instance
+ *
+ * This function disable asserts that check if we hold an RPM wakelock
+ * reference, while keeping the device-not-suspended checks still enabled.
+ * It's meant to be used only in special circumstances where our rule about
+ * the wakelock refcount wrt. the device power state doesn't hold. According
+ * to this rule at any point where we access the HW or want to keep the HW in
+ * an active state we must hold an RPM wakelock reference acquired via one of
+ * the intel_runtime_pm_get() helpers. Currently there are a few special spots
+ * where this rule doesn't hold: the IRQ and suspend/resume handlers, the
+ * forcewake release timer, and the GPU RPS and hangcheck works. All other
+ * users should avoid using this function.
+ *
+ * Any calls to this function must have a symmetric call to
+ * enable_rpm_wakeref_asserts().
+ */
+static inline void disable_rpm_wakeref_asserts(struct drm_i915_private *dev_priv)
+{
+	atomic_inc(&dev_priv->pm.wakeref_count);
+}
+
+/**
+ * enable_rpm_wakeref_asserts - re-enable the RPM assert checks
+ * @dev_priv: i915 device instance
+ *
+ * This function re-enables the RPM assert checks after disabling them with
+ * disable_rpm_wakeref_asserts. It's meant to be used only in special
+ * circumstances otherwise its use should be avoided.
+ *
+ * Any calls to this function must have a symmetric call to
+ * disable_rpm_wakeref_asserts().
+ */
+static inline void enable_rpm_wakeref_asserts(struct drm_i915_private *dev_priv)
+{
+	atomic_dec(&dev_priv->pm.wakeref_count);
 }
 
 void intel_runtime_pm_get(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index f93c4b9..6c08537 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7247,4 +7247,5 @@ void intel_pm_setup(struct drm_device *dev)
 	INIT_LIST_HEAD(&dev_priv->rps.mmioflips.link);
 
 	dev_priv->pm.suspended = false;
+	atomic_set(&dev_priv->pm.wakeref_count, 0);
 }
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 603d410..29157fb 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -2236,6 +2236,8 @@ void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
 	struct device *device = &dev->pdev->dev;
 
 	pm_runtime_get_sync(device);
+
+	atomic_inc(&dev_priv->pm.wakeref_count);
 	assert_rpm_wakelock_held(dev_priv);
 }
 
@@ -2263,6 +2265,8 @@ void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv)
 
 	assert_rpm_wakelock_held(dev_priv);
 	pm_runtime_get_noresume(device);
+
+	atomic_inc(&dev_priv->pm.wakeref_count);
 }
 
 /**
@@ -2278,6 +2282,8 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv)
 	struct drm_device *dev = dev_priv->dev;
 	struct device *device = &dev->pdev->dev;
 
+	atomic_dec(&dev_priv->pm.wakeref_count);
+
 	pm_runtime_mark_last_busy(device);
 	pm_runtime_put_autosuspend(device);
 }
-- 
2.5.0

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

[PATCH 08/10] drm/i915: check that we hold an RPM wakelock ref before we put it

[Imre Deak]

v2-v3:
- unchanged
v4:
- keep the corresponding check in the get helper (Chris)

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 29157fb..82c55a9 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -2282,6 +2282,7 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv)
 	struct drm_device *dev = dev_priv->dev;
 	struct device *device = &dev->pdev->dev;
 
+	assert_rpm_wakelock_held(dev_priv);
 	atomic_dec(&dev_priv->pm.wakeref_count);
 
 	pm_runtime_mark_last_busy(device);
-- 
2.5.0

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

[PATCH 09/10] drm/i915: add support for checking RPM atomic sections

[Imre Deak]

In some cases we want to check whether we hold an RPM wakelock reference
for the whole duration of a sequence. To achieve this add a new RPM atomic sequence
counter that we increment any time the wakelock refcount drops to zero.
Check whether the sequence number stays the same during the atomic
section and that we hold the wakelock at the beginning of the section.

Motivated by Chris.

v2-v3:
- unchanged
v4:
- swap the order of atomic_read() and assert_rpm_wakelock_held() in
  assert_rpm_atomic_begin() to avoid race

Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v3)
---
 drivers/gpu/drm/i915/i915_drv.h         |  1 +
 drivers/gpu/drm/i915/intel_drv.h        | 17 +++++++++++++++++
 drivers/gpu/drm/i915/intel_pm.c         |  1 +
 drivers/gpu/drm/i915/intel_runtime_pm.c |  3 ++-
 4 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2894716..00ce627 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1603,6 +1603,7 @@ struct skl_wm_level {
  */
 struct i915_runtime_pm {
 	atomic_t wakeref_count;
+	atomic_t atomic_seq;
 	bool suspended;
 	bool irqs_enabled;
 };
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d8e4aca..88d37eb 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1446,6 +1446,23 @@ assert_rpm_wakelock_held(struct drm_i915_private *dev_priv)
 		  "RPM wakelock ref not held during HW access");
 }
 
+static inline int
+assert_rpm_atomic_begin(struct drm_i915_private *dev_priv)
+{
+	int seq = atomic_read(&dev_priv->pm.atomic_seq);
+
+	assert_rpm_wakelock_held(dev_priv);
+
+	return seq;
+}
+
+static inline void
+assert_rpm_atomic_end(struct drm_i915_private *dev_priv, int begin_seq)
+{
+	WARN_ONCE(atomic_read(&dev_priv->pm.atomic_seq) != begin_seq,
+		 "HW access outside of RPM atomic section\n");
+}
+
 /**
  * disable_rpm_wakeref_asserts - disable the RPM assert checks
  * @dev_priv: i915 device instance
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 6c08537..6eb9606 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7248,4 +7248,5 @@ void intel_pm_setup(struct drm_device *dev)
 
 	dev_priv->pm.suspended = false;
 	atomic_set(&dev_priv->pm.wakeref_count, 0);
+	atomic_set(&dev_priv->pm.atomic_seq, 0);
 }
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 82c55a9..cee54ea 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -2283,7 +2283,8 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv)
 	struct device *device = &dev->pdev->dev;
 
 	assert_rpm_wakelock_held(dev_priv);
-	atomic_dec(&dev_priv->pm.wakeref_count);
+	if (atomic_dec_and_test(&dev_priv->pm.wakeref_count))
+		atomic_inc(&dev_priv->pm.atomic_seq);
 
 	pm_runtime_mark_last_busy(device);
 	pm_runtime_put_autosuspend(device);
-- 
2.5.0

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

[PATCH 10/10] drm/i915: check that we are in an RPM atomic section in GGTT PTE updaters

[Imre Deak]

The device should be on for the whole duration of the update, so check
for this.

v2:
- use the existing dev_priv directly everywhere (Ville)
v3:
- check also that we are in an RPM atomic section (Chris)
- add the assert to i915_ggtt_insert_entries/clear_range too (Chris)

Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 7cfa1b9..c14b8f8 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2351,6 +2351,9 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
 	int i = 0;
 	struct sg_page_iter sg_iter;
 	dma_addr_t addr = 0; /* shut up gcc */
+	int rpm_atomic_seq;
+
+	rpm_atomic_seq = assert_rpm_atomic_begin(dev_priv);
 
 	for_each_sg_page(st->sgl, &sg_iter, st->nents, 0) {
 		addr = sg_dma_address(sg_iter.sg) +
@@ -2377,6 +2380,8 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
 	 */
 	I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
 	POSTING_READ(GFX_FLSH_CNTL_GEN6);
+
+	assert_rpm_atomic_end(dev_priv, rpm_atomic_seq);
 }
 
 /*
@@ -2397,6 +2402,9 @@ static void gen6_ggtt_insert_entries(struct i915_address_space *vm,
 	int i = 0;
 	struct sg_page_iter sg_iter;
 	dma_addr_t addr = 0;
+	int rpm_atomic_seq;
+
+	rpm_atomic_seq = assert_rpm_atomic_begin(dev_priv);
 
 	for_each_sg_page(st->sgl, &sg_iter, st->nents, 0) {
 		addr = sg_page_iter_dma_address(&sg_iter);
@@ -2421,6 +2429,8 @@ static void gen6_ggtt_insert_entries(struct i915_address_space *vm,
 	 */
 	I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
 	POSTING_READ(GFX_FLSH_CNTL_GEN6);
+
+	assert_rpm_atomic_end(dev_priv, rpm_atomic_seq);
 }
 
 static void gen8_ggtt_clear_range(struct i915_address_space *vm,
@@ -2435,6 +2445,9 @@ static void gen8_ggtt_clear_range(struct i915_address_space *vm,
 		(gen8_pte_t __iomem *) dev_priv->gtt.gsm + first_entry;
 	const int max_entries = gtt_total_entries(dev_priv->gtt) - first_entry;
 	int i;
+	int rpm_atomic_seq;
+
+	rpm_atomic_seq = assert_rpm_atomic_begin(dev_priv);
 
 	if (WARN(num_entries > max_entries,
 		 "First entry = %d; Num entries = %d (max=%d)\n",
@@ -2447,6 +2460,8 @@ static void gen8_ggtt_clear_range(struct i915_address_space *vm,
 	for (i = 0; i < num_entries; i++)
 		gen8_set_pte(&gtt_base[i], scratch_pte);
 	readl(gtt_base);
+
+	assert_rpm_atomic_end(dev_priv, rpm_atomic_seq);
 }
 
 static void gen6_ggtt_clear_range(struct i915_address_space *vm,
@@ -2461,6 +2476,9 @@ static void gen6_ggtt_clear_range(struct i915_address_space *vm,
 		(gen6_pte_t __iomem *) dev_priv->gtt.gsm + first_entry;
 	const int max_entries = gtt_total_entries(dev_priv->gtt) - first_entry;
 	int i;
+	int rpm_atomic_seq;
+
+	rpm_atomic_seq = assert_rpm_atomic_begin(dev_priv);
 
 	if (WARN(num_entries > max_entries,
 		 "First entry = %d; Num entries = %d (max=%d)\n",
@@ -2473,6 +2491,8 @@ static void gen6_ggtt_clear_range(struct i915_address_space *vm,
 	for (i = 0; i < num_entries; i++)
 		iowrite32(scratch_pte, &gtt_base[i]);
 	readl(gtt_base);
+
+	assert_rpm_atomic_end(dev_priv, rpm_atomic_seq);
 }
 
 static void i915_ggtt_insert_entries(struct i915_address_space *vm,
@@ -2480,11 +2500,17 @@ static void i915_ggtt_insert_entries(struct i915_address_space *vm,
 				     uint64_t start,
 				     enum i915_cache_level cache_level, u32 unused)
 {
+	struct drm_i915_private *dev_priv = vm->dev->dev_private;
 	unsigned int flags = (cache_level == I915_CACHE_NONE) ?
 		AGP_USER_MEMORY : AGP_USER_CACHED_MEMORY;
+	int rpm_atomic_seq;
+
+	rpm_atomic_seq = assert_rpm_atomic_begin(dev_priv);
 
 	intel_gtt_insert_sg_entries(pages, start >> PAGE_SHIFT, flags);
 
+	assert_rpm_atomic_end(dev_priv, rpm_atomic_seq);
+
 }
 
 static void i915_ggtt_clear_range(struct i915_address_space *vm,
@@ -2492,9 +2518,16 @@ static void i915_ggtt_clear_range(struct i915_address_space *vm,
 				  uint64_t length,
 				  bool unused)
 {
+	struct drm_i915_private *dev_priv = vm->dev->dev_private;
 	unsigned first_entry = start >> PAGE_SHIFT;
 	unsigned num_entries = length >> PAGE_SHIFT;
+	int rpm_atomic_seq;
+
+	rpm_atomic_seq = assert_rpm_atomic_begin(dev_priv);
+
 	intel_gtt_clear_range(first_entry, num_entries);
+
+	assert_rpm_atomic_end(dev_priv, rpm_atomic_seq);
 }
 
 static int ggtt_bind_vma(struct i915_vma *vma,
-- 
2.5.0

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

Re: [PATCH 02/10] drm/i915: disable power well support on platforms without runtime PM support

[Ville Syrjälä]

On Tue, Dec 15, 2015 at 08:10:30PM +0200, Imre Deak wrote:
> All platforms with power well support have runtime PM support, so
> simplify things by explicitly disabling power well support on platforms
> without runtime PM support. This results in holding the init power domain
> reference whenever the driver is loaded in addition to an RPM reference,
> which reflects the reality better and makes it possible to simplify
> things by removing the HAS_RUNTIME_PM special casing from more places in
> a follow-up patch.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 9945040..f4ff5f5 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1908,6 +1908,11 @@ static int
>  sanitize_disable_power_well_option(const struct drm_i915_private *dev_priv,
>  				   int disable_power_well)
>  {
> +	if (!HAS_RUNTIME_PM(dev_priv)) {
> +		DRM_DEBUG_KMS("No runtime PM support, disabling display power well support\n");
> +		return 0;
> +	}

Feels a bit too magic to me. Needs a comment at least, otherwise someone
is going to change disable_power_well back into something you can disable
at runtime and then all the old stuff might break.

Grabbing an extra rpm reference explicitly for this purpose might be
less confusing.

> +
>  	if (disable_power_well >= 0)
>  		return !!disable_power_well;
>  
> -- 
> 2.5.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/10] drm/i915: add support for checking if we hold an RPM reference

[Chris Wilson]

On Tue, Dec 15, 2015 at 08:10:35PM +0200, Imre Deak wrote:
> Atm, we assert that the device is not suspended until the point when the
> device is truly put to a suspended state. This is fine, but we can catch
> more problems if we check that RPM refcount is non-zero. After that one
> drops to zero we shouldn't access the device any more, even if the actual
> device suspend may be delayed. Change assert_rpm_wakelock_held()
> accordingly to check for a non-zero RPM refcount in addition to the
> current device-not-suspended check.
> 
> For the new asserts to work we need to annotate every place explicitly in
> the code where we expect that the device is powered. The places where we
> only assume this, but may not hold an RPM reference:
> - driver load
>   We assume the device to be powered until we enable RPM. Make this
>   explicit by taking an RPM reference around the load function.
> - system and runtime sudpend/resume handlers
>   These handlers are called when the RPM reference becomes 0 and know the
>   exact point after which the device can get powered off. Disable the
>   RPM-reference-held check for their duration.
> - the IRQ, hangcheck and RPS work handlers
>   These handlers are flushed in the system/runtime suspend handler
>   before the device is powered off, so it's guaranteed that they won't
>   run while the device is powered off even though they don't hold any
>   RPM reference. Disable the RPM-reference-held check for their duration.

My current thinking is that the hangcheck/RPS tasks are wrong - and that
we do actually have explicit wakerefs that should cover their lifetimes
(but we fail to actually terminate them when we drop the associated
wakeref).

With respect to the current state (cancelling the work in rpm_suspend),
the assert disabling is correct, but I think we should be indicating
that we papering over a "bug" more strongly.

i.e. something like DISABLE_RPM_WAKEREF_ASSERT();
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/10] drm/i915: add support for checking if we hold an RPM reference

[Imre Deak]

On Tue, 2015-12-15 at 21:07 +0000, Chris Wilson wrote:
> On Tue, Dec 15, 2015 at 08:10:35PM +0200, Imre Deak wrote:
> > Atm, we assert that the device is not suspended until the point
> > when the
> > device is truly put to a suspended state. This is fine, but we can
> > catch
> > more problems if we check that RPM refcount is non-zero. After that
> > one
> > drops to zero we shouldn't access the device any more, even if the
> > actual
> > device suspend may be delayed. Change assert_rpm_wakelock_held()
> > accordingly to check for a non-zero RPM refcount in addition to the
> > current device-not-suspended check.
> > 
> > For the new asserts to work we need to annotate every place
> > explicitly in
> > the code where we expect that the device is powered. The places
> > where we
> > only assume this, but may not hold an RPM reference:
> > - driver load
> >   We assume the device to be powered until we enable RPM. Make this
> >   explicit by taking an RPM reference around the load function.
> > - system and runtime sudpend/resume handlers
> >   These handlers are called when the RPM reference becomes 0 and
> > know the
> >   exact point after which the device can get powered off. Disable
> > the
> >   RPM-reference-held check for their duration.
> > - the IRQ, hangcheck and RPS work handlers
> >   These handlers are flushed in the system/runtime suspend handler
> >   before the device is powered off, so it's guaranteed that they
> > won't
> >   run while the device is powered off even though they don't hold
> > any
> >   RPM reference. Disable the RPM-reference-held check for their
> > duration.
> 
> My current thinking is that the hangcheck/RPS tasks are wrong - and
> that
> we do actually have explicit wakerefs that should cover their
> lifetimes
> (but we fail to actually terminate them when we drop the associated
> wakeref).
> 
> With respect to the current state (cancelling the work in
> rpm_suspend),
> the assert disabling is correct, but I think we should be indicating
> that we papering over a "bug" more strongly.
> 
> i.e. something like DISABLE_RPM_WAKEREF_ASSERT();

But the other cases are still legitimate, so we'd keep the lower case
name for those and define the above macro as an alias simply to
emphasize the difference?

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

Re: [PATCH 02/10] drm/i915: disable power well support on platforms without runtime PM support

[Imre Deak]

On Tue, 2015-12-15 at 22:57 +0200, Ville Syrjälä wrote:
> On Tue, Dec 15, 2015 at 08:10:30PM +0200, Imre Deak wrote:
> > All platforms with power well support have runtime PM support, so
> > simplify things by explicitly disabling power well support on
> > platforms
> > without runtime PM support. This results in holding the init power
> > domain
> > reference whenever the driver is loaded in addition to an RPM
> > reference,
> > which reflects the reality better and makes it possible to simplify
> > things by removing the HAS_RUNTIME_PM special casing from more
> > places in
> > a follow-up patch.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 9945040..f4ff5f5 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -1908,6 +1908,11 @@ static int
> >  sanitize_disable_power_well_option(const struct drm_i915_private
> > *dev_priv,
> >  				   int disable_power_well)
> >  {
> > +	if (!HAS_RUNTIME_PM(dev_priv)) {
> > +		DRM_DEBUG_KMS("No runtime PM support, disabling
> > display power well support\n");
> > +		return 0;
> > +	}
> 
> Feels a bit too magic to me. Needs a comment at least, otherwise
> someone
> is going to change disable_power_well back into something you can
> disable
> at runtime and then all the old stuff might break.
> 
> Grabbing an extra rpm reference explicitly for this purpose might be
> less confusing.

Ok, agreed. But making power well support depend on RPM still ok with
you? I can add then the extra RPM get/put only for !HAS_RUNTIME_PM.

> 
> > +
> >  	if (disable_power_well >= 0)
> >  		return !!disable_power_well;
> >  
> > -- 
> > 2.5.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/10] drm/i915: add support for checking if we hold an RPM reference

[Chris Wilson]

On Wed, Dec 16, 2015 at 12:52:34AM +0200, Imre Deak wrote:
> On Tue, 2015-12-15 at 21:07 +0000, Chris Wilson wrote:
> > My current thinking is that the hangcheck/RPS tasks are wrong - and
> > that
> > we do actually have explicit wakerefs that should cover their
> > lifetimes
> > (but we fail to actually terminate them when we drop the associated
> > wakeref).
> > 
> > With respect to the current state (cancelling the work in
> > rpm_suspend),
> > the assert disabling is correct, but I think we should be indicating
> > that we papering over a "bug" more strongly.
> > 
> > i.e. something like DISABLE_RPM_WAKEREF_ASSERT();
> 
> But the other cases are still legitimate, so we'd keep the lower case
> name for those and define the above macro as an alias simply to
> emphasize the difference?

Yes. If you could put it in <blink> tags that would be a bonus.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v5 07/10] drm/i915: add support for checking if we hold an RPM reference

[Imre Deak]

Atm, we assert that the device is not suspended until the point when the
device is truly put to a suspended state. This is fine, but we can catch
more problems if we check that RPM refcount is non-zero. After that one
drops to zero we shouldn't access the device any more, even if the actual
device suspend may be delayed. Change assert_rpm_wakelock_held()
accordingly to check for a non-zero RPM refcount in addition to the
current device-not-suspended check.

For the new asserts to work we need to annotate every place explicitly in
the code where we expect that the device is powered. The places where we
only assume this, but may not hold an RPM reference:
- driver load
  We assume the device to be powered until we enable RPM. Make this
  explicit by taking an RPM reference around the load function.
- system and runtime sudpend/resume handlers
  These handlers are called when the RPM reference becomes 0 and know the
  exact point after which the device can get powered off. Disable the
  RPM-reference-held check for their duration.
- the IRQ, hangcheck and RPS work handlers
  These handlers are flushed in the system/runtime suspend handler
  before the device is powered off, so it's guaranteed that they won't
  run while the device is powered off even though they don't hold any
  RPM reference. Disable the RPM-reference-held check for their duration.

In all these cases we still check that the device is not suspended.
These explicit annotations also have the positive side effect of
documenting our assumptions better.

This caught additional WARNs from the atomic modeset path, those should
be fixed separately.

v2:
- remove the redundant HAS_RUNTIME_PM check (moved to patch 1) (Ville)
v3:
- use a new dedicated RPM wakelock refcount to also catch cases where
  our own RPM get/put functions were not called (Chris)
- assert also that the new RPM wakelock refcount is 0 in the RPM
  suspend handler (Chris)
- change the assert error message to be more meaningful (Chris)
- prevent false assert errors and check that the RPM wakelock is 0 in
  the RPM resume handler too
- prevent false assert errors in the hangcheck work too
- add a device not suspended assert check to the hangcheck work
v4:
- rename disable/enable_rpm_asserts to disable/enable_rpm_wakeref_asserts
  and wakelock_count to wakeref_count
- disable the wakeref asserts in the IRQ handlers and RPS work too
- update/clarify commit message
v5:
- mark places we plan to change to use proper RPM refcounting with
  separate DISABLE/ENABLE_RPM_WAKEREF_ASSERTS aliases (Chris)

Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v3)
---
 drivers/gpu/drm/i915/i915_dma.c         |  7 +++
 drivers/gpu/drm/i915/i915_drv.c         | 39 +++++++++++++++--
 drivers/gpu/drm/i915/i915_drv.h         |  1 +
 drivers/gpu/drm/i915/i915_irq.c         | 75 ++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_drv.h        | 49 +++++++++++++++++++++
 drivers/gpu/drm/i915/intel_pm.c         |  1 +
 drivers/gpu/drm/i915/intel_runtime_pm.c |  6 +++
 7 files changed, 168 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 84e2b20..077d64c 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -896,6 +896,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 
 	intel_pm_setup(dev);
 
+	intel_runtime_pm_get(dev_priv);
+
 	intel_display_crc_init(dev);
 
 	i915_dump_device_info(dev_priv);
@@ -1085,6 +1087,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 
 	i915_audio_component_init(dev_priv);
 
+	intel_runtime_pm_put(dev_priv);
+
 	return 0;
 
 out_power_well:
@@ -1120,6 +1124,9 @@ free_priv:
 	kmem_cache_destroy(dev_priv->requests);
 	kmem_cache_destroy(dev_priv->vmas);
 	kmem_cache_destroy(dev_priv->objects);
+
+	intel_runtime_pm_put(dev_priv);
+
 	kfree(dev_priv);
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 8ddfcce..61267e7 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -580,6 +580,8 @@ static int i915_drm_suspend(struct drm_device *dev)
 	dev_priv->modeset_restore = MODESET_SUSPENDED;
 	mutex_unlock(&dev_priv->modeset_restore_lock);
 
+	disable_rpm_wakeref_asserts(dev_priv);
+
 	/* We do a lot of poking in a lot of registers, make sure they work
 	 * properly. */
 	intel_display_set_init_power(dev_priv, true);
@@ -592,7 +594,7 @@ static int i915_drm_suspend(struct drm_device *dev)
 	if (error) {
 		dev_err(&dev->pdev->dev,
 			"GEM idle failed, resume might fail\n");
-		return error;
+		goto out;
 	}
 
 	intel_guc_suspend(dev);
@@ -635,7 +637,10 @@ static int i915_drm_suspend(struct drm_device *dev)
 	if (HAS_CSR(dev_priv))
 		flush_work(&dev_priv->csr.work);
 
-	return 0;
+out:
+	enable_rpm_wakeref_asserts(dev_priv);
+
+	return error;
 }
 
 static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation)
@@ -644,6 +649,8 @@ static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation)
 	bool fw_csr;
 	int ret;
 
+	disable_rpm_wakeref_asserts(dev_priv);
+
 	fw_csr = suspend_to_idle(dev_priv) && dev_priv->csr.dmc_payload;
 	/*
 	 * In case of firmware assisted context save/restore don't manually
@@ -662,7 +669,7 @@ static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation)
 		if (!fw_csr)
 			intel_power_domains_init_hw(dev_priv, true);
 
-		return ret;
+		goto out;
 	}
 
 	pci_disable_device(drm_dev->pdev);
@@ -683,7 +690,10 @@ static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation)
 
 	dev_priv->suspended_to_idle = suspend_to_idle(dev_priv);
 
-	return 0;
+out:
+	enable_rpm_wakeref_asserts(dev_priv);
+
+	return ret;
 }
 
 int i915_suspend_switcheroo(struct drm_device *dev, pm_message_t state)
@@ -714,6 +724,8 @@ static int i915_drm_resume(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	disable_rpm_wakeref_asserts(dev_priv);
+
 	mutex_lock(&dev->struct_mutex);
 	i915_gem_restore_gtt_mappings(dev);
 	mutex_unlock(&dev->struct_mutex);
@@ -778,6 +790,8 @@ static int i915_drm_resume(struct drm_device *dev)
 
 	drm_kms_helper_poll_enable(dev);
 
+	enable_rpm_wakeref_asserts(dev_priv);
+
 	return 0;
 }
 
@@ -802,6 +816,8 @@ static int i915_drm_resume_early(struct drm_device *dev)
 
 	pci_set_master(dev->pdev);
 
+	disable_rpm_wakeref_asserts(dev_priv);
+
 	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 		ret = vlv_resume_prepare(dev_priv, false);
 	if (ret)
@@ -823,6 +839,8 @@ static int i915_drm_resume_early(struct drm_device *dev)
 out:
 	dev_priv->suspended_to_idle = false;
 
+	enable_rpm_wakeref_asserts(dev_priv);
+
 	return ret;
 }
 
@@ -1455,6 +1473,9 @@ static int intel_runtime_suspend(struct device *device)
 
 		return -EAGAIN;
 	}
+
+	disable_rpm_wakeref_asserts(dev_priv);
+
 	/*
 	 * We are safe here against re-faults, since the fault handler takes
 	 * an RPM reference.
@@ -1474,10 +1495,15 @@ static int intel_runtime_suspend(struct device *device)
 		DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret);
 		intel_runtime_pm_enable_interrupts(dev_priv);
 
+		enable_rpm_wakeref_asserts(dev_priv);
+
 		return ret;
 	}
 
 	intel_uncore_forcewake_reset(dev, false);
+
+	enable_rpm_wakeref_asserts(dev_priv);
+	WARN_ON_ONCE(atomic_read(&dev_priv->pm.wakeref_count));
 	dev_priv->pm.suspended = true;
 
 	/*
@@ -1521,6 +1547,9 @@ static int intel_runtime_resume(struct device *device)
 
 	DRM_DEBUG_KMS("Resuming device\n");
 
+	WARN_ON_ONCE(atomic_read(&dev_priv->pm.wakeref_count));
+	disable_rpm_wakeref_asserts(dev_priv);
+
 	intel_opregion_notify_adapter(dev, PCI_D0);
 	dev_priv->pm.suspended = false;
 
@@ -1555,6 +1584,8 @@ static int intel_runtime_resume(struct device *device)
 
 	intel_enable_gt_powersave(dev);
 
+	enable_rpm_wakeref_asserts(dev_priv);
+
 	if (ret)
 		DRM_ERROR("Runtime resume failed, disabling it (%d)\n", ret);
 	else
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9124085..2894716 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1602,6 +1602,7 @@ struct skl_wm_level {
  * For more, read the Documentation/power/runtime_pm.txt.
  */
 struct i915_runtime_pm {
+	atomic_t wakeref_count;
 	bool suspended;
 	bool irqs_enabled;
 };
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 86664d1..3f8c753 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1103,6 +1103,14 @@ static void gen6_pm_rps_work(struct work_struct *work)
 		spin_unlock_irq(&dev_priv->irq_lock);
 		return;
 	}
+
+	/*
+	 * The RPS work is synced during runtime suspend, we don't require a
+	 * wakeref. TODO: instead of disabling the asserts make sure that we
+	 * always hold an RPM reference while the work is running.
+	 */
+	DISABLE_RPM_WAKEREF_ASSERTS(dev_priv);
+
 	pm_iir = dev_priv->rps.pm_iir;
 	dev_priv->rps.pm_iir = 0;
 	/* Make sure not to corrupt PMIMR state used by ringbuffer on GEN6 */
@@ -1115,7 +1123,7 @@ static void gen6_pm_rps_work(struct work_struct *work)
 	WARN_ON(pm_iir & ~dev_priv->pm_rps_events);
 
 	if ((pm_iir & dev_priv->pm_rps_events) == 0 && !client_boost)
-		return;
+		goto out;
 
 	mutex_lock(&dev_priv->rps.hw_lock);
 
@@ -1170,6 +1178,8 @@ static void gen6_pm_rps_work(struct work_struct *work)
 	intel_set_rps(dev_priv->dev, new_delay);
 
 	mutex_unlock(&dev_priv->rps.hw_lock);
+out:
+	ENABLE_RPM_WAKEREF_ASSERTS(dev_priv);
 }
 
 
@@ -1758,6 +1768,9 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
 	if (!intel_irqs_enabled(dev_priv))
 		return IRQ_NONE;
 
+	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
+	disable_rpm_wakeref_asserts(dev_priv);
+
 	while (true) {
 		/* Find, clear, then process each source of interrupt */
 
@@ -1792,6 +1805,8 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
 	}
 
 out:
+	enable_rpm_wakeref_asserts(dev_priv);
+
 	return ret;
 }
 
@@ -1805,6 +1820,9 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
 	if (!intel_irqs_enabled(dev_priv))
 		return IRQ_NONE;
 
+	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
+	disable_rpm_wakeref_asserts(dev_priv);
+
 	for (;;) {
 		master_ctl = I915_READ(GEN8_MASTER_IRQ) & ~GEN8_MASTER_IRQ_CONTROL;
 		iir = I915_READ(VLV_IIR);
@@ -1835,6 +1853,8 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
 		POSTING_READ(GEN8_MASTER_IRQ);
 	}
 
+	enable_rpm_wakeref_asserts(dev_priv);
+
 	return ret;
 }
 
@@ -2165,6 +2185,9 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 	if (!intel_irqs_enabled(dev_priv))
 		return IRQ_NONE;
 
+	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
+	disable_rpm_wakeref_asserts(dev_priv);
+
 	/* We get interrupts on unclaimed registers, so check for this before we
 	 * do any I915_{READ,WRITE}. */
 	intel_uncore_check_errors(dev);
@@ -2223,6 +2246,9 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 		POSTING_READ(SDEIER);
 	}
 
+	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
+	enable_rpm_wakeref_asserts(dev_priv);
+
 	return ret;
 }
 
@@ -2255,6 +2281,9 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 	if (!intel_irqs_enabled(dev_priv))
 		return IRQ_NONE;
 
+	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
+	disable_rpm_wakeref_asserts(dev_priv);
+
 	if (INTEL_INFO(dev_priv)->gen >= 9)
 		aux_mask |=  GEN9_AUX_CHANNEL_B | GEN9_AUX_CHANNEL_C |
 			GEN9_AUX_CHANNEL_D;
@@ -2262,7 +2291,7 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 	master_ctl = I915_READ_FW(GEN8_MASTER_IRQ);
 	master_ctl &= ~GEN8_MASTER_IRQ_CONTROL;
 	if (!master_ctl)
-		return IRQ_NONE;
+		goto out;
 
 	I915_WRITE_FW(GEN8_MASTER_IRQ, 0);
 
@@ -2393,6 +2422,9 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 	I915_WRITE_FW(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
 	POSTING_READ_FW(GEN8_MASTER_IRQ);
 
+out:
+	enable_rpm_wakeref_asserts(dev_priv);
+
 	return ret;
 }
 
@@ -2989,6 +3021,13 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
 	if (!i915.enable_hangcheck)
 		return;
 
+	/*
+	 * The hangcheck work is synced during runtime suspend, we don't
+	 * require a wakeref. TODO: instead of disabling the asserts make
+	 * sure that we hold a reference when this work is running.
+	 */
+	DISABLE_RPM_WAKEREF_ASSERTS(dev_priv);
+
 	for_each_ring(ring, dev_priv, i) {
 		u64 acthd;
 		u32 seqno;
@@ -3080,13 +3119,18 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
 		}
 	}
 
-	if (rings_hung)
-		return i915_handle_error(dev, true, "Ring hung");
+	if (rings_hung) {
+		i915_handle_error(dev, true, "Ring hung");
+		goto out;
+	}
 
 	if (busy_count)
 		/* Reset timer case chip hangs without another request
 		 * being added */
 		i915_queue_hangcheck(dev);
+
+out:
+	ENABLE_RPM_WAKEREF_ASSERTS(dev_priv);
 }
 
 void i915_queue_hangcheck(struct drm_device *dev)
@@ -3878,13 +3922,18 @@ static irqreturn_t i8xx_irq_handler(int irq, void *arg)
 	u16 flip_mask =
 		I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT |
 		I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT;
+	irqreturn_t ret;
 
 	if (!intel_irqs_enabled(dev_priv))
 		return IRQ_NONE;
 
+	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
+	disable_rpm_wakeref_asserts(dev_priv);
+
+	ret = IRQ_NONE;
 	iir = I915_READ16(IIR);
 	if (iir == 0)
-		return IRQ_NONE;
+		goto out;
 
 	while (iir & ~flip_mask) {
 		/* Can't rely on pipestat interrupt bit in iir as it might
@@ -3933,8 +3982,12 @@ static irqreturn_t i8xx_irq_handler(int irq, void *arg)
 
 		iir = new_iir;
 	}
+	ret = IRQ_HANDLED;
 
-	return IRQ_HANDLED;
+out:
+	enable_rpm_wakeref_asserts(dev_priv);
+
+	return ret;
 }
 
 static void i8xx_irq_uninstall(struct drm_device * dev)
@@ -4063,6 +4116,9 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)
 	if (!intel_irqs_enabled(dev_priv))
 		return IRQ_NONE;
 
+	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
+	disable_rpm_wakeref_asserts(dev_priv);
+
 	iir = I915_READ(IIR);
 	do {
 		bool irq_received = (iir & ~flip_mask) != 0;
@@ -4145,6 +4201,8 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)
 		iir = new_iir;
 	} while (iir & ~flip_mask);
 
+	enable_rpm_wakeref_asserts(dev_priv);
+
 	return ret;
 }
 
@@ -4284,6 +4342,9 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
 	if (!intel_irqs_enabled(dev_priv))
 		return IRQ_NONE;
 
+	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
+	disable_rpm_wakeref_asserts(dev_priv);
+
 	iir = I915_READ(IIR);
 
 	for (;;) {
@@ -4369,6 +4430,8 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
 		iir = new_iir;
 	}
 
+	enable_rpm_wakeref_asserts(dev_priv);
+
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9837a25..2cea88e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1442,8 +1442,57 @@ static inline void
 assert_rpm_wakelock_held(struct drm_i915_private *dev_priv)
 {
 	assert_rpm_device_not_suspended(dev_priv);
+	WARN_ONCE(!atomic_read(&dev_priv->pm.wakeref_count),
+		  "RPM wakelock ref not held during HW access");
 }
 
+/**
+ * disable_rpm_wakeref_asserts - disable the RPM assert checks
+ * @dev_priv: i915 device instance
+ *
+ * This function disable asserts that check if we hold an RPM wakelock
+ * reference, while keeping the device-not-suspended checks still enabled.
+ * It's meant to be used only in special circumstances where our rule about
+ * the wakelock refcount wrt. the device power state doesn't hold. According
+ * to this rule at any point where we access the HW or want to keep the HW in
+ * an active state we must hold an RPM wakelock reference acquired via one of
+ * the intel_runtime_pm_get() helpers. Currently there are a few special spots
+ * where this rule doesn't hold: the IRQ and suspend/resume handlers, the
+ * forcewake release timer, and the GPU RPS and hangcheck works. All other
+ * users should avoid using this function.
+ *
+ * Any calls to this function must have a symmetric call to
+ * enable_rpm_wakeref_asserts().
+ */
+static inline void disable_rpm_wakeref_asserts(struct drm_i915_private *dev_priv)
+{
+	atomic_inc(&dev_priv->pm.wakeref_count);
+}
+
+/**
+ * enable_rpm_wakeref_asserts - re-enable the RPM assert checks
+ * @dev_priv: i915 device instance
+ *
+ * This function re-enables the RPM assert checks after disabling them with
+ * disable_rpm_wakeref_asserts. It's meant to be used only in special
+ * circumstances otherwise its use should be avoided.
+ *
+ * Any calls to this function must have a symmetric call to
+ * disable_rpm_wakeref_asserts().
+ */
+static inline void enable_rpm_wakeref_asserts(struct drm_i915_private *dev_priv)
+{
+	atomic_dec(&dev_priv->pm.wakeref_count);
+}
+
+/* TODO: convert users of these to rely instead on proper RPM refcounting */
+#define DISABLE_RPM_WAKEREF_ASSERTS(dev_priv)	\
+	disable_rpm_wakeref_asserts(dev_priv)
+
+#define ENABLE_RPM_WAKEREF_ASSERTS(dev_priv)	\
+	enable_rpm_wakeref_asserts(dev_priv)
+
+
 void intel_runtime_pm_get(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index f93c4b9..6c08537 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7247,4 +7247,5 @@ void intel_pm_setup(struct drm_device *dev)
 	INIT_LIST_HEAD(&dev_priv->rps.mmioflips.link);
 
 	dev_priv->pm.suspended = false;
+	atomic_set(&dev_priv->pm.wakeref_count, 0);
 }
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 603d410..29157fb 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -2236,6 +2236,8 @@ void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
 	struct device *device = &dev->pdev->dev;
 
 	pm_runtime_get_sync(device);
+
+	atomic_inc(&dev_priv->pm.wakeref_count);
 	assert_rpm_wakelock_held(dev_priv);
 }
 
@@ -2263,6 +2265,8 @@ void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv)
 
 	assert_rpm_wakelock_held(dev_priv);
 	pm_runtime_get_noresume(device);
+
+	atomic_inc(&dev_priv->pm.wakeref_count);
 }
 
 /**
@@ -2278,6 +2282,8 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv)
 	struct drm_device *dev = dev_priv->dev;
 	struct device *device = &dev->pdev->dev;
 
+	atomic_dec(&dev_priv->pm.wakeref_count);
+
 	pm_runtime_mark_last_busy(device);
 	pm_runtime_put_autosuspend(device);
 }
-- 
2.5.0

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

Re: [PATCH 01/10] drm/i915: clarify comment about mandatory RPM put/get during driver load/unload

[Joonas Lahtinen]

Hi,

On ti, 2015-12-15 at 20:10 +0200, Imre Deak wrote:
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 2c2151f..9945040 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1975,9 +1975,15 @@ int intel_power_domains_init(struct
> drm_i915_private *dev_priv)
>   */
>  void intel_power_domains_fini(struct drm_i915_private *dev_priv)
>  {
> -	/* The i915.ko module is still not prepared to be loaded
> when
> +	/*
> +	 * The i915.ko module is still not prepared to be loaded
> when
>  	 * the power well is not enabled, so just enable it in case
> -	 * we're going to unload/reload. */
> +	 * we're going to unload/reload.
+	 * The following also reacquires the RPM reference the core
> passed
> +	 * to the driver during loading, which is dropped in
> +	 * intel_runtime_pm_enable(). We have to hand back the
> control of the
> +	 * device to the core with this reference held.
> +	 */
>  	intel_display_set_init_power(dev_priv, true);
>  
>  	/* Remove the refcount we took to keep power well support
> disabled. */
> @@ -2313,6 +2319,11 @@ void intel_runtime_pm_enable(struct
> drm_i915_private *dev_priv)
>  	pm_runtime_mark_last_busy(device);
>  	pm_runtime_use_autosuspend(device);
>  
> +	/*
> +	 * The core calls the driver load handler with an RPM
> reference held.
> +	 * We drop that here and will reacquire it during unloading
> in
> +	 * intel_power_domains_fini().
> +	 */
>  	pm_runtime_put_autosuspend(device);
>  }
>  
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

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

Re: [PATCH 03/10] drm/i915: refactor RPM disabling due to RC6 being disabled

[Joonas Lahtinen]

On ti, 2015-12-15 at 20:10 +0200, Imre Deak wrote:
> We can make the RPM dependency on RC6 explciit in the code by taking
> an
> actual RPM reference, instead of avoiding to drop the initial one.
> This
> will also enable us to remove the HAS_RUNTIME_PM special casing from
> more places in the next patch.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Comment below.

> ---
>  drivers/gpu/drm/i915/intel_pm.c         | 15 +++++++++++++++
>  drivers/gpu/drm/i915/intel_runtime_pm.c |  9 ---------
>  2 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 99f2642..f93c4b9 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> 
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> 

<SNIP>

> @@ -6025,10 +6035,15 @@ void intel_init_gt_powersave(struct
> drm_device *dev)
>  
>  void intel_cleanup_gt_powersave(struct drm_device *dev)
>  {
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
>  	if (IS_CHERRYVIEW(dev))
>  		return;
>  	else if (IS_VALLEYVIEW(dev))
>  		valleyview_cleanup_gt_powersave(dev);
> +
> +	if (!i915.enable_rc6)
> +		intel_runtime_pm_put(dev_priv);

Although intel_enable_rc6 is used in other places, I think this is fine
due to the variable being initialized in the counter function.

Regards, Joonas

>  }
>  
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

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

Re: [PATCH 04/10] drm/i915: remove HAS_RUNTIME_PM check from RPM get/put/assert helpers

[Joonas Lahtinen]

On ti, 2015-12-15 at 20:10 +0200, Imre Deak wrote:
> We don't really need to check this flag in the get/put/assert
> helpers,
> as on platforms without RPM support we won't ever enable RPM. That
> means
> pm.suspend will be always false and the assert will be always true.
> 
> Do this to simplify the code and to let us extend the RPM asserts to
> all
> platforms for a better coverage.
> 
> Motivated by Ville.
> 
> v2-v3:
> - unchanged
> v4:
> - remove the HAS_RUNTIME_PM check from intel_runtime_pm_enable() too
>   made possible by the previous two patches
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v3)

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 12 ------------
>  drivers/gpu/drm/i915/intel_uncore.c     |  3 +--
>  2 files changed, 1 insertion(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 342baa9..bca7ca7 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -2237,9 +2237,6 @@ void intel_runtime_pm_get(struct
> drm_i915_private *dev_priv)
>  	struct drm_device *dev = dev_priv->dev;
>  	struct device *device = &dev->pdev->dev;
>  
> -	if (!HAS_RUNTIME_PM(dev))
> -		return;
> -
>  	pm_runtime_get_sync(device);
>  	WARN(dev_priv->pm.suspended, "Device still suspended.\n");
>  }
> @@ -2266,9 +2263,6 @@ void intel_runtime_pm_get_noresume(struct
> drm_i915_private *dev_priv)
>  	struct drm_device *dev = dev_priv->dev;
>  	struct device *device = &dev->pdev->dev;
>  
> -	if (!HAS_RUNTIME_PM(dev))
> -		return;
> -
>  	WARN(dev_priv->pm.suspended, "Getting nosync-ref while
> suspended.\n");
>  	pm_runtime_get_noresume(device);
>  }
> @@ -2286,9 +2280,6 @@ void intel_runtime_pm_put(struct
> drm_i915_private *dev_priv)
>  	struct drm_device *dev = dev_priv->dev;
>  	struct device *device = &dev->pdev->dev;
>  
> -	if (!HAS_RUNTIME_PM(dev))
> -		return;
> -
>  	pm_runtime_mark_last_busy(device);
>  	pm_runtime_put_autosuspend(device);
>  }
> @@ -2308,9 +2299,6 @@ void intel_runtime_pm_enable(struct
> drm_i915_private *dev_priv)
>  	struct drm_device *dev = dev_priv->dev;
>  	struct device *device = &dev->pdev->dev;
>  
> -	if (!HAS_RUNTIME_PM(dev))
> -		return;
> -
>  	pm_runtime_set_autosuspend_delay(device, 10000); /* 10s */
>  	pm_runtime_mark_last_busy(device);
>  	pm_runtime_use_autosuspend(device);
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c
> b/drivers/gpu/drm/i915/intel_uncore.c
> index fcf04fe..0226776 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -53,8 +53,7 @@ intel_uncore_forcewake_domain_to_str(const enum
> forcewake_domain_id id)
>  static void
>  assert_device_not_suspended(struct drm_i915_private *dev_priv)
>  {
> -	WARN_ONCE(HAS_RUNTIME_PM(dev_priv->dev) && dev_priv
> ->pm.suspended,
> -		  "Device suspended\n");
> +	WARN_ONCE(dev_priv->pm.suspended, "Device suspended\n");
>  }
>  
>  static inline void
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

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

Re: [PATCH 08/10] drm/i915: check that we hold an RPM wakelock ref before we put it

[Joonas Lahtinen]

On ti, 2015-12-15 at 20:10 +0200, Imre Deak wrote:
> v2-v3:
> - unchanged
> v4:
> - keep the corresponding check in the get helper (Chris)

This is put helper you patched, so I think the above message is
incorrect.

Regards, Joonas

> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 29157fb..82c55a9 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -2282,6 +2282,7 @@ void intel_runtime_pm_put(struct
> drm_i915_private *dev_priv)
>  	struct drm_device *dev = dev_priv->dev;
>  	struct device *device = &dev->pdev->dev;
>  
> +	assert_rpm_wakelock_held(dev_priv);
>  	atomic_dec(&dev_priv->pm.wakeref_count);
>  
>  	pm_runtime_mark_last_busy(device);
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

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

Re: [PATCH 03/10] drm/i915: refactor RPM disabling due to RC6 being disabled

[Joonas Lahtinen]

On ke, 2015-12-16 at 12:54 +0200, Joonas Lahtinen wrote:
> On ti, 2015-12-15 at 20:10 +0200, Imre Deak wrote:
> > We can make the RPM dependency on RC6 explciit in the code by


Typo, s/explciit/explicit/

> > taking
> > an
> > actual RPM reference, instead of avoiding to drop the initial one.
> > This
> > will also enable us to remove the HAS_RUNTIME_PM special casing
> > from
> > more places in the next patch.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> 
> Comment below.
> 
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c         | 15 +++++++++++++++
> >  drivers/gpu/drm/i915/intel_runtime_pm.c |  9 ---------
> >  2 files changed, 15 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index 99f2642..f93c4b9 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > 
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > 
> 
> <SNIP>
> 
> > @@ -6025,10 +6035,15 @@ void intel_init_gt_powersave(struct
> > drm_device *dev)
> >  
> >  void intel_cleanup_gt_powersave(struct drm_device *dev)
> >  {
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> >  	if (IS_CHERRYVIEW(dev))
> >  		return;
> >  	else if (IS_VALLEYVIEW(dev))
> >  		valleyview_cleanup_gt_powersave(dev);
> > +
> > +	if (!i915.enable_rc6)
> > +		intel_runtime_pm_put(dev_priv);
> 
> Although intel_enable_rc6 is used in other places, I think this is
> fine
> due to the variable being initialized in the counter function.
> 
> Regards, Joonas
> 
> >  }
> >  
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

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

Re: [PATCH 09/10] drm/i915: add support for checking RPM atomic sections

[Joonas Lahtinen]

On ti, 2015-12-15 at 20:10 +0200, Imre Deak wrote:
> In some cases we want to check whether we hold an RPM wakelock
> reference
> for the whole duration of a sequence. To achieve this add a new RPM
> atomic sequence
> counter that we increment any time the wakelock refcount drops to
> zero.
> Check whether the sequence number stays the same during the atomic
> section and that we hold the wakelock at the beginning of the
> section.
> 
> Motivated by Chris.
> 
> v2-v3:
> - unchanged
> v4:
> - swap the order of atomic_read() and assert_rpm_wakelock_held() in
>   assert_rpm_atomic_begin() to avoid race
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v3)

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.h         |  1 +
>  drivers/gpu/drm/i915/intel_drv.h        | 17 +++++++++++++++++
>  drivers/gpu/drm/i915/intel_pm.c         |  1 +
>  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 ++-
>  4 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 2894716..00ce627 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1603,6 +1603,7 @@ struct skl_wm_level {
>   */
>  struct i915_runtime_pm {
>  	atomic_t wakeref_count;
> +	atomic_t atomic_seq;
>  	bool suspended;
>  	bool irqs_enabled;
>  };
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index d8e4aca..88d37eb 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1446,6 +1446,23 @@ assert_rpm_wakelock_held(struct
> drm_i915_private *dev_priv)
>  		  "RPM wakelock ref not held during HW access");
>  }
>  
> +static inline int
> +assert_rpm_atomic_begin(struct drm_i915_private *dev_priv)
> +{
> +	int seq = atomic_read(&dev_priv->pm.atomic_seq);
> +
> +	assert_rpm_wakelock_held(dev_priv);
> +
> +	return seq;
> +}
> +
> +static inline void
> +assert_rpm_atomic_end(struct drm_i915_private *dev_priv, int
> begin_seq)
> +{
> +	WARN_ONCE(atomic_read(&dev_priv->pm.atomic_seq) !=
> begin_seq,
> +		 "HW access outside of RPM atomic section\n");
> +}
> +
>  /**
>   * disable_rpm_wakeref_asserts - disable the RPM assert checks
>   * @dev_priv: i915 device instance
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 6c08537..6eb9606 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -7248,4 +7248,5 @@ void intel_pm_setup(struct drm_device *dev)
>  
>  	dev_priv->pm.suspended = false;
>  	atomic_set(&dev_priv->pm.wakeref_count, 0);
> +	atomic_set(&dev_priv->pm.atomic_seq, 0);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 82c55a9..cee54ea 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -2283,7 +2283,8 @@ void intel_runtime_pm_put(struct
> drm_i915_private *dev_priv)
>  	struct device *device = &dev->pdev->dev;
>  
>  	assert_rpm_wakelock_held(dev_priv);
> -	atomic_dec(&dev_priv->pm.wakeref_count);
> +	if (atomic_dec_and_test(&dev_priv->pm.wakeref_count))
> +		atomic_inc(&dev_priv->pm.atomic_seq);
>  
>  	pm_runtime_mark_last_busy(device);
>  	pm_runtime_put_autosuspend(device);
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

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

Re: [PATCH 08/10] drm/i915: check that we hold an RPM wakelock ref before we put it

[Chris Wilson]

On Wed, Dec 16, 2015 at 01:00:38PM +0200, Joonas Lahtinen wrote:
> On ti, 2015-12-15 at 20:10 +0200, Imre Deak wrote:
> > v2-v3:
> > - unchanged
> > v4:
> > - keep the corresponding check in the get helper (Chris)
> 
> This is put helper you patched, so I think the above message is
> incorrect.

It is referring to a chunk no longer in the patch. :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/10] drm/i915: disable power well support on platforms without runtime PM support

[Daniel Vetter]

On Tue, Dec 15, 2015 at 10:57:31PM +0200, Ville Syrjälä wrote:
> On Tue, Dec 15, 2015 at 08:10:30PM +0200, Imre Deak wrote:
> > All platforms with power well support have runtime PM support, so
> > simplify things by explicitly disabling power well support on platforms
> > without runtime PM support. This results in holding the init power domain
> > reference whenever the driver is loaded in addition to an RPM reference,
> > which reflects the reality better and makes it possible to simplify
> > things by removing the HAS_RUNTIME_PM special casing from more places in
> > a follow-up patch.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 9945040..f4ff5f5 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -1908,6 +1908,11 @@ static int
> >  sanitize_disable_power_well_option(const struct drm_i915_private *dev_priv,
> >  				   int disable_power_well)
> >  {
> > +	if (!HAS_RUNTIME_PM(dev_priv)) {
> > +		DRM_DEBUG_KMS("No runtime PM support, disabling display power well support\n");
> > +		return 0;
> > +	}
> 
> Feels a bit too magic to me. Needs a comment at least, otherwise someone
> is going to change disable_power_well back into something you can disable
> at runtime and then all the old stuff might break.
> 
> Grabbing an extra rpm reference explicitly for this purpose might be
> less confusing.

Changing module options that are marked as debug taints your kernel.
Keeping them read-only (so that at least nothing can change at runtime) is
imo much preferred, and it's what I started to require on the gem side.
Really not worth to have all these checks and complexity around for the
off chance some developers want to change the option without reloading the
driver.

But yeah comment might be useful here.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/10] drm/i915: add support for checking RPM atomic sections

[Daniel Vetter]

On Tue, Dec 15, 2015 at 08:10:37PM +0200, Imre Deak wrote:
> In some cases we want to check whether we hold an RPM wakelock reference
> for the whole duration of a sequence. To achieve this add a new RPM atomic sequence
> counter that we increment any time the wakelock refcount drops to zero.
> Check whether the sequence number stays the same during the atomic
> section and that we hold the wakelock at the beginning of the section.
> 
> Motivated by Chris.
> 
> v2-v3:
> - unchanged
> v4:
> - swap the order of atomic_read() and assert_rpm_wakelock_held() in
>   assert_rpm_atomic_begin() to avoid race
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v3)

Can we employ lockdep for some of this? In general lockdep doesn't mesh
with rpm references, since rpm references can be transferred between
processes. And lockdep doesn't like that.

But in many cases, and this one here sounds like one, we don't do that.
And those cases could be annotated/validated with the help of lockdep.
The bonus of lockdep is that we could then also tell lockdep that the rpm
suspend/resume functionsa acquire this lock context, which would catch
bugs like mutex_lock(dev->struct_mutex); before rpm_get().

Just a thought really.
> ---
>  drivers/gpu/drm/i915/i915_drv.h         |  1 +
>  drivers/gpu/drm/i915/intel_drv.h        | 17 +++++++++++++++++
>  drivers/gpu/drm/i915/intel_pm.c         |  1 +
>  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 ++-
>  4 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2894716..00ce627 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1603,6 +1603,7 @@ struct skl_wm_level {
>   */
>  struct i915_runtime_pm {
>  	atomic_t wakeref_count;
> +	atomic_t atomic_seq;
>  	bool suspended;
>  	bool irqs_enabled;
>  };
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d8e4aca..88d37eb 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1446,6 +1446,23 @@ assert_rpm_wakelock_held(struct drm_i915_private *dev_priv)
>  		  "RPM wakelock ref not held during HW access");
>  }
>  
> +static inline int
> +assert_rpm_atomic_begin(struct drm_i915_private *dev_priv)
> +{
> +	int seq = atomic_read(&dev_priv->pm.atomic_seq);
> +
> +	assert_rpm_wakelock_held(dev_priv);
> +
> +	return seq;
> +}
> +
> +static inline void
> +assert_rpm_atomic_end(struct drm_i915_private *dev_priv, int begin_seq)
> +{
> +	WARN_ONCE(atomic_read(&dev_priv->pm.atomic_seq) != begin_seq,
> +		 "HW access outside of RPM atomic section\n");
> +}
> +
>  /**
>   * disable_rpm_wakeref_asserts - disable the RPM assert checks
>   * @dev_priv: i915 device instance
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 6c08537..6eb9606 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -7248,4 +7248,5 @@ void intel_pm_setup(struct drm_device *dev)
>  
>  	dev_priv->pm.suspended = false;
>  	atomic_set(&dev_priv->pm.wakeref_count, 0);
> +	atomic_set(&dev_priv->pm.atomic_seq, 0);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 82c55a9..cee54ea 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -2283,7 +2283,8 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv)
>  	struct device *device = &dev->pdev->dev;
>  
>  	assert_rpm_wakelock_held(dev_priv);
> -	atomic_dec(&dev_priv->pm.wakeref_count);
> +	if (atomic_dec_and_test(&dev_priv->pm.wakeref_count))
> +		atomic_inc(&dev_priv->pm.atomic_seq);
>  
>  	pm_runtime_mark_last_busy(device);
>  	pm_runtime_put_autosuspend(device);
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/10] drm/i915: add assert_rpm_wakelock_held helper

[Chris Wilson]

On Tue, Dec 15, 2015 at 08:10:33PM +0200, Imre Deak wrote:
> As a preparation for follow-up patches add a new helper that checks
> whether we hold an RPM reference, since this is what we want most of
> the cases. Atm this helper will only check for the HW suspended state, a
> follow-up patch will do the actual change to check the refcount instead.
> One exception is the forcewake release timer function, where it's
> guaranteed that the HW is on even though the RPM refcount drops to zero.
> This guarantee is provided by flushing the timer in the runtime suspend
> handler. So leave the assert_device_not_suspended check in place there.
> 
> Also rename assert_device_suspended for consistency and export these
> helpers as a preparation for the follow-up patches.
> 
> No functional change.
> 
> v3:
> - change the assert warning message to be more meaningful (Chris)
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_drv.h    | 14 ++++++++++++++
>  drivers/gpu/drm/i915/intel_uncore.c | 20 +++++++-------------
>  2 files changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 798463e..9837a25 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1430,6 +1430,20 @@ void intel_display_power_get(struct drm_i915_private *dev_priv,
>  			     enum intel_display_power_domain domain);
>  void intel_display_power_put(struct drm_i915_private *dev_priv,
>  			     enum intel_display_power_domain domain);
> +
> +static inline void
> +assert_rpm_device_not_suspended(struct drm_i915_private *dev_priv)
> +{
> +	WARN_ONCE(dev_priv->pm.suspended,
> +		  "Device suspended during HW access\n");
> +}

On irc, Joonas expressed a wish to see all errors during an igt run,
i.e. something like

static inline void
assert_rpm_device_not_suspended(struct drm_i915_private *dev_priv)
{
	WARN(dev_priv->pm.suspended &&
	     atomic_inc_return(&dev_priv->pm.errors) < 0,
		  "Device suspended during HW access\n");
}

with

static int
i915_pm_errors_get(void *data, u64 *val)
{
	struct drm_device *dev = data;

	*val = atomic_read(&to_i915(dev)->pm.erors);
	return 0;
}

static int
i915_pm_errors_set(void *data, u64 val)
{
	struct drm_device *dev = data;

	atomic_set(&to_i915(dev)->pm.errors, val);
	return 0;
}

DEFINE_SIMPLE_ATTRIBUTE(i915_pm_errors_fops,
                        i915_pm_errors_get,
			i915_pm_errors_set,
			"0x%llx\n");


Then users only see the first WARN (and not swamped when it does go
wrong) and igt can echo INT_MIN > /sys/kernel/debug/dri/0/i915_pm_errors
to generate a WARN for each failure (and can even do a quick check for
any errors during a test by reading back i915_pm_errors).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/10] drm/i915: check that we hold an RPM wakelock ref before we put it

[Imre Deak]

On ke, 2015-12-16 at 11:07 +0000, Chris Wilson wrote:
> On Wed, Dec 16, 2015 at 01:00:38PM +0200, Joonas Lahtinen wrote:
> > On ti, 2015-12-15 at 20:10 +0200, Imre Deak wrote:
> > > v2-v3:
> > > - unchanged
> > > v4:
> > > - keep the corresponding check in the get helper (Chris)
> > 
> > This is put helper you patched, so I think the above message is
> > incorrect.
> 
> It is referring to a chunk no longer in the patch. :)

Yes, in v1 I removed an assert from intel_runtime_pm_get(), which was
reinstated in v4:
http://lists.freedesktop.org/archives/intel-gfx/2015-November/080048.html

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

Re: [PATCH 05/10] drm/i915: add assert_rpm_wakelock_held helper

[Imre Deak]

On ke, 2015-12-16 at 12:11 +0000, Chris Wilson wrote:
> On Tue, Dec 15, 2015 at 08:10:33PM +0200, Imre Deak wrote:
> > As a preparation for follow-up patches add a new helper that checks
> > whether we hold an RPM reference, since this is what we want most
> > of
> > the cases. Atm this helper will only check for the HW suspended
> > state, a
> > follow-up patch will do the actual change to check the refcount
> > instead.
> > One exception is the forcewake release timer function, where it's
> > guaranteed that the HW is on even though the RPM refcount drops to
> > zero.
> > This guarantee is provided by flushing the timer in the runtime
> > suspend
> > handler. So leave the assert_device_not_suspended check in place
> > there.
> > 
> > Also rename assert_device_suspended for consistency and export
> > these
> > helpers as a preparation for the follow-up patches.
> > 
> > No functional change.
> > 
> > v3:
> > - change the assert warning message to be more meaningful (Chris)
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h    | 14 ++++++++++++++
> >  drivers/gpu/drm/i915/intel_uncore.c | 20 +++++++-------------
> >  2 files changed, 21 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 798463e..9837a25 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1430,6 +1430,20 @@ void intel_display_power_get(struct
> > drm_i915_private *dev_priv,
> >  			     enum intel_display_power_domain
> > domain);
> >  void intel_display_power_put(struct drm_i915_private *dev_priv,
> >  			     enum intel_display_power_domain
> > domain);
> > +
> > +static inline void
> > +assert_rpm_device_not_suspended(struct drm_i915_private *dev_priv)
> > +{
> > +	WARN_ONCE(dev_priv->pm.suspended,
> > +		  "Device suspended during HW access\n");
> > +}
> 
> On irc, Joonas expressed a wish to see all errors during an igt run,
> i.e. something like
> 
> static inline void
> assert_rpm_device_not_suspended(struct drm_i915_private *dev_priv)
> {
> 	WARN(dev_priv->pm.suspended &&
> 	     atomic_inc_return(&dev_priv->pm.errors) < 0,
> 		  "Device suspended during HW access\n");
> }
> 
> with
> 
> static int
> i915_pm_errors_get(void *data, u64 *val)
> {
> 	struct drm_device *dev = data;
> 
> 	*val = atomic_read(&to_i915(dev)->pm.erors);
> 	return 0;
> }
> 
> static int
> i915_pm_errors_set(void *data, u64 val)
> {
> 	struct drm_device *dev = data;
> 
> 	atomic_set(&to_i915(dev)->pm.errors, val);
> 	return 0;
> }
> 
> DEFINE_SIMPLE_ATTRIBUTE(i915_pm_errors_fops,
>                         i915_pm_errors_get,
> 			i915_pm_errors_set,
> 			"0x%llx\n");
> 
> 
> Then users only see the first WARN (and not swamped when it does go
> wrong) and igt can echo INT_MIN >
> /sys/kernel/debug/dri/0/i915_pm_errors
> to generate a WARN for each failure (and can even do a quick check
> for
> any errors during a test by reading back i915_pm_errors).

Sounds good, we could use this also for other PM related error
reporting.

Are you ok to do this as a follow-up?

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

Re: [PATCH 05/10] drm/i915: add assert_rpm_wakelock_held helper

[Chris Wilson]

On Wed, Dec 16, 2015 at 02:54:43PM +0200, Imre Deak wrote:
> On ke, 2015-12-16 at 12:11 +0000, Chris Wilson wrote:
> > On Tue, Dec 15, 2015 at 08:10:33PM +0200, Imre Deak wrote:
> > > +static inline void
> > > +assert_rpm_device_not_suspended(struct drm_i915_private *dev_priv)
> > > +{
> > > +	WARN_ONCE(dev_priv->pm.suspended,
> > > +		  "Device suspended during HW access\n");
> > > +}
> > 
> > On irc, Joonas expressed a wish to see all errors during an igt run,
> > i.e. something like
> > 
> > static inline void
> > assert_rpm_device_not_suspended(struct drm_i915_private *dev_priv)
> > {
> > 	WARN(dev_priv->pm.suspended &&
> > 	     atomic_inc_return(&dev_priv->pm.errors) < 0,
> > 		  "Device suspended during HW access\n");
> > }
[snip]

> Sounds good, we could use this also for other PM related error
> reporting.
> 
> Are you ok to do this as a follow-up?

Definitely. We haven't changed any behaviour so far, so this is a new
feature.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/10] drm/i915: disable power well support on platforms without runtime PM support

[Imre Deak]

On ke, 2015-12-16 at 12:12 +0100, Daniel Vetter wrote:
> On Tue, Dec 15, 2015 at 10:57:31PM +0200, Ville Syrjälä wrote:
> > On Tue, Dec 15, 2015 at 08:10:30PM +0200, Imre Deak wrote:
> > > All platforms with power well support have runtime PM support, so
> > > simplify things by explicitly disabling power well support on
> > > platforms
> > > without runtime PM support. This results in holding the init
> > > power domain
> > > reference whenever the driver is loaded in addition to an RPM
> > > reference,
> > > which reflects the reality better and makes it possible to
> > > simplify
> > > things by removing the HAS_RUNTIME_PM special casing from more
> > > places in
> > > a follow-up patch.
> > > 
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > index 9945040..f4ff5f5 100644
> > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > @@ -1908,6 +1908,11 @@ static int
> > >  sanitize_disable_power_well_option(const struct drm_i915_private
> > > *dev_priv,
> > >  				   int disable_power_well)
> > >  {
> > > +	if (!HAS_RUNTIME_PM(dev_priv)) {
> > > +		DRM_DEBUG_KMS("No runtime PM support, disabling
> > > display power well support\n");
> > > +		return 0;
> > > +	}
> > 
> > Feels a bit too magic to me. Needs a comment at least, otherwise
> > someone
> > is going to change disable_power_well back into something you can
> > disable
> > at runtime and then all the old stuff might break.
> > 
> > Grabbing an extra rpm reference explicitly for this purpose might
> > be
> > less confusing.
> 
> Changing module options that are marked as debug taints your kernel.
> Keeping them read-only (so that at least nothing can change at
> runtime) is
> imo much preferred, and it's what I started to require on the gem
> side.
> Really not worth to have all these checks and complexity around for
> the
> off chance some developers want to change the option without
> reloading the
> driver.

Well, disabling power well support if the platform doesn't support RPM
is for simplicity, so we don't need to think about different code
paths. What Ville suggested is just for documentation purposes and for
ensuring that RPM won't get re-enabled if somebody decides in the
future to change how the disable_power_well option behaves. I think it
makes sense.

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

Re: [PATCH 05/10] drm/i915: add assert_rpm_wakelock_held helper

[Joonas Lahtinen]

On ke, 2015-12-16 at 13:02 +0000, Chris Wilson wrote:
> On Wed, Dec 16, 2015 at 02:54:43PM +0200, Imre Deak wrote:
> > On ke, 2015-12-16 at 12:11 +0000, Chris Wilson wrote:
> > > On Tue, Dec 15, 2015 at 08:10:33PM +0200, Imre Deak wrote:
> > > > +static inline void
> > > > +assert_rpm_device_not_suspended(struct drm_i915_private
> > > > *dev_priv)
> > > > +{
> > > > +	WARN_ONCE(dev_priv->pm.suspended,
> > > > +		  "Device suspended during HW access\n");
> > > > +}
> > > 
> > > On irc, Joonas expressed a wish to see all errors during an igt
> > > run,
> > > i.e. something like
> > > 
> > > static inline void
> > > assert_rpm_device_not_suspended(struct drm_i915_private
> > > *dev_priv)
> > > {
> > > 	WARN(dev_priv->pm.suspended &&
> > > 	     atomic_inc_return(&dev_priv->pm.errors) < 0,
> > > 		  "Device suspended during HW access\n");
> > > }
> [snip]
> 
> > Sounds good, we could use this also for other PM related error
> > reporting.
> > 
> > Are you ok to do this as a follow-up?
> 
> Definitely. We haven't changed any behaviour so far, so this is a new
> feature.

I'd prefer to currently add it as WARN instead of WARN_ONCE, and then
reduce the message amount with follow-up. This way we'll get more
useful CI results immediately.

Regards, Joonas

> -Chris
> 
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

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

Re: [PATCH 05/10] drm/i915: add assert_rpm_wakelock_held helper

[Imre Deak]

On ke, 2015-12-16 at 15:39 +0200, Joonas Lahtinen wrote:
> On ke, 2015-12-16 at 13:02 +0000, Chris Wilson wrote:
> > On Wed, Dec 16, 2015 at 02:54:43PM +0200, Imre Deak wrote:
> > > On ke, 2015-12-16 at 12:11 +0000, Chris Wilson wrote:
> > > > On Tue, Dec 15, 2015 at 08:10:33PM +0200, Imre Deak wrote:
> > > > > +static inline void
> > > > > +assert_rpm_device_not_suspended(struct drm_i915_private
> > > > > *dev_priv)
> > > > > +{
> > > > > +	WARN_ONCE(dev_priv->pm.suspended,
> > > > > +		  "Device suspended during HW access\n");
> > > > > +}
> > > > 
> > > > On irc, Joonas expressed a wish to see all errors during an igt
> > > > run,
> > > > i.e. something like
> > > > 
> > > > static inline void
> > > > assert_rpm_device_not_suspended(struct drm_i915_private
> > > > *dev_priv)
> > > > {
> > > > 	WARN(dev_priv->pm.suspended &&
> > > > 	     atomic_inc_return(&dev_priv->pm.errors) < 0,
> > > > 		  "Device suspended during HW access\n");
> > > > }
> > [snip]
> > 
> > > Sounds good, we could use this also for other PM related error
> > > reporting.
> > > 
> > > Are you ok to do this as a follow-up?
> > 
> > Definitely. We haven't changed any behaviour so far, so this is a
> > new
> > feature.
> 
> I'd prefer to currently add it as WARN instead of WARN_ONCE, and then
> reduce the message amount with follow-up. This way we'll get more
> useful CI results immediately.

And more annoyed upstream users.. Really adding that knob is a good
idea and it doesn't take long to implement it, but we could make
progress if we did it as a follow-up.

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

[PATCH 4.1/10] drm/i915: get a permanent RPM reference on platforms w/o RPM support

[Imre Deak]

Currently we get a permanent RPM reference if the platform doesn't
support RPM, but this is implicit via the dependency of the power well
functionality on RPM the RPM support, see
sanitize_disable_power_well_option(). Make things more explicit by
taking an RPM reference only for the purpose of keeping RPM disabled.
This provides better documentation and safety against future changes
that would break the above dependency.

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index cee54ea..d7f765a 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1992,6 +1992,13 @@ void intel_power_domains_fini(struct drm_i915_private *dev_priv)
 	/* Remove the refcount we took to keep power well support disabled. */
 	if (!i915.disable_power_well)
 		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
+
+	/*
+	 * Remove the refcount we took in intel_runtime_pm_enable() in case
+	 * the platform doesn't support runtime PM.
+	 */
+	if (!HAS_RUNTIME_PM(dev_priv))
+		intel_runtime_pm_get(dev_priv);
 }
 
 static void intel_power_domains_sync_hw(struct drm_i915_private *dev_priv)
@@ -2305,6 +2312,9 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
 	struct drm_device *dev = dev_priv->dev;
 	struct device *device = &dev->pdev->dev;
 
+	if (!HAS_RUNTIME_PM(dev_priv))
+		intel_runtime_pm_get(dev_priv);
+
 	pm_runtime_set_autosuspend_delay(device, 10000); /* 10s */
 	pm_runtime_mark_last_busy(device);
 	pm_runtime_use_autosuspend(device);
-- 
2.5.0

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

[PATCH v2 4.1/10] drm/i915: get a permanent RPM reference on platforms w/o RPM support

[Imre Deak]

Currently we get a permanent RPM reference if the platform doesn't
support RPM, but this is implicit via the dependency of the power well
functionality on RPM the RPM support, see
sanitize_disable_power_well_option(). Make things more explicit by
taking an RPM reference only for the purpose of keeping RPM disabled.
This provides better documentation and safety against future changes
that would break the above dependency.

v2:
- fix intel_runtime_pm_get vs. intel_runtime_pm_put in
  intel_power_domains_fini()

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index cee54ea..c35f12c 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1992,6 +1992,13 @@ void intel_power_domains_fini(struct drm_i915_private *dev_priv)
 	/* Remove the refcount we took to keep power well support disabled. */
 	if (!i915.disable_power_well)
 		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
+
+	/*
+	 * Remove the refcount we took in intel_runtime_pm_enable() in case
+	 * the platform doesn't support runtime PM.
+	 */
+	if (!HAS_RUNTIME_PM(dev_priv))
+		intel_runtime_pm_put(dev_priv);
 }
 
 static void intel_power_domains_sync_hw(struct drm_i915_private *dev_priv)
@@ -2305,6 +2312,9 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
 	struct drm_device *dev = dev_priv->dev;
 	struct device *device = &dev->pdev->dev;
 
+	if (!HAS_RUNTIME_PM(dev_priv))
+		intel_runtime_pm_get(dev_priv);
+
 	pm_runtime_set_autosuspend_delay(device, 10000); /* 10s */
 	pm_runtime_mark_last_busy(device);
 	pm_runtime_use_autosuspend(device);
-- 
2.5.0

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

Re: [PATCH v2 4.1/10] drm/i915: get a permanent RPM reference on platforms w/o RPM support

[Ville Syrjälä]

On Wed, Dec 16, 2015 at 03:53:32PM +0200, Imre Deak wrote:
> Currently we get a permanent RPM reference if the platform doesn't
> support RPM, but this is implicit via the dependency of the power well
> functionality on RPM the RPM support, see
> sanitize_disable_power_well_option(). Make things more explicit by
> taking an RPM reference only for the purpose of keeping RPM disabled.
> This provides better documentation and safety against future changes
> that would break the above dependency.
> 
> v2:
> - fix intel_runtime_pm_get vs. intel_runtime_pm_put in
>   intel_power_domains_fini()
> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index cee54ea..c35f12c 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1992,6 +1992,13 @@ void intel_power_domains_fini(struct drm_i915_private *dev_priv)
>  	/* Remove the refcount we took to keep power well support disabled. */
>  	if (!i915.disable_power_well)
>  		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> +
> +	/*
> +	 * Remove the refcount we took in intel_runtime_pm_enable() in case
> +	 * the platform doesn't support runtime PM.
> +	 */
> +	if (!HAS_RUNTIME_PM(dev_priv))
> +		intel_runtime_pm_put(dev_priv);
>  }
>  
>  static void intel_power_domains_sync_hw(struct drm_i915_private *dev_priv)
> @@ -2305,6 +2312,9 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
>  	struct drm_device *dev = dev_priv->dev;
>  	struct device *device = &dev->pdev->dev;
>  
> +	if (!HAS_RUNTIME_PM(dev_priv))
> +		intel_runtime_pm_get(dev_priv);
> +
>  	pm_runtime_set_autosuspend_delay(device, 10000); /* 10s */
>  	pm_runtime_mark_last_busy(device);
>  	pm_runtime_use_autosuspend(device);
> -- 
> 2.5.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/10] drm/i915: disable power well support on platforms without runtime PM support

[Ville Syrjälä]

On Wed, Dec 16, 2015 at 03:21:00PM +0200, Imre Deak wrote:
> On ke, 2015-12-16 at 12:12 +0100, Daniel Vetter wrote:
> > On Tue, Dec 15, 2015 at 10:57:31PM +0200, Ville Syrjälä wrote:
> > > On Tue, Dec 15, 2015 at 08:10:30PM +0200, Imre Deak wrote:
> > > > All platforms with power well support have runtime PM support, so
> > > > simplify things by explicitly disabling power well support on
> > > > platforms
> > > > without runtime PM support. This results in holding the init
> > > > power domain
> > > > reference whenever the driver is loaded in addition to an RPM
> > > > reference,
> > > > which reflects the reality better and makes it possible to
> > > > simplify
> > > > things by removing the HAS_RUNTIME_PM special casing from more
> > > > places in
> > > > a follow-up patch.
> > > > 
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > index 9945040..f4ff5f5 100644
> > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > @@ -1908,6 +1908,11 @@ static int
> > > >  sanitize_disable_power_well_option(const struct drm_i915_private
> > > > *dev_priv,
> > > >  				   int disable_power_well)
> > > >  {
> > > > +	if (!HAS_RUNTIME_PM(dev_priv)) {
> > > > +		DRM_DEBUG_KMS("No runtime PM support, disabling
> > > > display power well support\n");
> > > > +		return 0;
> > > > +	}
> > > 
> > > Feels a bit too magic to me. Needs a comment at least, otherwise
> > > someone
> > > is going to change disable_power_well back into something you can
> > > disable
> > > at runtime and then all the old stuff might break.
> > > 
> > > Grabbing an extra rpm reference explicitly for this purpose might
> > > be
> > > less confusing.
> > 
> > Changing module options that are marked as debug taints your kernel.
> > Keeping them read-only (so that at least nothing can change at
> > runtime) is
> > imo much preferred, and it's what I started to require on the gem
> > side.
> > Really not worth to have all these checks and complexity around for
> > the
> > off chance some developers want to change the option without
> > reloading the
> > driver.
> 
> Well, disabling power well support if the platform doesn't support RPM
> is for simplicity, so we don't need to think about different code
> paths. What Ville suggested is just for documentation purposes and for
> ensuring that RPM won't get re-enabled if somebody decides in the
> future to change how the disable_power_well option behaves. I think it
> makes sense.

Not sure about this still. I'm thinking that it might reduce coverage
if we ever add some power domain refcount asserts to some places.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/10] drm/i915: disable power well support on platforms without runtime PM support

[Imre Deak]

On ke, 2015-12-16 at 19:31 +0200, Ville Syrjälä wrote:
> On Wed, Dec 16, 2015 at 03:21:00PM +0200, Imre Deak wrote:
> > On ke, 2015-12-16 at 12:12 +0100, Daniel Vetter wrote:
> > > On Tue, Dec 15, 2015 at 10:57:31PM +0200, Ville Syrjälä wrote:
> > > > On Tue, Dec 15, 2015 at 08:10:30PM +0200, Imre Deak wrote:
> > > > > All platforms with power well support have runtime PM
> > > > > support, so
> > > > > simplify things by explicitly disabling power well support on
> > > > > platforms
> > > > > without runtime PM support. This results in holding the init
> > > > > power domain
> > > > > reference whenever the driver is loaded in addition to an RPM
> > > > > reference,
> > > > > which reflects the reality better and makes it possible to
> > > > > simplify
> > > > > things by removing the HAS_RUNTIME_PM special casing from
> > > > > more
> > > > > places in
> > > > > a follow-up patch.
> > > > > 
> > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 5 +++++
> > > > >  1 file changed, 5 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > index 9945040..f4ff5f5 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > @@ -1908,6 +1908,11 @@ static int
> > > > >  sanitize_disable_power_well_option(const struct
> > > > > drm_i915_private
> > > > > *dev_priv,
> > > > >  				   int disable_power_well)
> > > > >  {
> > > > > +	if (!HAS_RUNTIME_PM(dev_priv)) {
> > > > > +		DRM_DEBUG_KMS("No runtime PM support,
> > > > > disabling
> > > > > display power well support\n");
> > > > > +		return 0;
> > > > > +	}
> > > > 
> > > > Feels a bit too magic to me. Needs a comment at least,
> > > > otherwise
> > > > someone
> > > > is going to change disable_power_well back into something you
> > > > can
> > > > disable
> > > > at runtime and then all the old stuff might break.
> > > > 
> > > > Grabbing an extra rpm reference explicitly for this purpose
> > > > might
> > > > be
> > > > less confusing.
> > > 
> > > Changing module options that are marked as debug taints your
> > > kernel.
> > > Keeping them read-only (so that at least nothing can change at
> > > runtime) is
> > > imo much preferred, and it's what I started to require on the gem
> > > side.
> > > Really not worth to have all these checks and complexity around
> > > for
> > > the
> > > off chance some developers want to change the option without
> > > reloading the
> > > driver.
> > 
> > Well, disabling power well support if the platform doesn't support
> > RPM
> > is for simplicity, so we don't need to think about different code
> > paths. What Ville suggested is just for documentation purposes and
> > for
> > ensuring that RPM won't get re-enabled if somebody decides in the
> > future to change how the disable_power_well option behaves. I think
> > it
> > makes sense.
> 
> Not sure about this still. I'm thinking that it might reduce coverage
> if we ever add some power domain refcount asserts to some places.

Ok, I'll drop this, it was really just for code path simplification
nothing really depends on it.

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

Re: [PATCH v2 4.1/10] drm/i915: get a permanent RPM reference on platforms w/o RPM support

[Imre Deak]

On ke, 2015-12-16 at 19:26 +0200, Ville Syrjälä wrote:
> On Wed, Dec 16, 2015 at 03:53:32PM +0200, Imre Deak wrote:
> > Currently we get a permanent RPM reference if the platform doesn't
> > support RPM, but this is implicit via the dependency of the power
> > well
> > functionality on RPM the RPM support, see
> > sanitize_disable_power_well_option(). Make things more explicit by
> > taking an RPM reference only for the purpose of keeping RPM
> > disabled.
> > This provides better documentation and safety against future
> > changes
> > that would break the above dependency.
> > 
> > v2:
> > - fix intel_runtime_pm_get vs. intel_runtime_pm_put in
> >   intel_power_domains_fini()
> > 
> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> > ---
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index cee54ea..c35f12c 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -1992,6 +1992,13 @@ void intel_power_domains_fini(struct
> > drm_i915_private *dev_priv)
> >  	/* Remove the refcount we took to keep power well support
> > disabled. */
> >  	if (!i915.disable_power_well)
> >  		intel_display_power_put(dev_priv,
> > POWER_DOMAIN_INIT);
> > +
> > +	/*
> > +	 * Remove the refcount we took in
> > intel_runtime_pm_enable() in case
> > +	 * the platform doesn't support runtime PM.
> > +	 */
> > +	if (!HAS_RUNTIME_PM(dev_priv))
> > +		intel_runtime_pm_put(dev_priv);

Ville noticed another issue here, with this we wouldn't have RPM ref
tracking coverage on old platforms. We can solve this by using the low
level pm_runtime_put()/pm_runtime_get_sync() for this instead, will
send a v3 with this changed.

> >  }
> >  
> >  static void intel_power_domains_sync_hw(struct drm_i915_private
> > *dev_priv)
> > @@ -2305,6 +2312,9 @@ void intel_runtime_pm_enable(struct
> > drm_i915_private *dev_priv)
> >  	struct drm_device *dev = dev_priv->dev;
> >  	struct device *device = &dev->pdev->dev;
> >  
> > +	if (!HAS_RUNTIME_PM(dev_priv))
> > +		intel_runtime_pm_get(dev_priv);
> > +
> >  	pm_runtime_set_autosuspend_delay(device, 10000); /* 10s */
> >  	pm_runtime_mark_last_busy(device);
> >  	pm_runtime_use_autosuspend(device);
> > -- 
> > 2.5.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/10] drm/i915: add support for checking RPM atomic sections

[Imre Deak]

On Wed, 2015-12-16 at 12:18 +0100, Daniel Vetter wrote:
> On Tue, Dec 15, 2015 at 08:10:37PM +0200, Imre Deak wrote:
> > In some cases we want to check whether we hold an RPM wakelock
> > reference
> > for the whole duration of a sequence. To achieve this add a new RPM
> > atomic sequence
> > counter that we increment any time the wakelock refcount drops to
> > zero.
> > Check whether the sequence number stays the same during the atomic
> > section and that we hold the wakelock at the beginning of the
> > section.
> > 
> > Motivated by Chris.
> > 
> > v2-v3:
> > - unchanged
> > v4:
> > - swap the order of atomic_read() and assert_rpm_wakelock_held() in
> >   assert_rpm_atomic_begin() to avoid race
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v3)
> 
> Can we employ lockdep for some of this? In general lockdep doesn't
> mesh
> with rpm references, since rpm references can be transferred between
> processes. And lockdep doesn't like that.
> 
> But in many cases, and this one here sounds like one, we don't do
> that.
> And those cases could be annotated/validated with the help of
> lockdep.
> The bonus of lockdep is that we could then also tell lockdep that the
> rpm
> suspend/resume functionsa acquire this lock context, which would
> catch
> bugs like mutex_lock(dev->struct_mutex); before rpm_get().
> 
> Just a thought really.

Yes, as I mentioned I also played with the idea and it could be done by
separating the RPM wakelock users into "prolonged" and short time
users. Yea, couldn't think of a better name. I explained what I mean in
the top three patch of:
https://github.com/ideak/linux/commits/rpm-lockdep-annotations

But I would still need to do more testing with this, making sure the
annotations work as expected, in particular that I call lock_acquire()
with the proper parameters. So until that we could use this simpler
check that could reveal some issues already now.

--Imre

> > ---
> >  drivers/gpu/drm/i915/i915_drv.h         |  1 +
> >  drivers/gpu/drm/i915/intel_drv.h        | 17 +++++++++++++++++
> >  drivers/gpu/drm/i915/intel_pm.c         |  1 +
> >  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 ++-
> >  4 files changed, 21 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 2894716..00ce627 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1603,6 +1603,7 @@ struct skl_wm_level {
> >   */
> >  struct i915_runtime_pm {
> >  	atomic_t wakeref_count;
> > +	atomic_t atomic_seq;
> >  	bool suspended;
> >  	bool irqs_enabled;
> >  };
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index d8e4aca..88d37eb 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1446,6 +1446,23 @@ assert_rpm_wakelock_held(struct
> > drm_i915_private *dev_priv)
> >  		  "RPM wakelock ref not held during HW access");
> >  }
> >  
> > +static inline int
> > +assert_rpm_atomic_begin(struct drm_i915_private *dev_priv)
> > +{
> > +	int seq = atomic_read(&dev_priv->pm.atomic_seq);
> > +
> > +	assert_rpm_wakelock_held(dev_priv);
> > +
> > +	return seq;
> > +}
> > +
> > +static inline void
> > +assert_rpm_atomic_end(struct drm_i915_private *dev_priv, int
> > begin_seq)
> > +{
> > +	WARN_ONCE(atomic_read(&dev_priv->pm.atomic_seq) !=
> > begin_seq,
> > +		 "HW access outside of RPM atomic section\n");
> > +}
> > +
> >  /**
> >   * disable_rpm_wakeref_asserts - disable the RPM assert checks
> >   * @dev_priv: i915 device instance
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index 6c08537..6eb9606 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -7248,4 +7248,5 @@ void intel_pm_setup(struct drm_device *dev)
> >  
> >  	dev_priv->pm.suspended = false;
> >  	atomic_set(&dev_priv->pm.wakeref_count, 0);
> > +	atomic_set(&dev_priv->pm.atomic_seq, 0);
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 82c55a9..cee54ea 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -2283,7 +2283,8 @@ void intel_runtime_pm_put(struct
> > drm_i915_private *dev_priv)
> >  	struct device *device = &dev->pdev->dev;
> >  
> >  	assert_rpm_wakelock_held(dev_priv);
> > -	atomic_dec(&dev_priv->pm.wakeref_count);
> > +	if (atomic_dec_and_test(&dev_priv->pm.wakeref_count))
> > +		atomic_inc(&dev_priv->pm.atomic_seq);
> >  
> >  	pm_runtime_mark_last_busy(device);
> >  	pm_runtime_put_autosuspend(device);
> > -- 
> > 2.5.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3.1/10] drm/i915: get a permanent RPM reference on platforms w/o RPM support

[Imre Deak]

Currently we disable RPM functionality on platforms that doesn't support
this by not putting/getting the RPM reference we receive from the RPM
core during driver loading/unloading respectively. This is somewhat
obscure, so make it more explicit by keeping a reference dedicated for
this particular purpose whenever the driver is loaded. This makes it
possible to remove the HAS_RUNTIME_PM() special casing from every other
places in the next patch.

v2:
- fix intel_runtime_pm_get vs. intel_runtime_pm_put in
  intel_power_domains_fini()
v3:
- take only a low level RPM reference so the ref tracking asserts
  continue to work (Ville)
- update the commit message
- move the patch earlier for bisectability

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index fc7cf2c..bf2492f 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1975,6 +1975,8 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv)
  */
 void intel_power_domains_fini(struct drm_i915_private *dev_priv)
 {
+	struct device *device = &dev_priv->dev->pdev->dev;
+
 	/*
 	 * The i915.ko module is still not prepared to be loaded when
 	 * the power well is not enabled, so just enable it in case
@@ -1989,6 +1991,13 @@ void intel_power_domains_fini(struct drm_i915_private *dev_priv)
 	/* Remove the refcount we took to keep power well support disabled. */
 	if (!i915.disable_power_well)
 		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
+
+	/*
+	 * Remove the refcount we took in intel_runtime_pm_enable() in case
+	 * the platform doesn't support runtime PM.
+	 */
+	if (!HAS_RUNTIME_PM(dev_priv))
+		pm_runtime_put(device);
 }
 
 static void intel_power_domains_sync_hw(struct drm_i915_private *dev_priv)
@@ -2303,8 +2312,14 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
 	struct drm_device *dev = dev_priv->dev;
 	struct device *device = &dev->pdev->dev;
 
+	/*
+	 * Take a permanent reference to disable the RPM functionality and drop
+	 * it only when unloading the driver. Use the low level get/put helpers,
+	 * so the driver's own RPM reference tracking asserts also work on
+	 * platforms without RPM support.
+	 */
 	if (!HAS_RUNTIME_PM(dev))
-		return;
+		pm_runtime_get_sync(device);
 
 	pm_runtime_set_autosuspend_delay(device, 10000); /* 10s */
 	pm_runtime_mark_last_busy(device);
-- 
2.5.0

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

[PATCH v5 04/10] drm/i915: remove HAS_RUNTIME_PM check from RPM get/put/assert helpers

[Imre Deak]

We don't really need to check this flag in the get/put/assert helpers,
as on platforms without RPM support we won't ever enable RPM. That means
pm.suspend will be always false and the assert will be always true.

Do this to simplify the code and to let us extend the RPM asserts to all
platforms for a better coverage.

Motivated by Ville.

v2-v3:
- unchanged
v4:
- remove the HAS_RUNTIME_PM check from intel_runtime_pm_enable() too
  made possible by the previous two patches
v5:
- rebased on the previous new patch in the series that keeps
  HAS_RUNTIME_PM() in intel_runtime_pm_enable() with a permanent
  reference taken there

Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v3)
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> (v4)
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 9 ---------
 drivers/gpu/drm/i915/intel_uncore.c     | 3 +--
 2 files changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index bf2492f..cc0492e 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -2241,9 +2241,6 @@ void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
 	struct drm_device *dev = dev_priv->dev;
 	struct device *device = &dev->pdev->dev;
 
-	if (!HAS_RUNTIME_PM(dev))
-		return;
-
 	pm_runtime_get_sync(device);
 	WARN(dev_priv->pm.suspended, "Device still suspended.\n");
 }
@@ -2270,9 +2267,6 @@ void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv)
 	struct drm_device *dev = dev_priv->dev;
 	struct device *device = &dev->pdev->dev;
 
-	if (!HAS_RUNTIME_PM(dev))
-		return;
-
 	WARN(dev_priv->pm.suspended, "Getting nosync-ref while suspended.\n");
 	pm_runtime_get_noresume(device);
 }
@@ -2290,9 +2284,6 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv)
 	struct drm_device *dev = dev_priv->dev;
 	struct device *device = &dev->pdev->dev;
 
-	if (!HAS_RUNTIME_PM(dev))
-		return;
-
 	pm_runtime_mark_last_busy(device);
 	pm_runtime_put_autosuspend(device);
 }
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index fcf04fe..0226776 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -53,8 +53,7 @@ intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id)
 static void
 assert_device_not_suspended(struct drm_i915_private *dev_priv)
 {
-	WARN_ONCE(HAS_RUNTIME_PM(dev_priv->dev) && dev_priv->pm.suspended,
-		  "Device suspended\n");
+	WARN_ONCE(dev_priv->pm.suspended, "Device suspended\n");
 }
 
 static inline void
-- 
2.5.0

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

Re: [PATCH v5 04/10] drm/i915: remove HAS_RUNTIME_PM check from RPM get/put/assert helpers

[Joonas Lahtinen]

On to, 2015-12-17 at 13:48 +0200, Imre Deak wrote:
> We don't really need to check this flag in the get/put/assert
> helpers,
> as on platforms without RPM support we won't ever enable RPM. That
> means
> pm.suspend will be always false and the assert will be always true.
> 
> Do this to simplify the code and to let us extend the RPM asserts to
> all
> platforms for a better coverage.
> 
> Motivated by Ville.
> 
> v2-v3:
> - unchanged
> v4:
> - remove the HAS_RUNTIME_PM check from intel_runtime_pm_enable() too
>   made possible by the previous two patches
> v5:
> - rebased on the previous new patch in the series that keeps
>   HAS_RUNTIME_PM() in intel_runtime_pm_enable() with a permanent
>   reference taken there
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v3)
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> (v4)

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 9 ---------
>  drivers/gpu/drm/i915/intel_uncore.c     | 3 +--
>  2 files changed, 1 insertion(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index bf2492f..cc0492e 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -2241,9 +2241,6 @@ void intel_runtime_pm_get(struct
> drm_i915_private *dev_priv)
>  	struct drm_device *dev = dev_priv->dev;
>  	struct device *device = &dev->pdev->dev;
>  
> -	if (!HAS_RUNTIME_PM(dev))
> -		return;
> -
>  	pm_runtime_get_sync(device);
>  	WARN(dev_priv->pm.suspended, "Device still suspended.\n");
>  }
> @@ -2270,9 +2267,6 @@ void intel_runtime_pm_get_noresume(struct
> drm_i915_private *dev_priv)
>  	struct drm_device *dev = dev_priv->dev;
>  	struct device *device = &dev->pdev->dev;
>  
> -	if (!HAS_RUNTIME_PM(dev))
> -		return;
> -
>  	WARN(dev_priv->pm.suspended, "Getting nosync-ref while
> suspended.\n");
>  	pm_runtime_get_noresume(device);
>  }
> @@ -2290,9 +2284,6 @@ void intel_runtime_pm_put(struct
> drm_i915_private *dev_priv)
>  	struct drm_device *dev = dev_priv->dev;
>  	struct device *device = &dev->pdev->dev;
>  
> -	if (!HAS_RUNTIME_PM(dev))
> -		return;
> -
>  	pm_runtime_mark_last_busy(device);
>  	pm_runtime_put_autosuspend(device);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c
> b/drivers/gpu/drm/i915/intel_uncore.c
> index fcf04fe..0226776 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -53,8 +53,7 @@ intel_uncore_forcewake_domain_to_str(const enum
> forcewake_domain_id id)
>  static void
>  assert_device_not_suspended(struct drm_i915_private *dev_priv)
>  {
> -	WARN_ONCE(HAS_RUNTIME_PM(dev_priv->dev) && dev_priv
> ->pm.suspended,
> -		  "Device suspended\n");
> +	WARN_ONCE(dev_priv->pm.suspended, "Device suspended\n");
>  }
>  
>  static inline void
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

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

Re: [PATCH v4 00/10] drm/i915: improve the RPM device suspended assert

[Imre Deak]

On ti, 2015-12-15 at 20:10 +0200, Imre Deak wrote:
> This is v4 of [1]. It has the following changes:
> - fix module reload that got broken in v3 due to removal of HAS_RUNTIME_PM 
>   (added patch 1-3 + revised patch 4)
> - disable the wakeref asserts in the IRQ handlers and RPS work too
>   (revised patch 7)

[1] http://lists.freedesktop.org/archives/intel-gfx/2015-November/080005.html

Thanks for all the reviews, I pushed the patchset to dinq.

> Imre Deak (10):
>   drm/i915: clarify comment about mandatory RPM put/get during driver
>     load/unload
>   drm/i915: disable power well support on platforms without runtime PM
>     support
>   drm/i915: refactor RPM disabling due to RC6 being disabled
>   drm/i915: remove HAS_RUNTIME_PM check from RPM get/put/assert helpers
>   drm/i915: add assert_rpm_wakelock_held helper
>   drm/i915: use assert_rpm_wakelock_held instead of opencoding it
>   drm/i915: add support for checking if we hold an RPM reference
>   drm/i915: check that we hold an RPM wakelock ref before we put it
>   drm/i915: add support for checking RPM atomic sections
>   drm/i915: check that we are in an RPM atomic section in GGTT PTE
>     updaters
> 
>  drivers/gpu/drm/i915/i915_dma.c         |  7 ++++
>  drivers/gpu/drm/i915/i915_drv.c         | 39 ++++++++++++++++--
>  drivers/gpu/drm/i915/i915_drv.h         |  2 +
>  drivers/gpu/drm/i915/i915_gem_gtt.c     | 33 +++++++++++++++
>  drivers/gpu/drm/i915/i915_irq.c         | 73 ++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_drv.h        | 72 ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_pm.c         | 17 ++++++++
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 57 ++++++++++++-------------
>  drivers/gpu/drm/i915/intel_uncore.c     | 23 ++++-------
>  9 files changed, 270 insertions(+), 53 deletions(-)
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx