Re: [PATCH v4 2/2] drm/xe: Add DRM GPU frequency tracepoint to Xe

[Tvrtko Ursulin <tursulin@ursulin.net>]

On 17/09/2025 09:41, S Sebinraj wrote:
> Integrate xe PMU with the DRM-level GPU frequency tracepoint to provide
> efficient frequency monitoring with change detection.
> 
> Key changes:
> - Add frequency change detection
> - Implement per-GT frequency tracking using last_act_freq array
> - Only trace when GPU frequency actually changes per GT
> 
> The integration traces actual GPU frequency changes from xe_pmu during
> XE_PMU_EVENT_GT_ACTUAL_FREQUENCY reads.
> 
> Signed-off-by: S Sebinraj <s.sebinraj@intel.com>
> ---
>   drivers/gpu/drm/drm_gpu_frequency_trace.c     |  2 +-
>   drivers/gpu/drm/xe/xe_gpu_freq_trace.h        | 14 ++++++++++
>   drivers/gpu/drm/xe/xe_pmu.c                   | 26 +++++++++++++++++--
>   drivers/gpu/drm/xe/xe_pmu_types.h             |  4 +++
>   .../drm/drm_gpu_frequency_trace.h             |  2 +-
>   5 files changed, 44 insertions(+), 4 deletions(-)
>   create mode 100644 drivers/gpu/drm/xe/xe_gpu_freq_trace.h
>   rename {drivers/gpu => include}/drm/drm_gpu_frequency_trace.h (96%)
> 
> diff --git a/drivers/gpu/drm/drm_gpu_frequency_trace.c b/drivers/gpu/drm/drm_gpu_frequency_trace.c
> index b5fa5134226d..e33df068752d 100644
> --- a/drivers/gpu/drm/drm_gpu_frequency_trace.c
> +++ b/drivers/gpu/drm/drm_gpu_frequency_trace.c
> @@ -9,7 +9,7 @@
>   #ifdef CONFIG_DRM_GPU_FREQUENCY_TRACE
>   
>   #define CREATE_TRACE_POINTS
> -#include "drm_gpu_frequency_trace.h"
> +#include <drm/drm_gpu_frequency_trace.h>
>   
>   EXPORT_TRACEPOINT_SYMBOL_GPL(gpu_frequency);
>   
> diff --git a/drivers/gpu/drm/xe/xe_gpu_freq_trace.h b/drivers/gpu/drm/xe/xe_gpu_freq_trace.h
> new file mode 100644
> index 000000000000..c15d41761296
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_gpu_freq_trace.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * GPU frequency trace wrapper for xe_pmu.c
> + * This header provides access to the gpu_frequency tracepoint
> + */
> +#ifndef _XE_GPU_FREQ_TRACE_H_
> +#define _XE_GPU_FREQ_TRACE_H_
> +
> +#include <drm/drm_gpu_frequency_trace.h>
> +
> +/* Convert MHz to KHz for tracepoint */
> +#define MHZ_TO_KHZ(freq_mhz)	((freq_mhz) * 1000)
> +
> +#endif /* _XE_GPU_FREQ_TRACE_H_ */
> diff --git a/drivers/gpu/drm/xe/xe_pmu.c b/drivers/gpu/drm/xe/xe_pmu.c
> index cab51d826345..7d5a6e149247 100644
> --- a/drivers/gpu/drm/xe/xe_pmu.c
> +++ b/drivers/gpu/drm/xe/xe_pmu.c
> @@ -5,9 +5,11 @@
>   
>   #include <drm/drm_drv.h>
>   #include <linux/device.h>
> +#include <linux/types.h>
>   
>   #include "xe_device.h"
>   #include "xe_force_wake.h"
> +#include "xe_gpu_freq_trace.h"
>   #include "xe_gt_idle.h"
>   #include "xe_guc_engine_activity.h"
>   #include "xe_guc_pc.h"
> @@ -291,6 +293,19 @@ static u64 read_engine_events(struct xe_gt *gt, struct perf_event *event)
>   	return val;
>   }
>   
> +static void xe_pmu_trace_frequency_change(struct xe_gt *gt, u32 act_freq)
> +{
> +	struct xe_device *xe = gt_to_xe(gt);
> +	struct xe_pmu *pmu = &xe->pmu;
> +	u32 gt_id = gt->info.id;
> +
> +	/* Only trace if frequency changed for this GT */
> +	if (gt_id < XE_PMU_MAX_GT && pmu->last_act_freq[gt_id] != act_freq) {
> +		trace_gpu_frequency(MHZ_TO_KHZ(act_freq), gt_id);
> +		pmu->last_act_freq[gt_id] = act_freq;
> +	}
> +}
> +
>   static u64 __xe_pmu_event_read(struct perf_event *event)
>   {
>   	struct xe_gt *gt = event_to_gt(event);
> @@ -304,8 +319,12 @@ static u64 __xe_pmu_event_read(struct perf_event *event)
>   	case XE_PMU_EVENT_ENGINE_ACTIVE_TICKS:
>   	case XE_PMU_EVENT_ENGINE_TOTAL_TICKS:
>   		return read_engine_events(gt, event);
> -	case XE_PMU_EVENT_GT_ACTUAL_FREQUENCY:
> -		return xe_guc_pc_get_act_freq(&gt->uc.guc.pc);
> +	case XE_PMU_EVENT_GT_ACTUAL_FREQUENCY: {
> +		u32 act_freq = xe_guc_pc_get_act_freq(&gt->uc.guc.pc);
> +
> +		xe_pmu_trace_frequency_change(gt, act_freq);

Not my driver but IMO it does not sound very useful to emit a tracepoint 
*only if* someone has the PMU open and not on actual frequency changes 
but at a frequency of userspace doing PMU reads. Not least that at this 
point userspace already has the frequency information via PMU so 
tracepoint is just adding kernel code for what purpose?

Regards,

Tvrtko

> +		return act_freq;
> +	}
>   	case XE_PMU_EVENT_GT_REQUESTED_FREQUENCY:
>   		return xe_guc_pc_get_cur_freq_fw(&gt->uc.guc.pc);
>   	}
> @@ -572,6 +591,9 @@ int xe_pmu_register(struct xe_pmu *pmu)
>   	pmu->base.stop		= xe_pmu_event_stop;
>   	pmu->base.read		= xe_pmu_event_read;
>   
> +	/* Initialize frequency tracking array */
> +	memset(pmu->last_act_freq, 0, sizeof(pmu->last_act_freq));
> +
>   	set_supported_events(pmu);
>   
>   	ret = perf_pmu_register(&pmu->base, pmu->name, -1);
> diff --git a/drivers/gpu/drm/xe/xe_pmu_types.h b/drivers/gpu/drm/xe/xe_pmu_types.h
> index f5ba4d56622c..630da8442387 100644
> --- a/drivers/gpu/drm/xe/xe_pmu_types.h
> +++ b/drivers/gpu/drm/xe/xe_pmu_types.h
> @@ -34,6 +34,10 @@ struct xe_pmu {
>   	 * @supported_events: Bitmap of supported events, indexed by event id
>   	 */
>   	u64 supported_events;
> +	/**
> +	 * @last_act_freq: Last actual frequency for each GT (for tracing changes only)
> +	 */
> +	u32 last_act_freq[XE_PMU_MAX_GT];
>   };
>   
>   #endif
> diff --git a/drivers/gpu/drm/drm_gpu_frequency_trace.h b/include/drm/drm_gpu_frequency_trace.h
> similarity index 96%
> rename from drivers/gpu/drm/drm_gpu_frequency_trace.h
> rename to include/drm/drm_gpu_frequency_trace.h
> index cf6337847b3a..47f32fd295a4 100644
> --- a/drivers/gpu/drm/drm_gpu_frequency_trace.h
> +++ b/include/drm/drm_gpu_frequency_trace.h
> @@ -42,6 +42,6 @@ static inline void trace_gpu_frequency(unsigned int state, unsigned int gpu_id)
>   
>   #ifdef CONFIG_DRM_GPU_FREQUENCY_TRACE
>   #undef TRACE_INCLUDE_PATH
> -#define TRACE_INCLUDE_PATH ../../drivers/gpu/drm
> +#define TRACE_INCLUDE_PATH ../../include/drm
>   #include <trace/define_trace.h>
>   #endif