* [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;
as well as URLs for NNTP newsgroup(s).