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

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

On Thu, Feb 20, 2025 at 03:46:55PM -0600, Lucas De Marchi wrote:
>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@intel.com>
>>Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>>Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>>Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@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()?

Agree, not ideal, but I don't see an issue. This forcewake is only being 
taken for engine-* events and the domain is always XE_FW_GT. Looking at 
xe_force_wake_get(), I see that it returns a mask of domains enabled. In 
this case, it would be the XE_FW_GT. The return value is just stored so 
that the corresponding event destroy can put the forcewake.

>
>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. 

pmu->fw_ref should be identical for all events taking this forcewake.

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

The count is actually handled in domain->ref in the forcewake 
implementation and note that forcewake is always taken for every engine 
event that is being initialized and hence always being put for every 
event that is destroyed. This code is not refcounting that.

>
>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...

I am not opposed to that since that makes it future proof so that we can 
indeed have events taking different forcewake domains, but let me know 
if I missed something here since I think this alone should still work.

>
>>+
>>+	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?

When you group events, init is called for each event. From the Xe PMU 
implementation perspective, grouping shouldn't be any different.

Regards,
Umesh

>
>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
>>