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

["Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com>]

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?

If the overhead has already been considered and found to be acceptable, 
I am fine with avoiding unnecessary modifications to this patch.


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