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

[Belgaumkar, Vinay]

On 3/25/2025 3:45 PM, Dixit, Ashutosh wrote:
> 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()).
We are not requesting a frequency when GT is in C6. This register holds 
the last requested frequency by GuC (before GT got into C6). Also, just 
because the register is non-zero doesn't mean GT is not in C6.
>
> 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?).
I still don't understand why we should show zero requested freq when GT 
is in C6. We should show the real value of that register. The use case 
for PMU is typically when a workload is running, not when GT is idle. 
There are other ways (sysfs, residency registers) to find out if GT is 
in C6 without needing to show an artificial zero value.
>
> Let us wait for a second opinion on this. Thanks.

Sure.

Thanks,

Vinay.