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

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

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.