[PATCH 1/2] drm/i915: Compute the fw_domain id from the mask

[PATCH 1/2] drm/i915: Compute the fw_domain id from the mask

[Chris Wilson]

Storing both the mask and the id is redundant as we can trivially
compute one from the other. As the mask is more frequently used, remove
the id.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c |  2 +-
 drivers/gpu/drm/i915/intel_uncore.c | 12 ++++++------
 drivers/gpu/drm/i915/intel_uncore.h |  6 ++----
 3 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index bd9abef40c66..a2c9c3e792e1 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1468,7 +1468,7 @@ static int i915_forcewake_domains(struct seq_file *m, void *data)
 
 	for_each_fw_domain(fw_domain, i915, tmp)
 		seq_printf(m, "%s.wake_count = %u\n",
-			   intel_uncore_forcewake_domain_to_str(fw_domain->id),
+			   intel_uncore_forcewake_domain_to_str(fw_domain),
 			   READ_ONCE(fw_domain->wake_count));
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 08d7d08438c0..7eaa592aed26 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -40,8 +40,10 @@ static const char * const forcewake_domain_names[] = {
 };
 
 const char *
-intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id)
+intel_uncore_forcewake_domain_to_str(const struct intel_uncore_forcewake_domain *d)
 {
+	unsigned int id = ilog2(d->mask);
+
 	BUILD_BUG_ON(ARRAY_SIZE(forcewake_domain_names) != FW_DOMAIN_ID_COUNT);
 
 	if (id >= 0 && id < FW_DOMAIN_ID_COUNT)
@@ -77,7 +79,7 @@ fw_domain_wait_ack_clear(const struct drm_i915_private *i915,
 			     FORCEWAKE_KERNEL) == 0,
 			    FORCEWAKE_ACK_TIMEOUT_MS))
 		DRM_ERROR("%s: timed out waiting for forcewake ack to clear.\n",
-			  intel_uncore_forcewake_domain_to_str(d->id));
+			  intel_uncore_forcewake_domain_to_str(d));
 }
 
 static inline void
@@ -95,7 +97,7 @@ fw_domain_wait_ack(const struct drm_i915_private *i915,
 			     FORCEWAKE_KERNEL),
 			    FORCEWAKE_ACK_TIMEOUT_MS))
 		DRM_ERROR("%s: timed out waiting for forcewake ack request.\n",
-			  intel_uncore_forcewake_domain_to_str(d->id));
+			  intel_uncore_forcewake_domain_to_str(d));
 }
 
 static inline void
@@ -209,7 +211,7 @@ intel_uncore_fw_release_timer(struct hrtimer *timer)
 	struct intel_uncore_forcewake_domain *domain =
 	       container_of(timer, struct intel_uncore_forcewake_domain, timer);
 	struct drm_i915_private *dev_priv =
-		container_of(domain, struct drm_i915_private, uncore.fw_domain[domain->id]);
+		container_of(domain, struct drm_i915_private, uncore.fw_domain[ilog2(domain->mask)]);
 	unsigned long irqflags;
 
 	assert_rpm_device_not_suspended(dev_priv);
