* [PATCH 0/2] Rework workaround igt @ 2014-09-01 13:29 Arun Siluvery 2014-09-01 13:29 ` [PATCH 1/2] gem_workarounds: intel_wa_registers is now prefixed with i915 Arun Siluvery 2014-09-01 13:29 ` [PATCH 2/2] igt/gem_workarounds: rework igt to test workaround registers Arun Siluvery 0 siblings, 2 replies; 9+ messages in thread From: Arun Siluvery @ 2014-09-01 13:29 UTC (permalink / raw) To: intel-gfx Rework of the test as kernel patch is modified. Arun Siluvery (1): igt/gem_workarounds: rework igt to test workaround registers Damien Lespiau (1): gem_workarounds: intel_wa_registers is now prefixed with i915 tests/gem_workarounds.c | 46 ++++++++++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 22 deletions(-) -- 2.0.4 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] gem_workarounds: intel_wa_registers is now prefixed with i915 2014-09-01 13:29 [PATCH 0/2] Rework workaround igt Arun Siluvery @ 2014-09-01 13:29 ` Arun Siluvery 2014-09-01 13:29 ` [PATCH 2/2] igt/gem_workarounds: rework igt to test workaround registers Arun Siluvery 1 sibling, 0 replies; 9+ messages in thread From: Arun Siluvery @ 2014-09-01 13:29 UTC (permalink / raw) To: intel-gfx From: Damien Lespiau <damien.lespiau@intel.com> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> --- tests/gem_workarounds.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/gem_workarounds.c b/tests/gem_workarounds.c index 6826562..32156d2 100644 --- a/tests/gem_workarounds.c +++ b/tests/gem_workarounds.c @@ -184,7 +184,7 @@ igt_main devid = intel_get_drm_devid(drm_fd); batch = intel_batchbuffer_alloc(bufmgr, devid); - fd = igt_debugfs_open("intel_wa_registers", O_RDONLY); + fd = igt_debugfs_open("i915_wa_registers", O_RDONLY); igt_assert(fd >= 0); file = fdopen(fd, "r"); -- 2.0.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] igt/gem_workarounds: rework igt to test workaround registers 2014-09-01 13:29 [PATCH 0/2] Rework workaround igt Arun Siluvery 2014-09-01 13:29 ` [PATCH 1/2] gem_workarounds: intel_wa_registers is now prefixed with i915 Arun Siluvery @ 2014-09-01 13:29 ` Arun Siluvery 2014-09-01 14:12 ` Damien Lespiau 2014-09-02 9:18 ` [PATCH v2] " Arun Siluvery 1 sibling, 2 replies; 9+ messages in thread From: Arun Siluvery @ 2014-09-01 13:29 UTC (permalink / raw) To: intel-gfx kernel patch that exports w/a data to debugfs is reworked so update igt accordingly. Address review comments from Damien. - if kernel is not exposing w/a data instead of failing just skip instead. - if the platform is not exposing w/a table then no of workarounds applied are 0; we can use this data to skip platform checks. Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com> --- tests/gem_workarounds.c | 44 +++++++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/tests/gem_workarounds.c b/tests/gem_workarounds.c index 32156d2..fae4382 100644 --- a/tests/gem_workarounds.c +++ b/tests/gem_workarounds.c @@ -62,7 +62,7 @@ int drm_fd; uint32_t devid; static drm_intel_bufmgr *bufmgr; struct intel_batchbuffer *batch; -int num_wa_regs; +int num_wa_regs = 0; struct intel_wa_reg *wa_regs; @@ -153,7 +153,7 @@ static void check_workarounds(enum operation op, int num) igt_info("Address\tbefore\t\tafter\t\tw/a mask\tresult\n"); for (i = 0; i < num; ++i) { status = (current_wa[i].value & current_wa[i].mask) != - (wa_regs[i].value & wa_regs[i].mask); + wa_regs[i].mask; if (status) ++fail_count; @@ -171,21 +171,15 @@ out: igt_main { igt_fixture { - int i; int fd; int ret; FILE *file; char *line = NULL; size_t line_size; - drm_fd = drm_open_any(); - - bufmgr = drm_intel_bufmgr_gem_init(drm_fd, 4096); - devid = intel_get_drm_devid(drm_fd); - batch = intel_batchbuffer_alloc(bufmgr, devid); - fd = igt_debugfs_open("i915_wa_registers", O_RDONLY); - igt_assert(fd >= 0); + if (fd < 0) + igt_skip_on("No Workaround table available !!\n"); file = fdopen(fd, "r"); igt_assert(file > 0); @@ -193,32 +187,40 @@ igt_main ret = getline(&line, &line_size, file); igt_assert(ret > 0); sscanf(line, "Workarounds applied: %d", &num_wa_regs); - igt_assert(num_wa_regs > 0); - wa_regs = malloc(num_wa_regs * sizeof(*wa_regs)); + if (num_wa_regs) { + int i = 0; - i = 0; - while(getline(&line, &line_size, file) > 0) { - sscanf(line, "0x%X: 0x%08X, mask: 0x%08X", - &wa_regs[i].addr, &wa_regs[i].value, - &wa_regs[i].mask); - ++i; - } + wa_regs = malloc(num_wa_regs * sizeof(*wa_regs)); + while (getline(&line, &line_size, file) > 0) { + sscanf(line, "0x%X: 0x%08X, mask: 0x%08X", + &wa_regs[i].addr, &wa_regs[i].value, + &wa_regs[i].mask); + ++i; + } + } else + igt_info("No workarounds exported\n"); free(line); fclose(file); close(fd); + + drm_fd = drm_open_any(); + + bufmgr = drm_intel_bufmgr_gem_init(drm_fd, 4096); + devid = intel_get_drm_devid(drm_fd); + batch = intel_batchbuffer_alloc(bufmgr, devid); } igt_subtest("check-workaround-data-after-reset") { - if (IS_BROADWELL(devid)) + if (num_wa_regs) check_workarounds(GPU_RESET, num_wa_regs); else igt_skip_on("No Workaround table available!!\n"); } igt_subtest("check-workaround-data-after-suspend-resume") { - if (IS_BROADWELL(devid)) + if (num_wa_regs) check_workarounds(SUSPEND_RESUME, num_wa_regs); else igt_skip_on("No Workaround table available!!\n"); -- 2.0.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] igt/gem_workarounds: rework igt to test workaround registers 2014-09-01 13:29 ` [PATCH 2/2] igt/gem_workarounds: rework igt to test workaround registers Arun Siluvery @ 2014-09-01 14:12 ` Damien Lespiau 2014-09-02 9:18 ` [PATCH v2] " Arun Siluvery 1 sibling, 0 replies; 9+ messages in thread From: Damien Lespiau @ 2014-09-01 14:12 UTC (permalink / raw) To: Arun Siluvery; +Cc: intel-gfx On Mon, Sep 01, 2014 at 02:29:47PM +0100, Arun Siluvery wrote: > kernel patch that exports w/a data to debugfs is reworked so > update igt accordingly. > > Address review comments from Damien. > - if kernel is not exposing w/a data instead of failing just skip instead. > - if the platform is not exposing w/a table then no of workarounds > applied are 0; we can use this data to skip platform checks. > > Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com> > --- > tests/gem_workarounds.c | 44 +++++++++++++++++++++++--------------------- > 1 file changed, 23 insertions(+), 21 deletions(-) > > diff --git a/tests/gem_workarounds.c b/tests/gem_workarounds.c > index 32156d2..fae4382 100644 > --- a/tests/gem_workarounds.c > +++ b/tests/gem_workarounds.c > @@ -62,7 +62,7 @@ int drm_fd; > uint32_t devid; > static drm_intel_bufmgr *bufmgr; > struct intel_batchbuffer *batch; > -int num_wa_regs; > +int num_wa_regs = 0; > struct intel_wa_reg *wa_regs; > > > @@ -153,7 +153,7 @@ static void check_workarounds(enum operation op, int num) > igt_info("Address\tbefore\t\tafter\t\tw/a mask\tresult\n"); > for (i = 0; i < num; ++i) { > status = (current_wa[i].value & current_wa[i].mask) != > - (wa_regs[i].value & wa_regs[i].mask); > + wa_regs[i].mask; Hum. This looks so fishy it can't be right. The heart of the problem is that you're not clear what a mask or value should be. To me: - A mask selects bits - value is the reference W/A value (containing only the correct bits for a single W/A, some of those bits can be 0, mask is telling us which bits we're interested about. - read(reg) & mask is the W/A value read back from the register -- Damien ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] igt/gem_workarounds: rework igt to test workaround registers 2014-09-01 13:29 ` [PATCH 2/2] igt/gem_workarounds: rework igt to test workaround registers Arun Siluvery 2014-09-01 14:12 ` Damien Lespiau @ 2014-09-02 9:18 ` Arun Siluvery 2014-09-02 9:59 ` Damien Lespiau 1 sibling, 1 reply; 9+ messages in thread From: Arun Siluvery @ 2014-09-02 9:18 UTC (permalink / raw) To: intel-gfx kernel patch that exports w/a data to debugfs is reworked so update igt accordingly. Address review comments from Damien. - if kernel is not exposing w/a data instead of failing just skip instead. - if the platform is not exposing w/a table then no of workarounds applied are 0, use this data to skip platform checks. Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com> --- tests/gem_workarounds.c | 45 +++++++++++++++++++++++---------------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/tests/gem_workarounds.c b/tests/gem_workarounds.c index 32156d2..18ded9a 100644 --- a/tests/gem_workarounds.c +++ b/tests/gem_workarounds.c @@ -62,7 +62,7 @@ int drm_fd; uint32_t devid; static drm_intel_bufmgr *bufmgr; struct intel_batchbuffer *batch; -int num_wa_regs; +int num_wa_regs = 0; struct intel_wa_reg *wa_regs; @@ -152,8 +152,7 @@ static void check_workarounds(enum operation op, int num) igt_info("Address\tbefore\t\tafter\t\tw/a mask\tresult\n"); for (i = 0; i < num; ++i) { - status = (current_wa[i].value & current_wa[i].mask) != - (wa_regs[i].value & wa_regs[i].mask); + status = (current_wa[i].value & wa_regs[i].mask) != wa_regs[i].value; if (status) ++fail_count; @@ -171,21 +170,15 @@ out: igt_main { igt_fixture { - int i; int fd; int ret; FILE *file; char *line = NULL; size_t line_size; - drm_fd = drm_open_any(); - - bufmgr = drm_intel_bufmgr_gem_init(drm_fd, 4096); - devid = intel_get_drm_devid(drm_fd); - batch = intel_batchbuffer_alloc(bufmgr, devid); - fd = igt_debugfs_open("i915_wa_registers", O_RDONLY); - igt_assert(fd >= 0); + if (fd < 0) + igt_skip_on("No Workaround table available !!\n"); file = fdopen(fd, "r"); igt_assert(file > 0); @@ -193,32 +186,40 @@ igt_main ret = getline(&line, &line_size, file); igt_assert(ret > 0); sscanf(line, "Workarounds applied: %d", &num_wa_regs); - igt_assert(num_wa_regs > 0); - wa_regs = malloc(num_wa_regs * sizeof(*wa_regs)); + if (num_wa_regs) { + int i = 0; - i = 0; - while(getline(&line, &line_size, file) > 0) { - sscanf(line, "0x%X: 0x%08X, mask: 0x%08X", - &wa_regs[i].addr, &wa_regs[i].value, - &wa_regs[i].mask); - ++i; - } + wa_regs = malloc(num_wa_regs * sizeof(*wa_regs)); + while (getline(&line, &line_size, file) > 0) { + sscanf(line, "0x%X: 0x%08X, mask: 0x%08X", + &wa_regs[i].addr, &wa_regs[i].value, + &wa_regs[i].mask); + ++i; + } + } else + igt_info("No workarounds exported\n"); free(line); fclose(file); close(fd); + + drm_fd = drm_open_any(); + + bufmgr = drm_intel_bufmgr_gem_init(drm_fd, 4096); + devid = intel_get_drm_devid(drm_fd); + batch = intel_batchbuffer_alloc(bufmgr, devid); } igt_subtest("check-workaround-data-after-reset") { - if (IS_BROADWELL(devid)) + if (num_wa_regs) check_workarounds(GPU_RESET, num_wa_regs); else igt_skip_on("No Workaround table available!!\n"); } igt_subtest("check-workaround-data-after-suspend-resume") { - if (IS_BROADWELL(devid)) + if (num_wa_regs) check_workarounds(SUSPEND_RESUME, num_wa_regs); else igt_skip_on("No Workaround table available!!\n"); -- 2.0.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] igt/gem_workarounds: rework igt to test workaround registers 2014-09-02 9:18 ` [PATCH v2] " Arun Siluvery @ 2014-09-02 9:59 ` Damien Lespiau 2014-09-02 11:04 ` Daniel Vetter 0 siblings, 1 reply; 9+ messages in thread From: Damien Lespiau @ 2014-09-02 9:59 UTC (permalink / raw) To: Arun Siluvery; +Cc: intel-gfx On Tue, Sep 02, 2014 at 10:18:20AM +0100, Arun Siluvery wrote: > - igt_assert(fd >= 0); > + if (fd < 0) > + igt_skip_on("No Workaround table available !!\n"); That's not quite a correct use of the API. The _on is there to signal the first argument is an expression. This will work only because the string is evaluated to true. You probably want to use igt_skip_on_f() http://people.freedesktop.org/~danvet/igt/intel-gpu-tools-i-g-t-core.html#igt-skip-on-f > file = fdopen(fd, "r"); > igt_assert(file > 0); > @@ -193,32 +186,40 @@ igt_main > ret = getline(&line, &line_size, file); > igt_assert(ret > 0); > sscanf(line, "Workarounds applied: %d", &num_wa_regs); > - igt_assert(num_wa_regs > 0); > > - wa_regs = malloc(num_wa_regs * sizeof(*wa_regs)); > + if (num_wa_regs) { > + int i = 0; > > - i = 0; > - while(getline(&line, &line_size, file) > 0) { > - sscanf(line, "0x%X: 0x%08X, mask: 0x%08X", > - &wa_regs[i].addr, &wa_regs[i].value, > - &wa_regs[i].mask); > - ++i; > - } > + wa_regs = malloc(num_wa_regs * sizeof(*wa_regs)); > + while (getline(&line, &line_size, file) > 0) { > + sscanf(line, "0x%X: 0x%08X, mask: 0x%08X", > + &wa_regs[i].addr, &wa_regs[i].value, > + &wa_regs[i].mask); > + ++i; > + } > + } else > + igt_info("No workarounds exported\n"); It's a bit weird to just have an igt_info() here and skip in every single subtest after that. How about a: igt_skip_on_f(num_wa_regs == 0, "No workarounds exported\n"); and continue the rest of the test with the case (num_wa_regs == 0) out of the picture? -- Damien ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] igt/gem_workarounds: rework igt to test workaround registers 2014-09-02 9:59 ` Damien Lespiau @ 2014-09-02 11:04 ` Daniel Vetter 2014-09-02 11:12 ` Chris Wilson 0 siblings, 1 reply; 9+ messages in thread From: Daniel Vetter @ 2014-09-02 11:04 UTC (permalink / raw) To: Damien Lespiau; +Cc: intel-gfx On Tue, Sep 02, 2014 at 10:59:06AM +0100, Damien Lespiau wrote: > On Tue, Sep 02, 2014 at 10:18:20AM +0100, Arun Siluvery wrote: > > - igt_assert(fd >= 0); > > + if (fd < 0) > > + igt_skip_on("No Workaround table available !!\n"); > > That's not quite a correct use of the API. The _on is there to signal > the first argument is an expression. This will work only because the > string is evaluated to true. You probably want to use igt_skip_on_f() > > http://people.freedesktop.org/~danvet/igt/intel-gpu-tools-i-g-t-core.html#igt-skip-on-f Or just igt_skip_on - we dump the expression that was tested, which should be fairly informational. At least when fd would have a better name ;-) -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] igt/gem_workarounds: rework igt to test workaround registers 2014-09-02 11:04 ` Daniel Vetter @ 2014-09-02 11:12 ` Chris Wilson 2014-09-02 13:22 ` Daniel Vetter 0 siblings, 1 reply; 9+ messages in thread From: Chris Wilson @ 2014-09-02 11:12 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx On Tue, Sep 02, 2014 at 01:04:34PM +0200, Daniel Vetter wrote: > On Tue, Sep 02, 2014 at 10:59:06AM +0100, Damien Lespiau wrote: > > On Tue, Sep 02, 2014 at 10:18:20AM +0100, Arun Siluvery wrote: > > > - igt_assert(fd >= 0); > > > + if (fd < 0) > > > + igt_skip_on("No Workaround table available !!\n"); > > > > That's not quite a correct use of the API. The _on is there to signal > > the first argument is an expression. This will work only because the > > string is evaluated to true. You probably want to use igt_skip_on_f() > > > > http://people.freedesktop.org/~danvet/igt/intel-gpu-tools-i-g-t-core.html#igt-skip-on-f > > Or just igt_skip_on - we dump the expression that was tested, which should > be fairly informational. At least when fd would have a better name ;-) Use igt_require(), it produces more natural phrasing in usage and output. -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] igt/gem_workarounds: rework igt to test workaround registers 2014-09-02 11:12 ` Chris Wilson @ 2014-09-02 13:22 ` Daniel Vetter 0 siblings, 0 replies; 9+ messages in thread From: Daniel Vetter @ 2014-09-02 13:22 UTC (permalink / raw) To: Chris Wilson, Daniel Vetter, Damien Lespiau, intel-gfx On Tue, Sep 02, 2014 at 12:12:28PM +0100, Chris Wilson wrote: > On Tue, Sep 02, 2014 at 01:04:34PM +0200, Daniel Vetter wrote: > > On Tue, Sep 02, 2014 at 10:59:06AM +0100, Damien Lespiau wrote: > > > On Tue, Sep 02, 2014 at 10:18:20AM +0100, Arun Siluvery wrote: > > > > - igt_assert(fd >= 0); > > > > + if (fd < 0) > > > > + igt_skip_on("No Workaround table available !!\n"); > > > > > > That's not quite a correct use of the API. The _on is there to signal > > > the first argument is an expression. This will work only because the > > > string is evaluated to true. You probably want to use igt_skip_on_f() > > > > > > http://people.freedesktop.org/~danvet/igt/intel-gpu-tools-i-g-t-core.html#igt-skip-on-f > > > > Or just igt_skip_on - we dump the expression that was tested, which should > > be fairly informational. At least when fd would have a better name ;-) > > Use igt_require(), it produces more natural phrasing in usage and > output. Yeah, for new code I second this - we have too many macros ;-) But my practice has been to use igt_skip_on when changing existing code since I seem to be too incompetent to reliably invert boolean conditions ... Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-09-02 13:22 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-09-01 13:29 [PATCH 0/2] Rework workaround igt Arun Siluvery 2014-09-01 13:29 ` [PATCH 1/2] gem_workarounds: intel_wa_registers is now prefixed with i915 Arun Siluvery 2014-09-01 13:29 ` [PATCH 2/2] igt/gem_workarounds: rework igt to test workaround registers Arun Siluvery 2014-09-01 14:12 ` Damien Lespiau 2014-09-02 9:18 ` [PATCH v2] " Arun Siluvery 2014-09-02 9:59 ` Damien Lespiau 2014-09-02 11:04 ` Daniel Vetter 2014-09-02 11:12 ` Chris Wilson 2014-09-02 13:22 ` Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox