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

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Tue, 25 Mar 2025 15:01:47 -0700, Belgaumkar, Vinay wrote:
>
>
> On 3/25/2025 2:53 PM, Dixit, Ashutosh wrote:
> > On Tue, 25 Mar 2025 13:33:43 -0700, Belgaumkar, Vinay wrote:
> >> On 3/25/2025 10:15 AM, Dixit, Ashutosh wrote:
> >>> 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.
> >> Hi Ashutosh,
> >>
> >>    As part of a previous decision, in the Xe PMU implementation, we are
> >> doing a runtime_get() during pmu_init for all PMU sessions. So, device is
> >> going to be awake anyways. In this case, it does not make sense to just
> >> read the register without a fwake.
> > 
> > Even if that is ok... Let us take a completely idle device. What should the
> > actual and requested freq's be for this case? Both should be zero, correct?
> > Now, what are we going to show if we are taking fwake? At least the
> > requested freq will be non-zero? Is that ok? Or the requested freq will
> > show zero even if we take fwake? Thanks.
>
> If we take fwake before reading, requested frequency will not be zero even
> if GT is idle. HW always retains the last requested frequency in that
> register, never zeroes it like it does for actual_freq. So, showing the
> non-zero value is the correct thing to do instead of artifically zeroing
> it.

So now, this I disagree with. For an idle device, hopefully in C6, there
should be no reason to request a non-zero freq. So the requested freq
should show 0. As i915 does (using gt parked and unparked states and using
with_intel_runtime_pm_if_in_use()).

If we are already "doing a runtime_get() during pmu_init" (as mentioned
above) we need to devise some other way of making this happen (maybe by
looking at xe_force_wake_domain ref field?).

Let us wait for a second opinion on this. Thanks.