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

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

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). We now should have 3 
types? Non-engine-non-timer, non-engine-timer and per-engine?  Or just 
club together the non-engine ones?

Thanks,

Vinay.