[PATCH v10 1/4] drm/xe/pmu: Enable PMU interface
Rodrigo Vivi
rodrigo.vivi at intel.com
Fri Dec 20 21:03:36 UTC 2024
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?
> +
> + 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...};
> +}
> +
> +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...
> +
> + 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?
> +
> + 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...
> +
> + *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...
> +
> + 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...
> +
> + 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?
> + */
> +
> +#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...
> +
> +/*
> + * 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...
> +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