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

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

On 12/20/2024 11:32 AM, Lucas De Marchi wrote:
> 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.

The pmu_info added to drm_client in these patches is just a void pointer 
to the info. So, if a client doesn't support it, the freq/c6 stuff will 
not get displayed. Tool should still work in that case.

Thanks,

Vinay.

>
>>
>> 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@intel.com>
>>> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>> > Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>>> > Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@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(-)
>>> >