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

["Belgaumkar, Vinay" <vinay.belgaumkar@intel.com>]

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.

Thanks,

Vinay.

>
> Thanks.
> --
> Ashutosh