[Intel-gfx] [RFC 1/4] drm/i915/pmu: Make more struct i915_pmu centric
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Thu Aug 1 12:10:34 UTC 2019
On 01/08/2019 11:46, Michal Wajdeczko wrote:
> 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() ?
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 at intel.com>
If I convert get_rc6 to gt and leave the external API taking i915 for
now would you be happy?
Regards,
Tvrtko
More information about the Intel-gfx
mailing list