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

[Riana Tauro <riana.tauro@intel.com>]

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

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