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

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

On Mon, Feb 10, 2025 at 03:50:00PM +0530, Riana Tauro wrote:
>
>
>On 2/8/2025 5:01 AM, Umesh Nerlige Ramappa wrote:
>>On Fri, Feb 07, 2025 at 12:21:24PM +0530, Ghimiray, Himal Prasad wrote:
>>>
>>>
>>>On 07-02-2025 11:48, Riana Tauro wrote:
>>>>
>>>>Hi Himal
>>>>
>>>>On 2/7/2025 8:39 AM, Ghimiray, Himal Prasad wrote:
>>>>>
>>>>>
>>>>>On 06-02-2025 16:13, 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.
>>>>>>
>>>>>>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       | 47 
>>>>>>++++++++++++++++++++++++++ +++--
>>>>>>  drivers/gpu/drm/xe/xe_pmu_types.h |  8 ++++++
>>>>>>  2 files changed, 53 insertions(+), 2 deletions(-)
>>>>>>
>>>>>>diff --git a/drivers/gpu/drm/xe/xe_pmu.c b/drivers/gpu/drm/xe/xe_pmu.c
>>>>>>index 06a1c72a3838..5b5fe4424aba 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,36 @@ 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 void 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;
>>>>>>+
>>>>>>+    gt = xe_device_get_gt(xe, config_to_gt_id(config));
>>>>>>+    if (!gt || !is_engine_event(config))
>>>>>>+        return;
>>>>>>+
>>>>>>+    fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FW_GT);
>>>>>>+    if (!fw_ref)
>>>>>>+        return;
>>>>>>+
>>>>>>+    if (!pmu->fw_ref)
>>>>>>+        pmu->fw_ref = fw_ref;
>>>>>>+
>>>>>>+    pmu->fw_count++;
>>>>>>+}
>>>>>>+
>>>>>>  static bool event_supported(struct xe_pmu *pmu, unsigned int gt,
>>>>>>                  unsigned int id)
>>>>>>  {
>>>>>>@@ -144,6 +175,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_count--) {
>>>>>>+        gt = xe_device_get_gt(xe, config_to_gt_id(event- 
>>>>>>>attr.config));
>>>>>>+        xe_force_wake_put(gt_to_fw(gt), pmu->fw_ref);
>>>>>>+    }
>>>>>
>>>>>
>>>>>Considering that fw->lock will be acquired and released 
>>>>>multiple times during the put operation, this might create an 
>>>>>overhead.
>>>>>
>>>>>How about implementing a _put function that can take the 
>>>>>number of refcounts to decrement as an input parameter, 
>>>>>similar to xe_force_wake_put_many?
>>>>Could you give more details on your suggestion? Would put_many 
>>>>just decrement the count? But wouldn't that still require a 
>>>>lock? Multiple event_destroys can call the function at the same 
>>>>time right?
>>>
>>>I was thinking about putting all refcounts at the end of last 
>>>event destroy in case of multiple pmu's.
>>>
>>>>
>>>>
>>>>One thing that can be done is to take forcewake on first count 
>>>>and release it when the last event is destroyed in cases of 
>>>>multiple
>>>>pmu being used
>>
>>Unless there is a measured inefficiency, I would recommend not 
>>refcounting this in PMU. If a forcewake is already taken, the code 
>>in forcewake_get is just handling increments and not really 
>>accessing MMIO, so we should be okay here.
>>
>>Also, pmu->fw_count is not required, since the force_wake_get logic 
>>should be already handling that. We should just call get and put and 
>>this should be good enough.
>
>I added the counting because if forcewake get fails then destroy won't 
>know if it has to call xe_force_wake_put (multiple perf opened). If 
>there is count and ref set, then can be called based on that.
>
>If we can return -ENOTSUPPORTED if forcewake get fails for engine 
>events, then the above will work

Hmm, not sure I understand. If forcewake_get fails, then we don't need 
to call force_wake_put. Also if force_wake_get fails, we should fail the 
event init (which means the destroy should not get called).

Thanks,
Umesh

>
>Thanks
>Riana
>
>>
>>Thanks,
>>Umesh
>>
>>>
>>>This sounds even better.
>>>
>>>>>
>>>>>If the overhead has already been considered and found to be 
>>>>>acceptable, I am fine with avoiding unnecessary modifications 
>>>>>to this patch.
>>>>This is the first rev for this patch. Open to suggestions
>>>>
>>>>Background for this patch: force_wake is needed to read the timestamp
>>>>register required for engine events.Cannot take it while reading 
>>>>the register from pmu_read due to a lockdep splat 
>>>>(PROVE_RAW_LOCK_NESTING).
>>>>
>>>>The suggestion was to take forcewake throughout the duration of 
>>>>event being read
>>>>
>>>>Thanks
>>>>Riana
>>>>>
>>>>>
>>>>>>      drm_WARN_ON(&xe->drm, event->parent);
>>>>>>      xe_pm_runtime_put(xe);
>>>>>>@@ -183,18 +221,23 @@ static int xe_pmu_event_init(struct 
>>>>>>perf_event *event)
>>>>>>      if (!event->parent) {
>>>>>>          drm_dev_get(&xe->drm);
>>>>>>          xe_pm_runtime_get(xe);
>>>>>>+        event_gt_forcewake(event);
>>>>>>          event->destroy = xe_pmu_event_destroy;
>>>>>>      }
>>>>>>      return 0;
>>>>>>  }
>>>>>>-static u64 read_engine_events(struct perf_event *event)
>>>>>>+static u64 read_engine_events(struct perf_event *event, u64 prev)
>>>>>>  {
>>>>>>      struct xe_device *xe = container_of(event->pmu, 
>>>>>>typeof(*xe), pmu.base);
>>>>>>+    struct xe_pmu *pmu = &xe->pmu;
>>>>>>      struct xe_hw_engine *hwe;
>>>>>>      u64 val = 0;
>>>>>>+    if (!pmu->fw_count)
>>>>>>+        return prev;
>>>>>>+
>>>>>>      hwe = event_to_hwe(event);
>>>>>>      if (!hwe)
>>>>>>          drm_warn(&xe->drm, "unknown pmu engine\n");
>>>>>>@@ -218,7 +261,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(event);
>>>>>>+        return read_engine_events(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..134b3400b19c 100644
>>>>>>--- a/drivers/gpu/drm/xe/xe_pmu_types.h
>>>>>>+++ b/drivers/gpu/drm/xe/xe_pmu_types.h
>>>>>>@@ -30,6 +30,14 @@ struct xe_pmu {
>>>>>>       * @name: Name as registered with perf core.
>>>>>>       */
>>>>>>      const char *name;
>>>>>>+    /**
>>>>>>+     * @fw_ref: force_wake ref
>>>>>>+     */
>>>>>>+    unsigned int fw_ref;
>>>>>>+    /**
>>>>>>+     * @fw_count: force_wake count
>>>>>>+     */
>>>>>>+    unsigned int fw_count;
>>>>>>      /**
>>>>>>       * @supported_events: Bitmap of supported events, 
>>>>>>indexed by event id
>>>>>>       */
>>>>>
>>>>
>>>
>