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 Fri, Feb 21, 2025 at 12:26:56AM -0600, Lucas De Marchi wrote:
>On Thu, Feb 20, 2025 at 10:45:46PM -0600, Lucas De Marchi wrote:
>>On Thu, Feb 20, 2025 at 05:14:07PM -0800, Umesh Nerlige Ramappa wrote:
>>>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.
>>
>>so... we never set pmu->fw_ref back to 0 and any event destroy will try to
>>put the force wake? That seems a different bug that avoids the bug I
>>was thinking about.

oh, yeah missed that. That's a bug.

Thanks,
Umesh