| Message ID | 20190801101825.2784-2-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PMU update for more than one card | expand |
Quoting Tvrtko Ursulin (2019-08-01 11:18:22) > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Just tidy the code a bit by removing a sea of overly verbose i915->pmu.*. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Thu, 01 Aug 2019 12:18:22 +0200, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Just tidy the code a bit by removing a sea of overly verbose i915->pmu.*. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com>
Quoting Michal Wajdeczko (2019-08-01 11:46:06) > On Thu, 01 Aug 2019 12:18:22 +0200, Tvrtko Ursulin > <tvrtko.ursulin@linux.intel.com> wrote: > > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > Just tidy the code a bit by removing a sea of overly verbose i915->pmu.*. > > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@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 This is definitely not the final form, so I'm not bothered. I foresee there being a mix of device-level pmu and gt-level pmu, with this being part of the latter. -Chris
On 01/08/2019 11:46, Michal Wajdeczko wrote: > On Thu, 01 Aug 2019 12:18:22 +0200, Tvrtko Ursulin > <tvrtko.ursulin@linux.intel.com> wrote: > >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Just tidy the code a bit by removing a sea of overly verbose i915->pmu.*. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@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() ? Could do, but I thought we are moving away from such constructs? >> 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 I did not want to let the tentacles out of i915_pmu.c at this stage since it is a bit unknown how some bits will look in the future. The ones I did seemed like the safe ones and it satisifed me that after the series there are no more i915->pmu.<something> accesses in i915_pmu.c. (Well there is a singleton on engines_sample but I couldn't justify adding a local for one access.) > >> { >> - 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 Or even better gt? I think gt makes sense. > >> { >> #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@intel.com> If I convert get_rc6 to gt and leave the external API taking i915 for now would you be happy? Regards, Tvrtko
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); 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) { - 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) { - 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) { #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) { + 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) { - 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); }