@@ -1149,8 +1151,6 @@ static void fw_domain_init(struct drm_i915_private *dev_priv,
 	d->reg_set = reg_set;
 	d->reg_ack = reg_ack;
 
-	d->id = domain_id;
-
 	BUILD_BUG_ON(FORCEWAKE_RENDER != (1 << FW_DOMAIN_ID_RENDER));
 	BUILD_BUG_ON(FORCEWAKE_BLITTER != (1 << FW_DOMAIN_ID_BLITTER));
 	BUILD_BUG_ON(FORCEWAKE_MEDIA != (1 << FW_DOMAIN_ID_MEDIA));
diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
index ff6fe2bb0ccf..5fec5fd4346c 100644
--- a/drivers/gpu/drm/i915/intel_uncore.h
+++ b/drivers/gpu/drm/i915/intel_uncore.h
@@ -93,8 +93,7 @@ struct intel_uncore {
 	u32 fw_reset;
 
 	struct intel_uncore_forcewake_domain {
-		enum forcewake_domain_id id;
-		enum forcewake_domains mask;
+		unsigned int mask;
 		unsigned int wake_count;
 		struct hrtimer timer;
 		i915_reg_t reg_set;
@@ -123,8 +122,7 @@ void intel_uncore_resume_early(struct drm_i915_private *dev_priv);
 
 u64 intel_uncore_edram_size(struct drm_i915_private *dev_priv);
 void assert_forcewakes_inactive(struct drm_i915_private *dev_priv);
-const char *intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id);
-
+const char *intel_uncore_forcewake_domain_to_str(const struct intel_uncore_forcewake_domain *domain);
 enum forcewake_domains
 intel_uncore_forcewake_for_reg(struct drm_i915_private *dev_priv,
 			       i915_reg_t reg, unsigned int op);
-- 
2.11.0

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

[PATCH 2/2] drm/i915: Keep the forcewake timer alive for 1ms past the most recent use

[Chris Wilson]

Currently the timer is armed for 1ms after the first use and is killed
immediately, dropping the forcewake as early as possible. However, for
very frequent operations the forcewake dance has a large impact on
latency and keeping the timer alive until we are idle is preferred. To
achieve this, if we call intel_uncore_forcewake_get whilst the timer is
alive (repeated use), then set a flag to restart the timer on expiry
rather than drop the forcewake usage count. The timer is racy, the
consequence of the race is to expire the timer earlier than is now
desired but does not impact on correct behaviour. The offset the race
slightly, we set the active flag again on intel_uncore_forcewake_put.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 14 +++++++++++---
 drivers/gpu/drm/i915/intel_uncore.h |  1 +
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 7eaa592aed26..26b6c0c14441 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -216,6 +216,9 @@ intel_uncore_fw_release_timer(struct hrtimer *timer)
 
 	assert_rpm_device_not_suspended(dev_priv);
 
+	if (xchg(&domain->active, false))
+		return HRTIMER_RESTART;
+
 	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 	if (WARN_ON(domain->wake_count == 0))
 		domain->wake_count++;
@@ -453,9 +456,12 @@ static void __intel_uncore_forcewake_get(struct drm_i915_private *dev_priv,
 
 	fw_domains &= dev_priv->uncore.fw_domains;
 
-	for_each_fw_domain_masked(domain, fw_domains, dev_priv, tmp)
-		if (domain->wake_count++)
+	for_each_fw_domain_masked(domain, fw_domains, dev_priv, tmp) {
+		if (domain->wake_count++) {
 			fw_domains &= ~domain->mask;
+			domain->active = true;
+		}
+	}
 
 	if (fw_domains)
 		dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_domains);
@@ -520,8 +526,10 @@ static void __intel_uncore_forcewake_put(struct drm_i915_private *dev_priv,
 		if (WARN_ON(domain->wake_count == 0))
 			continue;
 
-		if (--domain->wake_count)
+		if (--domain->wake_count) {
+			domain->active = true;
 			continue;
+		}
 
 		fw_domain_arm_timer(domain);
 	}
diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
index 5fec5fd4346c..dfead585835c 100644
--- a/drivers/gpu/drm/i915/intel_uncore.h
+++ b/drivers/gpu/drm/i915/intel_uncore.h
@@ -95,6 +95,7 @@ struct intel_uncore {
 	struct intel_uncore_forcewake_domain {
 		unsigned int mask;
 		unsigned int wake_count;
+		bool active;
 		struct hrtimer timer;
 		i915_reg_t reg_set;
 		i915_reg_t reg_ack;
-- 
2.11.0

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

✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915: Compute the fw_domain id from the mask

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Compute the fw_domain id from the mask
URL   : https://patchwork.freedesktop.org/series/24359/
State : failure

== Summary ==

Series 24359v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/24359/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                skip       -> INCOMPLETE (fi-bsw-n3050) fdo#100989
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> DMESG-WARN (fi-byt-j1900)
                pass       -> DMESG-WARN (fi-byt-n2820)
                pass       -> INCOMPLETE (fi-skl-6700k)
                pass       -> INCOMPLETE (fi-kbl-7500u)
                pass       -> DMESG-WARN (fi-kbl-7560u)
        Subgroup basic-rte:
                pass       -> DMESG-WARN (fi-byt-j1900)
                pass       -> DMESG-WARN (fi-byt-n2820)
                pass       -> INCOMPLETE (fi-skl-6700hq)
                pass       -> INCOMPLETE (fi-kbl-7560u)
Test prime_vgem:
        Subgroup basic-fence-flip:
                pass       -> DMESG-WARN (fi-byt-j1900)
                pass       -> DMESG-WARN (fi-byt-n2820)

fdo#100989 https://bugs.freedesktop.org/show_bug.cgi?id=100989

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:449s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:432s
fi-bsw-n3050     total:236  pass:204  dwarn:0   dfail:0   fail:0   skip:31 
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:516s
fi-byt-j1900     total:278  pass:251  dwarn:3   dfail:0   fail:0   skip:24  time:493s
fi-byt-n2820     total:278  pass:247  dwarn:3   dfail:0   fail:0   skip:28  time:487s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:418s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:408s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:418s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:506s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:466s
fi-kbl-7500u     total:241  pass:218  dwarn:5   dfail:0   fail:0   skip:17 
fi-kbl-7560u     total:242  pass:227  dwarn:6   dfail:0   fail:0   skip:8  
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:458s
fi-skl-6700hq    total:242  pass:226  dwarn:0   dfail:0   fail:0   skip:15 
fi-skl-6700k     total:241  pass:223  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:504s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:436s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:537s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time:398s
fi-bxt-t5700 failed to collect. IGT log at Patchwork_4684/fi-bxt-t5700/igt.log

38d26beef9f31fa7ca983563cf86d4131defd94a drm-tip: 2017y-05m-12d-13h-15m-01s UTC integration manifest
c88076a932 drm/i915: Keep the forcewake timer alive for 1ms past the most recent use
19ec9ce drm/i915: Compute the fw_domain id from the mask

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4684/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915: Keep the forcewake timer alive for 1ms past the most recent use

[Chris Wilson]

Currently the timer is armed for 1ms after the first use and is killed
immediately, dropping the forcewake as early as possible. However, for
very frequent operations the forcewake dance has a large impact on
latency and keeping the timer alive until we are idle is preferred. To
achieve this, if we call intel_uncore_forcewake_get whilst the timer is
alive (repeated use), then set a flag to restart the timer on expiry
rather than drop the forcewake usage count. The timer is racy, the
consequence of the race is to expire the timer earlier than is now
desired but does not impact on correct behaviour. The offset the race
slightly, we set the active flag again on intel_uncore_forcewake_put.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 15 ++++++++++++---
 drivers/gpu/drm/i915/intel_uncore.h |  1 +
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 7eaa592aed26..2fd0989805eb 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -216,6 +216,9 @@ intel_uncore_fw_release_timer(struct hrtimer *timer)
 
 	assert_rpm_device_not_suspended(dev_priv);
 
+	if (xchg(&domain->active, false))
+		return HRTIMER_RESTART;
+
 	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 	if (WARN_ON(domain->wake_count == 0))
 		domain->wake_count++;
@@ -246,6 +249,7 @@ static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
 		active_domains = 0;
 
 		for_each_fw_domain(domain, dev_priv, tmp) {
+			smp_store_mb(domain->active, false);
 			if (hrtimer_cancel(&domain->timer) == 0)
 				continue;
 
@@ -453,9 +457,12 @@ static void __intel_uncore_forcewake_get(struct drm_i915_private *dev_priv,
 
 	fw_domains &= dev_priv->uncore.fw_domains;
 
-	for_each_fw_domain_masked(domain, fw_domains, dev_priv, tmp)
-		if (domain->wake_count++)
+	for_each_fw_domain_masked(domain, fw_domains, dev_priv, tmp) {
+		if (domain->wake_count++) {
 			fw_domains &= ~domain->mask;
+			domain->active = true;
+		}
+	}
 
 	if (fw_domains)
 		dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_domains);
@@ -520,8 +527,10 @@ static void __intel_uncore_forcewake_put(struct drm_i915_private *dev_priv,
 		if (WARN_ON(domain->wake_count == 0))
 			continue;
 
-		if (--domain->wake_count)
+		if (--domain->wake_count) {
+			domain->active = true;
 			continue;
+		}
 
 		fw_domain_arm_timer(domain);
 	}
diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
index 5fec5fd4346c..dfead585835c 100644
--- a/drivers/gpu/drm/i915/intel_uncore.h
+++ b/drivers/gpu/drm/i915/intel_uncore.h
@@ -95,6 +95,7 @@ struct intel_uncore {
 	struct intel_uncore_forcewake_domain {
 		unsigned int mask;
 		unsigned int wake_count;
+		bool active;
 		struct hrtimer timer;
 		i915_reg_t reg_set;
 		i915_reg_t reg_ack;
-- 
2.11.0

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

✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915: Compute the fw_domain id from the mask (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Compute the fw_domain id from the mask (rev2)
URL   : https://patchwork.freedesktop.org/series/24359/
State : warning

== Summary ==

Series 24359v2 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/24359/revisions/2/mbox/

Test kms_force_connector_basic:
        Subgroup force-connector-state:
                pass       -> SKIP       (fi-snb-2520m)
                pass       -> SKIP       (fi-ivb-3520m)
        Subgroup force-edid:
                pass       -> SKIP       (fi-ivb-3520m)
        Subgroup force-load-detect:
                pass       -> SKIP       (fi-ivb-3520m)
        Subgroup prune-stale-modes:
                pass       -> SKIP       (fi-ivb-3520m)

fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:434s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:517s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:497s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:484s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:426s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:421s
fi-ivb-3520m     total:278  pass:256  dwarn:0   dfail:0   fail:0   skip:22  time:495s
fi-kbl-7500u     total:278  pass:255  dwarn:5   dfail:0   fail:0   skip:18  time:466s
fi-kbl-7560u     total:278  pass:263  dwarn:5   dfail:0   fail:0   skip:10  time:578s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time:574s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:476s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:504s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:439s
fi-snb-2520m     total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time:539s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time:413s
fi-bxt-t5700 failed to collect. IGT log at Patchwork_4689/fi-bxt-t5700/igt.log

38d26beef9f31fa7ca983563cf86d4131defd94a drm-tip: 2017y-05m-12d-13h-15m-01s UTC integration manifest
e645e4f drm/i915: Keep the forcewake timer alive for 1ms past the most recent use
98e890f drm/i915: Compute the fw_domain id from the mask

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4689/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Keep the forcewake timer alive for 1ms past the most recent use

[Tvrtko Ursulin]

On 12/05/2017 23:16, Chris Wilson wrote:
> Currently the timer is armed for 1ms after the first use and is killed
> immediately, dropping the forcewake as early as possible. However, for

Correct for implicit grabs, but for explicit it is 1-2ms after the last use.

> very frequent operations the forcewake dance has a large impact on
> latency and keeping the timer alive until we are idle is preferred. To

What workloads see the difference and by how much?

At the time I've fixed the auto-release to go from 0-1 jiffies to 1-2ms, 
we talked about this conundrum - whether to consider the first grab or 
last put for the timer. But we decided thorough testing is needed to see 
if this would make a difference and what power side effects it might have.

> achieve this, if we call intel_uncore_forcewake_get whilst the timer is
> alive (repeated use), then set a flag to restart the timer on expiry
> rather than drop the forcewake usage count. The timer is racy, the
> consequence of the race is to expire the timer earlier than is now
> desired but does not impact on correct behaviour. The offset the race
> slightly, we set the active flag again on intel_uncore_forcewake_put.

Using the hrtimer API to modify the timer was too expensive?

>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 15 ++++++++++++---
>  drivers/gpu/drm/i915/intel_uncore.h |  1 +
>  2 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 7eaa592aed26..2fd0989805eb 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -216,6 +216,9 @@ intel_uncore_fw_release_timer(struct hrtimer *timer)
>
>  	assert_rpm_device_not_suspended(dev_priv);
>
> +	if (xchg(&domain->active, false))
> +		return HRTIMER_RESTART;
> +
>  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>  	if (WARN_ON(domain->wake_count == 0))
>  		domain->wake_count++;
> @@ -246,6 +249,7 @@ static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
>  		active_domains = 0;
>
>  		for_each_fw_domain(domain, dev_priv, tmp) {
> +			smp_store_mb(domain->active, false);
>  			if (hrtimer_cancel(&domain->timer) == 0)
>  				continue;
>
> @@ -453,9 +457,12 @@ static void __intel_uncore_forcewake_get(struct drm_i915_private *dev_priv,
>
>  	fw_domains &= dev_priv->uncore.fw_domains;
>
> -	for_each_fw_domain_masked(domain, fw_domains, dev_priv, tmp)
> -		if (domain->wake_count++)
> +	for_each_fw_domain_masked(domain, fw_domains, dev_priv, tmp) {
> +		if (domain->wake_count++) {
>  			fw_domains &= ~domain->mask;
> +			domain->active = true;
> +		}
> +	}
>
>  	if (fw_domains)
>  		dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_domains);
> @@ -520,8 +527,10 @@ static void __intel_uncore_forcewake_put(struct drm_i915_private *dev_priv,
>  		if (WARN_ON(domain->wake_count == 0))
>  			continue;
>
> -		if (--domain->wake_count)
> +		if (--domain->wake_count) {
> +			domain->active = true;
>  			continue;
> +		}
>
>  		fw_domain_arm_timer(domain);
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> index 5fec5fd4346c..dfead585835c 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.h
> +++ b/drivers/gpu/drm/i915/intel_uncore.h
> @@ -95,6 +95,7 @@ struct intel_uncore {
>  	struct intel_uncore_forcewake_domain {
>  		unsigned int mask;
>  		unsigned int wake_count;
> +		bool active;
>  		struct hrtimer timer;
>  		i915_reg_t reg_set;
>  		i915_reg_t reg_ack;
>

Minus the possible commit message improvements and discussion looks 
correct to me.

Regards,

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

Re: [PATCH 1/2] drm/i915: Compute the fw_domain id from the mask

[Tvrtko Ursulin]

On 12/05/2017 14:53, Chris Wilson wrote:
> Storing both the mask and the id is redundant as we can trivially
> compute one from the other. As the mask is more frequently used, remove
> the id.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |  2 +-
>  drivers/gpu/drm/i915/intel_uncore.c | 12 ++++++------
>  drivers/gpu/drm/i915/intel_uncore.h |  6 ++----
>  3 files changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index bd9abef40c66..a2c9c3e792e1 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1468,7 +1468,7 @@ static int i915_forcewake_domains(struct seq_file *m, void *data)
>
>  	for_each_fw_domain(fw_domain, i915, tmp)
>  		seq_printf(m, "%s.wake_count = %u\n",
> -			   intel_uncore_forcewake_domain_to_str(fw_domain->id),
> +			   intel_uncore_forcewake_domain_to_str(fw_domain),
>  			   READ_ONCE(fw_domain->wake_count));
>
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 08d7d08438c0..7eaa592aed26 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -40,8 +40,10 @@ static const char * const forcewake_domain_names[] = {
>  };
>
>  const char *
> -intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id)
> +intel_uncore_forcewake_domain_to_str(const struct intel_uncore_forcewake_domain *d)
>  {
> +	unsigned int id = ilog2(d->mask);
> +
>  	BUILD_BUG_ON(ARRAY_SIZE(forcewake_domain_names) != FW_DOMAIN_ID_COUNT);
>
>  	if (id >= 0 && id < FW_DOMAIN_ID_COUNT)
> @@ -77,7 +79,7 @@ fw_domain_wait_ack_clear(const struct drm_i915_private *i915,
>  			     FORCEWAKE_KERNEL) == 0,
>  			    FORCEWAKE_ACK_TIMEOUT_MS))
>  		DRM_ERROR("%s: timed out waiting for forcewake ack to clear.\n",
> -			  intel_uncore_forcewake_domain_to_str(d->id));
> +			  intel_uncore_forcewake_domain_to_str(d));
>  }
>
>  static inline void
> @@ -95,7 +97,7 @@ fw_domain_wait_ack(const struct drm_i915_private *i915,
>  			     FORCEWAKE_KERNEL),
>  			    FORCEWAKE_ACK_TIMEOUT_MS))
>  		DRM_ERROR("%s: timed out waiting for forcewake ack request.\n",
> -			  intel_uncore_forcewake_domain_to_str(d->id));
> +			  intel_uncore_forcewake_domain_to_str(d));
>  }
>
>  static inline void
> @@ -209,7 +211,7 @@ intel_uncore_fw_release_timer(struct hrtimer *timer)
>  	struct intel_uncore_forcewake_domain *domain =
>  	       container_of(timer, struct intel_uncore_forcewake_domain, timer);
>  	struct drm_i915_private *dev_priv =
> -		container_of(domain, struct drm_i915_private, uncore.fw_domain[domain->id]);
> +		container_of(domain, struct drm_i915_private, uncore.fw_domain[ilog2(domain->mask)]);

Not sure I see the benefit of removing one field and then needing this 
ilog2 in the timer callback.

Regards,

Tvrtko


>  	unsigned long irqflags;
>
>  	assert_rpm_device_not_suspended(dev_priv);
> @@ -1149,8 +1151,6 @@ static void fw_domain_init(struct drm_i915_private *dev_priv,
>  	d->reg_set = reg_set;
>  	d->reg_ack = reg_ack;
>
> -	d->id = domain_id;
> -
>  	BUILD_BUG_ON(FORCEWAKE_RENDER != (1 << FW_DOMAIN_ID_RENDER));
>  	BUILD_BUG_ON(FORCEWAKE_BLITTER != (1 << FW_DOMAIN_ID_BLITTER));
>  	BUILD_BUG_ON(FORCEWAKE_MEDIA != (1 << FW_DOMAIN_ID_MEDIA));
> diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> index ff6fe2bb0ccf..5fec5fd4346c 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.h
> +++ b/drivers/gpu/drm/i915/intel_uncore.h
> @@ -93,8 +93,7 @@ struct intel_uncore {
>  	u32 fw_reset;
>
>  	struct intel_uncore_forcewake_domain {
> -		enum forcewake_domain_id id;
> -		enum forcewake_domains mask;
> +		unsigned int mask;
>  		unsigned int wake_count;
>  		struct hrtimer timer;
>  		i915_reg_t reg_set;
> @@ -123,8 +122,7 @@ void intel_uncore_resume_early(struct drm_i915_private *dev_priv);
>
>  u64 intel_uncore_edram_size(struct drm_i915_private *dev_priv);
>  void assert_forcewakes_inactive(struct drm_i915_private *dev_priv);
> -const char *intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id);
> -
> +const char *intel_uncore_forcewake_domain_to_str(const struct intel_uncore_forcewake_domain *domain);
>  enum forcewake_domains
>  intel_uncore_forcewake_for_reg(struct drm_i915_private *dev_priv,
>  			       i915_reg_t reg, unsigned int op);
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Keep the forcewake timer alive for 1ms past the most recent use

[Chris Wilson]

On Mon, May 15, 2017 at 11:14:32AM +0100, Tvrtko Ursulin wrote:
> 
> On 12/05/2017 23:16, Chris Wilson wrote:
> >Currently the timer is armed for 1ms after the first use and is killed
> >immediately, dropping the forcewake as early as possible. However, for
> 
> Correct for implicit grabs, but for explicit it is 1-2ms after the last use.

From the put of the first, we don't rearm the timer on later puts.
 
> >very frequent operations the forcewake dance has a large impact on
> >latency and keeping the timer alive until we are idle is preferred. To
> 
> What workloads see the difference and by how much?

The issue I have is that we can't submit nops fast enough using
lite-restore. A large part of that was from the rpm get/put in the
tasklet, but I suspect ultimately it is the extra mmio/lite-restore that
is slowing us down. Now, I'm happy that the lite-restore to keep port[1]
accessible for the next context is a benefit, so I'm looking at how we
can improve the continual resubmission.
 
> At the time I've fixed the auto-release to go from 0-1 jiffies to
> 1-2ms, we talked about this conundrum - whether to consider the
> first grab or last put for the timer. But we decided thorough
> testing is needed to see if this would make a difference and what
> power side effects it might have.

In the above scenario, it never goes off so we are the paying the worst
price of a useless dance. It's the periodic 1ms poll on an idle system
that will suffer most, but in pratice this will delay turning off by an
extra 1ms - and that may be the difference between 0.1% and 50% in power
consumption :|
 
> >achieve this, if we call intel_uncore_forcewake_get whilst the timer is
> >alive (repeated use), then set a flag to restart the timer on expiry
> >rather than drop the forcewake usage count. The timer is racy, the
> >consequence of the race is to expire the timer earlier than is now
> >desired but does not impact on correct behaviour. The offset the race
> >slightly, we set the active flag again on intel_uncore_forcewake_put.
> 
> Using the hrtimer API to modify the timer was too expensive?

In the past it has been unsuitable for frequent adjustment, and we may
be using I915_READ every few instructions.
-Chris

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

Re: [PATCH 1/2] drm/i915: Compute the fw_domain id from the mask

[Chris Wilson]

On Mon, May 15, 2017 at 11:18:11AM +0100, Tvrtko Ursulin wrote:
> 
> On 12/05/2017 14:53, Chris Wilson wrote:
> >Storing both the mask and the id is redundant as we can trivially
> >compute one from the other. As the mask is more frequently used, remove
> >the id.
> >
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> >Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >---
> > drivers/gpu/drm/i915/i915_debugfs.c |  2 +-
> > drivers/gpu/drm/i915/intel_uncore.c | 12 ++++++------
> > drivers/gpu/drm/i915/intel_uncore.h |  6 ++----
> > 3 files changed, 9 insertions(+), 11 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> >index bd9abef40c66..a2c9c3e792e1 100644
> >--- a/drivers/gpu/drm/i915/i915_debugfs.c
> >+++ b/drivers/gpu/drm/i915/i915_debugfs.c
> >@@ -1468,7 +1468,7 @@ static int i915_forcewake_domains(struct seq_file *m, void *data)
> >
> > 	for_each_fw_domain(fw_domain, i915, tmp)
> > 		seq_printf(m, "%s.wake_count = %u\n",
> >-			   intel_uncore_forcewake_domain_to_str(fw_domain->id),
> >+			   intel_uncore_forcewake_domain_to_str(fw_domain),
> > 			   READ_ONCE(fw_domain->wake_count));
> >
> > 	return 0;
> >diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> >index 08d7d08438c0..7eaa592aed26 100644
> >--- a/drivers/gpu/drm/i915/intel_uncore.c
> >+++ b/drivers/gpu/drm/i915/intel_uncore.c
> >@@ -40,8 +40,10 @@ static const char * const forcewake_domain_names[] = {
> > };
> >
> > const char *
> >-intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id)
> >+intel_uncore_forcewake_domain_to_str(const struct intel_uncore_forcewake_domain *d)
> > {
> >+	unsigned int id = ilog2(d->mask);
> >+
> > 	BUILD_BUG_ON(ARRAY_SIZE(forcewake_domain_names) != FW_DOMAIN_ID_COUNT);
> >
> > 	if (id >= 0 && id < FW_DOMAIN_ID_COUNT)
> >@@ -77,7 +79,7 @@ fw_domain_wait_ack_clear(const struct drm_i915_private *i915,
> > 			     FORCEWAKE_KERNEL) == 0,
> > 			    FORCEWAKE_ACK_TIMEOUT_MS))
> > 		DRM_ERROR("%s: timed out waiting for forcewake ack to clear.\n",
> >-			  intel_uncore_forcewake_domain_to_str(d->id));
> >+			  intel_uncore_forcewake_domain_to_str(d));
> > }
> >
> > static inline void
> >@@ -95,7 +97,7 @@ fw_domain_wait_ack(const struct drm_i915_private *i915,
> > 			     FORCEWAKE_KERNEL),
> > 			    FORCEWAKE_ACK_TIMEOUT_MS))
> > 		DRM_ERROR("%s: timed out waiting for forcewake ack request.\n",
> >-			  intel_uncore_forcewake_domain_to_str(d->id));
> >+			  intel_uncore_forcewake_domain_to_str(d));
> > }
> >
> > static inline void
> >@@ -209,7 +211,7 @@ intel_uncore_fw_release_timer(struct hrtimer *timer)
> > 	struct intel_uncore_forcewake_domain *domain =
> > 	       container_of(timer, struct intel_uncore_forcewake_domain, timer);
> > 	struct drm_i915_private *dev_priv =
> >-		container_of(domain, struct drm_i915_private, uncore.fw_domain[domain->id]);
> >+		container_of(domain, struct drm_i915_private, uncore.fw_domain[ilog2(domain->mask)]);
> 
> Not sure I see the benefit of removing one field and then needing
> this ilog2 in the timer callback.

Timer was infrequent enough compared to the other paths that the extra
instruction seemed a matter of little concern, and out of the way.
-Chris

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

Re: [PATCH v2] drm/i915: Keep the forcewake timer alive for 1ms past the most recent use

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Currently the timer is armed for 1ms after the first use and is killed
> immediately, dropping the forcewake as early as possible. However, for
> very frequent operations the forcewake dance has a large impact on
> latency and keeping the timer alive until we are idle is preferred. To
> achieve this, if we call intel_uncore_forcewake_get whilst the timer is
> alive (repeated use), then set a flag to restart the timer on expiry
> rather than drop the forcewake usage count. The timer is racy, the
> consequence of the race is to expire the timer earlier than is now
> desired but does not impact on correct behaviour. The offset the race
> slightly, we set the active flag again on intel_uncore_forcewake_put.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 15 ++++++++++++---
>  drivers/gpu/drm/i915/intel_uncore.h |  1 +
>  2 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 7eaa592aed26..2fd0989805eb 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -216,6 +216,9 @@ intel_uncore_fw_release_timer(struct hrtimer *timer)
>  
>  	assert_rpm_device_not_suspended(dev_priv);
>  
> +	if (xchg(&domain->active, false))
> +		return HRTIMER_RESTART;
> +
>  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>  	if (WARN_ON(domain->wake_count == 0))
>  		domain->wake_count++;
> @@ -246,6 +249,7 @@ static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
>  		active_domains = 0;
>  
>  		for_each_fw_domain(domain, dev_priv, tmp) {
> +			smp_store_mb(domain->active, false);
>  			if (hrtimer_cancel(&domain->timer) == 0)
>  				continue;
>  
> @@ -453,9 +457,12 @@ static void __intel_uncore_forcewake_get(struct drm_i915_private *dev_priv,
>  
>  	fw_domains &= dev_priv->uncore.fw_domains;
>  
> -	for_each_fw_domain_masked(domain, fw_domains, dev_priv, tmp)
> -		if (domain->wake_count++)
> +	for_each_fw_domain_masked(domain, fw_domains, dev_priv, tmp) {
> +		if (domain->wake_count++) {
>  			fw_domains &= ~domain->mask;
> +			domain->active = true;
> +		}
> +	}
>  
>  	if (fw_domains)
>  		dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_domains);
> @@ -520,8 +527,10 @@ static void __intel_uncore_forcewake_put(struct drm_i915_private *dev_priv,
>  		if (WARN_ON(domain->wake_count == 0))
>  			continue;
>  
> -		if (--domain->wake_count)
> +		if (--domain->wake_count) {
> +			domain->active = true;

We dont wan't to set the active here for all domains that
are going to power off, and delay their release for one timer tick
to optimistically wait for next user?

-Mika

>  			continue;
> +		}
>  
>  		fw_domain_arm_timer(domain);
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> index 5fec5fd4346c..dfead585835c 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.h
> +++ b/drivers/gpu/drm/i915/intel_uncore.h
> @@ -95,6 +95,7 @@ struct intel_uncore {
>  	struct intel_uncore_forcewake_domain {
>  		unsigned int mask;
>  		unsigned int wake_count;
> +		bool active;
>  		struct hrtimer timer;
>  		i915_reg_t reg_set;
>  		i915_reg_t reg_ack;
> -- 
> 2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Keep the forcewake timer alive for 1ms past the most recent use

[Chris Wilson]

On Mon, May 15, 2017 at 02:20:24PM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> > @@ -520,8 +527,10 @@ static void __intel_uncore_forcewake_put(struct drm_i915_private *dev_priv,
> >  		if (WARN_ON(domain->wake_count == 0))
> >  			continue;
> >  
> > -		if (--domain->wake_count)
> > +		if (--domain->wake_count) {
> > +			domain->active = true;
> 
> We dont wan't to set the active here for all domains that
> are going to power off, and delay their release for one timer tick
> to optimistically wait for next user?

If we hit zero, that implies there is no timer and so we will arm it for
1ms. The question amounts to whether we want it to run for a minimum of
2ms rather than 1ms.
-Chris

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

Re: [PATCH v2] drm/i915: Keep the forcewake timer alive for 1ms past the most recent use

[Tvrtko Ursulin]

On 15/05/2017 11:41, Chris Wilson wrote:
> On Mon, May 15, 2017 at 11:14:32AM +0100, Tvrtko Ursulin wrote:
>>
>> On 12/05/2017 23:16, Chris Wilson wrote:
>>> Currently the timer is armed for 1ms after the first use and is killed
>>> immediately, dropping the forcewake as early as possible. However, for
>>
>> Correct for implicit grabs, but for explicit it is 1-2ms after the last use.
>
> From the put of the first, we don't rearm the timer on later puts.

What do you mean by first? I see the timer getting armed on the last put.

>>> very frequent operations the forcewake dance has a large impact on
>>> latency and keeping the timer alive until we are idle is preferred. To
>>
>> What workloads see the difference and by how much?
>
> The issue I have is that we can't submit nops fast enough using

Fast enough for what? You mean just for your liking ie. we can be faster?

> lite-restore. A large part of that was from the rpm get/put in the
> tasklet, but I suspect ultimately it is the extra mmio/lite-restore that
> is slowing us down. Now, I'm happy that the lite-restore to keep port[1]
> accessible for the next context is a benefit, so I'm looking at how we
> can improve the continual resubmission.

Ok.

>> At the time I've fixed the auto-release to go from 0-1 jiffies to
>> 1-2ms, we talked about this conundrum - whether to consider the
>> first grab or last put for the timer. But we decided thorough
>> testing is needed to see if this would make a difference and what
>> power side effects it might have.
>
> In the above scenario, it never goes off so we are the paying the worst
> price of a useless dance. It's the periodic 1ms poll on an idle system
> that will suffer most, but in pratice this will delay turning off by an
> extra 1ms - and that may be the difference between 0.1% and 50% in power
> consumption :|

What is a periodic 1ms poll on the idle system? If the system is idle 
the auto-release timer will not be running.

In the rapid short submission scenario I see the auto-release timer 
potentially racing with the tasklet and needlessly dropping the fw. But 
rapid short submission is so much faster than the 1-2ms auto-release 
that this race must be very infrequent.

I instead expect mostly to see the timer run and find the 
domain->wake_count > 0 due a tasklet running in parallel who has grabbed 
the fw for itself.

Worse case scenario sounds like it would be some submission period 
around the auto-release period but just shifted in "phase", no?

So I do see some benefit, but would just want to see some numbers in the 
commit message and a more precise description of the scenario it improves.

Regards,

Tvrtko

>>> achieve this, if we call intel_uncore_forcewake_get whilst the timer is
>>> alive (repeated use), then set a flag to restart the timer on expiry
>>> rather than drop the forcewake usage count. The timer is racy, the
>>> consequence of the race is to expire the timer earlier than is now
>>> desired but does not impact on correct behaviour. The offset the race
>>> slightly, we set the active flag again on intel_uncore_forcewake_put.
>>
>> Using the hrtimer API to modify the timer was too expensive?
>
> In the past it has been unsuitable for frequent adjustment, and we may
> be using I915_READ every few instructions.
> -Chris
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Keep the forcewake timer alive for 1ms past the most recent use

[Chris Wilson]

On Mon, May 15, 2017 at 01:06:47PM +0100, Tvrtko Ursulin wrote:
> 
> On 15/05/2017 11:41, Chris Wilson wrote:
> >On Mon, May 15, 2017 at 11:14:32AM +0100, Tvrtko Ursulin wrote:
> >>
> >>On 12/05/2017 23:16, Chris Wilson wrote:
> >>>Currently the timer is armed for 1ms after the first use and is killed
> >>>immediately, dropping the forcewake as early as possible. However, for
> >>
> >>Correct for implicit grabs, but for explicit it is 1-2ms after the last use.
> >
> >From the put of the first, we don't rearm the timer on later puts.
> 
> What do you mean by first? I see the timer getting armed on the last put.

Once the timer is armed, the next put will do nothing.

> >>>very frequent operations the forcewake dance has a large impact on
> >>>latency and keeping the timer alive until we are idle is preferred. To
> >>
> >>What workloads see the difference and by how much?
> >
> >The issue I have is that we can't submit nops fast enough using
> 
> Fast enough for what? You mean just for your liking ie. we can be faster?

We used to be faster (single context nop). And we are still not close to
ringbuffer. :|

To my liking, but looking at Vk traces they have lots of little batches
(bordering on the nop) with perhaps one or two big ones. But we do hit
the lite-restore repeated submission path many times per frame. (Still
orders of magnitude less impact than gem_exec_nop! ;)

> >lite-restore. A large part of that was from the rpm get/put in the
> >tasklet, but I suspect ultimately it is the extra mmio/lite-restore that
> >is slowing us down. Now, I'm happy that the lite-restore to keep port[1]
> >accessible for the next context is a benefit, so I'm looking at how we
> >can improve the continual resubmission.
> 
> Ok.
> 
> >>At the time I've fixed the auto-release to go from 0-1 jiffies to
> >>1-2ms, we talked about this conundrum - whether to consider the
> >>first grab or last put for the timer. But we decided thorough
> >>testing is needed to see if this would make a difference and what
> >>power side effects it might have.
> >
> >In the above scenario, it never goes off so we are the paying the worst
> >price of a useless dance. It's the periodic 1ms poll on an idle system
> >that will suffer most, but in pratice this will delay turning off by an
> >extra 1ms - and that may be the difference between 0.1% and 50% in power
> >consumption :|
> 
> What is a periodic 1ms poll on the idle system? If the system is
> idle the auto-release timer will not be running.

Otherwise idle, except for the hypothetical poll to keep arming the
timer.
 
> In the rapid short submission scenario I see the auto-release timer
> potentially racing with the tasklet and needlessly dropping the fw.
> But rapid short submission is so much faster than the 1-2ms
> auto-release that this race must be very infrequent.
> 
> I instead expect mostly to see the timer run and find the
> domain->wake_count > 0 due a tasklet running in parallel who has
> grabbed the fw for itself.
> 
> Worse case scenario sounds like it would be some submission period
> around the auto-release period but just shifted in "phase", no?
> 
> So I do see some benefit, but would just want to see some numbers in
> the commit message and a more precise description of the scenario it
> improves.

Would have to measure the 99.9th percentiles as we only affect those
rare racing nops, so the average we report is unaffected - certainly it
is not the 2us, on average, I'm trying to recover.
-Chris

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

Re: [PATCH 1/2] drm/i915: Compute the fw_domain id from the mask

[Tvrtko Ursulin]

On 15/05/2017 11:45, Chris Wilson wrote:
> On Mon, May 15, 2017 at 11:18:11AM +0100, Tvrtko Ursulin wrote:
>>
>> On 12/05/2017 14:53, Chris Wilson wrote:
>>> Storing both the mask and the id is redundant as we can trivially
>>> compute one from the other. As the mask is more frequently used, remove
>>> the id.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>> drivers/gpu/drm/i915/i915_debugfs.c |  2 +-
>>> drivers/gpu/drm/i915/intel_uncore.c | 12 ++++++------
>>> drivers/gpu/drm/i915/intel_uncore.h |  6 ++----
>>> 3 files changed, 9 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>>> index bd9abef40c66..a2c9c3e792e1 100644
>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>> @@ -1468,7 +1468,7 @@ static int i915_forcewake_domains(struct seq_file *m, void *data)
>>>
>>> 	for_each_fw_domain(fw_domain, i915, tmp)
>>> 		seq_printf(m, "%s.wake_count = %u\n",
>>> -			   intel_uncore_forcewake_domain_to_str(fw_domain->id),
>>> +			   intel_uncore_forcewake_domain_to_str(fw_domain),
>>> 			   READ_ONCE(fw_domain->wake_count));
>>>
>>> 	return 0;
>>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>>> index 08d7d08438c0..7eaa592aed26 100644
>>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>>> @@ -40,8 +40,10 @@ static const char * const forcewake_domain_names[] = {
>>> };
>>>
>>> const char *
>>> -intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id)
>>> +intel_uncore_forcewake_domain_to_str(const struct intel_uncore_forcewake_domain *d)
>>> {
>>> +	unsigned int id = ilog2(d->mask);
>>> +
>>> 	BUILD_BUG_ON(ARRAY_SIZE(forcewake_domain_names) != FW_DOMAIN_ID_COUNT);
>>>
>>> 	if (id >= 0 && id < FW_DOMAIN_ID_COUNT)
>>> @@ -77,7 +79,7 @@ fw_domain_wait_ack_clear(const struct drm_i915_private *i915,
>>> 			     FORCEWAKE_KERNEL) == 0,
>>> 			    FORCEWAKE_ACK_TIMEOUT_MS))
>>> 		DRM_ERROR("%s: timed out waiting for forcewake ack to clear.\n",
>>> -			  intel_uncore_forcewake_domain_to_str(d->id));
>>> +			  intel_uncore_forcewake_domain_to_str(d));
>>> }
>>>
>>> static inline void
>>> @@ -95,7 +97,7 @@ fw_domain_wait_ack(const struct drm_i915_private *i915,
>>> 			     FORCEWAKE_KERNEL),
>>> 			    FORCEWAKE_ACK_TIMEOUT_MS))
>>> 		DRM_ERROR("%s: timed out waiting for forcewake ack request.\n",
>>> -			  intel_uncore_forcewake_domain_to_str(d->id));
>>> +			  intel_uncore_forcewake_domain_to_str(d));
>>> }
>>>
>>> static inline void
>>> @@ -209,7 +211,7 @@ intel_uncore_fw_release_timer(struct hrtimer *timer)
>>> 	struct intel_uncore_forcewake_domain *domain =
>>> 	       container_of(timer, struct intel_uncore_forcewake_domain, timer);
>>> 	struct drm_i915_private *dev_priv =
>>> -		container_of(domain, struct drm_i915_private, uncore.fw_domain[domain->id]);
>>> +		container_of(domain, struct drm_i915_private, uncore.fw_domain[ilog2(domain->mask)]);
>>
>> Not sure I see the benefit of removing one field and then needing
>> this ilog2 in the timer callback.
>
> Timer was infrequent enough compared to the other paths that the extra
> instruction seemed a matter of little concern, and out of the way.

