[PATCH v4] drm/xe/pmu: Add GT frequency events

[Dixit, Ashutosh]

On Mon, 24 Mar 2025 19:37:32 -0700, Belgaumkar, Vinay wrote:
>

Hi Vinay,

> On 3/24/2025 5:18 PM, Dixit, Ashutosh wrote:
> > On Mon, 24 Mar 2025 16:24:02 -0700, Vinay Belgaumkar wrote:
> >> @@ -266,11 +274,24 @@ 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_REQUESTED_FREQUENCY:
> >> +		if (!xe_guc_pc_get_cur_freq(&gt->uc.guc.pc, &cur_gt_freq))
> > This is unconditionally taking the forcewake and waking the card up just to
> > get the sample. Do we really want to do that?
> >
> > So if we don't do that, both the actual and requested freq will be 0 if gt
> > is in C6.
>
> For actual frequency, the register(0xc60) does not belong to any fw domain -
>
> GEN_FW_RANGE(0xc00, 0xfff, 0),
>
> HW will report 0 when GT is in C6.

Yes, no issue about act_freq, see commit 22009b6dad66. I was referring only
to requested freq.

> The requested freq register is a
> shadowed register (0xa008), so that will not accrue fwake either.
>
> static const struct i915_range mtl_shadowed_regs[] = {
>         { .start =   0x2030, .end =   0x2030 },
>         { .start =   0x2510, .end =   0x2550 },
>         { .start =   0xA008, .end =   0xA00C },

So this still doesn't make sense because:

  1. The fact is that xe_guc_pc_get_cur_freq() *is* taking forcewake
  2. And that is in accord with the following comment in i915/intel_uncore.c

     * Shadowing only applies to writes; forcewake
     * must still be acquired when reading from registers in these ranges.

Also see intel_rps_read_punit_req() which is called from i915 PMU
(frequency_sample()) and uses with_intel_runtime_pm_if_in_use(), so we'd
need to do use the equivalent in xe.

Thanks.
--
Ashutosh