[03/10] drm/i915: Add support for asynchronous display power disabling

Submitted by Imre Deak on May 2, 2019, 11:26 p.m.

Details

Message ID 20190502232648.4450-4-imre.deak@intel.com
State New
Headers show
Series "drm/i915: Add support for asynchronous display power disabling" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Imre Deak May 2, 2019, 11:26 p.m.
By disabling a power domain asynchronously we can restrict holding a
reference on that power domain to the actual code sequence that
requires the power to be on for the HW access it's doing, by also
avoiding unneeded on-off-on togglings of the power domain (since the
disabling happens with a delay).

One benefit is potential power saving if the delay is chosen properly.

In the case of the AUX power domain holding the reference on the domain
for the minimal amount of time at defined spots is also a requirement:
on ICL we need a stricter control of when either kind of AUX power
domain (TBT-alt or DP-alt) can be enabled and the locking we need for
that becomes problematic (due to dependencies on other locks) if we
allow the reference to be held for arbitrarily long periods/places in
the code.

I chose the disabling delay to be 100msec for now to avoid the unneeded
toggling (and so not to introduce dmesg spamming) in the DP MST sideband
signaling code. We could optimize this delay later.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |   5 +
 drivers/gpu/drm/i915/intel_runtime_pm.c | 316 +++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_runtime_pm.h |   4 +
 3 files changed, 315 insertions(+), 10 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9fb26634a6be..53a6b0da3571 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -839,6 +839,11 @@  struct i915_power_domains {
 
 	struct mutex lock;
 	int domain_use_count[POWER_DOMAIN_NUM];
+
+	struct delayed_work async_put_work;
+	intel_wakeref_t async_put_wakeref;
+	u64 async_put_domains[2];
+
 	struct i915_power_well *power_wells;
 };
 
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index cc45cbcb43cb..bc0693e3614e 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1883,6 +1883,130 @@  static void chv_pipe_power_well_disable(struct drm_i915_private *dev_priv,
 	chv_set_pipe_power_well(dev_priv, power_well, false);
 }
 
+static intel_wakeref_t
+intel_runtime_pm_get_raw(struct drm_i915_private *i915);
+static void
+intel_runtime_pm_put_raw(struct drm_i915_private *i915, intel_wakeref_t wref);
+
+static u64 __async_put_domains_mask(struct i915_power_domains *power_domains)
+{
+	return power_domains->async_put_domains[0] |
+	       power_domains->async_put_domains[1];
+}
+
+#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
+
+static bool
+assert_async_put_domain_masks_disjoint(struct i915_power_domains *power_domains)
+{
+	return !WARN_ON(power_domains->async_put_domains[0] &
+			power_domains->async_put_domains[1]);
+}
+
+static bool
+__async_put_domains_state_ok(struct i915_power_domains *power_domains)
+{
+	enum intel_display_power_domain domain;
+	bool err = false;
+
+	err |= !assert_async_put_domain_masks_disjoint(power_domains);
+	err |= WARN_ON(!!power_domains->async_put_wakeref !=
+		       !!__async_put_domains_mask(power_domains));
+
+	for_each_power_domain(domain, __async_put_domains_mask(power_domains))
+		err |= WARN_ON(power_domains->domain_use_count[domain] != 1);
+
+	return !err;
+}
+
+static void print_power_domains(struct i915_power_domains *power_domains,
+				const char *prefix, u64 mask)
+{
+	enum intel_display_power_domain domain;
+
+	DRM_DEBUG_DRIVER("%s (%lu):\n", prefix, hweight64(mask));
+	for_each_power_domain(domain, mask)
+		DRM_DEBUG_DRIVER("%s use_count %d\n",
+				 intel_display_power_domain_str(domain),
+				 power_domains->domain_use_count[domain]);
+}
+
+static void
+print_async_put_domains_state(struct i915_power_domains *power_domains)
+{
+	DRM_DEBUG_DRIVER("async_put_wakeref %u\n",
+			 power_domains->async_put_wakeref);
+
+	print_power_domains(power_domains, "async_put_domains[0]",
+			    power_domains->async_put_domains[0]);
+	print_power_domains(power_domains, "async_put_domains[1]",
+			    power_domains->async_put_domains[1]);
+}
+
+static void
+verify_async_put_domains_state(struct i915_power_domains *power_domains)
+{
+	if (!__async_put_domains_state_ok(power_domains))
+		print_async_put_domains_state(power_domains);
+}
+
+#else
+
+static void
+assert_async_put_domain_masks_disjoint(struct i915_power_domains *power_domains)
+{
+}
+
+static void
+verify_async_put_domains_state(struct i915_power_domains *power_domains)
+{
+}
+
+#endif /* CONFIG_DRM_I915_DEBUG_RUNTIME_PM */
+
+static u64 async_put_domains_mask(struct i915_power_domains *power_domains)
+{
+	assert_async_put_domain_masks_disjoint(power_domains);
+
+	return __async_put_domains_mask(power_domains);
+}
+
+static void
+async_put_domains_clear_domain(struct i915_power_domains *power_domains,
+			       enum intel_display_power_domain domain)
+{
+	assert_async_put_domain_masks_disjoint(power_domains);
+
+	power_domains->async_put_domains[0] &= ~BIT_ULL(domain);
+	power_domains->async_put_domains[1] &= ~BIT_ULL(domain);
+}
+
+static bool
+intel_display_power_grab_async_put_ref(struct drm_i915_private *dev_priv,
+				       enum intel_display_power_domain domain)
+{
+	struct i915_power_domains *power_domains = &dev_priv->power_domains;
+	bool ret = false;
+
+	if (!(async_put_domains_mask(power_domains) & BIT_ULL(domain)))
+		goto out_verify;
+
+	async_put_domains_clear_domain(power_domains, domain);
+
+	ret = true;
+
+	if (async_put_domains_mask(power_domains))
+		goto out_verify;
+
+	cancel_delayed_work(&power_domains->async_put_work);
+	intel_runtime_pm_put_raw(dev_priv,
+				 fetch_and_zero(&power_domains->async_put_wakeref));
+out_verify:
+	verify_async_put_domains_state(power_domains);
+
+	return ret;
+}
+
 static void
 __intel_display_power_get_domain(struct drm_i915_private *dev_priv,
 				 enum intel_display_power_domain domain)
