[PATCH i-g-t v2 0/4] tools/gputop: Add PMU stats

[Lucas De Marchi]

On Fri, Dec 20, 2024 at 02:13:26PM -0500, Rodrigo Vivi wrote:
>On Fri, Dec 20, 2024 at 10:15:04AM +0000, Tvrtko Ursulin wrote:
>>
>> On 20/12/2024 00:37, Vinay Belgaumkar wrote:
>> > Use the PMU support being added in
>> > https://patchwork.freedesktop.org/series/139121/ to add freq/c6 stats.
>>
>> Brace yourself for the customary "why". So yes, why?
>>
>> Gputop so far was a reference showcase for DRM fdinfo, nothing more. Why add
>> a _subset_ of PMU stats to it? Any other drivers could be wired up? How far
>> do people intend to grow it, considering alternatives with nicer UI and more
>> featureful exist?
>
>Well, currently intel_gpu_top doesn't support Xe and we really don't
>have any intention to add xe support there.
>
>We don't believe it is a better UI and more features.
>
>Hopefully someday we can get qmassa [1] part of the distros and make that to
>be our default to use the uapis that we add in Xe.
>
>But for now we were in the hope that we could use gputop for that which is
>the current tool that supports Xe metrics. But well, I just noticed that
>at least in Fedora it is not packaged :/
>
>$ rpm -ql igt-gpu-tools-1.27.1-0.4.20230215git45da871.fc40.x86_64 | grep top
>/usr/bin/intel_gpu_top
>/usr/share/man/man1/intel_gpu_top.1.gz
>
>[1] - https://github.com/ulissesf/qmassa
>
>So, our options are:
>
>1. Add all the Xe support in intel_gpu_top
>2. Convince distros to build and package gputop along with intel_gpu_top in igt
>3. Convince distros to adopt qmaasa


I think we should handle gputop as a reference implementation for a
"top-like implementation for GPU".  I think end users have tools with
better UIs like qmassa, or nvtop, or htop or other graphical
applications and we shouldn't try to block that - doing something
beautiful in gputop would be a lot of effort for little benefit if end
users are already served by other tools.  Letting gputop as a reference
impl for these tools to borrow code or consult, would be ideal IMO.


As Tvrtko said, currently gputop is a reference showcase for DRM fdinfo.
I think it can grow some capabilities and be a reference
implementation for top-like application. If that means adding pmu, then
be it.  However the pmu support needs to be added in a proper way so
the tool always continue to be vendor-agnostic and that it's easy to
add support for events from other vendors. That probably means that
adding pmu-related things in the fdinfo or drm-client libs are probably
not the way to go as a) it's crossing unrelated abstraction and b)
breaking the vendor-agnostic nature of the tool.

>
>Meanwhile our PMU are blocked...

I don't think they should be blocked. The kernel impl was blocked for a
long time in other things, not having PMU included somewhere like
gputop.  If you want to read pmu, the #1 application is perf

I think we can add pmu in gputop as a reference. If someone can grow
gputop to have all the intel_gpu_top capabilities, but doing it in a
proper vendor-agnostic way, awesome. At that time we may then retire
intel_gpu_top and only use gputop as reference.

Lucas De Marchi

>
>Lucas, Thomas, thoughts?
>
>>
>> Or in case the conclusion ends up being "yes", then lets at least share some
>> more code between intel_gpu_top and this work. Ie. make it in a way gputop
>> completely subsumes and replaces intel_gpu_top might be an idea.
>
>with this I agree as well.
>
>>
>> Regards,
>>
>> Tvrtko
>>
>> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>> > Cc: Kamil Konieczny <kamil.konieczny at linux.intel.com>
>> > Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar at intel.com>
>> >
>> > Vinay Belgaumkar (4):
>> >    tools/gputop: Define data structs for PMU stats
>> >    lib/igt_drm_clients: Add pdev and driver info
>> >    lib/igt_perf: Add utils to extract PMU event info
>> >    tools/gputop: Add GT freq and c6 stats
>> >
>> >   lib/igt_drm_clients.c |   6 ++
>> >   lib/igt_drm_clients.h |   4 +
>> >   lib/igt_perf.c        |  68 +++++++++++++
>> >   lib/igt_perf.h        |   2 +
>> >   tools/gputop.c        | 225 ++++++++++++++++++++++++++++++++++++++++++
>> >   tools/meson.build     |   2 +-
>> >   6 files changed, 306 insertions(+), 1 deletion(-)
>> >