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

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

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.

Do you think that's fine?

Thanks,
Umesh

>
>Lucas De Marchi
>
>
>>
>>Thanks,
>>Aravind.
>>
>>>
>>>Thanks,
>>>
>>>Vinay.
>>>
>>>>
>>>>Lucas De Marchi