[PATCH v7 5/5] drm/xe/xe_pmu: Acquire forcewake on event init for engine events

[Lucas De Marchi]

On Fri, Feb 14, 2025 at 03:38:13PM +0530, Riana Tauro wrote:
>When the engine events are created, acquire GT forcewake to read gpm
>timestamp required for the events and release on event destroy. This
>cannot be done during read due to the raw spinlock held my pmu.
>
>v2: remove forcewake counting (Umesh)
>v3: remove extra space (Umesh)
>
>Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray at intel.com>
>Signed-off-by: Riana Tauro <riana.tauro at intel.com>
>Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa at intel.com>
>---
> drivers/gpu/drm/xe/xe_pmu.c       | 52 +++++++++++++++++++++++++++++--
> drivers/gpu/drm/xe/xe_pmu_types.h |  4 +++
> 2 files changed, 54 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_pmu.c b/drivers/gpu/drm/xe/xe_pmu.c
>index dc89fa6d0ec5..67693d642f5a 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_force_wake.h"
> #include "xe_gt_idle.h"
> #include "xe_guc_engine_activity.h"
> #include "xe_hw_engine.h"
>@@ -102,6 +103,37 @@ static struct xe_hw_engine *event_to_hwe(struct perf_event *event)
> 	return hwe;
> }
>
>+static bool is_engine_event(u64 config)
>+{
>+	unsigned int event_id = config_to_event_id(config);
>+
>+	return (event_id == XE_PMU_EVENT_ENGINE_TOTAL_TICKS ||
>+		event_id == XE_PMU_EVENT_ENGINE_ACTIVE_TICKS);
>+}
>+
>+static bool event_gt_forcewake(struct perf_event *event)
>+{
>+	struct xe_device *xe = container_of(event->pmu, typeof(*xe), pmu.base);
>+	u64 config = event->attr.config;
>+	struct xe_pmu *pmu = &xe->pmu;
>+	struct xe_gt *gt;
>+	unsigned int fw_ref;
>+
>+	if (!is_engine_event(config))
>+		return true;
>+
>+	gt = xe_device_get_gt(xe, config_to_gt_id(config));
>+
>+	fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FW_GT);
>+	if (!fw_ref)
>+		return false;
>+
>+	if (!pmu->fw_ref)
>+		pmu->fw_ref = fw_ref;

how this shared fw_ref is supposed to work for multiple
perf_event_open()?

fd1 = perf_event_open( ... gt=0 ...);

	event_get_forcewake()
		pmu->fw_ref = xe_force_wake_get()

fd2 = perf_event_open( ... gt=1 ...);

	event_get_forcewake()
		// get the forcewake, but don't save the ref

forcewake for gt1 is never put.


Or even multiple perf_event_open() for the same gt: we will not handle
the count correctly.

In summary I think this fw ref needs to be per event... an easy way to do
that is to use the event->pmu_private field, to be populated on init...

>+
>+	return true;
>+}
>+
> static bool event_supported(struct xe_pmu *pmu, unsigned int gt,
> 			    unsigned int id)
> {
>@@ -144,6 +176,13 @@ static bool event_param_valid(struct perf_event *event)
> static void xe_pmu_event_destroy(struct perf_event *event)
> {
> 	struct xe_device *xe = container_of(event->pmu, typeof(*xe), pmu.base);
>+	struct xe_pmu *pmu = &xe->pmu;
>+	struct xe_gt *gt;
>+
>+	if (pmu->fw_ref) {
>+		gt = xe_device_get_gt(xe, config_to_gt_id(event->attr.config));
>+		xe_force_wake_put(gt_to_fw(gt), pmu->fw_ref);
>+	}
>
> 	drm_WARN_ON(&xe->drm, event->parent);
> 	xe_pm_runtime_put(xe);
>@@ -183,18 +222,27 @@ static int xe_pmu_event_init(struct perf_event *event)
> 	if (!event->parent) {
> 		drm_dev_get(&xe->drm);
> 		xe_pm_runtime_get(xe);
>+		if (!event_gt_forcewake(event)) {

if you group an engine vs non-engine counter, this won't work I think.
Can we test it?

Lucas De Marchi

>+			xe_pm_runtime_put(xe);
>+			drm_dev_put(&xe->drm);
>+			return -EINVAL;
>+		}
> 		event->destroy = xe_pmu_event_destroy;
> 	}
>
> 	return 0;
> }
>
>-static u64 read_engine_events(struct xe_gt *gt, struct perf_event *event)
>+static u64 read_engine_events(struct xe_gt *gt, struct perf_event *event, u64 prev)
> {
> 	struct xe_device *xe = gt_to_xe(gt);
>+	struct xe_pmu *pmu = &xe->pmu;
> 	struct xe_hw_engine *hwe;
> 	u64 val = 0;
>
>+	if (!pmu->fw_ref)
>+		return prev;
>+
> 	hwe = event_to_hwe(event);
> 	if (!hwe)
> 		drm_warn(&xe->drm, "unknown engine\n");
>@@ -218,7 +266,7 @@ static u64 __xe_pmu_event_read(struct perf_event *event, u64 prev)
> 		return xe_gt_idle_residency_msec(&gt->gtidle);
> 	case XE_PMU_EVENT_ENGINE_ACTIVE_TICKS:
> 	case XE_PMU_EVENT_ENGINE_TOTAL_TICKS:
>-		return read_engine_events(gt, event);
>+		return read_engine_events(gt, event, prev);
> 	}
>
> 	return 0;
>diff --git a/drivers/gpu/drm/xe/xe_pmu_types.h b/drivers/gpu/drm/xe/xe_pmu_types.h
>index f5ba4d56622c..07c4e592106e 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;
>+	/**
>+	 * @fw_ref: force_wake ref
>+	 */
>+	unsigned int fw_ref;
> 	/**
> 	 * @supported_events: Bitmap of supported events, indexed by event id
> 	 */
>-- 
>2.47.1
>