[Intel-gfx] [RFC 1/4] drm/i915/pmu: Make more struct i915_pmu centric
Michal Wajdeczko
michal.wajdeczko at intel.com
Thu Aug 1 10:46:06 UTC 2019
On Thu, 01 Aug 2019 12:18:22 +0200, Tvrtko Ursulin
<tvrtko.ursulin at linux.intel.com> wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>
> Just tidy the code a bit by removing a sea of overly verbose i915->pmu.*.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
> drivers/gpu/drm/i915/i915_pmu.c | 194 +++++++++++++++++---------------
> 1 file changed, 104 insertions(+), 90 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c
> b/drivers/gpu/drm/i915/i915_pmu.c
> index eff86483bec0..12008966b00e 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -74,8 +74,9 @@ 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)
> +static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active)
> {
> + struct drm_i915_private *i915 = container_of(pmu, typeof(*i915), pmu);
maybe this can be promoted to pmu_to_i915() ?
> u64 enable;
> /*
> @@ -83,7 +84,7 @@ static bool pmu_needs_timer(struct drm_i915_private
> *i915, bool gpu_active)
> *
> * We start with a bitmask of all currently enabled events.
> */
> - enable = i915->pmu.enable;
> + enable = pmu->enable;
> /*
> * Mask out all the ones which do not need the timer, or in
> @@ -114,24 +115,26 @@ static bool pmu_needs_timer(struct
> drm_i915_private *i915, bool gpu_active)
> void i915_pmu_gt_parked(struct drm_i915_private *i915)
you should be more consistent and change this one too
> {
> - if (!i915->pmu.base.event_init)
> + struct i915_pmu *pmu = &i915->pmu;
> +
> + if (!pmu->base.event_init)
> return;
> - spin_lock_irq(&i915->pmu.lock);
> + spin_lock_irq(&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);
> + pmu->timer_enabled = pmu_needs_timer(pmu, false);
> + spin_unlock_irq(&pmu->lock);
> }
> -static void __i915_pmu_maybe_start_timer(struct drm_i915_private *i915)
> +static void __i915_pmu_maybe_start_timer(struct i915_pmu *pmu)
> {
> - if (!i915->pmu.timer_enabled && pmu_needs_timer(i915, true)) {
> - i915->pmu.timer_enabled = true;
> - i915->pmu.timer_last = ktime_get();
> - hrtimer_start_range_ns(&i915->pmu.timer,
> + if (!pmu->timer_enabled && pmu_needs_timer(pmu, true)) {
> + pmu->timer_enabled = true;
> + pmu->timer_last = ktime_get();
> + hrtimer_start_range_ns(&pmu->timer,
> ns_to_ktime(PERIOD), 0,
> HRTIMER_MODE_REL_PINNED);
> }
> @@ -139,15 +142,17 @@ static void __i915_pmu_maybe_start_timer(struct
> drm_i915_private *i915)
> void i915_pmu_gt_unparked(struct drm_i915_private *i915)
and here (and even some more in whole .c file)
> {
> - if (!i915->pmu.base.event_init)
> + struct i915_pmu *pmu = &i915->pmu;
> +
> + if (!pmu->base.event_init)
> return;
> - spin_lock_irq(&i915->pmu.lock);
> + spin_lock_irq(&pmu->lock);
> /*
> * Re-enable sampling timer when GPU goes active.
> */
> - __i915_pmu_maybe_start_timer(i915);
> - spin_unlock_irq(&i915->pmu.lock);
> + __i915_pmu_maybe_start_timer(pmu);
> + spin_unlock_irq(&pmu->lock);
> }
> static void
> @@ -251,15 +256,16 @@ static enum hrtimer_restart i915_sample(struct
> hrtimer *hrtimer)
> {
> struct drm_i915_private *i915 =
> container_of(hrtimer, struct drm_i915_private, pmu.timer);
> + struct i915_pmu *pmu = &i915->pmu;
> unsigned int period_ns;
> ktime_t now;
> - if (!READ_ONCE(i915->pmu.timer_enabled))
> + if (!READ_ONCE(pmu->timer_enabled))
> return HRTIMER_NORESTART;
> now = ktime_get();
> - period_ns = ktime_to_ns(ktime_sub(now, i915->pmu.timer_last));
> - i915->pmu.timer_last = now;
> + period_ns = ktime_to_ns(ktime_sub(now, pmu->timer_last));
> + pmu->timer_last = now;
> /*
> * Strictly speaking the passed in period may not be 100% accurate for
> @@ -443,6 +449,7 @@ static u64 get_rc6(struct drm_i915_private *i915)
this can also take *pmu instead of *i915
> {
> #if IS_ENABLED(CONFIG_PM)
> struct intel_runtime_pm *rpm = &i915->runtime_pm;
> + struct i915_pmu *pmu = &i915->pmu;
> intel_wakeref_t wakeref;
> unsigned long flags;
> u64 val;
> @@ -458,16 +465,16 @@ static u64 get_rc6(struct drm_i915_private *i915)
> * previously.
> */
> - spin_lock_irqsave(&i915->pmu.lock, flags);
> + spin_lock_irqsave(&pmu->lock, flags);
> - if (val >= i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur) {
> - i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = 0;
> - i915->pmu.sample[__I915_SAMPLE_RC6].cur = val;
> + if (val >= pmu->sample[__I915_SAMPLE_RC6_ESTIMATED].cur) {
> + pmu->sample[__I915_SAMPLE_RC6_ESTIMATED].cur = 0;
> + pmu->sample[__I915_SAMPLE_RC6].cur = val;
> } else {
> - val = i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur;
> + val = pmu->sample[__I915_SAMPLE_RC6_ESTIMATED].cur;
> }
> - spin_unlock_irqrestore(&i915->pmu.lock, flags);
> + spin_unlock_irqrestore(&pmu->lock, flags);
> } else {
> struct device *kdev = rpm->kdev;
> @@ -478,7 +485,7 @@ static u64 get_rc6(struct drm_i915_private *i915)
> * on top of the last known real value, as the approximated RC6
> * counter value.
> */
> - spin_lock_irqsave(&i915->pmu.lock, flags);
> + spin_lock_irqsave(&pmu->lock, flags);
> /*
> * After the above branch intel_runtime_pm_get_if_in_use failed
> @@ -494,20 +501,20 @@ static u64 get_rc6(struct drm_i915_private *i915)
> if (pm_runtime_status_suspended(kdev)) {
> val = pm_runtime_suspended_time(kdev);
> - if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
> - i915->pmu.suspended_time_last = val;
> + if (!pmu->sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
> + pmu->suspended_time_last = val;
> - val -= i915->pmu.suspended_time_last;
> - val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
> + val -= pmu->suspended_time_last;
> + val += pmu->sample[__I915_SAMPLE_RC6].cur;
> - i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
> - } else if (i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur) {
> - val = i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur;
> + pmu->sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
> + } else if (pmu->sample[__I915_SAMPLE_RC6_ESTIMATED].cur) {
> + val = pmu->sample[__I915_SAMPLE_RC6_ESTIMATED].cur;
> } else {
> - val = i915->pmu.sample[__I915_SAMPLE_RC6].cur;
> + val = pmu->sample[__I915_SAMPLE_RC6].cur;
> }
> - spin_unlock_irqrestore(&i915->pmu.lock, flags);
> + spin_unlock_irqrestore(&pmu->lock, flags);
> }
> return val;
> @@ -520,6 +527,7 @@ static u64 __i915_pmu_event_read(struct perf_event
> *event)
> {
> struct drm_i915_private *i915 =
> container_of(event->pmu, typeof(*i915), pmu.base);
> + struct i915_pmu *pmu = &i915->pmu;
> u64 val = 0;
> if (is_engine_event(event)) {
> @@ -542,12 +550,12 @@ static u64 __i915_pmu_event_read(struct perf_event
> *event)
> switch (event->attr.config) {
> case I915_PMU_ACTUAL_FREQUENCY:
> val =
> - div_u64(i915->pmu.sample[__I915_SAMPLE_FREQ_ACT].cur,
> + div_u64(pmu->sample[__I915_SAMPLE_FREQ_ACT].cur,
> USEC_PER_SEC /* to MHz */);
> break;
> case I915_PMU_REQUESTED_FREQUENCY:
> val =
> - div_u64(i915->pmu.sample[__I915_SAMPLE_FREQ_REQ].cur,
> + div_u64(pmu->sample[__I915_SAMPLE_FREQ_REQ].cur,
> USEC_PER_SEC /* to MHz */);
> break;
> case I915_PMU_INTERRUPTS:
> @@ -582,24 +590,25 @@ static void i915_pmu_enable(struct perf_event
> *event)
> struct drm_i915_private *i915 =
> container_of(event->pmu, typeof(*i915), pmu.base);
> unsigned int bit = event_enabled_bit(event);
> + struct i915_pmu *pmu = &i915->pmu;
> unsigned long flags;
> - spin_lock_irqsave(&i915->pmu.lock, flags);
> + spin_lock_irqsave(&pmu->lock, flags);
> /*
> * Update the bitmask of enabled events and increment
> * the event reference counter.
> */
> - BUILD_BUG_ON(ARRAY_SIZE(i915->pmu.enable_count) != I915_PMU_MASK_BITS);
> - GEM_BUG_ON(bit >= ARRAY_SIZE(i915->pmu.enable_count));
> - GEM_BUG_ON(i915->pmu.enable_count[bit] == ~0);
> - i915->pmu.enable |= BIT_ULL(bit);
> - i915->pmu.enable_count[bit]++;
> + BUILD_BUG_ON(ARRAY_SIZE(pmu->enable_count) != I915_PMU_MASK_BITS);
> + GEM_BUG_ON(bit >= ARRAY_SIZE(pmu->enable_count));
> + GEM_BUG_ON(pmu->enable_count[bit] == ~0);
> + pmu->enable |= BIT_ULL(bit);
> + pmu->enable_count[bit]++;
> /*
> * Start the sampling timer if needed and not already enabled.
> */
> - __i915_pmu_maybe_start_timer(i915);
> + __i915_pmu_maybe_start_timer(pmu);
> /*
> * For per-engine events the bitmask and reference counting
> @@ -625,7 +634,7 @@ static void i915_pmu_enable(struct perf_event *event)
> engine->pmu.enable_count[sample]++;
> }
> - spin_unlock_irqrestore(&i915->pmu.lock, flags);
> + spin_unlock_irqrestore(&pmu->lock, flags);
> /*
> * Store the current counter value so we can report the correct delta
> @@ -640,9 +649,10 @@ static void i915_pmu_disable(struct perf_event
> *event)
> struct drm_i915_private *i915 =
> container_of(event->pmu, typeof(*i915), pmu.base);
> unsigned int bit = event_enabled_bit(event);
> + struct i915_pmu *pmu = &i915->pmu;
> unsigned long flags;
> - spin_lock_irqsave(&i915->pmu.lock, flags);
> + spin_lock_irqsave(&pmu->lock, flags);
> if (is_engine_event(event)) {
> u8 sample = engine_event_sample(event);
> @@ -664,18 +674,18 @@ static void i915_pmu_disable(struct perf_event
> *event)
> engine->pmu.enable &= ~BIT(sample);
> }
> - GEM_BUG_ON(bit >= ARRAY_SIZE(i915->pmu.enable_count));
> - GEM_BUG_ON(i915->pmu.enable_count[bit] == 0);
> + GEM_BUG_ON(bit >= ARRAY_SIZE(pmu->enable_count));
> + GEM_BUG_ON(pmu->enable_count[bit] == 0);
> /*
> * Decrement the reference count and clear the enabled
> * bitmask when the last listener on an event goes away.
> */
> - if (--i915->pmu.enable_count[bit] == 0) {
> - i915->pmu.enable &= ~BIT_ULL(bit);
> - i915->pmu.timer_enabled &= pmu_needs_timer(i915, true);
> + if (--pmu->enable_count[bit] == 0) {
> + pmu->enable &= ~BIT_ULL(bit);
> + pmu->timer_enabled &= pmu_needs_timer(pmu, true);
> }
> - spin_unlock_irqrestore(&i915->pmu.lock, flags);
> + spin_unlock_irqrestore(&pmu->lock, flags);
> }
> static void i915_pmu_event_start(struct perf_event *event, int flags)
> @@ -824,8 +834,9 @@ add_pmu_attr(struct perf_pmu_events_attr *attr,
> const char *name,
> }
> static struct attribute **
> -create_event_attributes(struct drm_i915_private *i915)
> +create_event_attributes(struct i915_pmu *pmu)
> {
> + struct drm_i915_private *i915 = container_of(pmu, typeof(*i915), pmu);
> static const struct {
> u64 config;
> const char *name;
> @@ -939,8 +950,8 @@ create_event_attributes(struct drm_i915_private
> *i915)
> }
> }
> - i915->pmu.i915_attr = i915_attr;
> - i915->pmu.pmu_attr = pmu_attr;
> + pmu->i915_attr = i915_attr;
> + pmu->pmu_attr = pmu_attr;
> return attr;
> @@ -956,7 +967,7 @@ err:;
> return NULL;
> }
> -static void free_event_attributes(struct drm_i915_private *i915)
> +static void free_event_attributes(struct i915_pmu *pmu)
> {
> struct attribute **attr_iter = i915_pmu_events_attr_group.attrs;
> @@ -964,12 +975,12 @@ static void free_event_attributes(struct
> drm_i915_private *i915)
> kfree((*attr_iter)->name);
> kfree(i915_pmu_events_attr_group.attrs);
> - kfree(i915->pmu.i915_attr);
> - kfree(i915->pmu.pmu_attr);
> + kfree(pmu->i915_attr);
> + kfree(pmu->pmu_attr);
> i915_pmu_events_attr_group.attrs = NULL;
> - i915->pmu.i915_attr = NULL;
> - i915->pmu.pmu_attr = NULL;
> + pmu->i915_attr = NULL;
> + pmu->pmu_attr = NULL;
> }
> static int i915_pmu_cpu_online(unsigned int cpu, struct hlist_node *node)
> @@ -1006,7 +1017,7 @@ static int i915_pmu_cpu_offline(unsigned int cpu,
> struct hlist_node *node)
> static enum cpuhp_state cpuhp_slot = CPUHP_INVALID;
> -static int i915_pmu_register_cpuhp_state(struct drm_i915_private *i915)
> +static int i915_pmu_register_cpuhp_state(struct i915_pmu *pmu)
> {
> enum cpuhp_state slot;
> int ret;
> @@ -1019,7 +1030,7 @@ static int i915_pmu_register_cpuhp_state(struct
> drm_i915_private *i915)
> return ret;
> slot = ret;
> - ret = cpuhp_state_add_instance(slot, &i915->pmu.node);
> + ret = cpuhp_state_add_instance(slot, &pmu->node);
> if (ret) {
> cpuhp_remove_multi_state(slot);
> return ret;
> @@ -1029,15 +1040,16 @@ static int i915_pmu_register_cpuhp_state(struct
> drm_i915_private *i915)
> return 0;
> }
> -static void i915_pmu_unregister_cpuhp_state(struct drm_i915_private
> *i915)
> +static void i915_pmu_unregister_cpuhp_state(struct i915_pmu *pmu)
> {
> WARN_ON(cpuhp_slot == CPUHP_INVALID);
> - WARN_ON(cpuhp_state_remove_instance(cpuhp_slot, &i915->pmu.node));
> + WARN_ON(cpuhp_state_remove_instance(cpuhp_slot, &pmu->node));
> cpuhp_remove_multi_state(cpuhp_slot);
> }
> void i915_pmu_register(struct drm_i915_private *i915)
again
> {
> + struct i915_pmu *pmu = &i915->pmu;
> int ret;
> if (INTEL_GEN(i915) <= 2) {
> @@ -1045,56 +1057,58 @@ void i915_pmu_register(struct drm_i915_private
> *i915)
> return;
> }
> - i915_pmu_events_attr_group.attrs = create_event_attributes(i915);
> + i915_pmu_events_attr_group.attrs = create_event_attributes(pmu);
> if (!i915_pmu_events_attr_group.attrs) {
> ret = -ENOMEM;
> goto err;
> }
> - i915->pmu.base.attr_groups = i915_pmu_attr_groups;
> - i915->pmu.base.task_ctx_nr = perf_invalid_context;
> - i915->pmu.base.event_init = i915_pmu_event_init;
> - i915->pmu.base.add = i915_pmu_event_add;
> - i915->pmu.base.del = i915_pmu_event_del;
> - i915->pmu.base.start = i915_pmu_event_start;
> - i915->pmu.base.stop = i915_pmu_event_stop;
> - i915->pmu.base.read = i915_pmu_event_read;
> - i915->pmu.base.event_idx = i915_pmu_event_event_idx;
> -
> - spin_lock_init(&i915->pmu.lock);
> - hrtimer_init(&i915->pmu.timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> - i915->pmu.timer.function = i915_sample;
> -
> - ret = perf_pmu_register(&i915->pmu.base, "i915", -1);
> + pmu->base.attr_groups = i915_pmu_attr_groups;
> + pmu->base.task_ctx_nr = perf_invalid_context;
> + pmu->base.event_init = i915_pmu_event_init;
> + pmu->base.add = i915_pmu_event_add;
> + pmu->base.del = i915_pmu_event_del;
> + pmu->base.start = i915_pmu_event_start;
> + pmu->base.stop = i915_pmu_event_stop;
> + pmu->base.read = i915_pmu_event_read;
> + pmu->base.event_idx = i915_pmu_event_event_idx;
> +
> + spin_lock_init(&pmu->lock);
> + hrtimer_init(&pmu->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> + pmu->timer.function = i915_sample;
> +
> + ret = perf_pmu_register(&pmu->base, "i915", -1);
> if (ret)
> goto err;
> - ret = i915_pmu_register_cpuhp_state(i915);
> + ret = i915_pmu_register_cpuhp_state(pmu);
> if (ret)
> goto err_unreg;
> return;
> err_unreg:
> - perf_pmu_unregister(&i915->pmu.base);
> + perf_pmu_unregister(&pmu->base);
> err:
> - i915->pmu.base.event_init = NULL;
> - free_event_attributes(i915);
> + pmu->base.event_init = NULL;
> + free_event_attributes(pmu);
> DRM_NOTE("Failed to register PMU! (err=%d)\n", ret);
> }
> void i915_pmu_unregister(struct drm_i915_private *i915)
and again
> {
> - if (!i915->pmu.base.event_init)
> + struct i915_pmu *pmu = &i915->pmu;
> +
> + if (!pmu->base.event_init)
> return;
> - WARN_ON(i915->pmu.enable);
> + WARN_ON(pmu->enable);
> - hrtimer_cancel(&i915->pmu.timer);
> + hrtimer_cancel(&pmu->timer);
> - i915_pmu_unregister_cpuhp_state(i915);
> + i915_pmu_unregister_cpuhp_state(pmu);
> - perf_pmu_unregister(&i915->pmu.base);
> - i915->pmu.base.event_init = NULL;
> - free_event_attributes(i915);
> + perf_pmu_unregister(&pmu->base);
> + pmu->base.event_init = NULL;
> + free_event_attributes(pmu);
> }
with all possible replacements,
Reviewed-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
More information about the Intel-gfx
mailing list