[resend-for-CI,1/3] drm/i915: Use consistent forcewake auto-release timeout across kernel configs
diff mbox

Message ID 1459935194-2318-1-git-send-email-tvrtko.ursulin@linux.intel.com
State New
Headers show

Commit Message

Tvrtko Ursulin April 6, 2016, 9:33 a.m. UTC
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Because it is based on jiffies, current implementation releases the
forcewake at any time between straight away and between 1ms and 10ms,
depending on the kernel configuration (CONFIG_HZ).

This is probably not what has been desired, since the dynamics of keeping
parts of the GPU awake should not be correlated with this kernel
configuration parameter.

Change the auto-release mechanism to use hrtimers and set the timeout to
1ms with a 1ms of slack. This should make the GPU power consistent
across kernel configs, and timer slack should enable some timer coalescing
where multiple force-wake domains exist, or with unrelated timers.

For GlBench/T-Rex this decreases the number of forcewake releases from
~480 to ~300 per second, and for a heavy combined OGL/OCL test from
~670 to ~360 (HZ=1000 kernel).

Even though this reduction can be attributed to the average release period
extending from 0-1ms to 1-2ms, as discussed above, it will make the
forcewake timeout consistent for different CONFIG_HZ values.

Real life measurements with the above workload has shown that, with this
patch, both manage to auto-release the forcewake between 2-4 times per
10ms, even though the number of forcewake gets is dramatically different.

T-Rex requests between 5-10 explicit gets and 5-10 implict gets in each
10ms period, while the OGL/OCL test requests 250 and 380 times in the same
period.

The two data points together suggest that the nature of the forwake
accesses is bursty and that further changes and potential timeout
extensions, or moving the start of timeout from the first to the last
automatic forcewake grab, should be carefully measured for power and
performance effects.

v2:
  * Commit spelling. (Dave Gordon)
  * More discussion on numbers in the commit. (Chris Wilson)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h     |  2 +-
 drivers/gpu/drm/i915/intel_uncore.c | 25 ++++++++++++++++---------
 2 files changed, 17 insertions(+), 10 deletions(-)

Comments

Chris Wilson April 6, 2016, 11:05 a.m. UTC | #1
On Wed, Apr 06, 2016 at 10:28:29AM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [resend-for-CI,1/3] drm/i915: Use consistent forcewake auto-release timeout across kernel configs
> URL   : https://patchwork.freedesktop.org/series/5368/
> State : failure
> 
> == Summary ==
> 
> Series 5368v1 Series without cover letter
> http://patchwork.freedesktop.org/api/1.0/series/5368/revisions/1/mbox/
> 
> Test drv_module_reload_basic:
>                 pass       -> INCOMPLETE (snb-dellxps)
> Test gem_ctx_exec:
>         Subgroup basic:
>                 pass       -> DMESG-WARN (bsw-nuc-2)
> Test gem_exec_basic:
>         Subgroup gtt-vebox:
>                 pass       -> SKIP       (bsw-nuc-2)
> Test gem_exec_whisper:
>         Subgroup basic:
>                 pass       -> DMESG-FAIL (bsw-nuc-2)
> Test gem_mmap_gtt:
>         Subgroup basic-read-no-prefault:
>                 pass       -> DMESG-WARN (bsw-nuc-2)
>         Subgroup basic-small-copy:
>                 pass       -> DMESG-WARN (bsw-nuc-2)
> Test gem_ringfill:
>         Subgroup basic-default-forked:
>                 pass       -> SKIP       (bsw-nuc-2)
> Test gem_sync:
>         Subgroup basic-all:
>                 dmesg-fail -> PASS       (bsw-nuc-2)
>         Subgroup basic-bsd:
>                 pass       -> SKIP       (bsw-nuc-2)
> Test gem_tiled_blits:
>         Subgroup basic:
>                 pass       -> DMESG-FAIL (bsw-nuc-2)
> Test kms_addfb_basic:
>         Subgroup addfb25-framebuffer-vs-set-tiling:
>                 pass       -> DMESG-WARN (bsw-nuc-2)
>         Subgroup no-handle:
>                 pass       -> DMESG-WARN (bsw-nuc-2)
>         Subgroup unused-modifier:
>                 pass       -> DMESG-WARN (bsw-nuc-2)
> Test kms_flip:
>         Subgroup basic-flip-vs-dpms:
>                 dmesg-warn -> PASS       (ilk-hp8440p) UNSTABLE
> Test kms_force_connector_basic:
>         Subgroup force-load-detect:
>                 pass       -> SKIP       (ivb-t430s)
>         Subgroup prune-stale-modes:
>                 skip       -> PASS       (ivb-t430s)
> Test kms_pipe_crc_basic:
>         Subgroup suspend-read-crc-pipe-c:
>                 dmesg-warn -> PASS       (bsw-nuc-2)
> Test kms_setmode:
>         Subgroup basic-clone-single-crtc:
>                 pass       -> DMESG-WARN (bsw-nuc-2)
> Test pm_rpm:
>         Subgroup basic-pci-d3-state:
>                 pass       -> DMESG-WARN (bsw-nuc-2)

