Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 2/3] intel_gpu_top: Aggregate clients by PID by default

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 10/02/2021 10:35, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2021-02-10 09:37:55)
>> +static struct clients *aggregated_clients(struct clients *clients)
>> +{
>> +       struct client *ac, *c, *cp = NULL;
>> +       struct clients *aggregated;
>> +       int tmp, num = 0;
>> +
>> +       /* Sort by pid first to make it easy to aggregate while walking. */
>> +       sort_clients(clients, client_pid_cmp);
> 
> You could eliminate this tiny bit of duplication by always calling
> aggregated_clients() and returning here for !aggregate_pids.

Okay, I did something like that.

>> +       aggregated = calloc(1, sizeof(*clients));
>> +       assert(aggregated);
>> +
>> +       ac = calloc(clients->num_clients, sizeof(*c));
>> +       assert(ac);
>> +
>> +       aggregated->num_classes = clients->num_classes;
>> +       aggregated->class = clients->class;
>> +       aggregated->client = ac;
>> +
>> +       for_each_client(clients, c, tmp) {
>> +               unsigned int i;
>> +
>> +               if (c->status == FREE)
>> +                       break;
>> +
>> +               assert(c->status == ALIVE);
>> +
>> +               if ((cp && c->pid != cp->pid) || !cp) {
>> +                       ac = &aggregated->client[num];
>> +
>> +                       /* New pid. */
>> +                       ac->clients = aggregated;
>> +                       ac->status = ALIVE;
>> +                       ac->id = ++num;
>> +                       ac->pid = c->pid;
>> +                       strcpy(ac->name, c->name);
>> +                       strcpy(ac->print_name, c->print_name);
>> +                       ac->engines = c->engines;
>> +                       ac->val = calloc(clients->num_classes,
>> +                                        sizeof(ac->val[0]));
>> +                       assert(ac->val);
>> +                       ac->samples = 1;
>> +               }
>> +
>> +               cp = c;
>> +
>> +               if (c->samples < 2)
>> +                       continue;
>> +
>> +               ac->samples = 2; /* All what matters for display. */
>> +               ac->total_runtime += c->total_runtime;
>> +               ac->last_runtime += c->last_runtime;
>> +
>> +               for (i = 0; i < clients->num_classes; i++)
>> +                       ac->val[i] += c->val[i];
>> +       }
>> +
>> +       aggregated->num_clients = num;
>> +       aggregated->active_clients = num;
>> +
>> +       return sort_clients(aggregated, client_cmp);
>>   }
> 
> Ok, that works very well. Hmm. The sort order does seem a little jumpy
> though. May I suggest ac->id = -c->pid; instead of num;

Done it although I thought 1st pass being sort by pid already, num as id 
would follow a stable order. I guess your point was inversion to 
preserve order when cycling sort modes?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx