[PATCH v15 0/6] drm/xe/pmu: PMU interface for Xe

[PATCH v15 0/6] drm/xe/pmu: PMU interface for Xe

[Lucas De Marchi]

Previous versions: https://patchwork.freedesktop.org/series/139121/

For perf/core: first patch adds a new macro in include/linux/perf_event.h.
If it's ok, I'd like to merge it through the drm branch so we can add the
pmu implementation to the xe driver. I'm also Cc'ing linux-perf-users@vger.kernel.org
on the other patches to show its usage.

For "drm/xe/pmu: Enable PMU interface", previously from several authors,
I squashed several changes I suggested in the past to clean it up and
reduce boilerplate code that resulted from copy-paste from i915. More
detailed changes can be found at
https://gitlab.freedesktop.org/demarchi/xe/-/tags/tip-xe-pmu-v13

I tested this on Lunar Lake and ADL with and without workload running
and the gt-c6 residency seems reasonable.

v13: this only has 1 event as Vinay said he will rebase the
frequency event on top.

v14
  - simplify the gt-c6 event: no more estimate based on suspend time
since we can't take the spinlock. A simpler version is to get runtime pm
when initializing the event and putting it back when it's done.  In
future we can come back with fancier things that don´t block the runtime
pm.

v15:
  - clean up no longer needed include files
  - drop no longer needed struct members
  - drop patch adding PMU_EVENT_ATTR_ID_STRING: this can be done later
    on top if desired, converting more drivers. There's no pressure need
    to be different than the other drivers. Instead, do the attributes
    similarly to drivers/iommu/intel/perfmon.c, but still keep a single
    macro to define both event, scale and event group

Lucas De Marchi (4):
  drm/xe/pmu: Assert max gt
  drm/xe/pmu: Extract xe_pmu_event_update()
  drm/xe/pmu: Add attribute skeleton
  drm/xe/pmu: Get/put runtime pm on event init

Vinay Belgaumkar (2):
  drm/xe/pmu: Enable PMU interface
  drm/xe/pmu: Add GT C6 events

 drivers/gpu/drm/xe/Makefile          |   2 +
 drivers/gpu/drm/xe/xe_device.c       |   3 +
 drivers/gpu/drm/xe/xe_device_types.h |   4 +
 drivers/gpu/drm/xe/xe_pmu.c          | 361 +++++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_pmu.h          |  18 ++
 drivers/gpu/drm/xe/xe_pmu_types.h    |  39 +++
 6 files changed, 427 insertions(+)
 create mode 100644 drivers/gpu/drm/xe/xe_pmu.c
 create mode 100644 drivers/gpu/drm/xe/xe_pmu.h
 create mode 100644 drivers/gpu/drm/xe/xe_pmu_types.h

-- 
2.48.0

[PATCH v15 1/6] drm/xe/pmu: Enable PMU interface

[Lucas De Marchi]

From: Vinay Belgaumkar <vinay.belgaumkar@intel.com>

Basic PMU enabling patch. Setup the basic framework
for adding events/timers. This patch was previously
reviewed here -
https://patchwork.freedesktop.org/series/119504/

Based on previous versions by Bommu Krishnaiah, Aravind Iddamsetty and
Riana Tauro, using i915 and rapl as reference implementation.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/xe/Makefile          |   2 +
 drivers/gpu/drm/xe/xe_device.c       |   3 +
 drivers/gpu/drm/xe/xe_device_types.h |   4 +
 drivers/gpu/drm/xe/xe_pmu.c          | 289 +++++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_pmu.h          |  18 ++
 drivers/gpu/drm/xe/xe_pmu_types.h    |  35 ++++
 6 files changed, 351 insertions(+)
 create mode 100644 drivers/gpu/drm/xe/xe_pmu.c
 create mode 100644 drivers/gpu/drm/xe/xe_pmu.h
 create mode 100644 drivers/gpu/drm/xe/xe_pmu_types.h

diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
index 68861db5f27ce..aa0d981663e4c 100644
--- a/drivers/gpu/drm/xe/Makefile
+++ b/drivers/gpu/drm/xe/Makefile
@@ -305,6 +305,8 @@ endif
 xe-$(CONFIG_DRM_XE_DP_TUNNEL) += \
 	i915-display/intel_dp_tunnel.o
 
+xe-$(CONFIG_PERF_EVENTS) += xe_pmu.o
+
 obj-$(CONFIG_DRM_XE) += xe.o
 obj-$(CONFIG_DRM_XE_KUNIT_TEST) += tests/
 
diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
index bd6191e1ed3e7..f3f754beb812b 100644
--- a/drivers/gpu/drm/xe/xe_device.c
+++ b/drivers/gpu/drm/xe/xe_device.c
@@ -49,6 +49,7 @@
 #include "xe_pat.h"
 #include "xe_pcode.h"
 #include "xe_pm.h"
+#include "xe_pmu.h"
 #include "xe_query.h"
 #include "xe_sriov.h"
 #include "xe_tile.h"
@@ -871,6 +872,8 @@ int xe_device_probe(struct xe_device *xe)
 
 	xe_oa_register(xe);
 
+	xe_pmu_register(&xe->pmu);
+
 	xe_debugfs_register(xe);
 
 	xe_hwmon_register(xe);
diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
index 16ebb2859877f..58e79e19deaad 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -18,6 +18,7 @@
 #include "xe_memirq_types.h"
 #include "xe_oa_types.h"
 #include "xe_platform_types.h"
+#include "xe_pmu_types.h"
 #include "xe_pt_types.h"
 #include "xe_sriov_types.h"
 #include "xe_step_types.h"
@@ -514,6 +515,9 @@ struct xe_device {
 		int mode;
 	} wedged;
 
+	/** @pmu: performance monitoring unit */
+	struct xe_pmu pmu;
+
 #ifdef TEST_VM_OPS_ERROR
 	/**
 	 * @vm_inject_error_position: inject errors at different places in VM
diff --git a/drivers/gpu/drm/xe/xe_pmu.c b/drivers/gpu/drm/xe/xe_pmu.c
new file mode 100644
index 0000000000000..b3da3863928af
--- /dev/null
+++ b/drivers/gpu/drm/xe/xe_pmu.c
@@ -0,0 +1,289 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2025 Intel Corporation
+ */
+
+#include <drm/drm_drv.h>
+#include <linux/device.h>
+
+#include "xe_device.h"
+#include "xe_pmu.h"
+
+/**
+ * DOC: Xe PMU (Performance Monitoring Unit)
+ *
+ * Expose events/counters like GT-C6 residency and GT frequency to user land via
+ * the perf interface. Events are per device. The GT can be selected with an
+ * extra config sub-field (bits 60-63).
+ *
+ * All events are listed in sysfs:
+ *
+ *     $ ls -ld /sys/bus/event_source/devices/xe_*
+ *     $ ls /sys/bus/event_source/devices/xe_0000_00_02.0/events/
+ *     $ ls /sys/bus/event_source/devices/xe_0000_00_02.0/format/
+ *
+ * The format directory has info regarding the configs that can be used.
+ * The standard perf tool can be used to grep for a certain event as well.
+ * Example:
+ *
+ *     $ perf list | grep gt-c6
+ *
+ * To sample a specific event for a GT at regular intervals:
+ *
+ *     $ perf stat -e <event_name,gt=> -I <interval>
+ */
+
+#define XE_PMU_EVENT_GT_MASK		GENMASK_ULL(63, 60)
+#define XE_PMU_EVENT_ID_MASK		GENMASK_ULL(11, 0)
+
+static unsigned int config_to_event_id(u64 config)
+{
+	return FIELD_GET(XE_PMU_EVENT_ID_MASK, config);
+}
+
+static unsigned int config_to_gt_id(u64 config)
+{
+	return FIELD_GET(XE_PMU_EVENT_GT_MASK, config);
+}
+
+static struct xe_gt *event_to_gt(struct perf_event *event)
+{
+	struct xe_device *xe = container_of(event->pmu, typeof(*xe), pmu.base);
+	u64 gt = config_to_gt_id(event->attr.config);
+
+	return xe_device_get_gt(xe, gt);
+}
+
+static bool event_supported(struct xe_pmu *pmu, unsigned int gt,
+			    unsigned int id)
+{
+	if (gt >= XE_MAX_GT_PER_TILE)
+		return false;
+
+	return false;
+}
+
+static void xe_pmu_event_destroy(struct perf_event *event)
+{
+	struct xe_device *xe = container_of(event->pmu, typeof(*xe), pmu.base);
+
+	drm_WARN_ON(&xe->drm, event->parent);
+	drm_dev_put(&xe->drm);
+}
+
+static int xe_pmu_event_init(struct perf_event *event)
+{
+	struct xe_device *xe = container_of(event->pmu, typeof(*xe), pmu.base);
+	struct xe_pmu *pmu = &xe->pmu;
+	unsigned int id, gt;
+
+	if (!pmu->registered)
+		return -ENODEV;
+
+	if (event->attr.type != event->pmu->type)
+		return -ENOENT;
+
+	/* unsupported modes and filters */
+	if (event->attr.sample_period) /* no sampling */
+		return -EINVAL;
+
+	if (event->cpu < 0)
+		return -EINVAL;
+
+	gt = config_to_gt_id(event->attr.config);
+	id = config_to_event_id(event->attr.config);
+	if (!event_supported(pmu, gt, id))
+		return -ENOENT;
+
+	if (has_branch_stack(event))
+		return -EOPNOTSUPP;
+
+	if (!event->parent) {
+		drm_dev_get(&xe->drm);
+		event->destroy = xe_pmu_event_destroy;
+	}
+
+	return 0;
+}
+
+static u64 __xe_pmu_event_read(struct perf_event *event)
+{
+	struct xe_gt *gt = event_to_gt(event);
+	u64 val = 0;
+
+	if (!gt)
+		return 0;
+
+	return val;
+}
+
+static void xe_pmu_event_read(struct perf_event *event)
+{
+	struct xe_device *xe = container_of(event->pmu, typeof(*xe), pmu.base);
+	struct hw_perf_event *hwc = &event->hw;
+	struct xe_pmu *pmu = &xe->pmu;
+	u64 prev, new;
+
+	if (!pmu->registered) {
+		event->hw.state = PERF_HES_STOPPED;
+		return;
+	}
+
+	prev = local64_read(&hwc->prev_count);
+	do {
+		new = __xe_pmu_event_read(event);
+	} while (!local64_try_cmpxchg(&hwc->prev_count, &prev, new));
+
+	local64_add(new - prev, &event->count);
+}
+
+static void xe_pmu_enable(struct perf_event *event)
+{
+	/*
+	 * Store the current counter value so we can report the correct delta
+	 * for all listeners. Even when the event was already enabled and has
+	 * an existing non-zero value.
+	 */
+	local64_set(&event->hw.prev_count, __xe_pmu_event_read(event));
+}
+
+static void xe_pmu_event_start(struct perf_event *event, int flags)
+{
+	struct xe_device *xe = container_of(event->pmu, typeof(*xe), pmu.base);
+	struct xe_pmu *pmu = &xe->pmu;
+
+	if (!pmu->registered)
+		return;
+
+	xe_pmu_enable(event);
+	event->hw.state = 0;
+}
+
+static void xe_pmu_event_stop(struct perf_event *event, int flags)
+{
+	struct xe_device *xe = container_of(event->pmu, typeof(*xe), pmu.base);
+	struct xe_pmu *pmu = &xe->pmu;
+
+	if (pmu->registered)
+		if (flags & PERF_EF_UPDATE)
+			xe_pmu_event_read(event);
+
+	event->hw.state = PERF_HES_STOPPED;
+}
+
+static int xe_pmu_event_add(struct perf_event *event, int flags)
+{
+	struct xe_device *xe = container_of(event->pmu, typeof(*xe), pmu.base);
+	struct xe_pmu *pmu = &xe->pmu;
+
+	if (!pmu->registered)
+		return -ENODEV;
+
+	if (flags & PERF_EF_START)
+		xe_pmu_event_start(event, flags);
+
+	return 0;
+}
+
+static void xe_pmu_event_del(struct perf_event *event, int flags)
+{
+	xe_pmu_event_stop(event, PERF_EF_UPDATE);
+}
+
+PMU_FORMAT_ATTR(gt,	"config:60-63");
+PMU_FORMAT_ATTR(event,	"config:0-11");
+
+static struct attribute *pmu_format_attrs[] = {
+	&format_attr_event.attr,
+	&format_attr_gt.attr,
+	NULL,
+};
+
+static const struct attribute_group pmu_format_attr_group = {
+	.name = "format",
+	.attrs = pmu_format_attrs,
+};
+
+static struct attribute *pmu_event_attrs[] = {
+	/* No events yet */
+	NULL,
+};
+
+static const struct attribute_group pmu_events_attr_group = {
+	.name = "events",
+	.attrs = pmu_event_attrs,
+};
+
+/**
+ * xe_pmu_unregister() - Remove/cleanup PMU registration
+ * @arg: Ptr to pmu
+ */
+static void xe_pmu_unregister(void *arg)
+{
+	struct xe_pmu *pmu = arg;
+	struct xe_device *xe = container_of(pmu, typeof(*xe), pmu);
+
+	if (!pmu->registered)
+		return;
+
+	pmu->registered = false;
+
+	perf_pmu_unregister(&pmu->base);
+	kfree(pmu->name);
+}
+
+/**
+ * xe_pmu_register() - Define basic PMU properties for Xe and add event callbacks.
+ * @pmu: the PMU object
+ *
+ * Returns 0 on success and an appropriate error code otherwise
+ */
+int xe_pmu_register(struct xe_pmu *pmu)
+{
+	struct xe_device *xe = container_of(pmu, typeof(*xe), pmu);
+	static const struct attribute_group *attr_groups[] = {
+		&pmu_format_attr_group,
+		&pmu_events_attr_group,
+		NULL
+	};
+	int ret = -ENOMEM;
+	char *name;
+
+	if (IS_SRIOV_VF(xe))
+		return 0;
+
+	name = kasprintf(GFP_KERNEL, "xe_%s",
+			 dev_name(xe->drm.dev));
+	if (!name)
+		goto err;
+
+	/* tools/perf reserves colons as special. */
+	strreplace(name, ':', '_');
+
+	pmu->name		= name;
+	pmu->base.attr_groups	= attr_groups;
+	pmu->base.scope		= PERF_PMU_SCOPE_SYS_WIDE;
+	pmu->base.module	= THIS_MODULE;
+	pmu->base.task_ctx_nr	= perf_invalid_context;
+	pmu->base.event_init	= xe_pmu_event_init;
+	pmu->base.add		= xe_pmu_event_add;
+	pmu->base.del		= xe_pmu_event_del;
+	pmu->base.start		= xe_pmu_event_start;
+	pmu->base.stop		= xe_pmu_event_stop;
+	pmu->base.read		= xe_pmu_event_read;
+
+	ret = perf_pmu_register(&pmu->base, pmu->name, -1);
+	if (ret)
+		goto err_name;
+
+	pmu->registered = true;
+
+	return devm_add_action_or_reset(xe->drm.dev, xe_pmu_unregister, pmu);
+
+err_name:
+	kfree(name);
+err:
+	drm_err(&xe->drm, "Failed to register PMU (ret=%d)!\n", ret);
+
+	return ret;
+}
diff --git a/drivers/gpu/drm/xe/xe_pmu.h b/drivers/gpu/drm/xe/xe_pmu.h
new file mode 100644
index 0000000000000..60c37126f87ec
--- /dev/null
+++ b/drivers/gpu/drm/xe/xe_pmu.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2025 Intel Corporation
+ */
+
+#ifndef _XE_PMU_H_
+#define _XE_PMU_H_
+
+#include "xe_pmu_types.h"
+
+#if IS_ENABLED(CONFIG_PERF_EVENTS)
+int xe_pmu_register(struct xe_pmu *pmu);
+#else
+static inline int xe_pmu_register(struct xe_pmu *pmu) { return 0; }
+#endif
+
+#endif
+
diff --git a/drivers/gpu/drm/xe/xe_pmu_types.h b/drivers/gpu/drm/xe/xe_pmu_types.h
new file mode 100644
index 0000000000000..0e8faae6bc1b3
--- /dev/null
+++ b/drivers/gpu/drm/xe/xe_pmu_types.h
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2025 Intel Corporation
+ */
+
+#ifndef _XE_PMU_TYPES_H_
+#define _XE_PMU_TYPES_H_
+
+#include <linux/perf_event.h>
+#include <linux/spinlock_types.h>
+
+#define XE_PMU_MAX_GT 2
+
+/**
+ * struct xe_pmu - PMU related data per Xe device
+ *
+ * Stores per device PMU info that includes event/perf attributes and sampling
+ * counters across all GTs for this device.
+ */
+struct xe_pmu {
+	/**
+	 * @base: PMU base.
+	 */
+	struct pmu base;
+	/**
+	 * @registered: PMU is registered and not in the unregistering process.
+	 */
+	bool registered;
+	/**
+	 * @name: Name as registered with perf core.
+	 */
+	const char *name;
+};
+
+#endif
-- 
2.48.0

