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

[Lucas De Marchi]

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. 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? 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)

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
>