[RFC,1/4] drm/i915/pmu: Make more struct i915_pmu centric
diff mbox series

Message ID 20190801101825.2784-2-tvrtko.ursulin@linux.intel.com
State New
Headers show
Series
  • PMU update for more than one card
Related show

Commit Message

Tvrtko Ursulin Aug. 1, 2019, 10:18 a.m. UTC
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(-)

Comments

Chris Wilson Aug. 1, 2019, 10:31 a.m. UTC | #1
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
Michal Wajdeczko Aug. 1, 2019, 10:46 a.m. UTC | #2
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>
Chris Wilson Aug. 1, 2019, 10:53 a.m. UTC | #3
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
Tvrtko Ursulin Aug. 1, 2019, 12:10 p.m. UTC | #4
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

Patch
diff mbox series

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);
 }