Re: [PATCH v15 4/6] drm/xe/pmu: Add attribute skeleton

[Riana Tauro <riana.tauro@intel.com>]

On 1/23/2025 8:18 PM, Lucas De Marchi wrote:
> On Thu, Jan 23, 2025 at 07:24:22PM +0530, Riana Tauro wrote:
>> Hi Lucas
>>
>> On 1/23/2025 9:49 AM, Lucas De Marchi wrote:
>>> Add the generic support for defining new attributes. This uses
>>> gt-c6-residency as first attribute to bootstrap it, but its
>>> implementation will be added by a follow up commit: until proper support
>>> is added, it will always be invisible in sysfs since the corresponding
>>> bit is not set in the supported_events bitmap.
>>>
>>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>>> ---
>>>  drivers/gpu/drm/xe/xe_pmu.c       | 60 ++++++++++++++++++++++++++++---
>>>  drivers/gpu/drm/xe/xe_pmu_types.h |  4 +++
>>>  2 files changed, 60 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_pmu.c b/drivers/gpu/drm/xe/xe_pmu.c
>>> index 33598272db6aa..5e5a9fcf30ace 100644
>>> --- a/drivers/gpu/drm/xe/xe_pmu.c
>>> +++ b/drivers/gpu/drm/xe/xe_pmu.c
>>> @@ -46,6 +46,8 @@ static unsigned int config_to_gt_id(u64 config)
>>>      return FIELD_GET(XE_PMU_EVENT_GT_MASK, config);
>>>  }
>>> +#define XE_PMU_EVENT_GT_C6_RESIDENCY    0x01
>>> +
>>>  static struct xe_gt *event_to_gt(struct perf_event *event)
>>>  {
>>>      struct xe_device *xe = container_of(event->pmu, typeof(*xe), 
>>> pmu.base);
>>> @@ -60,7 +62,8 @@ static bool event_supported(struct xe_pmu *pmu, 
>>> unsigned int gt,
>>>      if (gt >= XE_MAX_GT_PER_TILE)
>>>          return false;
>>> -    return false;
>>> +    return id < sizeof(pmu->supported_events) * BITS_PER_BYTE &&
>>> +        pmu->supported_events & BIT_ULL(id);
>>>  }
>>>  static void xe_pmu_event_destroy(struct perf_event *event)
>>> @@ -210,16 +213,62 @@ static const struct attribute_group 
>>> pmu_format_attr_group = {
>>>      .attrs = pmu_format_attrs,
>>>  };
>>> -static struct attribute *pmu_event_attrs[] = {
>>> -    /* No events yet */
>>> +static ssize_t event_attr_show(struct device *dev,
>>> +                   struct device_attribute *attr, char *buf)
>>> +{
>>> +    struct perf_pmu_events_attr *pmu_attr =
>>> +        container_of(attr, struct perf_pmu_events_attr, attr);
>>> +
>>> +    return sprintf(buf, "event=%#04llx\n", pmu_attr->id);
>>> +}
>>> +
>>> +#define XE_EVENT_ATTR(name_, v_, id_, unit_)                \
>>> +    PMU_EVENT_ATTR(name_, pmu_event_ ## v_, id_, event_attr_show)    \
>>> +    PMU_EVENT_ATTR_STRING(name_.unit, pmu_event_unit_ ## v_, 
>>> unit_)    \
>>> +    static struct attribute *pmu_attr_ ##v_[] = {            \
>>> +        &pmu_event_ ##v_.attr.attr,                \
>>> +        &pmu_event_unit_ ##v_.attr.attr,            \
>>> +        NULL                            \
>>> +    };                                \
>>> +    static umode_t is_visible_##v_(struct kobject *kobj,        \
>>> +                       struct attribute *attr, int idx) \
>>> +    {                                \
>>> +        struct perf_pmu_events_attr *pmu_attr;            \
>>> +        struct xe_pmu *pmu;                    \
>>> +                                    \
>>> +        pmu_attr = container_of(attr, typeof(*pmu_attr), attr.attr); \
>>> +        pmu = container_of(dev_get_drvdata(kobj_to_dev(kobj)),    \
>>> +                   typeof(*pmu), base);            \
>>> +                                    \
>>> +        return event_supported(pmu, 0, id_) ? attr->mode : 0;    \
>>> +    }                                \
>>> +    static const struct attribute_group pmu_group_ ##v_ = {        \
>>> +        .name = "events",                    \
>>> +        .attrs = pmu_attr_ ## v_,                \
>>> +        .is_visible = is_visible_ ## v_,            \
>>> +    }
>>> +
>> There will be some events that do not have the unit. The previous rev 
>> was simpler to handle such events
>>
>> Can the previous revision be retained?
> 
> that would depend on merging the first patch in perf/core or bring that
> define here.
> 
> I think we can live with the similar approach of other drivers above and
> in future try to add PMU_EVENT_ATTR_ID_STRING. However I'd like to have pmu
> merged in xe without depending on that.
Sorry, my bad. I missed that the core patch was removed in this series
  And if we simply add the macro
