[Intel-gfx] [PATCH] drm/i915/pmu: Only enumerate available counters in sysfs

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-12-21 17:13:16)
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
> 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 at intel.com>
> ---
> +#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