Re: [PATCH] drm/i915/pmu: Only enumerate available counters in sysfs

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 22/12/2017 14:52, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2017-12-21 17:13:16)
>> From: Tvrtko Ursulin <tvrtko.ursulin@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@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" },

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx