[PATCH v3 0/7] drm/i915: improve the RPM device suspended assert

[PATCH v3 0/7] drm/i915: improve the RPM device suspended assert

[Imre Deak]

This is v3 of [1]. I addressed the review comments from Ville and Chris
and added an RPM atomic section check as well requested by Chris. I was
also considering using lockdep for more coverage, but decided to leave
that out for now, we can also add that later if needed.

The patchset depends on Patrik's DC rework patchset [2].

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

Imre Deak (7):
  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: sanitize the asserts in the RPM get/put helpers
  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         | 12 +++++-
 drivers/gpu/drm/i915/intel_drv.h        | 67 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_pm.c         |  2 +
 drivers/gpu/drm/i915/intel_runtime_pm.c | 24 +++++-------
 drivers/gpu/drm/i915/intel_uncore.c     | 19 +++-------
 9 files changed, 172 insertions(+), 33 deletions(-)

-- 
2.5.0

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

[PATCH v3 1/7] 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.

Signed-off-by: Imre Deak <imre.deak@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 9c4d17d..83ab03a 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -2131,9 +2131,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");
 }
@@ -2160,9 +2157,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);
 }
@@ -2180,9 +2174,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 5bb269c..6560272 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 v3 2/7] 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>
---
 drivers/gpu/drm/i915/intel_drv.h    | 14 ++++++++++++++
 drivers/gpu/drm/i915/intel_uncore.c | 16 +++++-----------
 2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0b86674..fabf639 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1427,6 +1427,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 6560272..01923e0 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); \
@@ -668,7 +662,7 @@ __gen2_read(64)
 #define GEN6_READ_HEADER(x) \
 	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 \
@@ -813,7 +807,7 @@ __gen6_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
 
@@ -852,7 +846,7 @@ __gen2_write(64)
 #define GEN6_WRITE_HEADER \
 	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 \
-- 
2.5.0

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

[PATCH v3 3/7] 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> (v1)
---
 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 83ab03a..3d7ddc3 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -464,8 +464,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);
 }
