On 22/12/2017 14:52, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2017-12-21 17:13:16)
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Switch over to dynamically creating device attributes, which are in turn
used by the perf core to expose available counters in sysfs.
This way we do not expose counters which are not avaiable on the current
platform, and are so more consistent between what we reply to open
attempts via the perf_event_open(2), and what is discoverable in sysfs.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
---
+#define __event(__config, __name, __unit) \
+{ \
+ .config = (__config), \
+ .name = (__name), \
+ .unit = (__unit), \
+}
+
+#define __engine_event(__sample, __name) \
+{ \
+ .sample = (__sample), \
+ .name = (__name), \
+}
+
+#define __i915_attr(__p, __name, __config) \
+{ \
+ (__p)->attr.attr.name = (__name); \
+ (__p)->attr.attr.mode = 0444; \
+ (__p)->attr.show = i915_pmu_event_show; \
+ (__p)->val = (__config); \
+}
+
+#define __pmu_attr(__p, __name, __str) \
+{ \
+ (__p)->attr.attr.name = (__name); \
+ (__p)->attr.attr.mode = 0444; \
+ (__p)->attr.show = perf_event_sysfs_show; \
+ (__p)->event_str = (__str); \
+}
+
+static struct attribute **
+create_event_attributes(struct drm_i915_private *i915)
+{
+ static const struct {
+ u64 config;
+ const char *name;
+ const char *unit;
+ } events[] = {
+ __event(I915_PMU_ACTUAL_FREQUENCY, "actual-frequency", "MHz"),
+ __event(I915_PMU_REQUESTED_FREQUENCY, "requested-frequency", "MHz"),
+ __event(I915_PMU_INTERRUPTS, "interrupts", NULL),
+ __event(I915_PMU_RC6_RESIDENCY, "rc6-residency", "ns"),
+ };
+ static const struct {
+ enum drm_i915_pmu_engine_sample sample;
+ char *name;
+ } engine_events[] = {
+ __engine_event(I915_SAMPLE_BUSY, "busy"),
+ __engine_event(I915_SAMPLE_SEMA, "sema"),
+ __engine_event(I915_SAMPLE_WAIT, "wait"),
Are these macros that useful? vs { I915_SAMPLE_BUSY, "busy" } and
{ I915_PMU_ACTUAL_FREQUENCY, "actual-frequency", "MHz" },
They are marginal.. I preferred to used named initialization to be more
mistake proof, in which case they do shorten the lines here a bit. It's
50-50 for me, or don't really care.
+ };
+ unsigned int count = 0;
+ struct perf_pmu_events_attr *pmu_attr, *pmu_p;
+ struct i915_ext_attribute *i915_attr, *i915_p;
+ struct intel_engine_cs *engine;
+ struct attribute **attr, **p;
+ enum intel_engine_id id;
+ unsigned int i;
+
+ /* Count how many counters we will be exposing. */
+ for (i = 0; i < ARRAY_SIZE(events); i++) {
+ if (!config_status(i915, events[i].config))
+ count++;
+ }
+
+ for_each_engine(engine, i915, id) {
+ for (i = 0; i < ARRAY_SIZE(engine_events); i++) {
+ if (!engine_event_status(engine,
+ engine_events[i].sample))
+ count++;
+ }
+ }
+
+ /* Allocate attribute objects and table. */
+ i915_attr = kzalloc(count * sizeof(*i915_attr), GFP_KERNEL);
+ if (!i915_attr)
+ return NULL;
+
+ pmu_attr = kzalloc(count * sizeof(*pmu_attr), GFP_KERNEL);
+ if (!pmu_attr) {
+ kfree(i915_attr);
+ return NULL;
+ }
+
+ /* Max one pointer of each attribute type plus a termination entry. */
+ attr = kzalloc((count * 2 + 1) * sizeof(attr), GFP_KERNEL);
+ if (!attr) {
+ kfree(pmu_attr);
+ kfree(i915_attr);
+ return NULL;
Joonas wants you to feed through the same error unwind.
Yeah, I've almost done it at one point. :)
+ }
+
+ i915_p = i915_attr;
+ pmu_p = pmu_attr;
+ p = attr;
i915_attr_iter, pmu_attr_iter and attr_iter?
Shrug, can do.
+ /* Initialize supported non-engine counters. */
+ for (i = 0; i < ARRAY_SIZE(events); i++) {
+ char *str;
+
+ if (config_status(i915, events[i].config))
+ continue;
+
+ str = kstrdup(events[i].name, GFP_KERNEL);
+ if (!str)
+ goto err;
+
+ __i915_attr(i915_p, str, events[i].config);
*attr_iter++ = &i915_attr_iter->attr.attr;
*i915_attr_iter++ = (struct i915_ext_attribute) {
.attr.attr.name = str,
.attr.attr.mode = 0444,
.attr.show = i915_pmu_event_show,
.val = events[i].config,
}
?
Partially prettier but since I had two instances of each attr
initialization I liked how the macro shrank the code. Perhaps I make the
macro increment the pointer and return it? Or make it a static inline even.
i915_attr_iter = __add_i915_attr(i915_attr_iter, str, ...);
?
Mere suggestions. Doesn't look that bad
Thanks for looking. I think the largest benefit is future proofing the
maintenance. Secondary, but also attractive, that the unsupported events
do not list.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
