Re: [PATCH 5/7] drm/xe: Add single engine busyness support

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

On Wed, Nov 13, 2024 at 10:25:47AM +0530, Riana Tauro wrote:
>GuC provides support to read engine active counters to calculate the
>engine utilization. KMD exposes two counters via the PMU interface to
>calculate engine busyness
>
>Engine Active Ticks(<engine>-busy-ticks-gt<n>) - number of active ticks
>						 for engine
>Total Ticks (<engine>-total-ticks-gt<n>) - total ticks GT has been active
>
>Busyness percentage can be calculated as below
>busyness % = (engine active ticks/total ticks) * 100.
>
>Signed-off-by: Riana Tauro <riana.tauro@intel.com>

Some minor comments below, 

>---
> drivers/gpu/drm/xe/Makefile                   |   1 +
> drivers/gpu/drm/xe/abi/guc_actions_abi.h      |   1 +
> drivers/gpu/drm/xe/regs/xe_gt_regs.h          |   2 +
> drivers/gpu/drm/xe/xe_engine_activity.c       | 317 ++++++++++++++++++
> drivers/gpu/drm/xe/xe_engine_activity.h       |  18 +
> drivers/gpu/drm/xe/xe_engine_activity_types.h |  85 +++++
> drivers/gpu/drm/xe/xe_guc_fwif.h              |  19 ++
> drivers/gpu/drm/xe/xe_guc_types.h             |   4 +
> 8 files changed, 447 insertions(+)
> create mode 100644 drivers/gpu/drm/xe/xe_engine_activity.c
> create mode 100644 drivers/gpu/drm/xe/xe_engine_activity.h
> create mode 100644 drivers/gpu/drm/xe/xe_engine_activity_types.h
>

[snip]

>+static u64 get_engine_active_ticks(struct xe_guc *guc, struct xe_hw_engine *hwe)
>+{
>+	struct engine_activity *ea = hw_engine_to_engine_activity(hwe);
>+	struct guc_engine_activity *cached_activity = &ea->activity;
>+	struct guc_engine_activity_metadata *cached_metadata = &ea->metadata;
>+	struct xe_engine_activity *engine_busy = &guc->engine_busy;
>+	struct activity_buffer *device_buffer = &engine_busy->device_buffer;
>+	struct xe_device *xe =  guc_to_xe(guc);
>+	struct xe_gt *gt = guc_to_gt(guc);
>+
>+	u16 guc_class = xe_engine_class_to_guc_class(hwe->class);
>+	size_t offset = offsetof(struct guc_engine_activity_data,
>+				 engine_activity[guc_class][hwe->logical_instance]);
>+	struct iosys_map engine_activity_map = IOSYS_MAP_INIT_OFFSET(activity_to_map(device_buffer),
>+								     offset);
>+	u32 last_update_tick, global_change_num;
>+	u64 active_ticks, gpm_ts;
>+	u16 change_num;
>+
>+	global_change_num = read_metadata_record(xe, device_buffer, global_change_num);
>+
>+	/* GuC has not initialized activity data yet, return 0 */
>+	if (!global_change_num)
>+		goto update;
>+
>+	if (global_change_num == cached_metadata->global_change_num)
>+		goto update;
>+	else
>+		cached_metadata->global_change_num = global_change_num;
>+
>+	change_num = read_engine_activity_record(xe, &engine_activity_map, change_num);
>+
>+	if (!change_num || change_num == cached_activity->change_num)
>+		goto update;
>+
>+	/* read engine activity values */
>+	last_update_tick = read_engine_activity_record(xe, &engine_activity_map, last_update_tick);
>+	active_ticks = read_engine_activity_record(xe, &engine_activity_map, active_ticks);

Whatever we read from the metadata record and the activity record could 
be added to a trace call so that we can debug issues using ftrace when 
needed.

>+
>+	/* activity calculations */
>+	ea->running = !!last_update_tick;
>+	ea->total += active_ticks - cached_activity->active_ticks;
>+	ea->active = 0;
>+
>+	/* cache the counter */
>+	cached_activity->change_num = change_num;
>+	cached_activity->last_update_tick = last_update_tick;
>+	cached_activity->active_ticks = active_ticks;
>+
>+update:
>+	if (ea->running) {
>+		gpm_ts  = xe_mmio_read64_2x32(&gt->mmio, MISC_STATUS_0) >>

extra space after gpm_ts ^.

Also, does the mmio read require a forcewake here?

>+			  engine_busy->gpm_timestamp_shift;
>+		ea->active = lower_32_bits(gpm_ts) - cached_activity->last_update_tick;
>+	}
>+
>+	return ea->total + ea->active;
>+}
>+

[snip]

>+static u32 gpm_timestamp_shift(struct xe_gt *gt)
>+{
>+	u32 reg;
>+
>+	reg = xe_mmio_read32(&gt->mmio, RPM_CONFIG0);

Does the mmio read require a forcewake here?

>+
>+	return 3 - REG_FIELD_GET(RPM_CONFIG0_CTC_SHIFT_PARAMETER_MASK, reg);
>+}

[snip]

>+struct engine_activity {
>+	/** @active: current activity */
>+	u64 active;
>+
>+	/** @last_cpu_ts: cpu timestamp in nsec of previous sample */
>+	u64 last_cpu_ts;
>+
>+	/** @quanta: total quanta used on HW */
>+	u64 quanta;
>+
>+	/** @quanta_ns: total quanta_ns used on HW */
>+	u64 quanta_ns;
>+
>+	/**
>+	 * @quanta_remainder_ns: remainder when the CPU time is scaled as
>+	 * per the quanta_ratio. This remainder is used insubsequent

s/insubsequent/in subsequent/

Thanks,
Umesh