@@ -1890,6 +2014,9 @@  __intel_display_power_get_domain(struct drm_i915_private *dev_priv,
 	struct i915_power_domains *power_domains = &dev_priv->power_domains;
 	struct i915_power_well *power_well;
 
+	if (intel_display_power_grab_async_put_ref(dev_priv, domain))
+		return;
+
 	for_each_power_domain_well(dev_priv, power_well, BIT_ULL(domain))
 		intel_power_well_get(dev_priv, power_well);
 
@@ -1915,9 +2042,7 @@  intel_wakeref_t intel_display_power_get(struct drm_i915_private *dev_priv,
 	intel_wakeref_t wakeref = intel_runtime_pm_get(dev_priv);
 
 	mutex_lock(&power_domains->lock);
-
 	__intel_display_power_get_domain(dev_priv, domain);
-
 	mutex_unlock(&power_domains->lock);
 
 	return wakeref;
@@ -1966,24 +2091,36 @@  intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
 	return wakeref;
 }
 
-static void __intel_display_power_put(struct drm_i915_private *dev_priv,
-				      enum intel_display_power_domain domain)
+static void
+__intel_display_power_put_domain(struct drm_i915_private *dev_priv,
+				 enum intel_display_power_domain domain)
 {
 	struct i915_power_domains *power_domains;
 	struct i915_power_well *power_well;
+	const char *name = intel_display_power_domain_str(domain);
 
 	power_domains = &dev_priv->power_domains;
 
-	mutex_lock(&power_domains->lock);
-
 	WARN(!power_domains->domain_use_count[domain],
 	     "Use count on domain %s is already zero\n",
-	     intel_display_power_domain_str(domain));
+	     name);
+	WARN(async_put_domains_mask(power_domains) & BIT_ULL(domain),
+	     "Async disabling of domain %s is pending\n",
+	     name);
+
 	power_domains->domain_use_count[domain]--;
 
 	for_each_power_domain_well_reverse(dev_priv, power_well, BIT_ULL(domain))
 		intel_power_well_put(dev_priv, power_well);
+}
 
+static void __intel_display_power_put(struct drm_i915_private *dev_priv,
+				      enum intel_display_power_domain domain)
+{
+	struct i915_power_domains *power_domains = &dev_priv->power_domains;
+
+	mutex_lock(&power_domains->lock);
+	__intel_display_power_put_domain(dev_priv, domain);
 	mutex_unlock(&power_domains->lock);
 }
 
@@ -2003,6 +2140,159 @@  void intel_display_power_put_unchecked(struct drm_i915_private *dev_priv,
 	intel_runtime_pm_put_unchecked(dev_priv);
 }
 
