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

[Aravind Iddamsetty]

On 01/09/23 05:28, Dixit, Ashutosh wrote:
HI Ashutosh,
> 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 for taking up the queries, yup let's stick to this series and
handle the future changes as they come.

Regards,
Aravind.
>
> Thanks.
> --
> Ashutosh