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

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

On 14-02-2025 15:38, 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;
> +
> +	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)) {
> +			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;

LGTM
Reviewed-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>

>   	/**
>   	 * @supported_events: Bitmap of supported events, indexed by event id
>   	 */