| Message ID | 20190703165956.16232-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [i-g-t] i915/gem_workarounds: Adapt to change in file format for per-engine wa | expand |
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@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@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". > + 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. > > 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. Regards, Tvrtko
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@chris-wilson.co.uk> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@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
On 04/07/2019 10:58, Chris Wilson wrote: > 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@chris-wilson.co.uk> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@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. Ok :) >>> + 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 I now see you already did it in the i915 patch: + seq_printf(m, "%s: Workarounds applied: %u\n", + engine->name, count); So nothing really. IGT can be extended later if needed. Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
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 */ + str = strstr(line, "Workarounds applied:"); + igt_assert(str); + sscanf(str, "Workarounds applied: %d", &num_wa_regs); igt_require(num_wa_regs > 0); 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,
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@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- tests/i915/gem_workarounds.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)