+static void
+queue_async_put_domains_work(struct i915_power_domains *power_domains,
+			     intel_wakeref_t wakeref)
+{
+	WARN_ON(power_domains->async_put_wakeref);
+	power_domains->async_put_wakeref = wakeref;
+	WARN_ON(!queue_delayed_work(system_unbound_wq,
+				    &power_domains->async_put_work,
+				    msecs_to_jiffies(100)));
+}
+
+static void
+release_async_put_domains(struct i915_power_domains *power_domains, u64 mask)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(power_domains, struct drm_i915_private,
+			     power_domains);
+	enum intel_display_power_domain domain;
+	intel_wakeref_t wakeref;
+
+	/*
+	 * The caller must hold already raw wakeref, upgrade that to a proper
+	 * wakeref to make the state checker happy about the HW access during
+	 * power well disabling.
+	 */
+	assert_raw_rpm_wakelock_held(dev_priv);
+	wakeref = intel_runtime_pm_get(dev_priv);
+
+	for_each_power_domain(domain, mask) {
+		/* Clear before put, so put's sanity check is happy. */
+		async_put_domains_clear_domain(power_domains, domain);
+		__intel_display_power_put_domain(dev_priv, domain);
+	}
+
+	intel_runtime_pm_put(dev_priv, wakeref);
+}
+
+static void
+intel_display_power_put_async_work(struct work_struct *work)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(work, struct drm_i915_private,
+			     power_domains.async_put_work.work);
+	struct i915_power_domains *power_domains = &dev_priv->power_domains;
+	intel_wakeref_t new_work_wakeref = intel_runtime_pm_get_raw(dev_priv);
+	intel_wakeref_t old_work_wakeref = 0;
+
+	mutex_lock(&power_domains->lock);
+
+	/*
+	 * Bail out if all the domain refs pending to be released were grabbed
+	 * by subsequent gets or a flush_work.
+	 */
+	old_work_wakeref = fetch_and_zero(&power_domains->async_put_wakeref);
+	if (!old_work_wakeref)
+		goto out_verify;
+
+	release_async_put_domains(power_domains,
+				  power_domains->async_put_domains[0]);
+
+	/* Requeue the work if more domains were async put meanwhile. */
+	if (power_domains->async_put_domains[1]) {
+		power_domains->async_put_domains[0] =
+			fetch_and_zero(&power_domains->async_put_domains[1]);
+		queue_async_put_domains_work(power_domains,
+					     fetch_and_zero(&new_work_wakeref));
+	}
+
+out_verify:
+	verify_async_put_domains_state(power_domains);
+
+	mutex_unlock(&power_domains->lock);
+
+	if (old_work_wakeref)
+		intel_runtime_pm_put_raw(dev_priv, old_work_wakeref);
+	if (new_work_wakeref)
+		intel_runtime_pm_put_raw(dev_priv, new_work_wakeref);
+}
+
+void intel_display_power_put_async(struct drm_i915_private *dev_priv,
+				   enum intel_display_power_domain domain,
+				   intel_wakeref_t wakeref)
+{
+	struct i915_power_domains *power_domains = &dev_priv->power_domains;
+	intel_wakeref_t work_wakeref = intel_runtime_pm_get_raw(dev_priv);
+
+	mutex_lock(&power_domains->lock);
+
+	if (power_domains->domain_use_count[domain] > 1) {
+		__intel_display_power_put_domain(dev_priv, domain);
+
+		goto out_verify;
+	}
+
+	WARN_ON(power_domains->domain_use_count[domain] != 1);
+
+	/* Let a pending work requeue itself or queue a new one. */
+	if (power_domains->async_put_wakeref) {
+		power_domains->async_put_domains[1] |= BIT_ULL(domain);
+	} else {
+		power_domains->async_put_domains[0] |= BIT_ULL(domain);
+		queue_async_put_domains_work(power_domains,
+					     fetch_and_zero(&work_wakeref));
+	}
+
+out_verify:
+	verify_async_put_domains_state(power_domains);
+
+	mutex_unlock(&power_domains->lock);
+
+	if (work_wakeref)
+		intel_runtime_pm_put_raw(dev_priv, work_wakeref);
+
+	intel_runtime_pm_put(dev_priv, wakeref);
+}
+
+void intel_display_power_flush_work(struct drm_i915_private *dev_priv)
+{
+	struct i915_power_domains *power_domains = &dev_priv->power_domains;
+	intel_wakeref_t work_wakeref;
+
+	mutex_lock(&power_domains->lock);
+
+	work_wakeref = fetch_and_zero(&power_domains->async_put_wakeref);
+	if (!work_wakeref)
+		goto out_verify;
+
+	release_async_put_domains(power_domains,
+				  async_put_domains_mask(power_domains));
+	cancel_delayed_work(&power_domains->async_put_work);
+
+out_verify:
+	verify_async_put_domains_state(power_domains);
+
+	mutex_unlock(&power_domains->lock);
+
+	if (work_wakeref)
+		intel_runtime_pm_put_raw(dev_priv, work_wakeref);
+}
+
+static void
+intel_display_power_flush_work_sync(struct drm_i915_private *dev_priv)
+{
+	struct i915_power_domains *power_domains = &dev_priv->power_domains;
+
+	intel_display_power_flush_work(dev_priv);
+	cancel_delayed_work_sync(&power_domains->async_put_work);
+
+	verify_async_put_domains_state(power_domains);
+
+	WARN_ON(power_domains->async_put_wakeref);
+}
+
 #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
 void intel_display_power_put(struct drm_i915_private *dev_priv,
 			     enum intel_display_power_domain domain,
@@ -3491,6 +3781,9 @@  int intel_power_domains_init(struct drm_i915_private *dev_priv)
 
 	mutex_init(&power_domains->lock);
 
+	INIT_DELAYED_WORK(&power_domains->async_put_work,
+			  intel_display_power_put_async_work);
+
 	/*
 	 * The enabling order will be from lower to higher indexed wells,
 	 * the disabling order is reversed.
@@ -4168,6 +4461,8 @@  void intel_power_domains_fini_hw(struct drm_i915_private *i915)
 	if (!i915_modparams.disable_power_well)
 		intel_display_power_put_unchecked(i915, POWER_DOMAIN_INIT);
 
+	intel_display_power_flush_work_sync(i915);
+
 	intel_power_domains_verify_state(i915);
 
 	/* Keep the power well enabled, but cancel its rpm wakeref. */
@@ -4243,6 +4538,7 @@  void intel_power_domains_suspend(struct drm_i915_private *i915,
 	if (!(i915->csr.allowed_dc_mask & DC_STATE_EN_DC9) &&
 	    suspend_mode == I915_DRM_SUSPEND_IDLE &&
 	    i915->csr.dmc_payload) {
+		intel_display_power_flush_work(i915);
 		intel_power_domains_verify_state(i915);
 		return;
 	}
@@ -4254,6 +4550,7 @@  void intel_power_domains_suspend(struct drm_i915_private *i915,
 	if (!i915_modparams.disable_power_well)
 		intel_display_power_put_unchecked(i915, POWER_DOMAIN_INIT);
 
+	intel_display_power_flush_work(i915);
 	intel_power_domains_verify_state(i915);
 
 	if (INTEL_GEN(i915) >= 11)
@@ -4332,6 +4629,8 @@  static void intel_power_domains_verify_state(struct drm_i915_private *i915)
 
 	mutex_lock(&power_domains->lock);
 
+	verify_async_put_domains_state(power_domains);
+
 	dump_domain_info = false;
 	for_each_power_well(i915, power_well) {
 		enum intel_display_power_domain domain;
@@ -4400,7 +4699,6 @@  static void __intel_runtime_pm_get(struct drm_i915_private *i915)
 	WARN_ONCE(ret < 0, "pm_runtime_get_sync() failed: %d\n", ret);
 }
 
-__attribute__((__used__))
 static intel_wakeref_t intel_runtime_pm_get_raw(struct drm_i915_private *i915)
 {
 	__intel_runtime_pm_get(i915);
@@ -4508,7 +4806,6 @@  void intel_runtime_pm_put_unchecked(struct drm_i915_private *i915)
 }
 
 #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
-__attribute__((__used__))
 static void intel_runtime_pm_put_raw(struct drm_i915_private *i915,
 				     intel_wakeref_t wref)
 {
@@ -4522,7 +4819,6 @@  void intel_runtime_pm_put(struct drm_i915_private *i915, intel_wakeref_t wref)
 	intel_runtime_pm_put_unchecked(i915);
 }
 #else
-__attribute__((__used__))
 static void intel_runtime_pm_put_raw(struct drm_i915_private *i915,
 				     intel_wakeref_t wref)
 {
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.h b/drivers/gpu/drm/i915/intel_runtime_pm.h
index 69227756de3e..cabf6f900273 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.h
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
@@ -57,6 +57,10 @@  intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
 				   enum intel_display_power_domain domain);
 void intel_display_power_put_unchecked(struct drm_i915_private *dev_priv,
 				       enum intel_display_power_domain domain);
+void intel_display_power_put_async(struct drm_i915_private *dev_priv,
+				   enum intel_display_power_domain domain,
+				   intel_wakeref_t wakeref);
+void intel_display_power_flush_work(struct drm_i915_private *dev_priv);
 #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
 void intel_display_power_put(struct drm_i915_private *dev_priv,
 			     enum intel_display_power_domain domain,

Comments

Quoting Imre Deak (2019-05-03 00:26:41)
> By disabling a power domain asynchronously we can restrict holding a
> reference on that power domain to the actual code sequence that
> requires the power to be on for the HW access it's doing, by also
> avoiding unneeded on-off-on togglings of the power domain (since the
> disabling happens with a delay).

That applies to no-delay. Are we not starting from a state where the
code acquires the powerwell for its immediate use, or it takes and holds
it for protracted durations even when the powerwell is not being used?

> One benefit is potential power saving if the delay is chosen properly.

Which is suggesting that some delay is better power saving than
no-delay? It's believable that powering on cost mores more energy than
normal. But do please fill in a few details on how the delay should be
chosen.

> In the case of the AUX power domain holding the reference on the domain
> for the minimal amount of time at defined spots is also a requirement:

Do you mean that there is a minimum duration for which the power well
must be asserted once acquired before being released?

> on ICL we need a stricter control of when either kind of AUX power
> domain (TBT-alt or DP-alt) can be enabled and the locking we need for
> that becomes problematic (due to dependencies on other locks) if we
> allow the reference to be held for arbitrarily long periods/places in
> the code.
> 
> I chose the disabling delay to be 100msec for now to avoid the unneeded
> toggling (and so not to introduce dmesg spamming) in the DP MST sideband
> signaling code. We could optimize this delay later.

Or removing the spam. Are we at a point where the error detection is
good enough to pin-point the lack of a particular powerwell wakeref?
-Chris
On Fri, May 03, 2019 at 01:16:04PM +0100, Chris Wilson wrote:
> Quoting Imre Deak (2019-05-03 00:26:41)
> > By disabling a power domain asynchronously we can restrict holding a
> > reference on that power domain to the actual code sequence that
> > requires the power to be on for the HW access it's doing, by also
> > avoiding unneeded on-off-on togglings of the power domain (since the
> > disabling happens with a delay).
> 
> That applies to no-delay. Are we not starting from a state where the
> code acquires the powerwell for its immediate use, or it takes and holds
> it for protracted durations even when the powerwell is not being used?

The latter: we hold it for the entire duration of the detect and hotplug
cycles, whereas we would only need it for the actual AUX transfers. By
the end of the patchset we only hold it for the required duration, that
is the AUX transfer, but to do that we would also want - imo - to avoid
the power well on/off ping-pong due to back-to-back AUX transfers.

> > One benefit is potential power saving if the delay is chosen properly.
> 
> Which is suggesting that some delay is better power saving than
> no-delay? It's believable that powering on cost mores more energy than
> normal. But do please fill in a few details on how the delay should be
> chosen.

It's just my assumption that an on-off-on sequence costs more then just
leaving the power well(s) on if the duration is short between the two on
events, I haven't measured this.

In the extreme case we could reduce the delay to 0, but still keep the
disabling asynchronous, which would mean not blocking on the disabling.
I measured the disabling time (with wait_for_us) to be ~1ms in average
on VLV with the worst case being 4ms.

> > In the case of the AUX power domain holding the reference on the domain
> > for the minimal amount of time at defined spots is also a requirement:
> 
> Do you mean that there is a minimum duration for which the power well
> must be asserted once acquired before being released?

I meant that we would need to keep the sequence where we hold the
reference short and a well defined place, like what we would have by
just holding the reference for the actual AUX transfer (achieved by the
end of the patchset). Anything else would make the locking we need for
ICL TypeC ports around this sequence rather problematic, adding for
instance unnecessary lockdep dependencies, which I would really like to
avoid.

> > on ICL we need a stricter control of when either kind of AUX power
> > domain (TBT-alt or DP-alt) can be enabled and the locking we need for
> > that becomes problematic (due to dependencies on other locks) if we
> > allow the reference to be held for arbitrarily long periods/places in
> > the code.
> > 
> > I chose the disabling delay to be 100msec for now to avoid the unneeded
> > toggling (and so not to introduce dmesg spamming) in the DP MST sideband
> > signaling code. We could optimize this delay later.
> 
> Or removing the spam. Are we at a point where the error detection is
> good enough to pin-point the lack of a particular powerwell wakeref?

We don't have a per-powerwell list of registers that we could check
against if that's what you mean. So we only detect if no wakeref is held
(by this or another thread) or an actual unclaimed access if the power
well is really off.

> -Chris