| Message ID | 20241220003734.69174-1-vinay.belgaumkar@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | tools/gputop: Add PMU stats | expand |
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? 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. 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(-) >
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 Meanwhile our PMU are blocked... 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(-) > >
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@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(-) >> >
On 20/12/2024 19:32, 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. Hm what? :) With "nicer UI and more featureful" I was referring to the alternatives Lucas later listed. All are nicer than intel_gpu_top, not to mention than gputop. >> 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. 100%. >> >> 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 Yes, perf was always enough and should still be. > 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. My 2p is that adding PMU support to gputop only makes sense if it will work with more than only xe and if people kind of see that would make having two top-like tools in IGT not great at that point. And that the road to completely subsume intel_gpu_top in gputop is not a long one once the first part is done properly. I wasn't arguing for some nice UI if that was misunderstood somehow. (Don't know how.) It may even come naturally because many of the nicer features of intel_gpu_top came by user demand. So assuming some PMU support gets added to xe users then might ask - "Hey can we have JSON/CSV output modes so we can use it like we used intel_gpu_top?". Or any other feature like sort modes or whatever. Regards, Tvrtko > > 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(-) >>> >
Use the PMU support being added in https://patchwork.freedesktop.org/series/139121/ to add freq/c6 stats. 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(-)