[PATCH v2 1/2] drm/xe/pmu: Add event tracking

[Lucas De Marchi]

On Tue, Feb 04, 2025 at 10:09:04AM -0800, Vinay Belgaumkar wrote:
>Keep track of which events are enabled and number of instances
>as preparation for the PMU frequency events. Also add locks where
>we update these structures.
>
>Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>Cc: Riana Tauro <riana.tauro at intel.com>
>Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar at intel.com>
>---
> drivers/gpu/drm/xe/xe_pmu.c       | 44 ++++++++++++++++++++++++++++++-
> drivers/gpu/drm/xe/xe_pmu_types.h | 14 ++++++++++
> 2 files changed, 57 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_pmu.c b/drivers/gpu/drm/xe/xe_pmu.c
>index 3910a82328ee..10603e7c3eff 100644
>--- a/drivers/gpu/drm/xe/xe_pmu.c
>+++ b/drivers/gpu/drm/xe/xe_pmu.c
>@@ -8,6 +8,7 @@
>
> #include "xe_device.h"
> #include "xe_gt_idle.h"
>+#include "xe_macros.h"
> #include "xe_pm.h"
> #include "xe_pmu.h"
>
>@@ -38,6 +39,11 @@
> #define XE_PMU_EVENT_GT_MASK		GENMASK_ULL(63, 60)
> #define XE_PMU_EVENT_ID_MASK		GENMASK_ULL(11, 0)
>
>+static struct xe_pmu *event_to_pmu(struct perf_event *event)
>+{
>+	return container_of(event->pmu, struct xe_pmu, base);
>+}
>+
> static unsigned int config_to_event_id(u64 config)
> {
> 	return FIELD_GET(XE_PMU_EVENT_ID_MASK, config);
>@@ -156,6 +162,21 @@ static void xe_pmu_event_read(struct perf_event *event)
>
> static void xe_pmu_enable(struct perf_event *event)
> {
>+	struct xe_pmu *pmu = event_to_pmu(event);
>+	struct xe_device *xe = container_of(pmu, typeof(*xe), pmu);
>+	const unsigned int event_bit = config_to_event_id(event->attr.config);
>+	unsigned long flags;
>+
>+	raw_spin_lock_irqsave(&pmu->lock, flags);
>+
>+	XE_WARN_ON(event_bit >= XE_PMU_MAX_EVENT_TYPES);

^ not compatible with raw_spin_lock... if the warn triggers you are
likely to bring the machine down. it's also called event_id, not
event_bit. 

Please do not copy and paste from i915. It's not a good reference.

>+	XE_WARN_ON(pmu->enable_count[event_bit] == ~0);
>+
>+	/* Save the event config */
>+	pmu->enable |= event->attr.config;

what does "enable" have to do with config?
we can't simply OR any event config since there are more things than the
event_id. Suppose userspace did this:

proc1:
perf_event_open(gt-c6, gt=3)

	pmu->enable = 3 << 60 | GT_C6_ID

proc2:
perf_event_open(freq, gt=1)

	pmu->enable =  1 << 60 | FREQ_ID

now when you close the fd in proc2, your pmu->enable field will have a
gt == 2.

it's not clear at all why you need to save the config. I will try to
figure out from next patch.

>+	pmu->enable_count[event_bit]++;
>+
>+	raw_spin_unlock_irqrestore(&pmu->lock, flags);
> 	/*
> 	 * Store the current counter value so we can report the correct delta
> 	 * for all listeners. Even when the event was already enabled and has
>@@ -176,15 +197,34 @@ static void xe_pmu_event_start(struct perf_event *event, int flags)
> 	event->hw.state = 0;
> }
>
>+static void xe_pmu_disable(struct perf_event *event)
>+{
>+	struct xe_device *xe = container_of(event->pmu, typeof(*xe), pmu.base);
>+	struct xe_pmu *pmu = &xe->pmu;
>+	unsigned long flags;
>+	unsigned int event_bit = config_to_event_id(event->attr.config);
>+
>+	XE_WARN_ON(event_bit >= XE_PMU_MAX_EVENT_TYPES);
>+	raw_spin_lock_irqsave(&pmu->lock, flags);
>+
>+	if (--pmu->enable_count[event_bit] == 0)
>+		pmu->enable &=  ~event->attr.config;
>+
>+	raw_spin_unlock_irqrestore(&pmu->lock, flags);
>+}
>+
> 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 (pmu->registered) {
> 		if (flags & PERF_EF_UPDATE)
> 			xe_pmu_event_update(event);
>
>+		xe_pmu_disable(event);
>+	}
>+
> 	event->hw.state = PERF_HES_STOPPED;
> }
>
>@@ -334,6 +374,8 @@ int xe_pmu_register(struct xe_pmu *pmu)
> 	if (IS_SRIOV_VF(xe))
> 		return 0;
>
>+	raw_spin_lock_init(&pmu->lock);
>+
> 	name = kasprintf(GFP_KERNEL, "xe_%s",
> 			 dev_name(xe->drm.dev));
> 	if (!name)
>diff --git a/drivers/gpu/drm/xe/xe_pmu_types.h b/drivers/gpu/drm/xe/xe_pmu_types.h
>index f5ba4d56622c..b15858d4d4a9 100644
>--- a/drivers/gpu/drm/xe/xe_pmu_types.h
>+++ b/drivers/gpu/drm/xe/xe_pmu_types.h
>@@ -10,6 +10,8 @@
> #include <linux/spinlock_types.h>
>
> #define XE_PMU_MAX_GT 2
>+/* Event config is defined as 0:11 */
>+#define XE_PMU_MAX_EVENT_TYPES 12
>
> /**
>  * struct xe_pmu - PMU related data per Xe device
>@@ -34,6 +36,18 @@ struct xe_pmu {
> 	 * @supported_events: Bitmap of supported events, indexed by event id
> 	 */
> 	u64 supported_events;
>+	/**
>+	 * @lock: Lock protecting enable mask and sampling
>+	 */
>+	raw_spinlock_t lock;
>+	/**
>+	 * @enable: Store requested PMU event config.
>+	 */
>+	u64 enable;
>+	/**
>+	 * @enable_count: Reference counts for the enabled events.
>+	 */
>+	unsigned int enable_count[XE_PMU_MAX_EVENT_TYPES];
> };
>
> #endif
>-- 
>2.38.1
>