> in xe, it means it will probably stay forever with nobody cleaning it up
> later.
> 
> Also, what event are you talking about? 
The upcoming per-engine-class-activity counters. They are similar to the 
per-client and have no units.
There are 2 or 3 alternatives:
> 
> 1) add a unit
> 
> 2) add an indirection to the macro above to allow defining an event
> without unit. That's doable by having 3 macros: __XE_EVENT_ATTR,
> XE_EVENT_ATTR, and XE_EVENT_ATTR_WITH_UNIT. Then you'd have:
> 
> XE_EVENT_ATTR_WITH_UNIT(foo, ...)
> XE_EVENT_ATTR(bar, ...)
> 
> 
> 3) Ungroup the macro so the group is separate, then we'd do:
> 
>      XE_EVENT_ATTR(foo)
>      XE_EVENT_ATTR_UNIT(foo, "ms")
>      XE_EVENT_GROUP(foo, &pmu_event_foo.attr.attr,
>                 &pmu_event_unit_foo.attr.attr)
> 
>      XE_EVENT_ATTR(bar)
>      XE_EVENT_GROUP(bar, &pmu_event_bar.attr.attr)
> 
Yeah will have to seperate it out.
Sorry for the confusion.

Thanks
Riana
> Lucas De Marchi
> 
>>
>> Thanks
>> Riana
>>> +XE_EVENT_ATTR(gt-c6-residency, gt_c6_residency, 
>>> XE_PMU_EVENT_GT_C6_RESIDENCY, "ms");
>>> +
>>> +static struct attribute *pmu_empty_event_attrs[] = {
>>> +    /* Empty - all events are added as groups with .attr_update() */
>>>      NULL,
>>>  };
>>>  static const struct attribute_group pmu_events_attr_group = {
>>>      .name = "events",
>>> -    .attrs = pmu_event_attrs,
>>> +    .attrs = pmu_empty_event_attrs,
>>>  };
>>> +static const struct attribute_group *pmu_events_attr_update[] = {
>>> +    &pmu_group_gt_c6_residency,
>>> +    NULL,
>>> +};
>>> +
>>> +static void set_supported_events(struct xe_pmu *pmu)
>>> +{
>>> +}
>>> +
>>>  /**
>>>   * xe_pmu_unregister() - Remove/cleanup PMU registration
>>>   * @arg: Ptr to pmu
>>> @@ -270,6 +319,7 @@ int xe_pmu_register(struct xe_pmu *pmu)
>>>      pmu->name        = name;
>>>      pmu->base.attr_groups    = attr_groups;
>>> +    pmu->base.attr_update    = pmu_events_attr_update;
>>>      pmu->base.scope        = PERF_PMU_SCOPE_SYS_WIDE;
>>>      pmu->base.module    = THIS_MODULE;
>>>      pmu->base.task_ctx_nr    = perf_invalid_context;
>>> @@ -280,6 +330,8 @@ int xe_pmu_register(struct xe_pmu *pmu)
>>>      pmu->base.stop        = xe_pmu_event_stop;
>>>      pmu->base.read        = xe_pmu_event_read;
>>> +    set_supported_events(pmu);
>>> +
>>>      ret = perf_pmu_register(&pmu->base, pmu->name, -1);
>>>      if (ret)
>>>          goto err_name;
>>> diff --git a/drivers/gpu/drm/xe/xe_pmu_types.h b/drivers/gpu/drm/xe/ 
>>> xe_pmu_types.h
>>> index 0e8faae6bc1b3..f5ba4d56622cb 100644
>>> --- a/drivers/gpu/drm/xe/xe_pmu_types.h
>>> +++ b/drivers/gpu/drm/xe/xe_pmu_types.h
>>> @@ -30,6 +30,10 @@ struct xe_pmu {
>>>       * @name: Name as registered with perf core.
>>>       */
>>>      const char *name;
>>> +    /**
>>> +     * @supported_events: Bitmap of supported events, indexed by 
>>> event id
>>> +     */
>>> +    u64 supported_events;
>>>  };
>>>  #endif
>>