[PATCH v15 2/6] drm/xe/pmu: Assert max gt

[Lucas De Marchi]

XE_PMU_MAX_GT needs to be used due to a circular dependency, but we
should make sure it doesn't go out of sync with XE_PMU_MAX_GT. Add a
compile check for that.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/xe/xe_pmu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/xe/xe_pmu.c b/drivers/gpu/drm/xe/xe_pmu.c
index b3da3863928af..df93ba96bdc1e 100644
--- a/drivers/gpu/drm/xe/xe_pmu.c
+++ b/drivers/gpu/drm/xe/xe_pmu.c
@@ -249,6 +249,8 @@ int xe_pmu_register(struct xe_pmu *pmu)
 	int ret = -ENOMEM;
 	char *name;
 
+	BUILD_BUG_ON(XE_MAX_GT_PER_TILE != XE_PMU_MAX_GT);
+
 	if (IS_SRIOV_VF(xe))
 		return 0;
 
-- 
2.48.0

[PATCH v15 3/6] drm/xe/pmu: Extract xe_pmu_event_update()

[Lucas De Marchi]

Like other pmu drivers, keep the update separate from the read so it can
be called from other methods (like stop()) without side effects.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/xe/xe_pmu.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_pmu.c b/drivers/gpu/drm/xe/xe_pmu.c
index df93ba96bdc1e..33598272db6aa 100644
--- a/drivers/gpu/drm/xe/xe_pmu.c
+++ b/drivers/gpu/drm/xe/xe_pmu.c
@@ -117,18 +117,11 @@ static u64 __xe_pmu_event_read(struct perf_event *event)
 	return val;
 }
 
