Re: [Intel-gfx] [RFC v5 05/11] drm/i915/pmu: Suspend sampling when GPU is idle
On 14/09/2017 20:57, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2017-09-13 11:34:17)
From: Tvrtko Ursulin
If only a subset of events is enabled we can afford to suspend
the sampling timer when the GPU is idle and so save some cycles
and power.
v2: Rebase and limit timer even more.
v3: Rebase.
v4: Rebase.
v5: Skip action if perf PMU failed to register.
Signed-off-by: Tvrtko Ursulin
---
drivers/gpu/drm/i915/i915_drv.h | 8
drivers/gpu/drm/i915/i915_gem.c | 1 +
drivers/gpu/drm/i915/i915_gem_request.c | 1 +
drivers/gpu/drm/i915/i915_pmu.c | 70 -
4 files changed, 70 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 62646b8dfb7a..70be8c5d9a65 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2244,6 +2244,10 @@ struct i915_pmu {
*/
unsigned int enable_count[I915_PMU_MASK_BITS];
/**
+* @timer_enabled: Should the internal sampling timer be running.
+*/
+ bool timer_enabled;
+ /**
* @sample: Current counter value for i915 events which need sampling.
*
* These counters are updated from the i915 PMU sampling timer.
@@ -3989,9 +3993,13 @@ extern void i915_perf_unregister(struct drm_i915_private
*dev_priv);
#ifdef CONFIG_PERF_EVENTS
extern void i915_pmu_register(struct drm_i915_private *i915);
extern void i915_pmu_unregister(struct drm_i915_private *i915);
+extern void i915_pmu_gt_idle(struct drm_i915_private *i915);
+extern void i915_pmu_gt_active(struct drm_i915_private *i915);
#else
static inline void i915_pmu_register(struct drm_i915_private *i915) {}
static inline void i915_pmu_unregister(struct drm_i915_private *i915) {}
+static inline void i915_pmu_gt_idle(struct drm_i915_private *i915) {}
+static inline void i915_pmu_gt_active(struct drm_i915_private *i915) {}
#endif
/* i915_suspend.c */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f445587c1a4b..201b09eda93b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3227,6 +3227,7 @@ i915_gem_idle_work_handler(struct work_struct *work)
intel_engines_mark_idle(dev_priv);
i915_gem_timelines_mark_idle(dev_priv);
+ i915_pmu_gt_idle(dev_priv);
GEM_BUG_ON(!dev_priv->gt.awake);
dev_priv->gt.awake = false;
diff --git a/drivers/gpu/drm/i915/i915_gem_request.c
b/drivers/gpu/drm/i915/i915_gem_request.c
index 813a3b546d6e..18a1e379253e 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -258,6 +258,7 @@ static void mark_busy(struct drm_i915_private *i915)
i915_update_gfx_val(i915);
if (INTEL_GEN(i915) >= 6)
gen6_rps_busy(i915);
+ i915_pmu_gt_active(i915);
queue_delayed_work(i915->wq,
&i915->gt.retire_work,
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index e82648e6635b..22246918757c 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -90,6 +90,52 @@ static unsigned int event_enabled_bit(struct perf_event
*event)
return config_enabled_bit(event->attr.config);
}
+static bool pmu_needs_timer(struct drm_i915_private *i915, bool gpu_active)
+{
+ u64 enable = i915->pmu.enable;
+
+ enable &= config_enabled_mask(I915_PMU_ACTUAL_FREQUENCY) |
+ config_enabled_mask(I915_PMU_REQUESTED_FREQUENCY) |
+ ENGINE_SAMPLE_MASK;
+
+ if (!gpu_active)
+ enable &= ~ENGINE_SAMPLE_MASK;
Ok.
I'll add a comment here. It gets even more advanced when busy stats are
added.
+
+ return enable;
+}
+
+void i915_pmu_gt_idle(struct drm_i915_private *i915)
+{
+ if (!i915->pmu.base.event_init)
+ return;
+
+ spin_lock_irq(&i915->pmu.lock);
+ /*
+* Signal sampling timer to stop if only engine events are enabled and
+* GPU went idle.
+*/
+ i915->pmu.timer_enabled = pmu_needs_timer(i915, false);
+ spin_unlock_irq(&i915->pmu.lock);
+}
+
+void i915_pmu_gt_active(struct drm_i915_private *i915)
+{
+ if (!i915->pmu.base.event_init)
+ return;
+
+ spin_lock_irq(&i915->pmu.lock);
+ /*
+* Re-enable sampling timer when GPU goes active.
+*/
+ if (!i915->pmu.timer_enabled && pmu_needs_timer(i915, true)) {
+ i915->pmu.timer_enabled = true;
+ hrtimer_start_range_ns(&i915->pmu.timer,
+ ns_to_ktime(PERIOD), 0,
+ HRTIMER_MODE_REL_PINNED);
+ }
Duplication here, __i915_pmu_gt_active() for the critical section. Then
also helps allay fears that we hold the spinlock which is not clear from
the diff context below.
Ag
Re: [Intel-gfx] [RFC v5 05/11] drm/i915/pmu: Suspend sampling when GPU is idle
Quoting Tvrtko Ursulin (2017-09-13 11:34:17)
> From: Tvrtko Ursulin
>
> If only a subset of events is enabled we can afford to suspend
> the sampling timer when the GPU is idle and so save some cycles
> and power.
>
> v2: Rebase and limit timer even more.
> v3: Rebase.
> v4: Rebase.
> v5: Skip action if perf PMU failed to register.
>
> Signed-off-by: Tvrtko Ursulin
> ---
> drivers/gpu/drm/i915/i915_drv.h | 8
> drivers/gpu/drm/i915/i915_gem.c | 1 +
> drivers/gpu/drm/i915/i915_gem_request.c | 1 +
> drivers/gpu/drm/i915/i915_pmu.c | 70
> -
> 4 files changed, 70 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 62646b8dfb7a..70be8c5d9a65 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2244,6 +2244,10 @@ struct i915_pmu {
> */
> unsigned int enable_count[I915_PMU_MASK_BITS];
> /**
> +* @timer_enabled: Should the internal sampling timer be running.
> +*/
> + bool timer_enabled;
> + /**
> * @sample: Current counter value for i915 events which need sampling.
> *
> * These counters are updated from the i915 PMU sampling timer.
> @@ -3989,9 +3993,13 @@ extern void i915_perf_unregister(struct
> drm_i915_private *dev_priv);
> #ifdef CONFIG_PERF_EVENTS
> extern void i915_pmu_register(struct drm_i915_private *i915);
> extern void i915_pmu_unregister(struct drm_i915_private *i915);
> +extern void i915_pmu_gt_idle(struct drm_i915_private *i915);
> +extern void i915_pmu_gt_active(struct drm_i915_private *i915);
> #else
> static inline void i915_pmu_register(struct drm_i915_private *i915) {}
> static inline void i915_pmu_unregister(struct drm_i915_private *i915) {}
> +static inline void i915_pmu_gt_idle(struct drm_i915_private *i915) {}
> +static inline void i915_pmu_gt_active(struct drm_i915_private *i915) {}
> #endif
>
> /* i915_suspend.c */
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index f445587c1a4b..201b09eda93b 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3227,6 +3227,7 @@ i915_gem_idle_work_handler(struct work_struct *work)
>
> intel_engines_mark_idle(dev_priv);
> i915_gem_timelines_mark_idle(dev_priv);
> + i915_pmu_gt_idle(dev_priv);
>
> GEM_BUG_ON(!dev_priv->gt.awake);
> dev_priv->gt.awake = false;
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c
> b/drivers/gpu/drm/i915/i915_gem_request.c
> index 813a3b546d6e..18a1e379253e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -258,6 +258,7 @@ static void mark_busy(struct drm_i915_private *i915)
> i915_update_gfx_val(i915);
> if (INTEL_GEN(i915) >= 6)
> gen6_rps_busy(i915);
> + i915_pmu_gt_active(i915);
>
> queue_delayed_work(i915->wq,
>&i915->gt.retire_work,
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index e82648e6635b..22246918757c 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -90,6 +90,52 @@ static unsigned int event_enabled_bit(struct perf_event
> *event)
> return config_enabled_bit(event->attr.config);
> }
>
> +static bool pmu_needs_timer(struct drm_i915_private *i915, bool gpu_active)
> +{
> + u64 enable = i915->pmu.enable;
> +
> + enable &= config_enabled_mask(I915_PMU_ACTUAL_FREQUENCY) |
> + config_enabled_mask(I915_PMU_REQUESTED_FREQUENCY) |
> + ENGINE_SAMPLE_MASK;
> +
> + if (!gpu_active)
> + enable &= ~ENGINE_SAMPLE_MASK;
Ok.
> +
> + return enable;
> +}
> +
> +void i915_pmu_gt_idle(struct drm_i915_private *i915)
> +{
> + if (!i915->pmu.base.event_init)
> + return;
> +
> + spin_lock_irq(&i915->pmu.lock);
> + /*
> +* Signal sampling timer to stop if only engine events are enabled and
> +* GPU went idle.
> +*/
> + i915->pmu.timer_enabled = pmu_needs_timer(i915, false);
> + spin_unlock_irq(&i915->pmu.lock);
> +}
> +
> +void i915_pmu_gt_active(struct drm_i915_private *i915)
> +{
> + if (!i915->pmu.base.event_init)
> + return;
> +
> + spin_lock_irq(&i915->pmu.lock);
> + /*
> +* Re-enable sampling timer when GPU goes active.
> +*/
> + if (!i915->pmu.timer_enabled && pmu_needs_timer(i915, true)) {
> + i915->pmu.timer_enabled = true;
> + hrtimer_start_range_ns(&i915->pmu.timer,
> + ns_to_ktime(PERIOD), 0,
> + HRTIMER_MODE_REL_PINNED);
> + }
Duplication here, __i915_pmu_gt_active() for the critical section. Then
also helps allay
[Intel-gfx] [RFC v5 05/11] drm/i915/pmu: Suspend sampling when GPU is idle
From: Tvrtko Ursulin
If only a subset of events is enabled we can afford to suspend
the sampling timer when the GPU is idle and so save some cycles
and power.
v2: Rebase and limit timer even more.
v3: Rebase.
v4: Rebase.
v5: Skip action if perf PMU failed to register.
Signed-off-by: Tvrtko Ursulin
---
drivers/gpu/drm/i915/i915_drv.h | 8
drivers/gpu/drm/i915/i915_gem.c | 1 +
drivers/gpu/drm/i915/i915_gem_request.c | 1 +
drivers/gpu/drm/i915/i915_pmu.c | 70 -
4 files changed, 70 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 62646b8dfb7a..70be8c5d9a65 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2244,6 +2244,10 @@ struct i915_pmu {
*/
unsigned int enable_count[I915_PMU_MASK_BITS];
/**
+* @timer_enabled: Should the internal sampling timer be running.
+*/
+ bool timer_enabled;
+ /**
* @sample: Current counter value for i915 events which need sampling.
*
* These counters are updated from the i915 PMU sampling timer.
@@ -3989,9 +3993,13 @@ extern void i915_perf_unregister(struct drm_i915_private
*dev_priv);
#ifdef CONFIG_PERF_EVENTS
extern void i915_pmu_register(struct drm_i915_private *i915);
extern void i915_pmu_unregister(struct drm_i915_private *i915);
+extern void i915_pmu_gt_idle(struct drm_i915_private *i915);
+extern void i915_pmu_gt_active(struct drm_i915_private *i915);
#else
static inline void i915_pmu_register(struct drm_i915_private *i915) {}
static inline void i915_pmu_unregister(struct drm_i915_private *i915) {}
+static inline void i915_pmu_gt_idle(struct drm_i915_private *i915) {}
+static inline void i915_pmu_gt_active(struct drm_i915_private *i915) {}
#endif
/* i915_suspend.c */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f445587c1a4b..201b09eda93b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3227,6 +3227,7 @@ i915_gem_idle_work_handler(struct work_struct *work)
intel_engines_mark_idle(dev_priv);
i915_gem_timelines_mark_idle(dev_priv);
+ i915_pmu_gt_idle(dev_priv);
GEM_BUG_ON(!dev_priv->gt.awake);
dev_priv->gt.awake = false;
diff --git a/drivers/gpu/drm/i915/i915_gem_request.c
b/drivers/gpu/drm/i915/i915_gem_request.c
index 813a3b546d6e..18a1e379253e 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -258,6 +258,7 @@ static void mark_busy(struct drm_i915_private *i915)
i915_update_gfx_val(i915);
if (INTEL_GEN(i915) >= 6)
gen6_rps_busy(i915);
+ i915_pmu_gt_active(i915);
queue_delayed_work(i915->wq,
&i915->gt.retire_work,
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index e82648e6635b..22246918757c 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -90,6 +90,52 @@ static unsigned int event_enabled_bit(struct perf_event
*event)
return config_enabled_bit(event->attr.config);
}
+static bool pmu_needs_timer(struct drm_i915_private *i915, bool gpu_active)
+{
+ u64 enable = i915->pmu.enable;
+
+ enable &= config_enabled_mask(I915_PMU_ACTUAL_FREQUENCY) |
+ config_enabled_mask(I915_PMU_REQUESTED_FREQUENCY) |
+ ENGINE_SAMPLE_MASK;
+
+ if (!gpu_active)
+ enable &= ~ENGINE_SAMPLE_MASK;
+
+ return enable;
+}
+
+void i915_pmu_gt_idle(struct drm_i915_private *i915)
+{
+ if (!i915->pmu.base.event_init)
+ return;
+
+ spin_lock_irq(&i915->pmu.lock);
+ /*
+* Signal sampling timer to stop if only engine events are enabled and
+* GPU went idle.
+*/
+ i915->pmu.timer_enabled = pmu_needs_timer(i915, false);
+ spin_unlock_irq(&i915->pmu.lock);
+}
+
+void i915_pmu_gt_active(struct drm_i915_private *i915)
+{
+ if (!i915->pmu.base.event_init)
+ return;
+
+ spin_lock_irq(&i915->pmu.lock);
+ /*
+* Re-enable sampling timer when GPU goes active.
+*/
+ if (!i915->pmu.timer_enabled && pmu_needs_timer(i915, true)) {
+ i915->pmu.timer_enabled = true;
+ hrtimer_start_range_ns(&i915->pmu.timer,
+ ns_to_ktime(PERIOD), 0,
+ HRTIMER_MODE_REL_PINNED);
+ }
+ spin_unlock_irq(&i915->pmu.lock);
+}
+
static bool grab_forcewake(struct drm_i915_private *i915, bool fw)
{
if (!fw)
@@ -180,7 +226,7 @@ static enum hrtimer_restart i915_sample(struct hrtimer
*hrtimer)
struct drm_i915_private *i915 =
container_of(hrtimer, struct drm_i915_private, pmu.timer);
- if (i915->pmu.enable == 0)
+ if (!READ_ONCE(
