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

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Tue, Feb 11, 2025 at 03:39:47PM +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)
>
>Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>Signed-off-by: Riana Tauro <riana.tauro@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..0539dd61ddc3 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;
>+
>+	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)) {
>+			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)

extra space before u64 prev. Strangely checkpatch does not complain!

Rest LGTM, 

Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

Thanks,
Umesh