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:57:25 -0700, Belgaumkar, Vinay wrote:
>
> On 8/31/2023 4:16 PM, Dixit, Ashutosh wrote:
> > 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?
>
> I guess that'll work, but the events in i915 are divided into ones that
> need the timer (non-engine) (max of I915_NUM_PMU_SAMPLERS) and the
> per-engine events (that don't need the timer).

i915 per engine events need timer. They are just stored in
engine->pmu->sample[] array, not in i915->pmu->sample[][]
array. __I915_NUM_PMU_SAMPLERS is the size of the 2nd array.

> We now should have 3 types?
> Non-engine-non-timer, non-engine-timer and per-engine?

RC6 events in i915 are also non-engine-non-timer (same as this patch). So
it's the same.

__XE_NUM_PMU_SAMPLERS is just the size of the xe->pmu->sample[][] array.

> Or just club together the non-engine ones?

I'd say let's restrict here to reviewing this patch. Everything in this
patch can be changed except the uapi, the implementation can be completely
changed/enhanced by future patches. So we should just make sure the uapi
should not change by future changes.

Incidentally I am against exposing freq through the PMU since it is
available through sysfs. But we can have that discussion when we get to it.

Thanks.
--
Ashutosh