-static void xe_pmu_event_read(struct perf_event *event)
+static void xe_pmu_event_update(struct perf_event *event)
 {
-	struct xe_device *xe = container_of(event->pmu, typeof(*xe), pmu.base);
 	struct hw_perf_event *hwc = &event->hw;
-	struct xe_pmu *pmu = &xe->pmu;
 	u64 prev, new;
 
-	if (!pmu->registered) {
-		event->hw.state = PERF_HES_STOPPED;
-		return;
-	}
-
 	prev = local64_read(&hwc->prev_count);
 	do {
 		new = __xe_pmu_event_read(event);
@@ -137,6 +130,19 @@ static void xe_pmu_event_read(struct perf_event *event)
 	local64_add(new - prev, &event->count);
 }
 
+static void xe_pmu_event_read(struct perf_event *event)
+{
+	struct xe_device *xe = container_of(event->pmu, typeof(*xe), pmu.base);
+	struct xe_pmu *pmu = &xe->pmu;
+
+	if (!pmu->registered) {
+		event->hw.state = PERF_HES_STOPPED;
+		return;
+	}
+
+	xe_pmu_event_update(event);
+}
+
 static void xe_pmu_enable(struct perf_event *event)
 {
 	/*
@@ -166,7 +172,7 @@ static void xe_pmu_event_stop(struct perf_event *event, int flags)
 
 	if (pmu->registered)
 		if (flags & PERF_EF_UPDATE)
-			xe_pmu_event_read(event);
+			xe_pmu_event_update(event);
 
 	event->hw.state = PERF_HES_STOPPED;
 }
-- 
2.48.0

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

[Lucas De Marchi]

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_,			\
+	}
+
+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
-- 
2.48.0

[PATCH v15 5/6] drm/xe/pmu: Get/put runtime pm on event init

[Lucas De Marchi]

When the event is created, make sure runtime pm is taken and later put:
in order to read an event counter the GPU needs to remain accessible and
doing a get/put during perf's read is not possible it's holding a
raw_spinlock.

Suggested-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/xe/xe_pmu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/xe/xe_pmu.c b/drivers/gpu/drm/xe/xe_pmu.c
index 5e5a9fcf30ace..2b66a7957b665 100644
--- a/drivers/gpu/drm/xe/xe_pmu.c
+++ b/drivers/gpu/drm/xe/xe_pmu.c
@@ -7,6 +7,7 @@
 #include <linux/device.h>
 
 #include "xe_device.h"
+#include "xe_pm.h"
 #include "xe_pmu.h"
 
 /**
@@ -71,6 +72,7 @@ static void xe_pmu_event_destroy(struct perf_event *event)
 	struct xe_device *xe = container_of(event->pmu, typeof(*xe), pmu.base);
 
 	drm_WARN_ON(&xe->drm, event->parent);
+	xe_pm_runtime_put(xe);
 	drm_dev_put(&xe->drm);
 }
 
@@ -103,6 +105,7 @@ static int xe_pmu_event_init(struct perf_event *event)
 
 	if (!event->parent) {
 		drm_dev_get(&xe->drm);
+		xe_pm_runtime_get(xe);
 		event->destroy = xe_pmu_event_destroy;
 	}
 
-- 
2.48.0

[PATCH v15 6/6] drm/xe/pmu: Add GT C6 events

[Lucas De Marchi]

From: Vinay Belgaumkar <vinay.belgaumkar@intel.com>

Provide a PMU interface for GT C6 residency counters. The interface is
similar to the one available for i915, but gt is passed in the config
when creating the event.

Sample usage and output:

	$ perf list | grep gt-c6
	  xe_0000_00_02.0/gt-c6-residency/                   [Kernel PMU event]

	$ tail /sys/bus/event_source/devices/xe_0000_00_02.0/events/gt-c6-residency*
	==> /sys/bus/event_source/devices/xe_0000_00_02.0/events/gt-c6-residency <==
	event=0x01

	==> /sys/bus/event_source/devices/xe_0000_00_02.0/events/gt-c6-residency.unit <==
	ms

	$ perf stat -e xe_0000_00_02.0/gt-c6-residency,gt=0/ -I1000
	#           time             counts unit events
	     1.001196056              1,001 ms   xe_0000_00_02.0/gt-c6-residency,gt=0/
	     2.005216219              1,003 ms   xe_0000_00_02.0/gt-c6-residency,gt=0/

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Reviewed-by: Riana Tauro <riana.tauro@intel.com>
Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/xe/xe_pmu.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_pmu.c b/drivers/gpu/drm/xe/xe_pmu.c
index 2b66a7957b665..850bc48053a6b 100644
--- a/drivers/gpu/drm/xe/xe_pmu.c
+++ b/drivers/gpu/drm/xe/xe_pmu.c
@@ -7,6 +7,7 @@
 #include <linux/device.h>
 
 #include "xe_device.h"
+#include "xe_gt_idle.h"
 #include "xe_pm.h"
 #include "xe_pmu.h"
 
@@ -115,12 +116,16 @@ static int xe_pmu_event_init(struct perf_event *event)
 static u64 __xe_pmu_event_read(struct perf_event *event)
 {
 	struct xe_gt *gt = event_to_gt(event);
-	u64 val = 0;
 
 	if (!gt)
 		return 0;
 
-	return val;
+	switch (config_to_event_id(event->attr.config)) {
+	case XE_PMU_EVENT_GT_C6_RESIDENCY:
+		return xe_gt_idle_residency_msec(&gt->gtidle);
+	}
+
+	return 0;
 }
 
 static void xe_pmu_event_update(struct perf_event *event)
@@ -270,6 +275,10 @@ static const struct attribute_group *pmu_events_attr_update[] = {
 
 static void set_supported_events(struct xe_pmu *pmu)
 {
+	struct xe_device *xe = container_of(pmu, typeof(*xe), pmu);
+
+	if (!xe->info.skip_guc_pc)
+		pmu->supported_events |= BIT_ULL(XE_PMU_EVENT_GT_C6_RESIDENCY);
 }
 
 /**
-- 
2.48.0

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

[Rodrigo Vivi]

On Wed, Jan 22, 2025 at 08:19:21PM -0800, 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.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> 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_,			\
> +	}
> +
> +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
> -- 
> 2.48.0
> 

Re: [PATCH v15 3/6] drm/xe/pmu: Extract xe_pmu_event_update()

[Rodrigo Vivi]

On Wed, Jan 22, 2025 at 08:19:20PM -0800, Lucas De Marchi wrote:
> Like other pmu drivers, keep the update separate from the read so it can
> be called from other methods (like stop()) without side effects.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_pmu.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_pmu.c b/drivers/gpu/drm/xe/xe_pmu.c
> index df93ba96bdc1e..33598272db6aa 100644
> --- a/drivers/gpu/drm/xe/xe_pmu.c
> +++ b/drivers/gpu/drm/xe/xe_pmu.c
> @@ -117,18 +117,11 @@ static u64 __xe_pmu_event_read(struct perf_event *event)
>  	return val;
>  }
>  
> -static void xe_pmu_event_read(struct perf_event *event)
> +static void xe_pmu_event_update(struct perf_event *event)
>  {
> -	struct xe_device *xe = container_of(event->pmu, typeof(*xe), pmu.base);
>  	struct hw_perf_event *hwc = &event->hw;
> -	struct xe_pmu *pmu = &xe->pmu;
>  	u64 prev, new;
>  
> -	if (!pmu->registered) {
> -		event->hw.state = PERF_HES_STOPPED;
> -		return;
> -	}
> -
>  	prev = local64_read(&hwc->prev_count);
>  	do {
>  		new = __xe_pmu_event_read(event);

I really have the feeling that the names are inverted, and that this
function should be the one actually called read, but I double
checked and everybody else is doing the same, so I guess I just need
more coffee...

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> @@ -137,6 +130,19 @@ static void xe_pmu_event_read(struct perf_event *event)
>  	local64_add(new - prev, &event->count);
>  }
>  
> +static void xe_pmu_event_read(struct perf_event *event)
> +{
> +	struct xe_device *xe = container_of(event->pmu, typeof(*xe), pmu.base);
> +	struct xe_pmu *pmu = &xe->pmu;
> +
> +	if (!pmu->registered) {
> +		event->hw.state = PERF_HES_STOPPED;
> +		return;
> +	}
> +
> +	xe_pmu_event_update(event);
> +}
> +
>  static void xe_pmu_enable(struct perf_event *event)
>  {
>  	/*
> @@ -166,7 +172,7 @@ static void xe_pmu_event_stop(struct perf_event *event, int flags)
>  
>  	if (pmu->registered)
>  		if (flags & PERF_EF_UPDATE)
> -			xe_pmu_event_read(event);
> +			xe_pmu_event_update(event);
>  
>  	event->hw.state = PERF_HES_STOPPED;
>  }
> -- 
> 2.48.0
> 

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

[Riana Tauro]

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?

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

Re: [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@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
>

Re: [PATCH v15 3/6] drm/xe/pmu: Extract xe_pmu_event_update()

[Lucas De Marchi]

On Thu, Jan 23, 2025 at 05:20:09AM -0500, Rodrigo Vivi wrote:
>On Wed, Jan 22, 2025 at 08:19:20PM -0800, Lucas De Marchi wrote:
>> Like other pmu drivers, keep the update separate from the read so it can
>> be called from other methods (like stop()) without side effects.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_pmu.c | 24 +++++++++++++++---------
>>  1 file changed, 15 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_pmu.c b/drivers/gpu/drm/xe/xe_pmu.c
>> index df93ba96bdc1e..33598272db6aa 100644
>> --- a/drivers/gpu/drm/xe/xe_pmu.c
>> +++ b/drivers/gpu/drm/xe/xe_pmu.c
>> @@ -117,18 +117,11 @@ static u64 __xe_pmu_event_read(struct perf_event *event)
>>  	return val;
>>  }
>>
>> -static void xe_pmu_event_read(struct perf_event *event)
>> +static void xe_pmu_event_update(struct perf_event *event)
>>  {
>> -	struct xe_device *xe = container_of(event->pmu, typeof(*xe), pmu.base);
>>  	struct hw_perf_event *hwc = &event->hw;
>> -	struct xe_pmu *pmu = &xe->pmu;
>>  	u64 prev, new;
>>
>> -	if (!pmu->registered) {
>> -		event->hw.state = PERF_HES_STOPPED;
>> -		return;
>> -	}
>> -
>>  	prev = local64_read(&hwc->prev_count);
>>  	do {
>>  		new = __xe_pmu_event_read(event);
>
>I really have the feeling that the names are inverted, and that this
>function should be the one actually called read, but I double
>checked and everybody else is doing the same, so I guess I just need
>more coffee...

yeah... that's because "read" here is used in 2 different contexts:

1) as implementation of the perf_pmu functor. We are implementing the
.event_read() so we call the function xe_pmu_event_read()
2) as the verb implying we are reading the HW since it's commonly used
in xe, so we call the function __xe_pmu_event_read()

however the perf_pmu implies that the HW values are updated not only in
the .read() call, but also in e.g. in the .stop() call. So we have

	xe_pmu_event_stop
	  xe_pmu_event_update
	    __xe_pmu_event_read

and

	xe_pmu_event_read
	  xe_pmu_event_update
	    __xe_pmu_event_read

__xe_pmu_event_read could be called event_hw_readout() or something like
that if it helps clearing up the confusion. Thoughts?

>
>Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

let me know what you think about the rename above and I will keep your
r-b.

thanks
Lucas De Marchi

>
>> @@ -137,6 +130,19 @@ static void xe_pmu_event_read(struct perf_event *event)
>>  	local64_add(new - prev, &event->count);
>>  }
>>
>> +static void xe_pmu_event_read(struct perf_event *event)
>> +{
>> +	struct xe_device *xe = container_of(event->pmu, typeof(*xe), pmu.base);
>> +	struct xe_pmu *pmu = &xe->pmu;
>> +
>> +	if (!pmu->registered) {
>> +		event->hw.state = PERF_HES_STOPPED;
>> +		return;
>> +	}
>> +
>> +	xe_pmu_event_update(event);
>> +}
>> +
>>  static void xe_pmu_enable(struct perf_event *event)
>>  {
>>  	/*
>> @@ -166,7 +172,7 @@ static void xe_pmu_event_stop(struct perf_event *event, int flags)
>>
>>  	if (pmu->registered)
>>  		if (flags & PERF_EF_UPDATE)
>> -			xe_pmu_event_read(event);
>> +			xe_pmu_event_update(event);
>>
>>  	event->hw.state = PERF_HES_STOPPED;
>>  }
>> --
>> 2.48.0
>>

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

[Riana Tauro]

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
>>

Re: [PATCH v15 3/6] drm/xe/pmu: Extract xe_pmu_event_update()

[Rodrigo Vivi]

On Thu, Jan 23, 2025 at 08:59:16AM -0600, Lucas De Marchi wrote:
> On Thu, Jan 23, 2025 at 05:20:09AM -0500, Rodrigo Vivi wrote:
> > On Wed, Jan 22, 2025 at 08:19:20PM -0800, Lucas De Marchi wrote:
> > > Like other pmu drivers, keep the update separate from the read so it can
> > > be called from other methods (like stop()) without side effects.
> > > 
> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_pmu.c | 24 +++++++++++++++---------
> > >  1 file changed, 15 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_pmu.c b/drivers/gpu/drm/xe/xe_pmu.c
> > > index df93ba96bdc1e..33598272db6aa 100644
> > > --- a/drivers/gpu/drm/xe/xe_pmu.c
> > > +++ b/drivers/gpu/drm/xe/xe_pmu.c
> > > @@ -117,18 +117,11 @@ static u64 __xe_pmu_event_read(struct perf_event *event)
> > >  	return val;
> > >  }
> > > 
> > > -static void xe_pmu_event_read(struct perf_event *event)
> > > +static void xe_pmu_event_update(struct perf_event *event)
> > >  {
> > > -	struct xe_device *xe = container_of(event->pmu, typeof(*xe), pmu.base);
> > >  	struct hw_perf_event *hwc = &event->hw;
> > > -	struct xe_pmu *pmu = &xe->pmu;
> > >  	u64 prev, new;
> > > 
> > > -	if (!pmu->registered) {
> > > -		event->hw.state = PERF_HES_STOPPED;
> > > -		return;
> > > -	}
> > > -
> > >  	prev = local64_read(&hwc->prev_count);
> > >  	do {
> > >  		new = __xe_pmu_event_read(event);
> > 
> > I really have the feeling that the names are inverted, and that this
> > function should be the one actually called read, but I double
> > checked and everybody else is doing the same, so I guess I just need
> > more coffee...
> 
> yeah... that's because "read" here is used in 2 different contexts:
> 
> 1) as implementation of the perf_pmu functor. We are implementing the
> .event_read() so we call the function xe_pmu_event_read()
> 2) as the verb implying we are reading the HW since it's commonly used
> in xe, so we call the function __xe_pmu_event_read()
> 
> however the perf_pmu implies that the HW values are updated not only in
> the .read() call, but also in e.g. in the .stop() call. So we have
> 
> 	xe_pmu_event_stop
> 	  xe_pmu_event_update
> 	    __xe_pmu_event_read
> 
> and
> 
> 	xe_pmu_event_read
> 	  xe_pmu_event_update
> 	    __xe_pmu_event_read
> 

This kind of makes sense now, or at least explains my own confusion...

> __xe_pmu_event_read could be called event_hw_readout() or something like
> that if it helps clearing up the confusion. Thoughts?
> 
> > 
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> let me know what you think about the rename above and I will keep your
> r-b.

No no, let's continue with the style that is used everywhere...

> 
> thanks
> Lucas De Marchi
> 
> > 
> > > @@ -137,6 +130,19 @@ static void xe_pmu_event_read(struct perf_event *event)
> > >  	local64_add(new - prev, &event->count);
> > >  }
> > > 
> > > +static void xe_pmu_event_read(struct perf_event *event)
> > > +{
> > > +	struct xe_device *xe = container_of(event->pmu, typeof(*xe), pmu.base);
> > > +	struct xe_pmu *pmu = &xe->pmu;
> > > +
> > > +	if (!pmu->registered) {
> > > +		event->hw.state = PERF_HES_STOPPED;
> > > +		return;
> > > +	}
> > > +
> > > +	xe_pmu_event_update(event);
> > > +}
> > > +
> > >  static void xe_pmu_enable(struct perf_event *event)
> > >  {
> > >  	/*
> > > @@ -166,7 +172,7 @@ static void xe_pmu_event_stop(struct perf_event *event, int flags)
> > > 
> > >  	if (pmu->registered)
> > >  		if (flags & PERF_EF_UPDATE)
> > > -			xe_pmu_event_read(event);
> > > +			xe_pmu_event_update(event);
> > > 
> > >  	event->hw.state = PERF_HES_STOPPED;
> > >  }
> > > --
> > > 2.48.0
> > > 

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

[Lucas De Marchi]

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@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
>>>
>