Re: [RFC 1/4] drm/i915/pmu: Make more struct i915_pmu centric

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 


On 01/08/2019 11:46, Michal Wajdeczko wrote:
On Thu, 01 Aug 2019 12:18:22 +0200, Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote: 


From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>

Just tidy the code a bit by removing a sea of overly verbose i915->pmu.*.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
---
 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@xxxxxxxxx>
If I convert get_rc6 to gt and leave the external API taking i915 for now would you be happy? 

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux