[PATCH i-g-t] gputop: Add memory only utilization type
Christian Gmeiner
christian.gmeiner at gmail.com
Fri Nov 22 20:19:44 UTC 2024
Hi
> > Hi Tvrtko
> >
> >>> From: Christian Gmeiner <cgmeiner at igalia.com>
> >>>
> >>> Add special handling for drivers that only provide memory utilization
> >>> fdinfo.
> >>
> >> To check I am following correctly - special in this context means avoid
> >> displaying empty fields, or avoid a crash, or?
> >
> > - Avoid early return in print_client(..) -> no output show.
> > - Avoid a crash in strcmp(..).
> >
> >>
> >>> Signed-off-by: Christian Gmeiner <cgmeiner at igalia.com>
> >>> ---
> >>> tools/gputop.c | 20 ++++++++++++++++++++
> >>> 1 file changed, 20 insertions(+)
> >>>
> >>> diff --git a/tools/gputop.c b/tools/gputop.c
> >>> index 43b01f566..4135d84c1 100644
> >>> --- a/tools/gputop.c
> >>> +++ b/tools/gputop.c
> >>> @@ -35,6 +35,7 @@
> >>> enum utilization_type {
> >>> UTILIZATION_TYPE_ENGINE_TIME,
> >>> UTILIZATION_TYPE_TOTAL_CYCLES,
> >>> + UTILIZATION_TYPE_MEMORY_ONLY,
> >>> };
> >>>
> >>> static const char *bars[] = { " ", "▏", "▎", "▍", "▌", "▋", "▊", "▉", "█" };
> >>> @@ -141,6 +142,9 @@ engines_identical(const struct igt_drm_client *c,
> >>> c->engines->max_engine_id != pc->engines->max_engine_id)
> >>> return false;
> >>>
> >>> + if (!c->engines->num_engines && !pc->engines->num_engines)
> >>> + return true;
> >>
> >> strcmp() below crashes?
> >>
> >
> > Correct.
> >
> >> I would move the check you added to be first in this function. I think
> >> that makes most sense from the flow wise - checking it before max_engine_id.
> >>
> >
> > Will do in v2.
> >
> >> With this fixed what remains broken?
> >>
> >
> > Show any output :)
>
> I totally missed it. Shows the value of commit messages. ;)
>
> Okay.. I wonder if we can get away without adding
> UTILIZATION_TYPE_MEMORY_ONLY and instead make the code skip unavailable
> data.
>
> Would something like this work:
>
> diff --git a/tools/gputop.c b/tools/gputop.c
> index 43b01f566ac8..9639a7f5fde8 100644
> --- a/tools/gputop.c
> +++ b/tools/gputop.c
> @@ -137,6 +137,9 @@ engines_identical(const struct igt_drm_client *c,
> {
> unsigned int i;
>
> + if (!c->engines->num_engines && !pc->engines->num_engines)
> + return true;
> +
> if (c->engines->num_engines != pc->engines->num_engines ||
> c->engines->max_engine_id != pc->engines->max_engine_id)
> return false;
> @@ -188,27 +191,20 @@ print_client(struct igt_drm_client *c, struct
> igt_drm_client **prevc,
> uint64_t sz;
> int len;
>
> + if (c->samples < 2)
> + return 0;
> +
> if (c->utilization_mask & IGT_DRM_CLIENT_UTILIZATION_TOTAL_CYCLES &&
> - c->utilization_mask & IGT_DRM_CLIENT_UTILIZATION_CYCLES)
> + c->utilization_mask & IGT_DRM_CLIENT_UTILIZATION_CYCLES) {
> utilization_type = UTILIZATION_TYPE_TOTAL_CYCLES;
> - else if (c->utilization_mask & IGT_DRM_CLIENT_UTILIZATION_ENGINE_TIME)
> + /* Filter out idle clients. */
> + if (c->engines->num_engines && !c->total_total_cycles)
> + return 0;
> + } else if (c->utilization_mask & IGT_DRM_CLIENT_UTILIZATION_ENGINE_TIME) {
> utilization_type = UTILIZATION_TYPE_ENGINE_TIME;
> - else
> - return 0;
> -
> - if (c->samples < 2)
> - return 0;
> -
> - /* Filter out idle clients. */
> - switch (utilization_type) {
> - case UTILIZATION_TYPE_ENGINE_TIME:
> - if (!c->total_engine_time)
> - return 0;
> - break;
> - case UTILIZATION_TYPE_TOTAL_CYCLES:
> - if (!c->total_total_cycles)
> - return 0;
> - break;
> + /* Filter out idle clients. */
> + if (c->engines->num_engines && !c->total_engine_time)
> + return 0;
> }
>
> /* Print header when moving to a different DRM card. */
> @@ -234,7 +230,9 @@ print_client(struct igt_drm_client *c, struct
> igt_drm_client **prevc,
>
> lines++;
>
> - for (i = 0; c->samples > 1 && i <= c->engines->max_engine_id; i++) {
> + for (i = 0;
> + c->engines->num_engines && i <= c->engines->max_engine_id;
> + i++) {
> double pct;
>
> if (!c->engines->capacity[i])
>
With your patch I get:
DRM minor 128
PID MEM RSS NAME
1308 16M 16M kmscube
1308 0B 0B kmscube
I think we should filter out the 0B MEM's one too - or?
diff --git a/tools/gputop.c b/tools/gputop.c
index 43b01f566..459cf213f 100644
--- a/tools/gputop.c
+++ b/tools/gputop.c
@@ -137,6 +137,9 @@ engines_identical(const struct igt_drm_client *c,
{
unsigned int i;
+ if (!c->engines->num_engines && !pc->engines->num_engines)
+ return true;
+
if (c->engines->num_engines != pc->engines->num_engines ||
c->engines->max_engine_id != pc->engines->max_engine_id)
return false;
@@ -188,27 +191,28 @@ print_client(struct igt_drm_client *c, struct
igt_drm_client **prevc,
uint64_t sz;
int len;
+ if (c->samples < 2)
+ return 0;
+
if (c->utilization_mask & IGT_DRM_CLIENT_UTILIZATION_TOTAL_CYCLES &&
- c->utilization_mask & IGT_DRM_CLIENT_UTILIZATION_CYCLES)
+ c->utilization_mask & IGT_DRM_CLIENT_UTILIZATION_CYCLES) {
utilization_type = UTILIZATION_TYPE_TOTAL_CYCLES;
- else if (c->utilization_mask & IGT_DRM_CLIENT_UTILIZATION_ENGINE_TIME)
+ /* Filter out idle clients. */
+ if (c->engines->num_engines && !c->total_total_cycles)
+ return 0;
+ } else if (c->utilization_mask & IGT_DRM_CLIENT_UTILIZATION_ENGINE_TIME) {
utilization_type = UTILIZATION_TYPE_ENGINE_TIME;
- else
- return 0;
+ /* Filter out idle clients. */
+ if (c->engines->num_engines && !c->total_engine_time)
+ return 0;
+ }
- if (c->samples < 2)
- return 0;
+ if (c->regions->num_regions) {
+ for (sz = 0, i = 0; i <= c->regions->max_region_id; i++)
+ sz += c->memory[i].total;
- /* Filter out idle clients. */
- switch (utilization_type) {
- case UTILIZATION_TYPE_ENGINE_TIME:
- if (!c->total_engine_time)
- return 0;
- break;
- case UTILIZATION_TYPE_TOTAL_CYCLES:
- if (!c->total_total_cycles)
- return 0;
- break;
+ if (sz == 0)
+ return 0;
}
/* Print header when moving to a different DRM card. */
@@ -234,7 +238,9 @@ print_client(struct igt_drm_client *c, struct
igt_drm_client **prevc,
lines++;
- for (i = 0; c->samples > 1 && i <= c->engines->max_engine_id; i++) {
+ for (i = 0;
+ c->engines->num_engines && i <= c->engines->max_engine_id;
+ i++) {
double pct;
if (!c->engines->capacity[i])
--
greets
--
Christian Gmeiner, MSc
https://christian-gmeiner.info/privacypolicy
More information about the igt-dev
mailing list