[PATCH v10 1/4] drm/xe/pmu: Enable PMU interface
Belgaumkar, Vinay
vinay.belgaumkar at intel.com
Tue Jan 7 18:08:55 UTC 2025
On 12/20/2024 1:03 PM, Rodrigo Vivi wrote:
> On Thu, Dec 19, 2024 at 05:19:07PM -0800, Vinay Belgaumkar wrote:
>> From: Aravind Iddamsetty<aravind.iddamsetty at linux.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/
>>
>> The pmu base implementation is still from the
>> i915 driver.
>>
>> v2: Review comments(Rodrigo) and do not init pmu for VFs
>> as they don't have access to freq and c6 residency anyways.
>> v3: Fix kunit issue, move xe_pmu entry in Makefile (Jani) and
>> move drm uapi definitions (Lucas)
>> v4: Adapt Lucas's recent PMU fixes for i915
>> v5: Fix some kernel doc issues
>> v6: Address comments from Lucas.
>> v7: Define events per device and use xe_gt_id to choose GT (Lucas)
>> v8: Use config field bits for gt_id as well. Use raw_spinlock_t
>> to avoid deadlocks with perf subsystem (Lucas)
>>
>> Co-developed-by: Bommu Krishnaiah<krishnaiah.bommu at intel.com>
>> Signed-off-by: Bommu Krishnaiah<krishnaiah.bommu at intel.com>
>> Signed-off-by: Aravind Iddamsetty<aravind.iddamsetty at linux.intel.com>
>> Signed-off-by: Riana Tauro<riana.tauro at intel.com>
>> Cc: Rodrigo Vivi<rodrigo.vivi at intel.com>
>> Reviewed-by: Rodrigo Vivi<rodrigo.vivi at intel.com> #v4
>> Cc: Lucas De Marchi<lucas.demarchi at intel.com>
>> Co-developed-by: Vinay Belgaumkar<vinay.belgaumkar at intel.com>
>> Signed-off-by: Vinay Belgaumkar<vinay.belgaumkar at 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_module.c | 5 +
>> drivers/gpu/drm/xe/xe_pmu.c | 621 +++++++++++++++++++++++++++
>> drivers/gpu/drm/xe/xe_pmu.h | 26 ++
>> drivers/gpu/drm/xe/xe_pmu_types.h | 72 ++++
>> 7 files changed, 733 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 7730e0596299..ab4a1af795fc 100644
>> --- a/drivers/gpu/drm/xe/Makefile
>> +++ b/drivers/gpu/drm/xe/Makefile
>> @@ -300,6 +300,8 @@ ifeq ($(CONFIG_DEBUG_FS),y)
>> i915-display/intel_pipe_crc.o
>> endif
>>
>> +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 bf36e4fb4679..ca557c22c979 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"
>> @@ -760,6 +761,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 8a7b15972413..bd1e72d318f7 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"
>> @@ -525,6 +526,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_module.c b/drivers/gpu/drm/xe/xe_module.c
>> index 0f2c20e9204a..d3a9d4bc9bee 100644
>> --- a/drivers/gpu/drm/xe/xe_module.c
>> +++ b/drivers/gpu/drm/xe/xe_module.c
>> @@ -14,6 +14,7 @@
>> #include "xe_hw_fence.h"
>> #include "xe_pci.h"
>> #include "xe_pm.h"
>> +#include "xe_pmu.h"
>> #include "xe_observation.h"
>> #include "xe_sched_job.h"
>>
>> @@ -96,6 +97,10 @@ static const struct init_funcs init_funcs[] = {
>> .init = xe_sched_job_module_init,
>> .exit = xe_sched_job_module_exit,
>> },
>> + {
>> + .init = xe_pmu_init,
>> + .exit = xe_pmu_exit,
>> + },
>> {
>> .init = xe_register_pci_driver,
>> .exit = xe_unregister_pci_driver,
>> diff --git a/drivers/gpu/drm/xe/xe_pmu.c b/drivers/gpu/drm/xe/xe_pmu.c
>> new file mode 100644
>> index 000000000000..e6d25e8b7b7c
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_pmu.c
>> @@ -0,0 +1,621 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2024 Intel Corporation
>> + */
>> +
>> +#include <drm/drm_drv.h>
>> +#include <drm/drm_managed.h>
>> +#include <drm/xe_drm.h>
>> +
>> +#include "regs/xe_gt_regs.h"
>> +#include "xe_device.h"
>> +#include "xe_force_wake.h"
>> +#include "xe_gt_clock.h"
>> +#include "xe_mmio.h"
>> +#include "xe_macros.h"
>> +#include "xe_pm.h"
>> +#include "xe_pmu.h"
>> +
>> +/*
>> + * CPU mask is defined/initialized at a module level. All devices
>> + * inside this module share this mask.
>> + */
>> +static cpumask_t xe_pmu_cpumask;
>> +static unsigned int xe_pmu_target_cpu = -1;
>> +
>> +/**
>> + * DOC: Xe PMU (Performance Monitoring Unit)
>> + *
>> + * Expose events/counters like C6 residency and GT frequency to user land.
>> + * Events will be per device, the GT can be selected with an extra config
>> + * sub-field (bits 60-63).
>> + *
>> + * Perf tool can be used to list these counters from the command line.
>> + *
>> + * Example commands to list/record supported perf events-
>> + *
>> + * $ 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-
>> + * $ perf list | grep c6
>> + *
>> + * To list a specific event for a GT at regular intervals-
>> + * $ perf stat -e <event_name,xe_gt_id=> -I <interval>
>> + *
>> + */
>> +
>> +static unsigned int config_gt_id(const u64 config)
>> +{
>> + return config >> __XE_PMU_GT_SHIFT;
>> +}
>> +
>> +static u64 config_counter(const u64 config)
>> +{
>> + return config & ~(~0ULL << __XE_PMU_GT_SHIFT);
>> +}
>> +
>> +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
>> +config_status(struct xe_device *xe, u64 config)
>> +{
>> + unsigned int gt_id = config_gt_id(config);
>> +
>> + if (gt_id >= XE_MAX_GT_PER_TILE)
>> + return -ENOENT;
>> +
>> + switch (config_counter(config)) {
>> + default:
>> + return -ENOENT;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +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;
>> + int ret;
>> + u64 event_config;
>> +
>> + 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 (has_branch_stack(event))
>> + return -EOPNOTSUPP;
> can we do all the EINVAL checks first and this one right after the
> einval code?
ok.
>> +
>> + if (event->cpu < 0)
>> + return -EINVAL;
>> +
>> + /* only allow running on one cpu at a time */
>> + if (!cpumask_test_cpu(event->cpu, &xe_pmu_cpumask))
>> + return -EINVAL;
>> +
>> + event_config = event->attr.config;
>> + ret = config_status(xe, event_config);
>> + if (ret)
>> + return ret;
>> +
>> + 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_device *xe =
>> + container_of(event->pmu, typeof(*xe), pmu.base);
>> + const u64 config = event->attr.config;
>> + const u64 gt_id = config >> __XE_PMU_GT_SHIFT;
>> + struct xe_gt *gt = xe_device_get_gt(xe, gt_id);
>> + u64 val = 0;
>> +
>> + switch (config_counter(config)) {
>> + default:
>> + drm_warn(>->tile->xe->drm, "unknown pmu event\n");
>> + }
>> +
>> + 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)
>> + goto out;
>> +
>> + if (flags & PERF_EF_UPDATE)
>> + xe_pmu_event_read(event);
>> +
>> +out:
>> + event->hw.state = PERF_HES_STOPPED;
> please avoid this goto here by using if/else on the if (!pmu->registered) { if (flags...} else { event->hw.state...};
ok.
>> +}
>> +
>> +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);
>> +}
>> +
>> +static int xe_pmu_event_event_idx(struct perf_event *event)
>> +{
>> + return 0;
>> +}
>> +
>> +struct xe_str_attribute {
>> + struct device_attribute attr;
>> + const char *str;
>> +};
>> +
>> +static ssize_t xe_pmu_format_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct xe_str_attribute *eattr;
>> +
>> + eattr = container_of(attr, struct xe_str_attribute, attr);
>> + return sprintf(buf, "%s\n", eattr->str);
>> +}
>> +
>> +#define XE_PMU_FORMAT_ATTR(_name, _config) \
>> + (&((struct xe_str_attribute[]) { \
>> + { .attr = __ATTR(_name, 0444, xe_pmu_format_show, NULL), \
>> + .str = _config, } \
>> + })[0].attr.attr)
>> +
>> +static struct attribute *xe_pmu_format_attrs[] = {
>> + XE_PMU_FORMAT_ATTR(event_id, "config:0-20"),
>> + XE_PMU_FORMAT_ATTR(gt_id, "config:60-63"),
>> + NULL,
>> +};
>> +
>> +static const struct attribute_group xe_pmu_format_attr_group = {
>> + .name = "format",
>> + .attrs = xe_pmu_format_attrs,
>> +};
>> +
>> +struct xe_ext_attribute {
>> + struct device_attribute attr;
>> + unsigned long val;
>> +};
>> +
>> +static ssize_t xe_pmu_event_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct xe_ext_attribute *eattr;
>> +
>> + eattr = container_of(attr, struct xe_ext_attribute, attr);
>> + return sprintf(buf, "config=0x%lx\n", eattr->val);
>> +}
>> +
>> +static ssize_t cpumask_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + return cpumap_print_to_pagebuf(true, buf, &xe_pmu_cpumask);
>> +}
>> +
>> +static DEVICE_ATTR_RO(cpumask);
>> +
>> +static struct attribute *xe_cpumask_attrs[] = {
>> + &dev_attr_cpumask.attr,
>> + NULL,
>> +};
>> +
>> +static const struct attribute_group xe_pmu_cpumask_attr_group = {
>> + .attrs = xe_cpumask_attrs,
>> +};
>> +
>> +#define __event(__counter, __name, __unit) \
>> +{ \
>> + .counter = (__counter), \
>> + .name = (__name), \
>> + .unit = (__unit), \
>> +}
>> +
>> +static struct xe_ext_attribute *
>> +add_xe_attr(struct xe_ext_attribute *attr, const char *name, u64 config)
>> +{
>> + sysfs_attr_init(&attr->attr.attr);
>> + attr->attr.attr.name = name;
>> + attr->attr.attr.mode = 0444;
>> + attr->attr.show = xe_pmu_event_show;
>> + attr->val = config;
>> +
>> + return ++attr;
>> +}
>> +
>> +static struct perf_pmu_events_attr *
>> +add_pmu_attr(struct perf_pmu_events_attr *attr, const char *name,
>> + const char *str)
>> +{
>> + sysfs_attr_init(&attr->attr.attr);
>> + attr->attr.attr.name = name;
>> + attr->attr.attr.mode = 0444;
>> + attr->attr.show = perf_event_sysfs_show;
>> + attr->event_str = str;
>> +
>> + return ++attr;
>> +}
>> +
>> +static struct attribute **
>> +create_event_attributes(struct xe_pmu *pmu)
>> +{
>> + struct xe_device *xe = container_of(pmu, typeof(*xe), pmu);
>> + static const struct {
>> + unsigned int counter;
>> + const char *name;
>> + const char *unit;
>> + } events[] = {
>> + };
>> +
>> + struct perf_pmu_events_attr *pmu_attr = NULL, *pmu_iter;
>> + struct xe_ext_attribute *xe_attr = NULL, *xe_iter;
>> + struct attribute **attr = NULL, **attr_iter;
>> + unsigned int count = 0;
>> + unsigned int i;
>> +
>> + /* Count how many counters we will be exposing. */
>> + for (i = 0; i < ARRAY_SIZE(events); i++) {
>> + u64 config = __XE_PMU_PM(events[i].counter);
>> +
>> + if (!config_status(xe, config))
>> + count++;
>> + }
>> +
>> + /* Allocate attribute objects and table. */
>> + xe_attr = kcalloc(count, sizeof(*xe_attr), GFP_KERNEL);
>> + if (!xe_attr)
>> + goto err_alloc;
> this looks wrong to me, you don't need to kfree anyone in this case...
ok.
>
>> +
>> + pmu_attr = kcalloc(count, sizeof(*pmu_attr), GFP_KERNEL);
>> + if (!pmu_attr)
>> + goto err_alloc;
> same here, you only need to kfree xe_attr in this case...
>
>> +
>> + /* Max one pointer of each attribute type plus a termination entry. */
>> + attr = kcalloc(count * 2 + 1, sizeof(*attr), GFP_KERNEL);
>> + if (!attr)
>> + goto err_alloc;
> and here, you only need to kfree xe_attr and pmu_attr in this case...
>
> I know, copied from i915 and 'it works there'... I believe it works
> because kfree itself skips if NULL, not because the code is in the
> best shape...
>
> but perhaps I'm just paranoid?
Ok.
>
>> +
>> + xe_iter = xe_attr;
>> + pmu_iter = pmu_attr;
>> + attr_iter = attr;
>> +
>> + for (i = 0; i < ARRAY_SIZE(events); i++) {
>> + u64 config = __XE_PMU_PM(events[i].counter);
>> + char *str;
>> +
>> + if (config_status(xe, config))
>> + continue;
>> +
>> + str = kasprintf(GFP_KERNEL, "%s",
>> + events[i].name);
>> + if (!str)
>> + goto err;
> hmmm, here you don't need to free everyone, but only all the above before
> the loop and the ones that you have run in the loop already, but not the
> entire loop again below...
>
> or perhaps the for loop below takes care of this and I'm just paranoid...
We are freeing all of them. If there are sys/memory issues where even a
string cannot be allocated, might not be worth continuing with anything?
>
>> +
>> + *attr_iter++ = &xe_iter->attr.attr;
>> + xe_iter = add_xe_attr(xe_iter, str, config);
>> +
>> + if (events[i].unit) {
>> + str = kasprintf(GFP_KERNEL, "%s.unit",
>> + events[i].name);
>> + if (!str)
>> + goto err;
>> +
>> + *attr_iter++ = &pmu_iter->attr.attr;
>> + pmu_iter = add_pmu_attr(pmu_iter, str,
>> + events[i].unit);
>> + }
>> + }
>> +
>> + pmu->xe_attr = xe_attr;
>> + pmu->pmu_attr = pmu_attr;
>> +
>> + return attr;
>> +
>> +err:
>> + for (attr_iter = attr; *attr_iter; attr_iter++)
>> + kfree((*attr_iter)->name);
>> +
>> +err_alloc:
>> + kfree(attr);
>> + kfree(xe_attr);
>> + kfree(pmu_attr);
>> +
>> + return NULL;
>> +}
>> +
>> +static void free_event_attributes(struct xe_pmu *pmu)
>> +{
>> + struct attribute **attr_iter = pmu->events_attr_group.attrs;
>> +
>> + for (; *attr_iter; attr_iter++)
>> + kfree((*attr_iter)->name);
>> +
>> + kfree(pmu->events_attr_group.attrs);
>> + kfree(pmu->xe_attr);
>> + kfree(pmu->pmu_attr);
>> +
>> + pmu->events_attr_group.attrs = NULL;
>> + pmu->xe_attr = NULL;
>> + pmu->pmu_attr = NULL;
>> +}
>> +
>> +static int xe_pmu_cpu_online(unsigned int cpu, struct hlist_node *node)
>> +{
>> + struct xe_pmu *pmu = hlist_entry_safe(node, typeof(*pmu), cpuhp.node);
>> +
>> + /* Select the first online CPU as a designated reader. */
>> + if (cpumask_empty(&xe_pmu_cpumask))
>> + cpumask_set_cpu(cpu, &xe_pmu_cpumask);
>> +
>> + return 0;
>> +}
>> +
>> +static int xe_pmu_cpu_offline(unsigned int cpu, struct hlist_node *node)
>> +{
>> + struct xe_pmu *pmu = hlist_entry_safe(node, typeof(*pmu), cpuhp.node);
>> + unsigned int target = xe_pmu_target_cpu;
>> +
>> + /*
>> + * Unregistering an instance generates a CPU offline event which we must
>> + * ignore to avoid incorrectly modifying the shared xe_pmu_cpumask.
>> + */
>> + if (!pmu->registered)
>> + return 0;
>> +
>> + if (cpumask_test_and_clear_cpu(cpu, &xe_pmu_cpumask)) {
>> + target = cpumask_any_but(topology_sibling_cpumask(cpu), cpu);
>> +
>> + /* Migrate events if there is a valid target */
>> + if (target < nr_cpu_ids) {
>> + cpumask_set_cpu(target, &xe_pmu_cpumask);
>> + xe_pmu_target_cpu = target;
>> + }
>> + }
>> +
>> + if (target < nr_cpu_ids && target != pmu->cpuhp.cpu) {
>> + perf_pmu_migrate_context(&pmu->base, cpu, target);
>> + pmu->cpuhp.cpu = target;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static enum cpuhp_state cpuhp_state = CPUHP_INVALID;
>> +
>> +/**
>> + * xe_pmu_init() - Setup CPU hotplug state/callbacks for Xe PMU
>> + *
>> + * Returns: 0 if successful, else error code
>> + */
>> +int xe_pmu_init(void)
>> +{
>> + int ret;
>> +
>> + ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN,
>> + "perf/x86/intel/xe:online",
>> + xe_pmu_cpu_online,
>> + xe_pmu_cpu_offline);
>> + if (ret < 0)
>> + pr_notice("Failed to setup cpuhp state for xe PMU! (%d)\n",
>> + ret);
>> + else
>> + cpuhp_state = ret;
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * xe_pmu_exit() - Remove CPU hotplug state/callbacks for Xe PMU
>> + */
>> +void xe_pmu_exit(void)
>> +{
>> + if (cpuhp_state != CPUHP_INVALID)
>> + cpuhp_remove_multi_state(cpuhp_state);
>> +}
>> +
>> +static int xe_pmu_register_cpuhp_state(struct xe_pmu *pmu)
>> +{
>> + if (cpuhp_state == CPUHP_INVALID)
>> + return -EINVAL;
>> +
>> + return cpuhp_state_add_instance(cpuhp_state, &pmu->cpuhp.node);
>> +}
>> +
>> +static void xe_pmu_unregister_cpuhp_state(struct xe_pmu *pmu)
>> +{
>> + cpuhp_state_remove_instance(cpuhp_state, &pmu->cpuhp.node);
>> +}
>> +
>> +/**
>> + * xe_pmu_unregister() - Remove/cleanup PMU registration
>> + * @arg: Ptr to pmu
>> + */
>> +void xe_pmu_unregister(void *arg)
>> +{
>> + struct xe_pmu *pmu = arg;
>> + struct xe_device *xe = container_of(pmu, typeof(*xe), pmu);
>> +
>> + if (IS_SRIOV_VF(xe))
>> + return;
>> +
>> + if (!pmu->registered)
>> + return;
>> +
>> + pmu->registered = false;
>> +
>> + xe_pmu_unregister_cpuhp_state(pmu);
>> +
>> + perf_pmu_unregister(&pmu->base);
>> + kfree(pmu->base.attr_groups);
>> + kfree(pmu->name);
>> + free_event_attributes(pmu);
>> +}
>> +
>> +/**
>> + * xe_pmu_register() - Define basic PMU properties for Xe and add event callbacks.
>> + * @pmu: the PMU object
>> + *
>> + */
>> +void xe_pmu_register(struct xe_pmu *pmu)
>> +{
>> + struct xe_device *xe = container_of(pmu, typeof(*xe), pmu);
>> + const struct attribute_group *attr_groups[] = {
>> + &xe_pmu_format_attr_group,
>> + &pmu->events_attr_group,
>> + &xe_pmu_cpumask_attr_group,
>> + NULL
>> + };
>> +
>> + int ret = -ENOMEM;
> unused ret variable in a void function...
The ret is used internal to this function.
>
>> +
>> + if (IS_SRIOV_VF(xe))
>> + return;
>> +
>> + raw_spin_lock_init(&pmu->lock);
>> + pmu->cpuhp.cpu = -1;
>> +
>> + pmu->name = kasprintf(GFP_KERNEL,
>> + "xe_%s",
>> + dev_name(xe->drm.dev));
>> + if (pmu->name) {
>> + /* tools/perf reserves colons as special. */
>> + strreplace((char *)pmu->name, ':', '_');
>> + }
>> +
>> + if (!pmu->name)
>> + goto err;
> move this above, then you can avoid the if (pmu->name) condition in the
> other block...
ok.
>
>> +
>> + pmu->events_attr_group.name = "events";
>> + pmu->events_attr_group.attrs = create_event_attributes(pmu);
>> + if (!pmu->events_attr_group.attrs)
>> + goto err_name;
> this function is indeed handling the goto error conditions much better
> than the other one above...
>
>> +
>> + pmu->base.attr_groups = kmemdup(attr_groups, sizeof(attr_groups),
>> + GFP_KERNEL);
>> + if (!pmu->base.attr_groups)
>> + goto err_attr;
>> +
>> + 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;
>> + pmu->base.event_idx = xe_pmu_event_event_idx;
>> +
>> + ret = perf_pmu_register(&pmu->base, pmu->name, -1);
>> + if (ret)
>> + goto err_groups;
>> +
>> + ret = xe_pmu_register_cpuhp_state(pmu);
>> + if (ret)
>> + goto err_unreg;
>> +
>> + ret = devm_add_action_or_reset(xe->drm.dev, xe_pmu_unregister, pmu);
>> + if (ret)
>> + goto err_cpuhp;
>> +
>> + pmu->registered = true;
>> +
>> + return;
>> +
>> +err_cpuhp:
>> + xe_pmu_unregister_cpuhp_state(pmu);
>> +err_unreg:
>> + perf_pmu_unregister(&pmu->base);
>> +err_groups:
>> + kfree(pmu->base.attr_groups);
>> +err_attr:
>> + free_event_attributes(pmu);
>> +err_name:
>> + kfree(pmu->name);
>> +err:
>> + drm_notice(&xe->drm, "Failed to register PMU!\n");
>> +}
>> diff --git a/drivers/gpu/drm/xe/xe_pmu.h b/drivers/gpu/drm/xe/xe_pmu.h
>> new file mode 100644
>> index 000000000000..d07e5dfdfec0
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_pmu.h
>> @@ -0,0 +1,26 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2024 Intel Corporation
> probably good to remind to change this and every other header in this series to
> 2025 already?
ok.
>
>> + */
>> +
>> +#ifndef _XE_PMU_H_
>> +#define _XE_PMU_H_
>> +
>> +#include "xe_pmu_types.h"
>> +
>> +struct xe_gt;
>> +
>> +#if IS_ENABLED(CONFIG_PERF_EVENTS)
>> +int xe_pmu_init(void);
>> +void xe_pmu_exit(void);
>> +void xe_pmu_register(struct xe_pmu *pmu);
>> +void xe_pmu_unregister(void *arg);
>> +#else
>> +static inline int xe_pmu_init(void) { return 0; }
>> +static inline void xe_pmu_exit(void) {}
>> +static inline void xe_pmu_register(struct xe_pmu *pmu) {}
>> +static inline void xe_pmu_unregister(void *arg) {}
>> +#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 000000000000..2b3f8982023f
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_pmu_types.h
>> @@ -0,0 +1,72 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2024 Intel Corporation
>> + */
>> +
>> +#ifndef _XE_PMU_TYPES_H_
>> +#define _XE_PMU_TYPES_H_
>> +
>> +#include <linux/perf_event.h>
>> +#include <linux/spinlock_types.h>
>> +
>> +enum {
>> + __XE_NUM_PMU_SAMPLERS
>> +};
>> +
>> +#define XE_PMU_MAX_GT 2
> is there a way to make this more generic?
> otherwise whenever we have a platform with
> different max we will forget to adjust here...
Seems hard to do. MAX_GTS is defined in xe_device_types, which includes
xe_pmu_types, so using that leads to circular ref. Doesn't look like
there is a clean solution here, as having the sole definition in
xe_pmu_types may not be the right choice.
>
>> +
>> +/*
>> + * Top bits of every counter are GT id.
>> + */
>> +#define __XE_PMU_GT_SHIFT (60)
>> +
>> +#define ___XE_PMU_PM(gt, x) \
>> + (((__u64)(x)) | ((__u64)(gt) << __XE_PMU_GT_SHIFT))
>> +
>> +#define __XE_PMU_PM(x) ___XE_PMU_PM(0, x)
>> +
> please add also a doc comment for the xe_pmu struct itself, not only for its
> components...
ok.
Thanks,
Vinay.
>
>> +struct xe_pmu {
>> + /**
>> + * @cpuhp: Struct used for CPU hotplug handling.
>> + */
>> + struct {
>> + struct hlist_node node;
>> + unsigned int cpu;
>> + } cpuhp;
>> + /**
>> + * @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;
>> + /**
>> + * @lock: Lock protecting enable mask and ref count handling.
>> + */
>> + raw_spinlock_t lock;
>> + /**
>> + * @sample: Current and previous (raw) counters.
>> + *
>> + * These counters are updated when the device is awake.
>> + */
>> + u64 sample[XE_PMU_MAX_GT][__XE_NUM_PMU_SAMPLERS];
>> + /**
>> + * @events_attr_group: Device events attribute group.
>> + */
>> + struct attribute_group events_attr_group;
>> + /**
>> + * @xe_attr: Memory block holding device attributes.
>> + */
>> + void *xe_attr;
>> + /**
>> + * @pmu_attr: Memory block holding device attributes.
>> + */
>> + void *pmu_attr;
>> +};
>> +
>> +#endif
>> --
>> 2.38.1
>>
More information about the Intel-xe
mailing list