Re: [igt-dev] [PATCH i-g-t 6/8] benchmarks/gem_wsim: allow comments in workload description files

["Bernatowicz, Marcin" <marcin.bernatowicz@linux.intel.com>]

Hi,

On 9/20/2023 6:13 PM, Tvrtko Ursulin wrote:
> 
> On 06/09/2023 16:51, Marcin Bernatowicz wrote:
>> Lines starting with '#' are skipped.
>> If command line step separator (',') is encountered after '#'
>> it is replaced with ';' to not break parsing.
>>
>> Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
>> ---
>>   benchmarks/gem_wsim.c  | 41 ++++++++++++++++++++++++++++++++---------
>>   benchmarks/wsim/README |  2 ++
>>   2 files changed, 34 insertions(+), 9 deletions(-)
>>
>> diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
>> index 0c1b58727..ec9fdc2d0 100644
>> --- a/benchmarks/gem_wsim.c
>> +++ b/benchmarks/gem_wsim.c
>> @@ -43,6 +43,7 @@
>>   #include <limits.h>
>>   #include <pthread.h>
>>   #include <math.h>
>> +#include <ctype.h>
>>   #include "drm.h"
>>   #include "drmtest.h"
>> @@ -94,6 +95,7 @@ enum w_type {
>>       TERMINATE,
>>       SSEU,
>>       WORKINGSET,
>> +    SKIP,
>>   };
>>   struct dep_entry {
>> @@ -930,6 +932,12 @@ parse_workload(struct w_arg *arg, unsigned int 
>> flags, double scale_dur,
>>           if (field) {
>>               fstart = NULL;
>> +            /* line starting with # is a comment */
>> +            if (field[0] == '#') {
>> +                step.type = SKIP;
>> +                goto add_step;
>> +            }
> 
> Do they need to be recorded as steps and couldn't be simply silently 
> skipped over while parsing?

Looks indeed a bool skip may be enough. It's a dummy step not stored in 
workload steps.

> 
> How does relative step referencing works when comments are present? 
> (Batch implicit dependencies and 'a' and 's' commands.)

Comments may be added to existing workloads without a problem as the 
SKIPs are not incrementing nr_step (does not create a record).
Maybe a small confusion is that step.idx does not correspond to line 
number, so in case of failed parse we get a step number without a line :/

Regards,
Marcin
> 
> Regards,
> 
> Tvrtko
> 
>> +
>>               if (!strcmp(field, "d")) {
>>                   int_field(DELAY, delay, tmp <= 0,
>>                         "Invalid delay at step %u!\n");
>> @@ -1194,7 +1202,8 @@ parse_workload(struct w_arg *arg, unsigned int 
>> flags, double scale_dur,
>>           if (field) {
>>               fstart = NULL;
>> -            check_arg(strlen(field) != 1 ||
>> +            check_arg(!strlen(field) ||
>> +                  (strlen(field) > 1 && !isspace(field[1]) && 
>> field[1] != '#') ||
>>                     (field[0] != '0' && field[0] != '1'),
>>                     "Invalid wait boolean at step %u!\n",
>>                     nr_steps);
>> @@ -1208,18 +1217,23 @@ parse_workload(struct w_arg *arg, unsigned int 
>> flags, double scale_dur,
>>           step.type = BATCH;
>>   add_step:
>> -        if (step.type == DELAY)
>> -            step.delay = __duration(step.delay, scale_time);
>> +        if (step.type == SKIP) {
>> +            if (verbose > 3)
>> +                printf("skipped STEP: %s\n", _token);
>> +        } else {
>> +            if (step.type == DELAY)
>> +                step.delay = __duration(step.delay, scale_time);
>> -        step.idx = nr_steps++;
>> -        step.request = -1;
>> -        steps = realloc(steps, sizeof(step) * nr_steps);
>> -        igt_assert(steps);
>> +            step.idx = nr_steps++;
>> +            step.request = -1;
>> +            steps = realloc(steps, sizeof(step) * nr_steps);
>> +            igt_assert(steps);
>> -        memcpy(&steps[nr_steps - 1], &step, sizeof(step));
>> +            memcpy(&steps[nr_steps - 1], &step, sizeof(step));
>> +        }
>>           free(token);
>> -    }
>> +    } // while ((_token = strtok_r(tstart, ",", &tctx))) {
>>       if (app_w) {
>>           steps = realloc(steps, sizeof(step) *
>> @@ -2304,6 +2318,8 @@ static void *run_workload(void *data)
>>               enum intel_engine_id engine = w->engine;
>>               int do_sleep = 0;
>> +            igt_assert(w->type != SKIP);
>> +
>>               if (w->type == DELAY) {
>>                   do_sleep = w->delay;
>>               } else if (w->type == PERIOD) {
>> @@ -2543,6 +2559,13 @@ static char *load_workload_descriptor(char 
>> *filename)
>>       close(infd);
>>       for (i = 0; i < len; i++) {
>> +        /* '#' starts comment till end of line */
>> +        if (buf[i] == '#')
>> +            /* replace ',' in comments to not break parsing */
>> +            while (++i < len && buf[i] != '\n')
>> +                if (buf[i] == ',')
>> +                    buf[i] = ';';
>> +
>>           if (buf[i] == '\n')
>>               buf[i] = ',';
>>       }
>> diff --git a/benchmarks/wsim/README b/benchmarks/wsim/README
>> index 8c71f2fe6..e4fd61645 100644
>> --- a/benchmarks/wsim/README
>> +++ b/benchmarks/wsim/README
>> @@ -1,6 +1,8 @@
>>   Workload descriptor format
>>   ==========================
>> +Lines starting with '#' are treated as comments (do not create work 
>> step).
>> +
>>   ctx.engine.duration_us.dependency.wait,...
>>   <uint>.<str>.<uint>[-<uint>]|*.<int <= 0>[/<int <= 0>][...].<0|1>,...
>>   B.<uint>