@@ -479,8 +478,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)
@@ -2132,7 +2130,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);
 }
 
 /**
@@ -2157,7 +2155,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 01923e0..bd3d101 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 v3 4/7] 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
HW is truly put to a suspended state. This is fine, but we can catch
more problems if we check the RPM refcount. After that one drops to zero
we shouldn't access the HW any more, although the actual suspend may be
delayed. Based on this change the assert_rpm_wakelock_held helper to
check the refcount instead of the device suspended state.

After this change we need to annotate every place explicitly in the code
where we expect that the HW is in D0 state. Atm in the driver load
function the D0 state is implicit until we enable runtime PM, but for
these asserts to work we need to add explicit RPM get/put calls, so fix
this up.

Another place where the D0 state is implicit even with a 0 RPM refcount
is the system and runtime sudpend/resume handlers and the hangcheck
work. In the former case the susend/resume handlers themselves determine
at which exact spot the HW is truly on/off and in the latter case the
work will be flushed in the suspend handler before turning off the HW.
We regard these cases special and disable the RPM asserts for their
duration. In the hangcheck work we can nevertheless check that the
device is not suspended. Fix up these things.

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

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 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         | 12 ++++++++--
 drivers/gpu/drm/i915/intel_drv.h        | 39 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_pm.c         |  1 +
 drivers/gpu/drm/i915/intel_runtime_pm.c |  6 +++++
 7 files changed, 99 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index a7289be..780cbcd 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 77d183d..24d21bf 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -636,6 +636,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_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);
@@ -648,7 +650,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);
@@ -695,7 +697,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_asserts(dev_priv);
+
+	return error;
 }
 
 static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation)
@@ -703,6 +708,8 @@ static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation)
 	struct drm_i915_private *dev_priv = drm_dev->dev_private;
 	int ret;
 
+	disable_rpm_asserts(dev_priv);
+
 	intel_power_domains_suspend(dev_priv);
 
 	ret = intel_suspend_complete(dev_priv);
@@ -710,7 +717,7 @@ static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation)
 	if (ret) {
 		DRM_ERROR("Suspend complete failed: %d\n", ret);
 
-		return ret;
+		goto out;
 	}
 
 	pci_disable_device(drm_dev->pdev);
@@ -729,7 +736,10 @@ static int i915_drm_suspend_late(struct drm_device *drm_dev, bool hibernation)
 	if (!(hibernation && INTEL_INFO(dev_priv)->gen < 6))
 		pci_set_power_state(drm_dev->pdev, PCI_D3hot);
 
-	return 0;
+out:
+	enable_rpm_asserts(dev_priv);
+
+	return ret;
 }
 
 int i915_suspend_switcheroo(struct drm_device *dev, pm_message_t state)
@@ -760,6 +770,8 @@ static int i915_drm_resume(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	disable_rpm_asserts(dev_priv);
+
 	mutex_lock(&dev->struct_mutex);
 	i915_gem_restore_gtt_mappings(dev);
 	mutex_unlock(&dev->struct_mutex);
@@ -824,6 +836,8 @@ static int i915_drm_resume(struct drm_device *dev)
 
 	drm_kms_helper_poll_enable(dev);
 
+	enable_rpm_asserts(dev_priv);
+
 	return 0;
 }
 
@@ -846,6 +860,8 @@ static int i915_drm_resume_early(struct drm_device *dev)
 
 	pci_set_master(dev->pdev);
 
+	disable_rpm_asserts(dev_priv);
+
 	if (IS_VALLEYVIEW(dev_priv))
 		ret = vlv_resume_prepare(dev_priv, false);
 	if (ret)
@@ -862,6 +878,8 @@ static int i915_drm_resume_early(struct drm_device *dev)
 	intel_uncore_sanitize(dev);
 	intel_power_domains_init_hw(dev_priv, true);
 
+	enable_rpm_asserts(dev_priv);
+
 	return ret;
 }
 
@@ -1494,6 +1512,9 @@ static int intel_runtime_suspend(struct device *device)
 
 		return -EAGAIN;
 	}
+
+	disable_rpm_asserts(dev_priv);
+
 	/*
 	 * We are safe here against re-faults, since the fault handler takes
 	 * an RPM reference.
@@ -1511,11 +1532,16 @@ 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_asserts(dev_priv);
+
 		return ret;
 	}
 
 	cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
 	intel_uncore_forcewake_reset(dev, false);
+
+	enable_rpm_asserts(dev_priv);
+	WARN_ON_ONCE(atomic_read(&dev_priv->pm.wakelock_count));
 	dev_priv->pm.suspended = true;
 
 	/*
@@ -1559,6 +1585,9 @@ static int intel_runtime_resume(struct device *device)
 
 	DRM_DEBUG_KMS("Resuming device\n");
 
+	WARN_ON_ONCE(atomic_read(&dev_priv->pm.wakelock_count));
+	disable_rpm_asserts(dev_priv);
+
 	intel_opregion_notify_adapter(dev, PCI_D0);
 	dev_priv->pm.suspended = false;
 
@@ -1593,6 +1622,8 @@ static int intel_runtime_resume(struct device *device)
 
 	intel_enable_gt_powersave(dev);
 
+	enable_rpm_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 5628c5a..658cb9b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1599,6 +1599,7 @@ struct skl_wm_level {
  * For more, read the Documentation/power/runtime_pm.txt.
  */
 struct i915_runtime_pm {
+	atomic_t wakelock_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 825114a..ee3ef69 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2962,6 +2962,9 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
 	if (!i915.enable_hangcheck)
 		return;
 
+	assert_rpm_device_not_suspended(dev_priv);
+	disable_rpm_asserts(dev_priv);
+
 	for_each_ring(ring, dev_priv, i) {
 		u64 acthd;
 		u32 seqno;
@@ -3053,13 +3056,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_asserts(dev_priv);
 }
 
 void i915_queue_hangcheck(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index fabf639..ee403d7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1439,6 +1439,45 @@ 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.wakelock_count),
+		  "RPM wakelock not held during HW access");
+}
+
+/**
+ * disable_rpm_asserts - disable the RPM assert checks
+ * @dev_priv: i915 device instance
+ *
+ * This function disables all the RPM assert checks. It's meant to be used
+ * only in special circumstances where our rule about the RPM 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 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 suspend/resume handlers, the forcewake release timer, and the
+ * GPU hangcheck work. All other users should avoid using this function.
+ *
+ * Any calls to this function must have a symmetric call to
+ * enable_rpm_asserts().
+ */
+static inline void disable_rpm_asserts(struct drm_i915_private *dev_priv)
+{
+	atomic_inc(&dev_priv->pm.wakelock_count);
+}
+
+/**
+ * enable_rpm_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_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_asserts().
+ */
+static inline void enable_rpm_asserts(struct drm_i915_private *dev_priv)
+{
+	atomic_dec(&dev_priv->pm.wakelock_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 647c0ff..6d74d32 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7214,4 +7214,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.wakelock_count, 0);
 }
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 3d7ddc3..64da5af 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -2130,6 +2130,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.wakelock_count);
 	assert_rpm_wakelock_held(dev_priv);
 }
 