On one hand that's true, one the other it just looks inelegant.

Would storing a backpointer to intel_uncore from each domain be 
acceptable or it would defeat the purpose?

Regards,

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

Re: [PATCH v2] drm/i915: Keep the forcewake timer alive for 1ms past the most recent use

[Tvrtko Ursulin]

On 15/05/2017 13:35, Chris Wilson wrote:
> On Mon, May 15, 2017 at 01:06:47PM +0100, Tvrtko Ursulin wrote:
>>
>> On 15/05/2017 11:41, Chris Wilson wrote:
>>> On Mon, May 15, 2017 at 11:14:32AM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> On 12/05/2017 23:16, Chris Wilson wrote:
>>>>> Currently the timer is armed for 1ms after the first use and is killed
>>>>> immediately, dropping the forcewake as early as possible. However, for
>>>>
>>>> Correct for implicit grabs, but for explicit it is 1-2ms after the last use.
>>>
>> >From the put of the first, we don't rearm the timer on later puts.
>>
>> What do you mean by first? I see the timer getting armed on the last put.
>
> Once the timer is armed, the next put will do nothing.
>
>>>>> very frequent operations the forcewake dance has a large impact on
>>>>> latency and keeping the timer alive until we are idle is preferred. To
>>>>
>>>> What workloads see the difference and by how much?
>>>
>>> The issue I have is that we can't submit nops fast enough using
>>
>> Fast enough for what? You mean just for your liking ie. we can be faster?
>
> We used to be faster (single context nop). And we are still not close to
> ringbuffer. :|
>
> To my liking, but looking at Vk traces they have lots of little batches
> (bordering on the nop) with perhaps one or two big ones. But we do hit
> the lite-restore repeated submission path many times per frame. (Still
> orders of magnitude less impact than gem_exec_nop! ;)
>
>>> lite-restore. A large part of that was from the rpm get/put in the
>>> tasklet, but I suspect ultimately it is the extra mmio/lite-restore that
>>> is slowing us down. Now, I'm happy that the lite-restore to keep port[1]
>>> accessible for the next context is a benefit, so I'm looking at how we
>>> can improve the continual resubmission.
>>
>> Ok.
>>
>>>> At the time I've fixed the auto-release to go from 0-1 jiffies to
>>>> 1-2ms, we talked about this conundrum - whether to consider the
>>>> first grab or last put for the timer. But we decided thorough
>>>> testing is needed to see if this would make a difference and what
>>>> power side effects it might have.
>>>
>>> In the above scenario, it never goes off so we are the paying the worst
>>> price of a useless dance. It's the periodic 1ms poll on an idle system
>>> that will suffer most, but in pratice this will delay turning off by an
>>> extra 1ms - and that may be the difference between 0.1% and 50% in power
>>> consumption :|
>>
>> What is a periodic 1ms poll on the idle system? If the system is
>> idle the auto-release timer will not be running.
>
> Otherwise idle, except for the hypothetical poll to keep arming the
> timer.
>
>> In the rapid short submission scenario I see the auto-release timer
>> potentially racing with the tasklet and needlessly dropping the fw.
>> But rapid short submission is so much faster than the 1-2ms
>> auto-release that this race must be very infrequent.
>>
>> I instead expect mostly to see the timer run and find the
>> domain->wake_count > 0 due a tasklet running in parallel who has
>> grabbed the fw for itself.
>>
>> Worse case scenario sounds like it would be some submission period
>> around the auto-release period but just shifted in "phase", no?
>>
>> So I do see some benefit, but would just want to see some numbers in
>> the commit message and a more precise description of the scenario it
>> improves.
>
> Would have to measure the 99.9th percentiles as we only affect those
> rare racing nops, so the average we report is unaffected - certainly it
> is not the 2us, on average, I'm trying to recover.

I've changed my mind over the weekend and started thinking we should 
have this even if the commit message did not fully explain everything.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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