[PATCH i-g-t] gputop: Add memory only utilization type
Tvrtko Ursulin
tvrtko.ursulin at igalia.com
Mon Nov 25 09:03:19 UTC 2024
On 22/11/2024 20:19, Christian Gmeiner wrote:
> 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?
Yeah it makes sense to be consistent.
> 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;
Looks like this should work just fine. I could suggest how the loop
could be consolidated with the one a bit lower in the function and maybe
even "idle client" check consolidated to a helper but it is probably not
worth the effort.
Regards,
Tvrtko
>
> - /* 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])
>
>
More information about the igt-dev
mailing list