[PATCH i-g-t] gputop: Add memory only utilization type
Tvrtko Ursulin
tvrtko.ursulin at igalia.com
Fri Nov 22 08:54:57 UTC 2024
On 21/11/2024 18:35, Christian Gmeiner wrote:
> 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])
Regards,
Tvrtko
>> The loop in print_client() should ideally be skipped but even as is I
>> think exits on the capacity check, no? At the moment it seems like this
>> could be enough but maybe I am missing something.
>>
>
> You are correct - it works without the goto thing.
>
>> Regards,
>>
>> Tvrtko
>>
>>> +
>>> for (i = 0; i <= c->engines->max_engine_id; i++)
>>> if (c->engines->capacity[i] != pc->engines->capacity[i] ||
>>> !!c->engines->names[i] != !!pc->engines->names[i] ||
>>> @@ -193,6 +197,8 @@ print_client(struct igt_drm_client *c, struct igt_drm_client **prevc,
>>> utilization_type = UTILIZATION_TYPE_TOTAL_CYCLES;
>>> else if (c->utilization_mask & IGT_DRM_CLIENT_UTILIZATION_ENGINE_TIME)
>>> utilization_type = UTILIZATION_TYPE_ENGINE_TIME;
>>> + else if (c->regions->num_regions)
>>> + utilization_type = UTILIZATION_TYPE_MEMORY_ONLY;
>>> else
>>> return 0;
>>>
>>> @@ -209,6 +215,13 @@ print_client(struct igt_drm_client *c, struct igt_drm_client **prevc,
>>> if (!c->total_total_cycles)
>>> return 0;
>>> break;
>>> + case UTILIZATION_TYPE_MEMORY_ONLY:
>>> + for (sz = 0, i = 0; i <= c->regions->max_region_id; i++)
>>> + sz += c->memory[i].total;
>>> +
>>> + if (sz == 0)
>>> + return 0;
>>> + break;
>>> }
>>>
>>> /* Print header when moving to a different DRM card. */
>>> @@ -234,6 +247,9 @@ print_client(struct igt_drm_client *c, struct igt_drm_client **prevc,
>>>
>>> lines++;
>>>
>>> + if (utilization_type == UTILIZATION_TYPE_MEMORY_ONLY)
>>> + goto print_name;
>>> +
>>> for (i = 0; c->samples > 1 && i <= c->engines->max_engine_id; i++) {
>>> double pct;
>>>
>>> @@ -249,6 +265,9 @@ print_client(struct igt_drm_client *c, struct igt_drm_client **prevc,
>>> pct = (double)c->utilization[i].delta_cycles / c->utilization[i].delta_total_cycles * 100 /
>>> c->engines->capacity[i];
>>> break;
>>> + case UTILIZATION_TYPE_MEMORY_ONLY:
>>> + /* Nothing to do. */
>>> + break;
>>> }
>>>
>>> /*
>>> @@ -262,6 +281,7 @@ print_client(struct igt_drm_client *c, struct igt_drm_client **prevc,
>>> len += *engine_w;
>>> }
>>>
>>> +print_name:
>>> printf(" %-*s\n", con_w - len - 1, c->print_name);
>>>
>>> return lines;
>
>
>
More information about the igt-dev
mailing list