[PATCH 1/4] drm/xe/pmu: Enable PMU interface

[Riana Tauro]

Hi Lucas

On 11/26/2024 12:37 AM, Umesh Nerlige Ramappa wrote:
> On Mon, Sep 02, 2024 at 10:01:11PM -0500, Lucas De Marchi wrote:
>> On Mon, Sep 02, 2024 at 11:29:05AM GMT, Aravind Iddamsetty wrote:
>>>
>>> On 30/08/24 03:40, Belgaumkar, Vinay wrote:
>>>>
>>>> On 8/28/2024 12:33 PM, Lucas De Marchi wrote:
>>>>> On Tue, Aug 27, 2024 at 09:41:04AM GMT, Vinay Belgaumkar wrote:
>>>>>> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
>>>>>> index b6fbe4988f2e..de6f39db618c 100644
>>>>>> --- a/include/uapi/drm/xe_drm.h
>>>>>> +++ b/include/uapi/drm/xe_drm.h
>>>>>> @@ -1389,6 +1389,40 @@ struct drm_xe_wait_user_fence {
>>>>>>     __u64 reserved[2];
>>>>>> };
>>>>>>
>>>>>> +/**
>>>>>> + * DOC: XE PMU event config IDs
>>>>>> + *
>>>>>> + * Check 'man perf_event_open' to use the ID's XE_PMU_XXXX listed 
>>>>>> in xe_drm.h
>>>>>> + * in 'struct perf_event_attr' as part of perf_event_open syscall 
>>>>>> to read a
>>>>>> + * particular event.
>>>>>> + *
>>>>>> + * For example to open the XE_PMU_RENDER_GROUP_BUSY(0):
>>>>>> + *
>>>>>> + * .. code-block:: C
>>>>>> + *
>>>>>> + *    struct perf_event_attr attr;
>>>>>> + *    long long count;
>>>>>> + *    int cpu = 0;
>>>>>> + *    int fd;
>>>>>> + *
>>>>>> + *    memset(&attr, 0, sizeof(struct perf_event_attr));
>>>>>> + *    attr.type = type; // eg: /sys/bus/event_source/devices/ 
>>>>>> xe_0000_56_00.0/type
>>>>>> + *    attr.read_format = PERF_FORMAT_TOTAL_TIME_ENABLED;
>>>>>> + *    attr.use_clockid = 1;
>>>>>> + *    attr.clockid = CLOCK_MONOTONIC;
>>>>>> + *    attr.config = XE_PMU_RENDER_GROUP_BUSY(0);
>>>>>> + *
>>>>>> + *    fd = syscall(__NR_perf_event_open, &attr, -1, cpu, -1, 0);
>>>>>> + */
>>>>>> +
>>>>>> +/*
>>>>>> + * Top bits of every counter are GT id.
>>>>>> + */
>>>>>> +#define __XE_PMU_GT_SHIFT (56)
>>>>>> +
>>>>>> +#define ___XE_PMU_OTHER(gt, x) \
>>>>>> +    (((__u64)(x)) | ((__u64)(gt) << __XE_PMU_GT_SHIFT))
>>>>>> +
>>>>>
>>>>> The perf uapi is self-describing and users should look up on sysfs 
>>>>> what
>>>>> to use. Example for i915 since it's what I'm currently working on:
>>>>>
>>>>>     $ cat /sys/bus/event_source/devices/i915/events/actual-frequency
>>>>>     config=0x100000
>>>>>     $ cat /sys/bus/event_source/devices/i915/events/actual- 
>>>>> frequency.unit
>>>>>     M
>>>>>
>>>>> `perf list` works fine and doesn't know anything about this xe-only
>>>>> header. Why would we add anything here rather than encourage other 
>>>>> users
>>>>> to read from the generic interface?
>>>>
>>>> Agree. perf list | grep rc6 is sufficient.
>>>
>>> This was previously asked by Rodrigo https:// 
>>> patchwork.freedesktop.org/patch/555013/?series=119504&rev=5
>>>
>>> so what changed from then to now.
>>
>> 2 different things. That rev you are pointing out had:
>>
>> include/uapi/drm/xe_drm.h            |  16 +
>>
>> so the uapi was being changed without proper doc. Rodrigo asked for the
>> documentation to be added, because that is the proper way to add things
>> to the uapi header.
>>
>> In this review what I noticed though is that the change shouldn't be
>> there in the first place. We shouldn't change anything in the drm/xe
>> UAPI header because this is actually an UAPI (via sysfs) coming from
>> perf.
> 
> @Lucas,
> 
> Agree. This is carried over from i915, where other constraints may have 
> led to adding some header bits for gt.
> 
> I want to mention that now there are 2 events per engine - ticks (actual 
> engine run ticks) and total_ticks (total ticks that the engine was 
> alloted) which enable us to take a ratio of deltas to calculate 
> utilization at the engine level. If we add the config for each possible 
> combination of gt and vf_id, we would have
> 
> num_engines * 2 * num_gts * num_vfs
> 
> where num_vfs should (ideally) be the number of vfs provisioned.

Here it will be max_vfs instead of provisioned num_vfs. Pmu event 
attributes are added on pmu_register.

num_engines * 2 * num_gts * max_vfs

Did not find a way to add/remove events dynamically.

Thanks
Riana

> 
> Do you think that's fine?
> 
> Thanks,
> Umesh
> 
>>
>> Lucas De Marchi
>>
>>
>>>
>>> Thanks,
>>> Aravind.
>>>
>>>>
>>>> Thanks,
>>>>
>>>> Vinay.
>>>>
>>>>>
>>>>> Lucas De Marchi