@@ -2157,6 +2159,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.wakelock_count);
 }
 
 /**
@@ -2172,6 +2176,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.wakelock_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 v3 5/7] drm/i915: sanitize the asserts in the RPM get/put helpers

[Imre Deak]

There is no point in checking the refcount just after increasing it so
remove the assert from the get helper. Otoh, we should check the
refcount before decreasing it, so add it to the put helper.

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

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 64da5af..db63b8a 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -2132,7 +2132,6 @@ void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
 	pm_runtime_get_sync(device);
 
 	atomic_inc(&dev_priv->pm.wakelock_count);
-	assert_rpm_wakelock_held(dev_priv);
 }
 
 /**
@@ -2176,6 +2175,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.wakelock_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 v3 6/7] 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.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |  1 +
 drivers/gpu/drm/i915/intel_drv.h        | 14 ++++++++++++++
 drivers/gpu/drm/i915/intel_pm.c         |  1 +
 drivers/gpu/drm/i915/intel_runtime_pm.c |  3 ++-
 4 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 658cb9b..c265d47 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1600,6 +1600,7 @@ struct skl_wm_level {
  */
 struct i915_runtime_pm {
 	atomic_t wakelock_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 ee403d7..a884dc7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1443,6 +1443,20 @@ assert_rpm_wakelock_held(struct drm_i915_private *dev_priv)
 		  "RPM wakelock not held during HW access");
 }
 
