[igt-dev] [PATCH i-g-t] i915/gem_workarounds: Adapt to change in file format for per-engine wa

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-07-04 10:51:44)
> 
> On 03/07/2019 17:59, Chris Wilson wrote:
> > To reduce the assumptions of RCS0 in the kernel, we want to make the
> > debugfs engine agnostic and so we need to adapt the igt parser for
> > flexibility.
> > 
> > If we could just adequately simulate S3/S4 in the kernel we could forgo
> > this test entirely...
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > ---
> >   tests/i915/gem_workarounds.c | 12 ++++++++++--
> >   1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tests/i915/gem_workarounds.c b/tests/i915/gem_workarounds.c
> > index 403863c0b..81c356f06 100644
> > --- a/tests/i915/gem_workarounds.c
> > +++ b/tests/i915/gem_workarounds.c
> > @@ -249,6 +249,7 @@ igt_main
> >       igt_fixture {
> >               FILE *file;
> >               char *line = NULL;
> > +             char *str;
> >               size_t line_size;
> >               int i, fd;
> >   
> > @@ -261,9 +262,13 @@ igt_main
> >   
> >               fd = igt_debugfs_open(device, "i915_wa_registers", O_RDONLY);
> >               file = fdopen(fd, "r");
> > -             igt_assert(getline(&line, &line_size, file) > 0);
> > +             igt_require(getline(&line, &line_size, file) > 0);
> >               igt_debug("i915_wa_registers: %s", line);
> > -             sscanf(line, "Workarounds applied: %d", &num_wa_regs);
> > +
> > +             /* We assume that the first batch is for rcs */
> 
> I think you mean "first line".

I was thinking of batch as in set of workarounds.

> > +             str = strstr(line, "Workarounds applied:");
> > +             igt_assert(str);
> > +             sscanf(str, "Workarounds applied: %d", &num_wa_regs);
> >               igt_require(num_wa_regs > 0);
> 
> I have a suspicion all the above section could be simplified using 
> fscanf and just checking return value and errno but that would be asking 
> for too much work for what this is.

Speaking my language :)

> >   
> >               wa_regs = malloc(num_wa_regs * sizeof(*wa_regs));
> > @@ -271,6 +276,9 @@ igt_main
> >   
> >               i = 0;
> >               while (getline(&line, &line_size, file) > 0) {
> > +                     if (strstr(line, "Workarounds applied:"))
> > +                             break;
> > +
> >                       igt_debug("%s", line);
> >                       if (sscanf(line, "0x%X: 0x%08X, mask: 0x%08X",
> >                                  &wa_regs[i].addr,
> > 
> 
> I just had a thought that since we are fiddling with this we should add 
> engine names into strings. And/or class:instance pairs.

Expand upon that thought: where do you want this? I presume you are
thinking of an igt helper for rcs0 -> class:RENDER, inst:0
-Chris