[PATCH v15 4/6] drm/xe/pmu: Add attribute skeleton
Lucas De Marchi
lucas.demarchi at intel.com
Thu Jan 23 16:36:25 UTC 2025
On Thu, Jan 23, 2025 at 08:32:39PM +0530, Riana Tauro wrote:
>
>
>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 at 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.
on the per-client report to userspace we call it cycles to note
it's a time on the gpu domain, so just calling it "cycles" would
maintain consistency:
https://docs.kernel.org/gpu/drm-usage-stats.html
drm-cycles-<keystr>: <uint>
drm-total-cycles-<keystr>: <uint>
assuming userspace knows the CS frequency they can then even convert to
time unit.
>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.
I'm not against keep it unit less as I see several other drivers doing
so for cycle counters. If we are going to have unitless counters, I can
do that split in this patch.
thanks
Lucas De Marchi
>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
>>>
>
More information about the Intel-xe
mailing list