+static inline int
+assert_rpm_atomic_begin(struct drm_i915_private *dev_priv)
+{
+	assert_rpm_wakelock_held(dev_priv);
+	return atomic_read(&dev_priv->pm.atomic_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_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 6d74d32..2390237 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7215,4 +7215,5 @@ void intel_pm_setup(struct drm_device *dev)
 
 	dev_priv->pm.suspended = false;
 	atomic_set(&dev_priv->pm.wakelock_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 db63b8a..69375a9 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -2176,7 +2176,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.wakelock_count);
+	if (atomic_dec_and_test(&dev_priv->pm.wakelock_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 v3 7/7] 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>
---
 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 016739e..b9cd332 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2357,6 +2357,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) +
@@ -2383,6 +2386,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);
 }
 
 /*
@@ -2403,6 +2408,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);
@@ -2427,6 +2435,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,
@@ -2441,6 +2451,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",
@@ -2453,6 +2466,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,
@@ -2467,6 +2482,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",
@@ -2479,6 +2497,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,
@@ -2486,11 +2506,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,
@@ -2498,9 +2524,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 v3 4/7] drm/i915: add support for checking if we hold an RPM reference

[Chris Wilson]

On Thu, Nov 12, 2015 at 06:40:18PM +0200, Imre Deak wrote:
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 825114a..ee3ef69 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2962,6 +2962,9 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
>  	if (!i915.enable_hangcheck)
>  		return;
>  
> +	assert_rpm_device_not_suspended(dev_priv);
> +	disable_rpm_asserts(dev_priv);
> +
>  	for_each_ring(ring, dev_priv, i) {
>  		u64 acthd;
>  		u32 seqno;
> @@ -3053,13 +3056,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_asserts(dev_priv);

Nice catch!

Since the rpm wakelock here is covered by
intel_mark_busy/intel_mark_idle(), we should be able to do something
like:

if (!intel_runtime_pm_tryget()
	return;

where intel_runtime_pm_tryget does something like
atomic_inc_unless_zero().

Is something like that possible?

As it stands since we don't actually cancel the hangcheck when we drop
the rpm wakelock in intel_mark_idle() it can very well come to pass that
we execute this whilst the device is asleep. However, if the device is
alseep, we now that we are no longer executing.
-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 v3 4/7] drm/i915: add support for checking if we hold an RPM reference

[Imre Deak]

On to, 2015-11-12 at 17:04 +0000, Chris Wilson wrote:
> On Thu, Nov 12, 2015 at 06:40:18PM +0200, Imre Deak wrote:
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > b/drivers/gpu/drm/i915/i915_irq.c
> > index 825114a..ee3ef69 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -2962,6 +2962,9 @@ static void i915_hangcheck_elapsed(struct
> > work_struct *work)
> >  	if (!i915.enable_hangcheck)
> >  		return;
> >  
> > +	assert_rpm_device_not_suspended(dev_priv);
> > +	disable_rpm_asserts(dev_priv);
> > +
> >  	for_each_ring(ring, dev_priv, i) {
> >  		u64 acthd;
> >  		u32 seqno;
> > @@ -3053,13 +3056,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_asserts(dev_priv);
> 
> Nice catch!

It triggered the new assert easily just before going to runtime
suspend..

> Since the rpm wakelock here is covered by
> intel_mark_busy/intel_mark_idle(), we should be able to do something
> like:
> 
> if (!intel_runtime_pm_tryget()
> 	return;
> 
> where intel_runtime_pm_tryget does something like
> atomic_inc_unless_zero().
> 
> Is something like that possible?

Yea, I could add this, but I'd like to better understand the need, see
below.

> As it stands since we don't actually cancel the hangcheck when we
drop
> the rpm wakelock in intel_mark_idle() it can very well come to pass
> that
> we execute this whilst the device is asleep. However, if the device
> is
> alseep, we now that we are no longer executing.

But how could this run while asleep, since we flush the work in the
runtime suspend handler before turning off the HW? But even if it can't
run your idea would be clearer imo..

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

Re: [PATCH v3 4/7] drm/i915: add support for checking if we hold an RPM reference

[Chris Wilson]

On Thu, Nov 12, 2015 at 07:50:09PM +0200, Imre Deak wrote:
> On to, 2015-11-12 at 17:04 +0000, Chris Wilson wrote:
> > As it stands since we don't actually cancel the hangcheck when we
> drop
> > the rpm wakelock in intel_mark_idle() it can very well come to pass
> > that
> > we execute this whilst the device is asleep. However, if the device
> > is
> > alseep, we now that we are no longer executing.
> 
> But how could this run while asleep, since we flush the work in the
> runtime suspend handler before turning off the HW? But even if it can't
> run your idea would be clearer imo..

We shouldn't be flushing the hangcheck worker there. That's a leaky
abstraction attempting to paper over something that shouldn't have been
a problem in the first place. Note that there is a similar issue with
the existing request waiters that currently may race against
intel_mark_idle().
-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 v3 4/7] drm/i915: add support for checking if we hold an RPM reference

[Imre Deak]

On Thu, 2015-11-12 at 20:41 +0000, Chris Wilson wrote:
> On Thu, Nov 12, 2015 at 07:50:09PM +0200, Imre Deak wrote:
> > On to, 2015-11-12 at 17:04 +0000, Chris Wilson wrote:
> > > As it stands since we don't actually cancel the hangcheck when we
> > drop
> > > the rpm wakelock in intel_mark_idle() it can very well come to pass
> > > that
> > > we execute this whilst the device is asleep. However, if the device
> > > is
> > > alseep, we now that we are no longer executing.
> > 
> > But how could this run while asleep, since we flush the work in the
> > runtime suspend handler before turning off the HW? But even if it can't
> > run your idea would be clearer imo..
> 
> We shouldn't be flushing the hangcheck worker there. That's a leaky
> abstraction attempting to paper over something that shouldn't have been
> a problem in the first place. Note that there is a similar issue with
> the existing request waiters that currently may race against
> intel_mark_idle().

Ok. I think your idea is an improvement, but it will change
functionality, so how about doing it as a follow-up?

--Imre

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

Re: [PATCH v3 4/7] drm/i915: add support for checking if we hold an RPM reference

[Chris Wilson]

On Thu, Nov 12, 2015 at 10:49:29PM +0200, Imre Deak wrote:
> On Thu, 2015-11-12 at 20:41 +0000, Chris Wilson wrote:
> > On Thu, Nov 12, 2015 at 07:50:09PM +0200, Imre Deak wrote:
> > > On to, 2015-11-12 at 17:04 +0000, Chris Wilson wrote:
> > > > As it stands since we don't actually cancel the hangcheck when we
> > > drop
> > > > the rpm wakelock in intel_mark_idle() it can very well come to pass
> > > > that
> > > > we execute this whilst the device is asleep. However, if the device
> > > > is
> > > > alseep, we now that we are no longer executing.
> > > 
> > > But how could this run while asleep, since we flush the work in the
> > > runtime suspend handler before turning off the HW? But even if it can't
> > > run your idea would be clearer imo..
> > 
> > We shouldn't be flushing the hangcheck worker there. That's a leaky
> > abstraction attempting to paper over something that shouldn't have been
> > a problem in the first place. Note that there is a similar issue with
> > the existing request waiters that currently may race against
> > intel_mark_idle().
> 
> Ok. I think your idea is an improvement, but it will change
> functionality, so how about doing it as a follow-up?

Yes, since intel_runtime_suspend() already has a mechanism that should
avoid the assert, relaxing that assert and removing that bit of
unnecessary work from the rpm suspend path can be done separately. (I
hadn't realised that cancel_work had snuck in there.)
-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 v3 1/7] drm/i915: remove HAS_RUNTIME_PM check from RPM get/put/assert helpers

[Chris Wilson]

On Thu, Nov 12, 2015 at 06:40:15PM +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.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-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 v3 2/7] drm/i915: add assert_rpm_wakelock_held helper

[Chris Wilson]

On Thu, Nov 12, 2015 at 06:40:16PM +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>
-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 v3 3/7] drm/i915: use assert_rpm_wakelock_held instead of opencoding it

[Chris Wilson]

On Thu, Nov 12, 2015 at 06:40:17PM +0200, Imre Deak wrote:
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v1)
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-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 v3 4/7] drm/i915: add support for checking if we hold an RPM reference

[Chris Wilson]

On Thu, Nov 12, 2015 at 06:40:18PM +0200, Imre Deak wrote:
> Atm, we assert that the device is not suspended until the point when the
> HW is truly put to a suspended state. This is fine, but we can catch
> more problems if we check the RPM refcount. After that one drops to zero
> we shouldn't access the HW any more, although the actual suspend may be
> delayed. Based on this change the assert_rpm_wakelock_held helper to
> check the refcount instead of the device suspended state.
> 
> After this change we need to annotate every place explicitly in the code
> where we expect that the HW is in D0 state. Atm in the driver load
> function the D0 state is implicit until we enable runtime PM, but for
> these asserts to work we need to add explicit RPM get/put calls, so fix
> this up.
> 
> Another place where the D0 state is implicit even with a 0 RPM refcount
> is the system and runtime sudpend/resume handlers and the hangcheck
> work. In the former case the susend/resume handlers themselves determine
> at which exact spot the HW is truly on/off and in the latter case the
> work will be flushed in the suspend handler before turning off the HW.
> We regard these cases special and disable the RPM asserts for their
> duration. In the hangcheck work we can nevertheless check that the
> device is not suspended. Fix up these things.
> 
> 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
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-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 v3 5/7] drm/i915: sanitize the asserts in the RPM get/put helpers

[Chris Wilson]

On Thu, Nov 12, 2015 at 06:40:19PM +0200, Imre Deak wrote:
> There is no point in checking the refcount just after increasing it so
> remove the assert from the get helper. Otoh, we should check the
> refcount before decreasing it, so add it to the put helper.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 64da5af..db63b8a 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -2132,7 +2132,6 @@ void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
>  	pm_runtime_get_sync(device);
>  
>  	atomic_inc(&dev_priv->pm.wakelock_count);
> -	assert_rpm_wakelock_held(dev_priv);

Otoh, assert_device_not_suspended() still makes sense here.
-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 v3 7/7] drm/i915: check that we are in an RPM atomic section in GGTT PTE updaters

[Chris Wilson]

On Thu, Nov 12, 2015 at 06:40:21PM +0200, Imre Deak wrote:
> 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>
For this and its companion patch 6/7,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-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 v3 0/7] drm/i915: improve the RPM device suspended assert

[Jani Nikula]

On Thu, 12 Nov 2015, Imre Deak <imre.deak@intel.com> wrote:
> This is v3 of [1]. I addressed the review comments from Ville and Chris
> and added an RPM atomic section check as well requested by Chris. I was
> also considering using lockdep for more coverage, but decided to leave
> that out for now, we can also add that later if needed.
>
> The patchset depends on Patrik's DC rework patchset [2].

Uh-oh, so now we have a reviewed series, except it depends on a series
that isn't ready, which has its own dependencies, and I've lost track of
all the series in flight by now... Imre, I trust you to move the review
of the dependencies forward, pinging people as necessary.

BR,
Jani.



>
> [1] http://lists.freedesktop.org/archives/intel-gfx/2015-November/079777.html
> [2]
> http://lists.freedesktop.org/archives/intel-gfx/2015-November/079758.html
>
> Imre Deak (7):
>   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: sanitize the asserts in the RPM get/put helpers
>   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         | 12 +++++-
>  drivers/gpu/drm/i915/intel_drv.h        | 67 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_pm.c         |  2 +
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 24 +++++-------
>  drivers/gpu/drm/i915/intel_uncore.c     | 19 +++-------
>  9 files changed, 172 insertions(+), 33 deletions(-)
>
> -- 
> 2.5.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 5/7] 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 02b390e..44b0575 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -2185,6 +2185,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.wakelock_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 v4 6/7] 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>
---
 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 658cb9b..c265d47 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1600,6 +1600,7 @@ struct skl_wm_level {
  */
 struct i915_runtime_pm {
 	atomic_t wakelock_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 ee403d7..9e975f3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1443,6 +1443,23 @@ assert_rpm_wakelock_held(struct drm_i915_private *dev_priv)
 		  "RPM wakelock 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_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 6d74d32..2390237 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7215,4 +7215,5 @@ void intel_pm_setup(struct drm_device *dev)
 
 	dev_priv->pm.suspended = false;
 	atomic_set(&dev_priv->pm.wakelock_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 44b0575..4334758 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -2186,7 +2186,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.wakelock_count);
+	if (atomic_dec_and_test(&dev_priv->pm.wakelock_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

Re: [PATCH v3 0/7] drm/i915: improve the RPM device suspended assert

[Imre Deak]

On pe, 2015-11-13 at 10:52 +0200, Jani Nikula wrote:
> On Thu, 12 Nov 2015, Imre Deak <imre.deak@intel.com> wrote:
> > This is v3 of [1]. I addressed the review comments from Ville and
> > Chris
> > and added an RPM atomic section check as well requested by Chris. I
> > was
> > also considering using lockdep for more coverage, but decided to
> > leave
> > that out for now, we can also add that later if needed.
> > 
> > The patchset depends on Patrik's DC rework patchset [2].
> 
> Uh-oh, so now we have a reviewed series, except it depends on a
> series
> that isn't ready, which has its own dependencies, and I've lost track
> of
> all the series in flight by now... Imre, I trust you to move the
> review
> of the dependencies forward, pinging people as necessary.

Yes, there are the two DC/DMC patchsets you mentioned that would need
to get merged before this one, they are being reviewed atm. There is no
functional dependency, I only want to avoid merge conflicts. For
reference and if someone wants to experiment with it there is the
following branch with all 3 patchsets:

https://github.com/ideak/linux/commits/rpm-assert-improvements

--Imre

> 
> BR,
> Jani.
> 
> 
> 
> > 
> > [1] http://lists.freedesktop.org/archives/intel-gfx/2015-November/0
> > 79777.html
> > [2]
> > http://lists.freedesktop.org/archives/intel-gfx/2015-November/07975
> > 8.html
> > 
> > Imre Deak (7):
> >   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: sanitize the asserts in the RPM get/put helpers
> >   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         | 12 +++++-
> >  drivers/gpu/drm/i915/intel_drv.h        | 67
> > +++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_pm.c         |  2 +
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 24 +++++-------
> >  drivers/gpu/drm/i915/intel_uncore.c     | 19 +++-------
> >  9 files changed, 172 insertions(+), 33 deletions(-)
> > 
> > -- 
> > 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