Re: [Intel-xe] [PATCH v5 3/3] drm/xe/pmu: Enable PMU interface

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

On Thu, 31 Aug 2023 16:22:10 -0700, Belgaumkar, Vinay wrote:
>
> >>>>> +static void xe_pmu_event_read(struct perf_event *event)
> >>>>> +{
> >>>>> +    struct xe_device *xe =
> >>>>> +        container_of(event->pmu, typeof(*xe), pmu.base);
> >>>>> +    struct hw_perf_event *hwc = &event->hw;
> >>>>> +    struct xe_pmu *pmu = &xe->pmu;
> >>>>> +    u64 prev, new;
> >>>>> +
> >>>>> +    if (pmu->closed) {
> >>>>> +        event->hw.state = PERF_HES_STOPPED;
> >>>>> +        return;
> >>>>> +    }
> >>>>> +again:
> >>>>> +    prev = local64_read(&hwc->prev_count);
> >>>>> +    new = __xe_pmu_event_read(event);
> >>>>> +
> >>>>> +    if (local64_cmpxchg(&hwc->prev_count, prev, new) != prev)
> >>>>> +        goto again;
> >>>>> +
> >>>>> +    local64_add(new - prev, &event->count);
> >>>>> +}
> >>>>> +
> >>>>> +static void xe_pmu_enable(struct perf_event *event)
> >>>>> +{
> >> The i915_pmu code checks which event is requested here and accordingly sets pmu->enable (which doesn't seem to be defined here yet). Any reason we are not doing this yet?
> >
> > in i915 pmu->enable is only used by events for which there is an internal timer sampler
> > which periodically samples those events, this series is not adding such events.
>
> Ok, the tracked events are bound by I915_NUM_PMU_SAMPLERS in i915, whereas
> you have already defined the max non/tracking ones as
> __XE_NUM_PMU_SAMPLERS, hence my confusion. Should we use a different name
> in lieu of the tracked events which will be defined in subsequent patches?
>
> enum {
>
>         __I915_SAMPLE_FREQ_ACT = 0,
>
>         __I915_SAMPLE_FREQ_REQ,
>         __I915_SAMPLE_RC6,
>         __I915_SAMPLE_RC6_LAST_REPORTED,
>         __I915_NUM_PMU_SAMPLERS
>
> };

+enum {
+       __XE_SAMPLE_RENDER_GROUP_BUSY,
+       __XE_SAMPLE_COPY_GROUP_BUSY,
+       __XE_SAMPLE_MEDIA_GROUP_BUSY,
+       __XE_SAMPLE_ANY_ENGINE_GROUP_BUSY,
+       __XE_NUM_PMU_SAMPLERS
+};
+

I'd think any future events will be added after
__XE_SAMPLE_ANY_ENGINE_GROUP_BUSY, changing the value of
__XE_NUM_PMU_SAMPLERS, no? Why do we need a different name?