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" },
> + };
> + 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.
> + }
> +
> + i915_p = i915_attr;
> + pmu_p = pmu_attr;
> + p = attr;
i915_attr_iter, pmu_attr_iter and attr_iter?
> + /* 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,
}
?
Mere suggestions. Doesn't look that bad
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