Braswell is snafu.

However, Braswell is also one of the devices with the more interesting
fw domains, so I guess we should wait for the all-clear?
-Chris

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 313bc3576d87..8ee5f78d45bc 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -666,7 +666,7 @@  struct intel_uncore {
 		struct drm_i915_private *i915;
 		enum forcewake_domain_id id;
 		unsigned wake_count;
-		struct timer_list timer;
+		struct hrtimer timer;
 		i915_reg_t reg_set;
 		u32 val_set;
 		u32 val_clear;
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index fbc1d215ca67..2e3e92469947 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -60,7 +60,11 @@  fw_domain_reset(const struct intel_uncore_forcewake_domain *d)
 static inline void
 fw_domain_arm_timer(struct intel_uncore_forcewake_domain *d)
 {
-	mod_timer_pinned(&d->timer, jiffies + 1);
+	d->wake_count++;
+	hrtimer_start_range_ns(&d->timer,
+			       ktime_set(0, NSEC_PER_MSEC),
+			       NSEC_PER_MSEC,
+			       HRTIMER_MODE_REL);
 }
 
 static inline void
@@ -224,9 +228,11 @@  static int __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
-static void intel_uncore_fw_release_timer(unsigned long arg)
+static enum hrtimer_restart
+intel_uncore_fw_release_timer(struct hrtimer *timer)
 {
-	struct intel_uncore_forcewake_domain *domain = (void *)arg;
+	struct intel_uncore_forcewake_domain *domain =
+	       container_of(timer, struct intel_uncore_forcewake_domain, timer);
 	unsigned long irqflags;
 
 	assert_rpm_device_not_suspended(domain->i915);
@@ -240,6 +246,8 @@  static void intel_uncore_fw_release_timer(unsigned long arg)
 							  1 << domain->id);
 
 	spin_unlock_irqrestore(&domain->i915->uncore.lock, irqflags);
+
+	return HRTIMER_NORESTART;
 }
 
 void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
@@ -259,16 +267,16 @@  void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
 		active_domains = 0;
 
 		for_each_fw_domain(domain, dev_priv, id) {
-			if (del_timer_sync(&domain->timer) == 0)
+			if (hrtimer_cancel(&domain->timer) == 0)
 				continue;
 
-			intel_uncore_fw_release_timer((unsigned long)domain);
+			intel_uncore_fw_release_timer(&domain->timer);
 		}
 
 		spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 
 		for_each_fw_domain(domain, dev_priv, id) {
-			if (timer_pending(&domain->timer))
+			if (hrtimer_active(&domain->timer))
 				active_domains |= (1 << id);
 		}
 
@@ -491,7 +499,6 @@  static void __intel_uncore_forcewake_put(struct drm_i915_private *dev_priv,
 		if (--domain->wake_count)
 			continue;
 
-		domain->wake_count++;
 		fw_domain_arm_timer(domain);
 	}
 }
@@ -732,7 +739,6 @@  static inline void __force_wake_auto(struct drm_i915_private *dev_priv,
 			continue;
 		}
 
-		domain->wake_count++;
 		fw_domain_arm_timer(domain);
 	}
 
@@ -1150,7 +1156,8 @@  static void fw_domain_init(struct drm_i915_private *dev_priv,
 	d->i915 = dev_priv;
 	d->id = domain_id;
 
-	setup_timer(&d->timer, intel_uncore_fw_release_timer, (unsigned long)d);
+	hrtimer_init(&d->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	d->timer.function = intel_uncore_fw_release_timer;
 
 	dev_priv->uncore.fw_domains |= (1 << domain_id);