[PATCH v2 1/2] drm/xe/pmu: Add event tracking
Lucas De Marchi
lucas.demarchi at intel.com
Wed Feb 5 01:07:30 UTC 2025
On Tue, Feb 04, 2025 at 03:20:08PM -0800, Belgaumkar, Vinay wrote:
>
>On 2/4/2025 2:30 PM, Lucas De Marchi wrote:
>>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.
>Not meant to be a copy paste, wanted to add protection to ensure the
I'm talking about the place warnings are added and the names.
event_bit() for example. It's not even being used as a bit since you
are doing enable_count[event_bit]. In this file it's referred as id,
not bit. Also a field called **enable** that is the OR of e1->config,
e2->config, e3->config is very odd and brought here from i915.
The missing lock on read, the missing lock on add_sample_mult(), that
function name, the conversion from nsec to usec to store the value. This
all come from i915 and I don't want to have the same mistakes in xe. It
will be hard to fix i915 and remove the hack we added in
topic/core-for-CI.
So, looking at i915 to figure out the HW registers and HW interface
should be ok. For perf or userspace interface, I'd like to avoid.
For events currently being sampled, perf core calls those events
"active".
$ git grep -l perf_pmu_register | xargs git grep active_lis
arch/x86/events/intel/uncore.c: list_for_each_entry(event, &box->active_list, active_entry) {
arch/x86/events/intel/uncore.c: INIT_LIST_HEAD(&box->active_list);
arch/x86/events/intel/uncore.c: list_add_tail(&event->active_entry, &box->active_list);
arch/x86/events/intel/uncore.c: list_add(&box->active_list, &allocated);
arch/x86/events/intel/uncore.c: list_for_each_entry_safe(box, tmp, &allocated, active_list) {
arch/x86/events/intel/uncore.c: list_del_init(&box->active_list);
arch/x86/events/intel/uncore.c: list_for_each_entry_safe(box, tmp, &allocated, active_list) {
arch/x86/events/intel/uncore.c: list_del_init(&box->active_list);
arch/x86/events/rapl.c: struct list_head active_list;
arch/x86/events/rapl.c: list_for_each_entry(event, &rapl_pmu->active_list, active_entry)
arch/x86/events/rapl.c: list_add_tail(&event->active_entry, &rapl_pmu->active_list);
arch/x86/events/rapl.c: INIT_LIST_HEAD(&rapl_pmu->active_list);
drivers/powercap/intel_rapl_common.c: list_add_tail(&event->active_entry, &data->active_list);
drivers/powercap/intel_rapl_common.c: list_for_each_entry(event, &data->active_list, active_entry)
drivers/powercap/intel_rapl_common.c: INIT_LIST_HEAD(&data->active_list);
When trying to use common approach/names/etc I very much prefer using as
reference the ones in kernel/events/core.c or rapl or others in
arch/x86/events/
>bit index doesn't go out of bounds. The config-0:11 bits should match
>the array size. Maybe check should be before spin_lock().
for the pmu_enable (that is called on the start() callback) it's not
clear to me the context. If there's already a raw_spin_lock taken, then
we should add this warn elsewhere. Probably the event init, that
validades the event being added.
Lucas De Marchi
>>
>>>+ XE_WARN_ON(pmu->enable_count[event_bit] == ~0);
>Same here, should be before the spin_lock() in case it is not compatible.
>>>
>>>+
>>>+ /* Save the event config */
>>>+ pmu->enable |= event->attr.config;
>>
>>what does "enable" have to do with config?
>We need this info while toggling/using the internal timer for
>frequency stats (next patch).
>>
>>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.
>
>Hmm, true. We need a per GT event bitmask along with the count.
>
>Thanks,
>
>Vinay.
>
>>
>>
>>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
>>>
More information about the Intel-xe
mailing list