* [Intel-gfx] [PATCH i-g-t] i915/gem_exec_parse_blt: Fix COND_BBEND used by bb-start-(cmd|far)
@ 2019-12-05 1:22 ` Chris Wilson
0 siblings, 0 replies; 9+ messages in thread
From: Chris Wilson @ 2019-12-05 1:22 UTC (permalink / raw)
To: intel-gfx; +Cc: igt-dev
Correct the COND_BBEND instruction to perform the compare and apply the
relocation so that it looks at the correct address. In the process,
prepare for pipelined failures.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
tests/i915/gem_exec_parse_blt.c | 116 +++++++++++++++++---------------
1 file changed, 61 insertions(+), 55 deletions(-)
diff --git a/tests/i915/gem_exec_parse_blt.c b/tests/i915/gem_exec_parse_blt.c
index 58d1b2b32..854c59863 100644
--- a/tests/i915/gem_exec_parse_blt.c
+++ b/tests/i915/gem_exec_parse_blt.c
@@ -30,6 +30,7 @@
#include "igt.h"
#include "i915/gem_submission.h"
+#include "sw_sync.h"
/* To help craft commands known to be invalid across all engines */
#define INSTR_CLIENT_SHIFT 29
@@ -53,6 +54,30 @@
#define HANDLE_SIZE 4096
+static int
+__checked_execbuf(int i915, struct drm_i915_gem_execbuffer2 *eb)
+{
+ int fence;
+ int err;
+
+ igt_assert(!(eb->flags & I915_EXEC_FENCE_OUT));
+ eb->flags |= I915_EXEC_FENCE_OUT;
+ err = __gem_execbuf_wr(i915, eb);
+ eb->flags &= ~I915_EXEC_FENCE_OUT;
+ if (err)
+ return err;
+
+ fence = eb->rsvd2 >> 32;
+
+ sync_fence_wait(fence, -1);
+ err = sync_fence_status(fence);
+ close(fence);
+ if (err < 0)
+ return err;
+
+ return 0;
+}
+
static int
__exec_batch_patched(int i915, int engine,
uint32_t cmd_bo, const uint32_t *cmds, int size,
@@ -85,7 +110,7 @@ __exec_batch_patched(int i915, int engine,
execbuf.batch_len = size;
execbuf.flags = engine;
- return __gem_execbuf(i915, &execbuf);
+ return __checked_execbuf(i915, &execbuf);
}
static void exec_batch_patched(int i915, int engine,
@@ -129,7 +154,7 @@ static int __exec_batch(int i915, int engine, uint32_t cmd_bo,
execbuf.batch_len = size;
execbuf.flags = engine;
- return __gem_execbuf(i915, &execbuf);
+ return __checked_execbuf(i915, &execbuf);
}
#if 0
@@ -188,7 +213,7 @@ static void exec_split_batch(int i915, int engine, const uint32_t *cmds,
0x8);
execbuf.flags = engine;
- igt_assert_eq(__gem_execbuf(i915, &execbuf), expected_ret);
+ igt_assert_eq(__checked_execbuf(i915, &execbuf), expected_ret);
gem_close(i915, cmd_bo);
}
@@ -251,7 +276,7 @@ static void exec_batch_chained(int i915, int engine,
execbuf.batch_len = sizeof(first_level_cmds);
execbuf.flags = engine;
- ret = __gem_execbuf(i915, &execbuf);
+ ret = __checked_execbuf(i915, &execbuf);
if (expected_return && ret == expected_return)
goto out;
@@ -402,7 +427,7 @@ static void test_bb_secure(const int i915, const uint32_t handle)
execbuf.batch_len = sizeof(batch_secure);
execbuf.flags = I915_EXEC_BLT;
- igt_assert_eq(__gem_execbuf(i915, &execbuf), -EACCES);
+ igt_assert_eq(__checked_execbuf(i915, &execbuf), -EACCES);
}
#define BB_START_PARAM 0
@@ -414,12 +439,10 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
{
struct drm_i915_gem_execbuffer2 execbuf;
struct drm_i915_gem_exec_object2 obj[2];
- struct drm_i915_gem_relocation_entry reloc[3];
+ struct drm_i915_gem_relocation_entry reloc[4];
const uint32_t target_bo = gem_create(i915, 4096);
- uint32_t *dst;
- int ret;
unsigned int jump_off, footer_pos;
- const uint32_t batch_header[] = {
+ uint32_t batch[1024] = {
MI_NOOP,
MI_NOOP,
MI_NOOP,
@@ -432,10 +455,11 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
4,
0,
2,
- MI_COND_BATCH_BUFFER_END | 1,
+ MI_COND_BATCH_BUFFER_END | MI_DO_COMPARE | 2,
0,
0,
- 0
+ 0,
+ MI_ARB_CHECK,
};
const uint32_t batch_footer[] = {
MI_BATCH_BUFFER_START | MI_BATCH_NON_SECURE_I965 | 1,
@@ -443,13 +467,10 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
0,
MI_BATCH_BUFFER_END,
};
- uint32_t batch[1024];
+ uint32_t *dst;
igt_require(gem_can_store_dword(i915, I915_EXEC_BLT));
- memset(batch, 0, sizeof(batch));
- memcpy(batch, batch_header, sizeof(batch_header));
-
switch (test) {
case BB_START_PARAM:
jump_off = 5 * sizeof(uint32_t);
@@ -460,12 +481,13 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
break;
default:
jump_off = 0xf00d0000;
+ break;
}
if (test == BB_START_FAR)
- footer_pos = (sizeof(batch) - sizeof(batch_footer));
+ footer_pos = sizeof(batch) - sizeof(batch_footer);
else
- footer_pos = sizeof(batch_header);
+ footer_pos = 17 * sizeof(uint32_t);
memcpy(batch + footer_pos / sizeof(uint32_t),
batch_footer, sizeof(batch_footer));
@@ -481,24 +503,28 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
reloc[0].delta = 0;
reloc[0].read_domains = I915_GEM_DOMAIN_COMMAND;
reloc[0].write_domain = I915_GEM_DOMAIN_COMMAND;
- reloc[0].presumed_offset = -1;
reloc[1].offset = 9 * sizeof(uint32_t);
reloc[1].target_handle = obj[0].handle;
reloc[1].delta = 1 * sizeof(uint32_t);
reloc[1].read_domains = I915_GEM_DOMAIN_COMMAND;
reloc[1].write_domain = I915_GEM_DOMAIN_COMMAND;
- reloc[1].presumed_offset = -1;
- reloc[2].offset = footer_pos + 1 * sizeof(uint32_t);
- reloc[2].target_handle = obj[1].handle;
- reloc[2].delta = jump_off;
+ reloc[2].offset = 14 * sizeof(uint32_t);
+ reloc[2].target_handle = obj[0].handle;
+ reloc[2].delta = 0;
reloc[2].read_domains = I915_GEM_DOMAIN_COMMAND;
reloc[2].write_domain = 0;
- reloc[2].presumed_offset = -1;
+
+ reloc[3].offset = footer_pos + 1 * sizeof(uint32_t);
+ reloc[3].target_handle = obj[1].handle;
+ reloc[3].delta = jump_off;
+ reloc[3].read_domains = I915_GEM_DOMAIN_COMMAND;
+ reloc[3].write_domain = 0;
+ reloc[3].presumed_offset = -1;
obj[1].relocs_ptr = to_user_pointer(reloc);
- obj[1].relocation_count = 3;
+ obj[1].relocation_count = ARRAY_SIZE(reloc);
memset(&execbuf, 0, sizeof(execbuf));
execbuf.buffers_ptr = to_user_pointer(obj);
@@ -506,21 +532,20 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
execbuf.batch_len = sizeof(batch);
execbuf.flags = I915_EXEC_BLT;
- dst = gem_mmap__cpu(i915, obj[0].handle, 0, 4096,
- PROT_READ | PROT_WRITE);
+ dst = gem_mmap__wc(i915, obj[0].handle, 0, 4096, PROT_WRITE);
igt_assert_eq(dst[0], 0);
igt_assert_eq(dst[1], 0);
- ret = __gem_execbuf(i915, &execbuf);
-
switch (test) {
case BB_START_PARAM:
- igt_assert_eq(ret, -EINVAL);
+ case BB_START_OUT:
+ igt_assert_eq(__checked_execbuf(i915, &execbuf), -EINVAL);
break;
+
case BB_START_CMD:
case BB_START_FAR:
- igt_assert_eq(ret, 0);
+ gem_execbuf(i915, &execbuf);
while (READ_ONCE(dst[0]) == 0)
;
@@ -531,19 +556,8 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
igt_assert_eq(dst[0], 1);
igt_assert_eq(dst[1], 2);
- igt_info("values now %x %x\n", dst[0], dst[1]);
-
dst[0] = 0;
-
- igt_info("values now %x %x\n", dst[0], dst[1]);
-
- igt_assert_eq(dst[0], 0);
- igt_assert_eq(dst[1], 2);
-
- break;
-
- case BB_START_OUT:
- igt_assert_eq(ret, -EINVAL);
+ __sync_synchronize();
break;
}
@@ -896,7 +910,7 @@ static void test_unaligned_jump(const int i915, const uint32_t handle)
memcpy(&batch[1], &offset, sizeof(offset));
gem_write(i915, handle, 4000, batch, sizeof(batch));
- igt_assert_f(__gem_execbuf(i915, &execbuf) == -EINVAL,
+ igt_assert_f(__checked_execbuf(i915, &execbuf) == -EINVAL,
"unaligned jump accepted to %d; batch=%08x\n",
reloc.delta, batch[reloc.delta / 4]);
}
@@ -1032,19 +1046,11 @@ igt_main
igt_subtest("unaligned-jump")
test_unaligned_jump(i915, handle);
- igt_subtest_group {
- igt_hang_t hang;
-
- igt_fixture igt_allow_hang(i915, 0, 0);
+ igt_subtest("bb-start-cmd")
+ test_bb_start(i915, handle, BB_START_CMD);
- igt_subtest("bb-start-cmd")
- test_bb_start(i915, handle, BB_START_CMD);
-
- igt_subtest("bb-start-far")
- test_bb_start(i915, handle, BB_START_FAR);
-
- igt_fixture igt_disallow_hang(i915, hang);
- }
+ igt_subtest("bb-start-far")
+ test_bb_start(i915, handle, BB_START_FAR);
igt_fixture {
igt_stop_hang_detector();
--
2.24.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [igt-dev] [PATCH i-g-t] i915/gem_exec_parse_blt: Fix COND_BBEND used by bb-start-(cmd|far)
@ 2019-12-05 1:22 ` Chris Wilson
0 siblings, 0 replies; 9+ messages in thread
From: Chris Wilson @ 2019-12-05 1:22 UTC (permalink / raw)
To: intel-gfx; +Cc: igt-dev
Correct the COND_BBEND instruction to perform the compare and apply the
relocation so that it looks at the correct address. In the process,
prepare for pipelined failures.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
tests/i915/gem_exec_parse_blt.c | 116 +++++++++++++++++---------------
1 file changed, 61 insertions(+), 55 deletions(-)
diff --git a/tests/i915/gem_exec_parse_blt.c b/tests/i915/gem_exec_parse_blt.c
index 58d1b2b32..854c59863 100644
--- a/tests/i915/gem_exec_parse_blt.c
+++ b/tests/i915/gem_exec_parse_blt.c
@@ -30,6 +30,7 @@
#include "igt.h"
#include "i915/gem_submission.h"
+#include "sw_sync.h"
/* To help craft commands known to be invalid across all engines */
#define INSTR_CLIENT_SHIFT 29
@@ -53,6 +54,30 @@
#define HANDLE_SIZE 4096
+static int
+__checked_execbuf(int i915, struct drm_i915_gem_execbuffer2 *eb)
+{
+ int fence;
+ int err;
+
+ igt_assert(!(eb->flags & I915_EXEC_FENCE_OUT));
+ eb->flags |= I915_EXEC_FENCE_OUT;
+ err = __gem_execbuf_wr(i915, eb);
+ eb->flags &= ~I915_EXEC_FENCE_OUT;
+ if (err)
+ return err;
+
+ fence = eb->rsvd2 >> 32;
+
+ sync_fence_wait(fence, -1);
+ err = sync_fence_status(fence);
+ close(fence);
+ if (err < 0)
+ return err;
+
+ return 0;
+}
+
static int
__exec_batch_patched(int i915, int engine,
uint32_t cmd_bo, const uint32_t *cmds, int size,
@@ -85,7 +110,7 @@ __exec_batch_patched(int i915, int engine,
execbuf.batch_len = size;
execbuf.flags = engine;
- return __gem_execbuf(i915, &execbuf);
+ return __checked_execbuf(i915, &execbuf);
}
static void exec_batch_patched(int i915, int engine,
@@ -129,7 +154,7 @@ static int __exec_batch(int i915, int engine, uint32_t cmd_bo,
execbuf.batch_len = size;
execbuf.flags = engine;
- return __gem_execbuf(i915, &execbuf);
+ return __checked_execbuf(i915, &execbuf);
}
#if 0
@@ -188,7 +213,7 @@ static void exec_split_batch(int i915, int engine, const uint32_t *cmds,
0x8);
execbuf.flags = engine;
- igt_assert_eq(__gem_execbuf(i915, &execbuf), expected_ret);
+ igt_assert_eq(__checked_execbuf(i915, &execbuf), expected_ret);
gem_close(i915, cmd_bo);
}
@@ -251,7 +276,7 @@ static void exec_batch_chained(int i915, int engine,
execbuf.batch_len = sizeof(first_level_cmds);
execbuf.flags = engine;
- ret = __gem_execbuf(i915, &execbuf);
+ ret = __checked_execbuf(i915, &execbuf);
if (expected_return && ret == expected_return)
goto out;
@@ -402,7 +427,7 @@ static void test_bb_secure(const int i915, const uint32_t handle)
execbuf.batch_len = sizeof(batch_secure);
execbuf.flags = I915_EXEC_BLT;
- igt_assert_eq(__gem_execbuf(i915, &execbuf), -EACCES);
+ igt_assert_eq(__checked_execbuf(i915, &execbuf), -EACCES);
}
#define BB_START_PARAM 0
@@ -414,12 +439,10 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
{
struct drm_i915_gem_execbuffer2 execbuf;
struct drm_i915_gem_exec_object2 obj[2];
- struct drm_i915_gem_relocation_entry reloc[3];
+ struct drm_i915_gem_relocation_entry reloc[4];
const uint32_t target_bo = gem_create(i915, 4096);
- uint32_t *dst;
- int ret;
unsigned int jump_off, footer_pos;
- const uint32_t batch_header[] = {
+ uint32_t batch[1024] = {
MI_NOOP,
MI_NOOP,
MI_NOOP,
@@ -432,10 +455,11 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
4,
0,
2,
- MI_COND_BATCH_BUFFER_END | 1,
+ MI_COND_BATCH_BUFFER_END | MI_DO_COMPARE | 2,
0,
0,
- 0
+ 0,
+ MI_ARB_CHECK,
};
const uint32_t batch_footer[] = {
MI_BATCH_BUFFER_START | MI_BATCH_NON_SECURE_I965 | 1,
@@ -443,13 +467,10 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
0,
MI_BATCH_BUFFER_END,
};
- uint32_t batch[1024];
+ uint32_t *dst;
igt_require(gem_can_store_dword(i915, I915_EXEC_BLT));
- memset(batch, 0, sizeof(batch));
- memcpy(batch, batch_header, sizeof(batch_header));
-
switch (test) {
case BB_START_PARAM:
jump_off = 5 * sizeof(uint32_t);
@@ -460,12 +481,13 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
break;
default:
jump_off = 0xf00d0000;
+ break;
}
if (test == BB_START_FAR)
- footer_pos = (sizeof(batch) - sizeof(batch_footer));
+ footer_pos = sizeof(batch) - sizeof(batch_footer);
else
- footer_pos = sizeof(batch_header);
+ footer_pos = 17 * sizeof(uint32_t);
memcpy(batch + footer_pos / sizeof(uint32_t),
batch_footer, sizeof(batch_footer));
@@ -481,24 +503,28 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
reloc[0].delta = 0;
reloc[0].read_domains = I915_GEM_DOMAIN_COMMAND;
reloc[0].write_domain = I915_GEM_DOMAIN_COMMAND;
- reloc[0].presumed_offset = -1;
reloc[1].offset = 9 * sizeof(uint32_t);
reloc[1].target_handle = obj[0].handle;
reloc[1].delta = 1 * sizeof(uint32_t);
reloc[1].read_domains = I915_GEM_DOMAIN_COMMAND;
reloc[1].write_domain = I915_GEM_DOMAIN_COMMAND;
- reloc[1].presumed_offset = -1;
- reloc[2].offset = footer_pos + 1 * sizeof(uint32_t);
- reloc[2].target_handle = obj[1].handle;
- reloc[2].delta = jump_off;
+ reloc[2].offset = 14 * sizeof(uint32_t);
+ reloc[2].target_handle = obj[0].handle;
+ reloc[2].delta = 0;
reloc[2].read_domains = I915_GEM_DOMAIN_COMMAND;
reloc[2].write_domain = 0;
- reloc[2].presumed_offset = -1;
+
+ reloc[3].offset = footer_pos + 1 * sizeof(uint32_t);
+ reloc[3].target_handle = obj[1].handle;
+ reloc[3].delta = jump_off;
+ reloc[3].read_domains = I915_GEM_DOMAIN_COMMAND;
+ reloc[3].write_domain = 0;
+ reloc[3].presumed_offset = -1;
obj[1].relocs_ptr = to_user_pointer(reloc);
- obj[1].relocation_count = 3;
+ obj[1].relocation_count = ARRAY_SIZE(reloc);
memset(&execbuf, 0, sizeof(execbuf));
execbuf.buffers_ptr = to_user_pointer(obj);
@@ -506,21 +532,20 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
execbuf.batch_len = sizeof(batch);
execbuf.flags = I915_EXEC_BLT;
- dst = gem_mmap__cpu(i915, obj[0].handle, 0, 4096,
- PROT_READ | PROT_WRITE);
+ dst = gem_mmap__wc(i915, obj[0].handle, 0, 4096, PROT_WRITE);
igt_assert_eq(dst[0], 0);
igt_assert_eq(dst[1], 0);
- ret = __gem_execbuf(i915, &execbuf);
-
switch (test) {
case BB_START_PARAM:
- igt_assert_eq(ret, -EINVAL);
+ case BB_START_OUT:
+ igt_assert_eq(__checked_execbuf(i915, &execbuf), -EINVAL);
break;
+
case BB_START_CMD:
case BB_START_FAR:
- igt_assert_eq(ret, 0);
+ gem_execbuf(i915, &execbuf);
while (READ_ONCE(dst[0]) == 0)
;
@@ -531,19 +556,8 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
igt_assert_eq(dst[0], 1);
igt_assert_eq(dst[1], 2);
- igt_info("values now %x %x\n", dst[0], dst[1]);
-
dst[0] = 0;
-
- igt_info("values now %x %x\n", dst[0], dst[1]);
-
- igt_assert_eq(dst[0], 0);
- igt_assert_eq(dst[1], 2);
-
- break;
-
- case BB_START_OUT:
- igt_assert_eq(ret, -EINVAL);
+ __sync_synchronize();
break;
}
@@ -896,7 +910,7 @@ static void test_unaligned_jump(const int i915, const uint32_t handle)
memcpy(&batch[1], &offset, sizeof(offset));
gem_write(i915, handle, 4000, batch, sizeof(batch));
- igt_assert_f(__gem_execbuf(i915, &execbuf) == -EINVAL,
+ igt_assert_f(__checked_execbuf(i915, &execbuf) == -EINVAL,
"unaligned jump accepted to %d; batch=%08x\n",
reloc.delta, batch[reloc.delta / 4]);
}
@@ -1032,19 +1046,11 @@ igt_main
igt_subtest("unaligned-jump")
test_unaligned_jump(i915, handle);
- igt_subtest_group {
- igt_hang_t hang;
-
- igt_fixture igt_allow_hang(i915, 0, 0);
+ igt_subtest("bb-start-cmd")
+ test_bb_start(i915, handle, BB_START_CMD);
- igt_subtest("bb-start-cmd")
- test_bb_start(i915, handle, BB_START_CMD);
-
- igt_subtest("bb-start-far")
- test_bb_start(i915, handle, BB_START_FAR);
-
- igt_fixture igt_disallow_hang(i915, hang);
- }
+ igt_subtest("bb-start-far")
+ test_bb_start(i915, handle, BB_START_FAR);
igt_fixture {
igt_stop_hang_detector();
--
2.24.0
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [igt-dev] ✓ Fi.CI.BAT: success for i915/gem_exec_parse_blt: Fix COND_BBEND used by bb-start-(cmd|far)
2019-12-05 1:22 ` [igt-dev] " Chris Wilson
(?)
@ 2019-12-05 2:44 ` Patchwork
-1 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2019-12-05 2:44 UTC (permalink / raw)
To: Chris Wilson; +Cc: igt-dev
== Series Details ==
Series: i915/gem_exec_parse_blt: Fix COND_BBEND used by bb-start-(cmd|far)
URL : https://patchwork.freedesktop.org/series/70468/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_7486 -> IGTPW_3816
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3816/index.html
Known issues
------------
Here are the changes found in IGTPW_3816 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@i915_selftest@live_gem_contexts:
- fi-skl-lmem: [PASS][1] -> [INCOMPLETE][2] ([i915#424])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7486/fi-skl-lmem/igt@i915_selftest@live_gem_contexts.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3816/fi-skl-lmem/igt@i915_selftest@live_gem_contexts.html
- fi-byt-j1900: [PASS][3] -> [INCOMPLETE][4] ([i915#45])
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7486/fi-byt-j1900/igt@i915_selftest@live_gem_contexts.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3816/fi-byt-j1900/igt@i915_selftest@live_gem_contexts.html
* igt@kms_chamelium@hdmi-hpd-fast:
- fi-kbl-7500u: [PASS][5] -> [FAIL][6] ([fdo#111096] / [i915#323])
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7486/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3816/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
#### Possible fixes ####
* igt@gem_sync@basic-many-each:
- {fi-tgl-u}: [INCOMPLETE][7] ([i915#472] / [i915#707]) -> [PASS][8]
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7486/fi-tgl-u/igt@gem_sync@basic-many-each.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3816/fi-tgl-u/igt@gem_sync@basic-many-each.html
* igt@i915_selftest@live_blt:
- fi-hsw-4770: [DMESG-FAIL][9] ([i915#683]) -> [PASS][10]
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7486/fi-hsw-4770/igt@i915_selftest@live_blt.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3816/fi-hsw-4770/igt@i915_selftest@live_blt.html
- fi-bsw-nick: [DMESG-FAIL][11] ([i915#563]) -> [PASS][12]
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7486/fi-bsw-nick/igt@i915_selftest@live_blt.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3816/fi-bsw-nick/igt@i915_selftest@live_blt.html
- fi-hsw-4770r: [DMESG-FAIL][13] ([i915#683]) -> [PASS][14]
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7486/fi-hsw-4770r/igt@i915_selftest@live_blt.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3816/fi-hsw-4770r/igt@i915_selftest@live_blt.html
* igt@i915_selftest@live_gem_contexts:
- fi-byt-n2820: [INCOMPLETE][15] ([i915#45]) -> [PASS][16]
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7486/fi-byt-n2820/igt@i915_selftest@live_gem_contexts.html
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3816/fi-byt-n2820/igt@i915_selftest@live_gem_contexts.html
* igt@kms_frontbuffer_tracking@basic:
- fi-hsw-peppy: [DMESG-WARN][17] ([i915#44]) -> [PASS][18]
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7486/fi-hsw-peppy/igt@kms_frontbuffer_tracking@basic.html
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3816/fi-hsw-peppy/igt@kms_frontbuffer_tracking@basic.html
#### Warnings ####
* igt@kms_flip@basic-flip-vs-dpms:
- fi-kbl-x1275: [DMESG-WARN][19] ([i915#62] / [i915#92]) -> [DMESG-WARN][20] ([i915#62] / [i915#92] / [i915#95]) +2 similar issues
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7486/fi-kbl-x1275/igt@kms_flip@basic-flip-vs-dpms.html
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3816/fi-kbl-x1275/igt@kms_flip@basic-flip-vs-dpms.html
* igt@kms_force_connector_basic@force-edid:
- fi-kbl-x1275: [DMESG-WARN][21] ([i915#62] / [i915#92] / [i915#95]) -> [DMESG-WARN][22] ([i915#62] / [i915#92]) +3 similar issues
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7486/fi-kbl-x1275/igt@kms_force_connector_basic@force-edid.html
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3816/fi-kbl-x1275/igt@kms_force_connector_basic@force-edid.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#111096]: https://bugs.freedesktop.org/show_bug.cgi?id=111096
[fdo#111735]: https://bugs.freedesktop.org/show_bug.cgi?id=111735
[i915#323]: https://gitlab.freedesktop.org/drm/intel/issues/323
[i915#424]: https://gitlab.freedesktop.org/drm/intel/issues/424
[i915#44]: https://gitlab.freedesktop.org/drm/intel/issues/44
[i915#45]: https://gitlab.freedesktop.org/drm/intel/issues/45
[i915#472]: https://gitlab.freedesktop.org/drm/intel/issues/472
[i915#563]: https://gitlab.freedesktop.org/drm/intel/issues/563
[i915#592]: https://gitlab.freedesktop.org/drm/intel/issues/592
[i915#62]: https://gitlab.freedesktop.org/drm/intel/issues/62
[i915#683]: https://gitlab.freedesktop.org/drm/intel/issues/683
[i915#707]: https://gitlab.freedesktop.org/drm/intel/issues/707
[i915#92]: https://gitlab.freedesktop.org/drm/intel/issues/92
[i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95
Participating hosts (53 -> 47)
------------------------------
Additional (1): fi-cfl-8109u
Missing (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper fi-bdw-samus
Build changes
-------------
* CI: CI-20190529 -> None
* IGT: IGT_5331 -> IGTPW_3816
CI-20190529: 20190529
CI_DRM_7486: 8bd05bdd30fdd1b56307b780ab0f0e8de878c75a @ git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_3816: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3816/index.html
IGT_5331: dad473697b2e6bb5c45d7fec533b20d5dbe4fa17 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3816/index.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Intel-gfx] [PATCH i-g-t] i915/gem_exec_parse_blt: Fix COND_BBEND used by bb-start-(cmd|far)
2019-12-05 1:22 ` [igt-dev] " Chris Wilson
@ 2019-12-05 10:18 ` Mika Kuoppala
-1 siblings, 0 replies; 9+ messages in thread
From: Mika Kuoppala @ 2019-12-05 10:18 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: igt-dev
Chris Wilson <chris@chris-wilson.co.uk> writes:
> Correct the COND_BBEND instruction to perform the compare and apply the
> relocation so that it looks at the correct address. In the process,
> prepare for pipelined failures.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
> tests/i915/gem_exec_parse_blt.c | 116 +++++++++++++++++---------------
> 1 file changed, 61 insertions(+), 55 deletions(-)
>
> diff --git a/tests/i915/gem_exec_parse_blt.c b/tests/i915/gem_exec_parse_blt.c
> index 58d1b2b32..854c59863 100644
> --- a/tests/i915/gem_exec_parse_blt.c
> +++ b/tests/i915/gem_exec_parse_blt.c
> @@ -30,6 +30,7 @@
>
> #include "igt.h"
> #include "i915/gem_submission.h"
> +#include "sw_sync.h"
>
> /* To help craft commands known to be invalid across all engines */
> #define INSTR_CLIENT_SHIFT 29
> @@ -53,6 +54,30 @@
>
> #define HANDLE_SIZE 4096
>
> +static int
> +__checked_execbuf(int i915, struct drm_i915_gem_execbuffer2 *eb)
> +{
> + int fence;
> + int err;
> +
> + igt_assert(!(eb->flags & I915_EXEC_FENCE_OUT));
s/!// ?
> + eb->flags |= I915_EXEC_FENCE_OUT;
> + err = __gem_execbuf_wr(i915, eb);
> + eb->flags &= ~I915_EXEC_FENCE_OUT;
> + if (err)
> + return err;
> +
> + fence = eb->rsvd2 >> 32;
> +
> + sync_fence_wait(fence, -1);
> + err = sync_fence_status(fence);
> + close(fence);
> + if (err < 0)
> + return err;
> +
> + return 0;
> +}
> +
> static int
> __exec_batch_patched(int i915, int engine,
> uint32_t cmd_bo, const uint32_t *cmds, int size,
> @@ -85,7 +110,7 @@ __exec_batch_patched(int i915, int engine,
> execbuf.batch_len = size;
> execbuf.flags = engine;
>
> - return __gem_execbuf(i915, &execbuf);
> + return __checked_execbuf(i915, &execbuf);
> }
>
> static void exec_batch_patched(int i915, int engine,
> @@ -129,7 +154,7 @@ static int __exec_batch(int i915, int engine, uint32_t cmd_bo,
> execbuf.batch_len = size;
> execbuf.flags = engine;
>
> - return __gem_execbuf(i915, &execbuf);
> + return __checked_execbuf(i915, &execbuf);
> }
>
> #if 0
> @@ -188,7 +213,7 @@ static void exec_split_batch(int i915, int engine, const uint32_t *cmds,
> 0x8);
> execbuf.flags = engine;
>
> - igt_assert_eq(__gem_execbuf(i915, &execbuf), expected_ret);
> + igt_assert_eq(__checked_execbuf(i915, &execbuf), expected_ret);
>
> gem_close(i915, cmd_bo);
> }
> @@ -251,7 +276,7 @@ static void exec_batch_chained(int i915, int engine,
> execbuf.batch_len = sizeof(first_level_cmds);
> execbuf.flags = engine;
>
> - ret = __gem_execbuf(i915, &execbuf);
> + ret = __checked_execbuf(i915, &execbuf);
> if (expected_return && ret == expected_return)
> goto out;
>
> @@ -402,7 +427,7 @@ static void test_bb_secure(const int i915, const uint32_t handle)
> execbuf.batch_len = sizeof(batch_secure);
> execbuf.flags = I915_EXEC_BLT;
>
> - igt_assert_eq(__gem_execbuf(i915, &execbuf), -EACCES);
> + igt_assert_eq(__checked_execbuf(i915, &execbuf), -EACCES);
> }
>
> #define BB_START_PARAM 0
> @@ -414,12 +439,10 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
> {
> struct drm_i915_gem_execbuffer2 execbuf;
> struct drm_i915_gem_exec_object2 obj[2];
> - struct drm_i915_gem_relocation_entry reloc[3];
> + struct drm_i915_gem_relocation_entry reloc[4];
> const uint32_t target_bo = gem_create(i915, 4096);
> - uint32_t *dst;
> - int ret;
> unsigned int jump_off, footer_pos;
> - const uint32_t batch_header[] = {
> + uint32_t batch[1024] = {
> MI_NOOP,
> MI_NOOP,
> MI_NOOP,
> @@ -432,10 +455,11 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
> 4,
> 0,
> 2,
> - MI_COND_BATCH_BUFFER_END | 1,
> + MI_COND_BATCH_BUFFER_END | MI_DO_COMPARE | 2,
> 0,
> 0,
> - 0
> + 0,
> + MI_ARB_CHECK,
> };
> const uint32_t batch_footer[] = {
> MI_BATCH_BUFFER_START | MI_BATCH_NON_SECURE_I965 | 1,
> @@ -443,13 +467,10 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
> 0,
> MI_BATCH_BUFFER_END,
> };
> - uint32_t batch[1024];
> + uint32_t *dst;
>
> igt_require(gem_can_store_dword(i915, I915_EXEC_BLT));
>
> - memset(batch, 0, sizeof(batch));
> - memcpy(batch, batch_header, sizeof(batch_header));
> -
> switch (test) {
> case BB_START_PARAM:
> jump_off = 5 * sizeof(uint32_t);
> @@ -460,12 +481,13 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
> break;
> default:
> jump_off = 0xf00d0000;
> + break;
> }
>
> if (test == BB_START_FAR)
> - footer_pos = (sizeof(batch) - sizeof(batch_footer));
> + footer_pos = sizeof(batch) - sizeof(batch_footer);
> else
> - footer_pos = sizeof(batch_header);
> + footer_pos = 17 * sizeof(uint32_t);
>
> memcpy(batch + footer_pos / sizeof(uint32_t),
> batch_footer, sizeof(batch_footer));
> @@ -481,24 +503,28 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
> reloc[0].delta = 0;
> reloc[0].read_domains = I915_GEM_DOMAIN_COMMAND;
> reloc[0].write_domain = I915_GEM_DOMAIN_COMMAND;
> - reloc[0].presumed_offset = -1;
>
> reloc[1].offset = 9 * sizeof(uint32_t);
> reloc[1].target_handle = obj[0].handle;
> reloc[1].delta = 1 * sizeof(uint32_t);
> reloc[1].read_domains = I915_GEM_DOMAIN_COMMAND;
> reloc[1].write_domain = I915_GEM_DOMAIN_COMMAND;
> - reloc[1].presumed_offset = -1;
>
> - reloc[2].offset = footer_pos + 1 * sizeof(uint32_t);
> - reloc[2].target_handle = obj[1].handle;
> - reloc[2].delta = jump_off;
> + reloc[2].offset = 14 * sizeof(uint32_t);
> + reloc[2].target_handle = obj[0].handle;
> + reloc[2].delta = 0;
> reloc[2].read_domains = I915_GEM_DOMAIN_COMMAND;
> reloc[2].write_domain = 0;
> - reloc[2].presumed_offset = -1;
> +
> + reloc[3].offset = footer_pos + 1 * sizeof(uint32_t);
> + reloc[3].target_handle = obj[1].handle;
> + reloc[3].delta = jump_off;
> + reloc[3].read_domains = I915_GEM_DOMAIN_COMMAND;
> + reloc[3].write_domain = 0;
> + reloc[3].presumed_offset = -1;
Why we need to set the presumed only in this last reloc?
-Mika
>
> obj[1].relocs_ptr = to_user_pointer(reloc);
> - obj[1].relocation_count = 3;
> + obj[1].relocation_count = ARRAY_SIZE(reloc);
>
> memset(&execbuf, 0, sizeof(execbuf));
> execbuf.buffers_ptr = to_user_pointer(obj);
> @@ -506,21 +532,20 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
> execbuf.batch_len = sizeof(batch);
> execbuf.flags = I915_EXEC_BLT;
>
> - dst = gem_mmap__cpu(i915, obj[0].handle, 0, 4096,
> - PROT_READ | PROT_WRITE);
> + dst = gem_mmap__wc(i915, obj[0].handle, 0, 4096, PROT_WRITE);
>
> igt_assert_eq(dst[0], 0);
> igt_assert_eq(dst[1], 0);
>
> - ret = __gem_execbuf(i915, &execbuf);
> -
> switch (test) {
> case BB_START_PARAM:
> - igt_assert_eq(ret, -EINVAL);
> + case BB_START_OUT:
> + igt_assert_eq(__checked_execbuf(i915, &execbuf), -EINVAL);
> break;
> +
> case BB_START_CMD:
> case BB_START_FAR:
> - igt_assert_eq(ret, 0);
> + gem_execbuf(i915, &execbuf);
>
> while (READ_ONCE(dst[0]) == 0)
> ;
> @@ -531,19 +556,8 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
> igt_assert_eq(dst[0], 1);
> igt_assert_eq(dst[1], 2);
>
> - igt_info("values now %x %x\n", dst[0], dst[1]);
> -
> dst[0] = 0;
> -
> - igt_info("values now %x %x\n", dst[0], dst[1]);
> -
> - igt_assert_eq(dst[0], 0);
> - igt_assert_eq(dst[1], 2);
> -
> - break;
> -
> - case BB_START_OUT:
> - igt_assert_eq(ret, -EINVAL);
> + __sync_synchronize();
> break;
> }
>
> @@ -896,7 +910,7 @@ static void test_unaligned_jump(const int i915, const uint32_t handle)
> memcpy(&batch[1], &offset, sizeof(offset));
> gem_write(i915, handle, 4000, batch, sizeof(batch));
>
> - igt_assert_f(__gem_execbuf(i915, &execbuf) == -EINVAL,
> + igt_assert_f(__checked_execbuf(i915, &execbuf) == -EINVAL,
> "unaligned jump accepted to %d; batch=%08x\n",
> reloc.delta, batch[reloc.delta / 4]);
> }
> @@ -1032,19 +1046,11 @@ igt_main
> igt_subtest("unaligned-jump")
> test_unaligned_jump(i915, handle);
>
> - igt_subtest_group {
> - igt_hang_t hang;
> -
> - igt_fixture igt_allow_hang(i915, 0, 0);
> + igt_subtest("bb-start-cmd")
> + test_bb_start(i915, handle, BB_START_CMD);
>
> - igt_subtest("bb-start-cmd")
> - test_bb_start(i915, handle, BB_START_CMD);
> -
> - igt_subtest("bb-start-far")
> - test_bb_start(i915, handle, BB_START_FAR);
> -
> - igt_fixture igt_disallow_hang(i915, hang);
> - }
> + igt_subtest("bb-start-far")
> + test_bb_start(i915, handle, BB_START_FAR);
>
> igt_fixture {
> igt_stop_hang_detector();
> --
> 2.24.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] i915/gem_exec_parse_blt: Fix COND_BBEND used by bb-start-(cmd|far)
@ 2019-12-05 10:18 ` Mika Kuoppala
0 siblings, 0 replies; 9+ messages in thread
From: Mika Kuoppala @ 2019-12-05 10:18 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: igt-dev
Chris Wilson <chris@chris-wilson.co.uk> writes:
> Correct the COND_BBEND instruction to perform the compare and apply the
> relocation so that it looks at the correct address. In the process,
> prepare for pipelined failures.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
> tests/i915/gem_exec_parse_blt.c | 116 +++++++++++++++++---------------
> 1 file changed, 61 insertions(+), 55 deletions(-)
>
> diff --git a/tests/i915/gem_exec_parse_blt.c b/tests/i915/gem_exec_parse_blt.c
> index 58d1b2b32..854c59863 100644
> --- a/tests/i915/gem_exec_parse_blt.c
> +++ b/tests/i915/gem_exec_parse_blt.c
> @@ -30,6 +30,7 @@
>
> #include "igt.h"
> #include "i915/gem_submission.h"
> +#include "sw_sync.h"
>
> /* To help craft commands known to be invalid across all engines */
> #define INSTR_CLIENT_SHIFT 29
> @@ -53,6 +54,30 @@
>
> #define HANDLE_SIZE 4096
>
> +static int
> +__checked_execbuf(int i915, struct drm_i915_gem_execbuffer2 *eb)
> +{
> + int fence;
> + int err;
> +
> + igt_assert(!(eb->flags & I915_EXEC_FENCE_OUT));
s/!// ?
> + eb->flags |= I915_EXEC_FENCE_OUT;
> + err = __gem_execbuf_wr(i915, eb);
> + eb->flags &= ~I915_EXEC_FENCE_OUT;
> + if (err)
> + return err;
> +
> + fence = eb->rsvd2 >> 32;
> +
> + sync_fence_wait(fence, -1);
> + err = sync_fence_status(fence);
> + close(fence);
> + if (err < 0)
> + return err;
> +
> + return 0;
> +}
> +
> static int
> __exec_batch_patched(int i915, int engine,
> uint32_t cmd_bo, const uint32_t *cmds, int size,
> @@ -85,7 +110,7 @@ __exec_batch_patched(int i915, int engine,
> execbuf.batch_len = size;
> execbuf.flags = engine;
>
> - return __gem_execbuf(i915, &execbuf);
> + return __checked_execbuf(i915, &execbuf);
> }
>
> static void exec_batch_patched(int i915, int engine,
> @@ -129,7 +154,7 @@ static int __exec_batch(int i915, int engine, uint32_t cmd_bo,
> execbuf.batch_len = size;
> execbuf.flags = engine;
>
> - return __gem_execbuf(i915, &execbuf);
> + return __checked_execbuf(i915, &execbuf);
> }
>
> #if 0
> @@ -188,7 +213,7 @@ static void exec_split_batch(int i915, int engine, const uint32_t *cmds,
> 0x8);
> execbuf.flags = engine;
>
> - igt_assert_eq(__gem_execbuf(i915, &execbuf), expected_ret);
> + igt_assert_eq(__checked_execbuf(i915, &execbuf), expected_ret);
>
> gem_close(i915, cmd_bo);
> }
> @@ -251,7 +276,7 @@ static void exec_batch_chained(int i915, int engine,
> execbuf.batch_len = sizeof(first_level_cmds);
> execbuf.flags = engine;
>
> - ret = __gem_execbuf(i915, &execbuf);
> + ret = __checked_execbuf(i915, &execbuf);
> if (expected_return && ret == expected_return)
> goto out;
>
> @@ -402,7 +427,7 @@ static void test_bb_secure(const int i915, const uint32_t handle)
> execbuf.batch_len = sizeof(batch_secure);
> execbuf.flags = I915_EXEC_BLT;
>
> - igt_assert_eq(__gem_execbuf(i915, &execbuf), -EACCES);
> + igt_assert_eq(__checked_execbuf(i915, &execbuf), -EACCES);
> }
>
> #define BB_START_PARAM 0
> @@ -414,12 +439,10 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
> {
> struct drm_i915_gem_execbuffer2 execbuf;
> struct drm_i915_gem_exec_object2 obj[2];
> - struct drm_i915_gem_relocation_entry reloc[3];
> + struct drm_i915_gem_relocation_entry reloc[4];
> const uint32_t target_bo = gem_create(i915, 4096);
> - uint32_t *dst;
> - int ret;
> unsigned int jump_off, footer_pos;
> - const uint32_t batch_header[] = {
> + uint32_t batch[1024] = {
> MI_NOOP,
> MI_NOOP,
> MI_NOOP,
> @@ -432,10 +455,11 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
> 4,
> 0,
> 2,
> - MI_COND_BATCH_BUFFER_END | 1,
> + MI_COND_BATCH_BUFFER_END | MI_DO_COMPARE | 2,
> 0,
> 0,
> - 0
> + 0,
> + MI_ARB_CHECK,
> };
> const uint32_t batch_footer[] = {
> MI_BATCH_BUFFER_START | MI_BATCH_NON_SECURE_I965 | 1,
> @@ -443,13 +467,10 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
> 0,
> MI_BATCH_BUFFER_END,
> };
> - uint32_t batch[1024];
> + uint32_t *dst;
>
> igt_require(gem_can_store_dword(i915, I915_EXEC_BLT));
>
> - memset(batch, 0, sizeof(batch));
> - memcpy(batch, batch_header, sizeof(batch_header));
> -
> switch (test) {
> case BB_START_PARAM:
> jump_off = 5 * sizeof(uint32_t);
> @@ -460,12 +481,13 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
> break;
> default:
> jump_off = 0xf00d0000;
> + break;
> }
>
> if (test == BB_START_FAR)
> - footer_pos = (sizeof(batch) - sizeof(batch_footer));
> + footer_pos = sizeof(batch) - sizeof(batch_footer);
> else
> - footer_pos = sizeof(batch_header);
> + footer_pos = 17 * sizeof(uint32_t);
>
> memcpy(batch + footer_pos / sizeof(uint32_t),
> batch_footer, sizeof(batch_footer));
> @@ -481,24 +503,28 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
> reloc[0].delta = 0;
> reloc[0].read_domains = I915_GEM_DOMAIN_COMMAND;
> reloc[0].write_domain = I915_GEM_DOMAIN_COMMAND;
> - reloc[0].presumed_offset = -1;
>
> reloc[1].offset = 9 * sizeof(uint32_t);
> reloc[1].target_handle = obj[0].handle;
> reloc[1].delta = 1 * sizeof(uint32_t);
> reloc[1].read_domains = I915_GEM_DOMAIN_COMMAND;
> reloc[1].write_domain = I915_GEM_DOMAIN_COMMAND;
> - reloc[1].presumed_offset = -1;
>
> - reloc[2].offset = footer_pos + 1 * sizeof(uint32_t);
> - reloc[2].target_handle = obj[1].handle;
> - reloc[2].delta = jump_off;
> + reloc[2].offset = 14 * sizeof(uint32_t);
> + reloc[2].target_handle = obj[0].handle;
> + reloc[2].delta = 0;
> reloc[2].read_domains = I915_GEM_DOMAIN_COMMAND;
> reloc[2].write_domain = 0;
> - reloc[2].presumed_offset = -1;
> +
> + reloc[3].offset = footer_pos + 1 * sizeof(uint32_t);
> + reloc[3].target_handle = obj[1].handle;
> + reloc[3].delta = jump_off;
> + reloc[3].read_domains = I915_GEM_DOMAIN_COMMAND;
> + reloc[3].write_domain = 0;
> + reloc[3].presumed_offset = -1;
Why we need to set the presumed only in this last reloc?
-Mika
>
> obj[1].relocs_ptr = to_user_pointer(reloc);
> - obj[1].relocation_count = 3;
> + obj[1].relocation_count = ARRAY_SIZE(reloc);
>
> memset(&execbuf, 0, sizeof(execbuf));
> execbuf.buffers_ptr = to_user_pointer(obj);
> @@ -506,21 +532,20 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
> execbuf.batch_len = sizeof(batch);
> execbuf.flags = I915_EXEC_BLT;
>
> - dst = gem_mmap__cpu(i915, obj[0].handle, 0, 4096,
> - PROT_READ | PROT_WRITE);
> + dst = gem_mmap__wc(i915, obj[0].handle, 0, 4096, PROT_WRITE);
>
> igt_assert_eq(dst[0], 0);
> igt_assert_eq(dst[1], 0);
>
> - ret = __gem_execbuf(i915, &execbuf);
> -
> switch (test) {
> case BB_START_PARAM:
> - igt_assert_eq(ret, -EINVAL);
> + case BB_START_OUT:
> + igt_assert_eq(__checked_execbuf(i915, &execbuf), -EINVAL);
> break;
> +
> case BB_START_CMD:
> case BB_START_FAR:
> - igt_assert_eq(ret, 0);
> + gem_execbuf(i915, &execbuf);
>
> while (READ_ONCE(dst[0]) == 0)
> ;
> @@ -531,19 +556,8 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
> igt_assert_eq(dst[0], 1);
> igt_assert_eq(dst[1], 2);
>
> - igt_info("values now %x %x\n", dst[0], dst[1]);
> -
> dst[0] = 0;
> -
> - igt_info("values now %x %x\n", dst[0], dst[1]);
> -
> - igt_assert_eq(dst[0], 0);
> - igt_assert_eq(dst[1], 2);
> -
> - break;
> -
> - case BB_START_OUT:
> - igt_assert_eq(ret, -EINVAL);
> + __sync_synchronize();
> break;
> }
>
> @@ -896,7 +910,7 @@ static void test_unaligned_jump(const int i915, const uint32_t handle)
> memcpy(&batch[1], &offset, sizeof(offset));
> gem_write(i915, handle, 4000, batch, sizeof(batch));
>
> - igt_assert_f(__gem_execbuf(i915, &execbuf) == -EINVAL,
> + igt_assert_f(__checked_execbuf(i915, &execbuf) == -EINVAL,
> "unaligned jump accepted to %d; batch=%08x\n",
> reloc.delta, batch[reloc.delta / 4]);
> }
> @@ -1032,19 +1046,11 @@ igt_main
> igt_subtest("unaligned-jump")
> test_unaligned_jump(i915, handle);
>
> - igt_subtest_group {
> - igt_hang_t hang;
> -
> - igt_fixture igt_allow_hang(i915, 0, 0);
> + igt_subtest("bb-start-cmd")
> + test_bb_start(i915, handle, BB_START_CMD);
>
> - igt_subtest("bb-start-cmd")
> - test_bb_start(i915, handle, BB_START_CMD);
> -
> - igt_subtest("bb-start-far")
> - test_bb_start(i915, handle, BB_START_FAR);
> -
> - igt_fixture igt_disallow_hang(i915, hang);
> - }
> + igt_subtest("bb-start-far")
> + test_bb_start(i915, handle, BB_START_FAR);
>
> igt_fixture {
> igt_stop_hang_detector();
> --
> 2.24.0
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Intel-gfx] [PATCH i-g-t] i915/gem_exec_parse_blt: Fix COND_BBEND used by bb-start-(cmd|far)
2019-12-05 10:18 ` [igt-dev] " Mika Kuoppala
@ 2019-12-05 10:33 ` Chris Wilson
-1 siblings, 0 replies; 9+ messages in thread
From: Chris Wilson @ 2019-12-05 10:33 UTC (permalink / raw)
To: Mika Kuoppala, intel-gfx; +Cc: igt-dev
Quoting Mika Kuoppala (2019-12-05 10:18:34)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
>
> > Correct the COND_BBEND instruction to perform the compare and apply the
> > relocation so that it looks at the correct address. In the process,
> > prepare for pipelined failures.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > ---
> > tests/i915/gem_exec_parse_blt.c | 116 +++++++++++++++++---------------
> > 1 file changed, 61 insertions(+), 55 deletions(-)
> >
> > diff --git a/tests/i915/gem_exec_parse_blt.c b/tests/i915/gem_exec_parse_blt.c
> > index 58d1b2b32..854c59863 100644
> > --- a/tests/i915/gem_exec_parse_blt.c
> > +++ b/tests/i915/gem_exec_parse_blt.c
> > @@ -30,6 +30,7 @@
> >
> > #include "igt.h"
> > #include "i915/gem_submission.h"
> > +#include "sw_sync.h"
> >
> > /* To help craft commands known to be invalid across all engines */
> > #define INSTR_CLIENT_SHIFT 29
> > @@ -53,6 +54,30 @@
> >
> > #define HANDLE_SIZE 4096
> >
> > +static int
> > +__checked_execbuf(int i915, struct drm_i915_gem_execbuffer2 *eb)
> > +{
> > + int fence;
> > + int err;
> > +
> > + igt_assert(!(eb->flags & I915_EXEC_FENCE_OUT));
>
> s/!// ?
It's not a BUG_ON :)
We insist that the caller isn't expecting to use the out-fence
themselves.
> > + eb->flags |= I915_EXEC_FENCE_OUT;
> > + err = __gem_execbuf_wr(i915, eb);
> > + eb->flags &= ~I915_EXEC_FENCE_OUT;
> > + if (err)
> > + return err;
> > +
> > + fence = eb->rsvd2 >> 32;
> > +
> > + sync_fence_wait(fence, -1);
> > + err = sync_fence_status(fence);
> > + close(fence);
> > + if (err < 0)
> > + return err;
> > +
> > + return 0;
> > +}
> > +
> > static int
> > __exec_batch_patched(int i915, int engine,
> > uint32_t cmd_bo, const uint32_t *cmds, int size,
> > @@ -85,7 +110,7 @@ __exec_batch_patched(int i915, int engine,
> > execbuf.batch_len = size;
> > execbuf.flags = engine;
> >
> > - return __gem_execbuf(i915, &execbuf);
> > + return __checked_execbuf(i915, &execbuf);
> > }
> >
> > static void exec_batch_patched(int i915, int engine,
> > @@ -129,7 +154,7 @@ static int __exec_batch(int i915, int engine, uint32_t cmd_bo,
> > execbuf.batch_len = size;
> > execbuf.flags = engine;
> >
> > - return __gem_execbuf(i915, &execbuf);
> > + return __checked_execbuf(i915, &execbuf);
> > }
> >
> > #if 0
> > @@ -188,7 +213,7 @@ static void exec_split_batch(int i915, int engine, const uint32_t *cmds,
> > 0x8);
> > execbuf.flags = engine;
> >
> > - igt_assert_eq(__gem_execbuf(i915, &execbuf), expected_ret);
> > + igt_assert_eq(__checked_execbuf(i915, &execbuf), expected_ret);
> >
> > gem_close(i915, cmd_bo);
> > }
> > @@ -251,7 +276,7 @@ static void exec_batch_chained(int i915, int engine,
> > execbuf.batch_len = sizeof(first_level_cmds);
> > execbuf.flags = engine;
> >
> > - ret = __gem_execbuf(i915, &execbuf);
> > + ret = __checked_execbuf(i915, &execbuf);
> > if (expected_return && ret == expected_return)
> > goto out;
> >
> > @@ -402,7 +427,7 @@ static void test_bb_secure(const int i915, const uint32_t handle)
> > execbuf.batch_len = sizeof(batch_secure);
> > execbuf.flags = I915_EXEC_BLT;
> >
> > - igt_assert_eq(__gem_execbuf(i915, &execbuf), -EACCES);
> > + igt_assert_eq(__checked_execbuf(i915, &execbuf), -EACCES);
> > }
> >
> > #define BB_START_PARAM 0
> > @@ -414,12 +439,10 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
> > {
> > struct drm_i915_gem_execbuffer2 execbuf;
> > struct drm_i915_gem_exec_object2 obj[2];
> > - struct drm_i915_gem_relocation_entry reloc[3];
> > + struct drm_i915_gem_relocation_entry reloc[4];
> > const uint32_t target_bo = gem_create(i915, 4096);
> > - uint32_t *dst;
> > - int ret;
> > unsigned int jump_off, footer_pos;
> > - const uint32_t batch_header[] = {
> > + uint32_t batch[1024] = {
> > MI_NOOP,
> > MI_NOOP,
> > MI_NOOP,
> > @@ -432,10 +455,11 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
> > 4,
> > 0,
> > 2,
> > - MI_COND_BATCH_BUFFER_END | 1,
> > + MI_COND_BATCH_BUFFER_END | MI_DO_COMPARE | 2,
> > 0,
> > 0,
> > - 0
> > + 0,
> > + MI_ARB_CHECK,
> > };
> > const uint32_t batch_footer[] = {
> > MI_BATCH_BUFFER_START | MI_BATCH_NON_SECURE_I965 | 1,
> > @@ -443,13 +467,10 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
> > 0,
> > MI_BATCH_BUFFER_END,
> > };
> > - uint32_t batch[1024];
> > + uint32_t *dst;
> >
> > igt_require(gem_can_store_dword(i915, I915_EXEC_BLT));
> >
> > - memset(batch, 0, sizeof(batch));
> > - memcpy(batch, batch_header, sizeof(batch_header));
> > -
> > switch (test) {
> > case BB_START_PARAM:
> > jump_off = 5 * sizeof(uint32_t);
> > @@ -460,12 +481,13 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
> > break;
> > default:
> > jump_off = 0xf00d0000;
> > + break;
> > }
> >
> > if (test == BB_START_FAR)
> > - footer_pos = (sizeof(batch) - sizeof(batch_footer));
> > + footer_pos = sizeof(batch) - sizeof(batch_footer);
> > else
> > - footer_pos = sizeof(batch_header);
> > + footer_pos = 17 * sizeof(uint32_t);
> >
> > memcpy(batch + footer_pos / sizeof(uint32_t),
> > batch_footer, sizeof(batch_footer));
> > @@ -481,24 +503,28 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
> > reloc[0].delta = 0;
> > reloc[0].read_domains = I915_GEM_DOMAIN_COMMAND;
> > reloc[0].write_domain = I915_GEM_DOMAIN_COMMAND;
> > - reloc[0].presumed_offset = -1;
> >
> > reloc[1].offset = 9 * sizeof(uint32_t);
> > reloc[1].target_handle = obj[0].handle;
> > reloc[1].delta = 1 * sizeof(uint32_t);
> > reloc[1].read_domains = I915_GEM_DOMAIN_COMMAND;
> > reloc[1].write_domain = I915_GEM_DOMAIN_COMMAND;
> > - reloc[1].presumed_offset = -1;
> >
> > - reloc[2].offset = footer_pos + 1 * sizeof(uint32_t);
> > - reloc[2].target_handle = obj[1].handle;
> > - reloc[2].delta = jump_off;
> > + reloc[2].offset = 14 * sizeof(uint32_t);
> > + reloc[2].target_handle = obj[0].handle;
> > + reloc[2].delta = 0;
> > reloc[2].read_domains = I915_GEM_DOMAIN_COMMAND;
> > reloc[2].write_domain = 0;
> > - reloc[2].presumed_offset = -1;
> > +
> > + reloc[3].offset = footer_pos + 1 * sizeof(uint32_t);
> > + reloc[3].target_handle = obj[1].handle;
> > + reloc[3].delta = jump_off;
> > + reloc[3].read_domains = I915_GEM_DOMAIN_COMMAND;
> > + reloc[3].write_domain = 0;
> > + reloc[3].presumed_offset = -1;
>
> Why we need to set the presumed only in this last reloc?
It's the only one that is _not_ preset to presumed.offset + delta.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] i915/gem_exec_parse_blt: Fix COND_BBEND used by bb-start-(cmd|far)
@ 2019-12-05 10:33 ` Chris Wilson
0 siblings, 0 replies; 9+ messages in thread
From: Chris Wilson @ 2019-12-05 10:33 UTC (permalink / raw)
To: Mika Kuoppala, intel-gfx; +Cc: igt-dev
Quoting Mika Kuoppala (2019-12-05 10:18:34)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
>
> > Correct the COND_BBEND instruction to perform the compare and apply the
> > relocation so that it looks at the correct address. In the process,
> > prepare for pipelined failures.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > ---
> > tests/i915/gem_exec_parse_blt.c | 116 +++++++++++++++++---------------
> > 1 file changed, 61 insertions(+), 55 deletions(-)
> >
> > diff --git a/tests/i915/gem_exec_parse_blt.c b/tests/i915/gem_exec_parse_blt.c
> > index 58d1b2b32..854c59863 100644
> > --- a/tests/i915/gem_exec_parse_blt.c
> > +++ b/tests/i915/gem_exec_parse_blt.c
> > @@ -30,6 +30,7 @@
> >
> > #include "igt.h"
> > #include "i915/gem_submission.h"
> > +#include "sw_sync.h"
> >
> > /* To help craft commands known to be invalid across all engines */
> > #define INSTR_CLIENT_SHIFT 29
> > @@ -53,6 +54,30 @@
> >
> > #define HANDLE_SIZE 4096
> >
> > +static int
> > +__checked_execbuf(int i915, struct drm_i915_gem_execbuffer2 *eb)
> > +{
> > + int fence;
> > + int err;
> > +
> > + igt_assert(!(eb->flags & I915_EXEC_FENCE_OUT));
>
> s/!// ?
It's not a BUG_ON :)
We insist that the caller isn't expecting to use the out-fence
themselves.
> > + eb->flags |= I915_EXEC_FENCE_OUT;
> > + err = __gem_execbuf_wr(i915, eb);
> > + eb->flags &= ~I915_EXEC_FENCE_OUT;
> > + if (err)
> > + return err;
> > +
> > + fence = eb->rsvd2 >> 32;
> > +
> > + sync_fence_wait(fence, -1);
> > + err = sync_fence_status(fence);
> > + close(fence);
> > + if (err < 0)
> > + return err;
> > +
> > + return 0;
> > +}
> > +
> > static int
> > __exec_batch_patched(int i915, int engine,
> > uint32_t cmd_bo, const uint32_t *cmds, int size,
> > @@ -85,7 +110,7 @@ __exec_batch_patched(int i915, int engine,
> > execbuf.batch_len = size;
> > execbuf.flags = engine;
> >
> > - return __gem_execbuf(i915, &execbuf);
> > + return __checked_execbuf(i915, &execbuf);
> > }
> >
> > static void exec_batch_patched(int i915, int engine,
> > @@ -129,7 +154,7 @@ static int __exec_batch(int i915, int engine, uint32_t cmd_bo,
> > execbuf.batch_len = size;
> > execbuf.flags = engine;
> >
> > - return __gem_execbuf(i915, &execbuf);
> > + return __checked_execbuf(i915, &execbuf);
> > }
> >
> > #if 0
> > @@ -188,7 +213,7 @@ static void exec_split_batch(int i915, int engine, const uint32_t *cmds,
> > 0x8);
> > execbuf.flags = engine;
> >
> > - igt_assert_eq(__gem_execbuf(i915, &execbuf), expected_ret);
> > + igt_assert_eq(__checked_execbuf(i915, &execbuf), expected_ret);
> >
> > gem_close(i915, cmd_bo);
> > }
> > @@ -251,7 +276,7 @@ static void exec_batch_chained(int i915, int engine,
> > execbuf.batch_len = sizeof(first_level_cmds);
> > execbuf.flags = engine;
> >
> > - ret = __gem_execbuf(i915, &execbuf);
> > + ret = __checked_execbuf(i915, &execbuf);
> > if (expected_return && ret == expected_return)
> > goto out;
> >
> > @@ -402,7 +427,7 @@ static void test_bb_secure(const int i915, const uint32_t handle)
> > execbuf.batch_len = sizeof(batch_secure);
> > execbuf.flags = I915_EXEC_BLT;
> >
> > - igt_assert_eq(__gem_execbuf(i915, &execbuf), -EACCES);
> > + igt_assert_eq(__checked_execbuf(i915, &execbuf), -EACCES);
> > }
> >
> > #define BB_START_PARAM 0
> > @@ -414,12 +439,10 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
> > {
> > struct drm_i915_gem_execbuffer2 execbuf;
> > struct drm_i915_gem_exec_object2 obj[2];
> > - struct drm_i915_gem_relocation_entry reloc[3];
> > + struct drm_i915_gem_relocation_entry reloc[4];
> > const uint32_t target_bo = gem_create(i915, 4096);
> > - uint32_t *dst;
> > - int ret;
> > unsigned int jump_off, footer_pos;
> > - const uint32_t batch_header[] = {
> > + uint32_t batch[1024] = {
> > MI_NOOP,
> > MI_NOOP,
> > MI_NOOP,
> > @@ -432,10 +455,11 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
> > 4,
> > 0,
> > 2,
> > - MI_COND_BATCH_BUFFER_END | 1,
> > + MI_COND_BATCH_BUFFER_END | MI_DO_COMPARE | 2,
> > 0,
> > 0,
> > - 0
> > + 0,
> > + MI_ARB_CHECK,
> > };
> > const uint32_t batch_footer[] = {
> > MI_BATCH_BUFFER_START | MI_BATCH_NON_SECURE_I965 | 1,
> > @@ -443,13 +467,10 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
> > 0,
> > MI_BATCH_BUFFER_END,
> > };
> > - uint32_t batch[1024];
> > + uint32_t *dst;
> >
> > igt_require(gem_can_store_dword(i915, I915_EXEC_BLT));
> >
> > - memset(batch, 0, sizeof(batch));
> > - memcpy(batch, batch_header, sizeof(batch_header));
> > -
> > switch (test) {
> > case BB_START_PARAM:
> > jump_off = 5 * sizeof(uint32_t);
> > @@ -460,12 +481,13 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
> > break;
> > default:
> > jump_off = 0xf00d0000;
> > + break;
> > }
> >
> > if (test == BB_START_FAR)
> > - footer_pos = (sizeof(batch) - sizeof(batch_footer));
> > + footer_pos = sizeof(batch) - sizeof(batch_footer);
> > else
> > - footer_pos = sizeof(batch_header);
> > + footer_pos = 17 * sizeof(uint32_t);
> >
> > memcpy(batch + footer_pos / sizeof(uint32_t),
> > batch_footer, sizeof(batch_footer));
> > @@ -481,24 +503,28 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
> > reloc[0].delta = 0;
> > reloc[0].read_domains = I915_GEM_DOMAIN_COMMAND;
> > reloc[0].write_domain = I915_GEM_DOMAIN_COMMAND;
> > - reloc[0].presumed_offset = -1;
> >
> > reloc[1].offset = 9 * sizeof(uint32_t);
> > reloc[1].target_handle = obj[0].handle;
> > reloc[1].delta = 1 * sizeof(uint32_t);
> > reloc[1].read_domains = I915_GEM_DOMAIN_COMMAND;
> > reloc[1].write_domain = I915_GEM_DOMAIN_COMMAND;
> > - reloc[1].presumed_offset = -1;
> >
> > - reloc[2].offset = footer_pos + 1 * sizeof(uint32_t);
> > - reloc[2].target_handle = obj[1].handle;
> > - reloc[2].delta = jump_off;
> > + reloc[2].offset = 14 * sizeof(uint32_t);
> > + reloc[2].target_handle = obj[0].handle;
> > + reloc[2].delta = 0;
> > reloc[2].read_domains = I915_GEM_DOMAIN_COMMAND;
> > reloc[2].write_domain = 0;
> > - reloc[2].presumed_offset = -1;
> > +
> > + reloc[3].offset = footer_pos + 1 * sizeof(uint32_t);
> > + reloc[3].target_handle = obj[1].handle;
> > + reloc[3].delta = jump_off;
> > + reloc[3].read_domains = I915_GEM_DOMAIN_COMMAND;
> > + reloc[3].write_domain = 0;
> > + reloc[3].presumed_offset = -1;
>
> Why we need to set the presumed only in this last reloc?
It's the only one that is _not_ preset to presumed.offset + delta.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Intel-gfx] [PATCH i-g-t] i915/gem_exec_parse_blt: Fix COND_BBEND used by bb-start-(cmd|far)
2019-12-05 10:33 ` [igt-dev] " Chris Wilson
@ 2019-12-05 10:57 ` Mika Kuoppala
-1 siblings, 0 replies; 9+ messages in thread
From: Mika Kuoppala @ 2019-12-05 10:57 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: igt-dev
Chris Wilson <chris@chris-wilson.co.uk> writes:
> Quoting Mika Kuoppala (2019-12-05 10:18:34)
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>>
>> > Correct the COND_BBEND instruction to perform the compare and apply the
>> > relocation so that it looks at the correct address. In the process,
>> > prepare for pipelined failures.
>> >
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> > ---
>> > tests/i915/gem_exec_parse_blt.c | 116 +++++++++++++++++---------------
>> > 1 file changed, 61 insertions(+), 55 deletions(-)
>> >
>> > diff --git a/tests/i915/gem_exec_parse_blt.c b/tests/i915/gem_exec_parse_blt.c
>> > index 58d1b2b32..854c59863 100644
>> > --- a/tests/i915/gem_exec_parse_blt.c
>> > +++ b/tests/i915/gem_exec_parse_blt.c
>> > @@ -30,6 +30,7 @@
>> >
>> > #include "igt.h"
>> > #include "i915/gem_submission.h"
>> > +#include "sw_sync.h"
>> >
>> > /* To help craft commands known to be invalid across all engines */
>> > #define INSTR_CLIENT_SHIFT 29
>> > @@ -53,6 +54,30 @@
>> >
>> > #define HANDLE_SIZE 4096
>> >
>> > +static int
>> > +__checked_execbuf(int i915, struct drm_i915_gem_execbuffer2 *eb)
>> > +{
>> > + int fence;
>> > + int err;
>> > +
>> > + igt_assert(!(eb->flags & I915_EXEC_FENCE_OUT));
>>
>> s/!// ?
>
> It's not a BUG_ON :)
Could be the exact thought pattern of why I did got it
wrong...spooky!
>
> We insist that the caller isn't expecting to use the out-fence
> themselves.
>
>> > + eb->flags |= I915_EXEC_FENCE_OUT;
>> > + err = __gem_execbuf_wr(i915, eb);
>> > + eb->flags &= ~I915_EXEC_FENCE_OUT;
>> > + if (err)
>> > + return err;
>> > +
>> > + fence = eb->rsvd2 >> 32;
>> > +
>> > + sync_fence_wait(fence, -1);
>> > + err = sync_fence_status(fence);
>> > + close(fence);
>> > + if (err < 0)
>> > + return err;
>> > +
>> > + return 0;
>> > +}
>> > +
>> > static int
>> > __exec_batch_patched(int i915, int engine,
>> > uint32_t cmd_bo, const uint32_t *cmds, int size,
>> > @@ -85,7 +110,7 @@ __exec_batch_patched(int i915, int engine,
>> > execbuf.batch_len = size;
>> > execbuf.flags = engine;
>> >
>> > - return __gem_execbuf(i915, &execbuf);
>> > + return __checked_execbuf(i915, &execbuf);
>> > }
>> >
>> > static void exec_batch_patched(int i915, int engine,
>> > @@ -129,7 +154,7 @@ static int __exec_batch(int i915, int engine, uint32_t cmd_bo,
>> > execbuf.batch_len = size;
>> > execbuf.flags = engine;
>> >
>> > - return __gem_execbuf(i915, &execbuf);
>> > + return __checked_execbuf(i915, &execbuf);
>> > }
>> >
>> > #if 0
>> > @@ -188,7 +213,7 @@ static void exec_split_batch(int i915, int engine, const uint32_t *cmds,
>> > 0x8);
>> > execbuf.flags = engine;
>> >
>> > - igt_assert_eq(__gem_execbuf(i915, &execbuf), expected_ret);
>> > + igt_assert_eq(__checked_execbuf(i915, &execbuf), expected_ret);
>> >
>> > gem_close(i915, cmd_bo);
>> > }
>> > @@ -251,7 +276,7 @@ static void exec_batch_chained(int i915, int engine,
>> > execbuf.batch_len = sizeof(first_level_cmds);
>> > execbuf.flags = engine;
>> >
>> > - ret = __gem_execbuf(i915, &execbuf);
>> > + ret = __checked_execbuf(i915, &execbuf);
>> > if (expected_return && ret == expected_return)
>> > goto out;
>> >
>> > @@ -402,7 +427,7 @@ static void test_bb_secure(const int i915, const uint32_t handle)
>> > execbuf.batch_len = sizeof(batch_secure);
>> > execbuf.flags = I915_EXEC_BLT;
>> >
>> > - igt_assert_eq(__gem_execbuf(i915, &execbuf), -EACCES);
>> > + igt_assert_eq(__checked_execbuf(i915, &execbuf), -EACCES);
>> > }
>> >
>> > #define BB_START_PARAM 0
>> > @@ -414,12 +439,10 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
>> > {
>> > struct drm_i915_gem_execbuffer2 execbuf;
>> > struct drm_i915_gem_exec_object2 obj[2];
>> > - struct drm_i915_gem_relocation_entry reloc[3];
>> > + struct drm_i915_gem_relocation_entry reloc[4];
>> > const uint32_t target_bo = gem_create(i915, 4096);
>> > - uint32_t *dst;
>> > - int ret;
>> > unsigned int jump_off, footer_pos;
>> > - const uint32_t batch_header[] = {
>> > + uint32_t batch[1024] = {
>> > MI_NOOP,
>> > MI_NOOP,
>> > MI_NOOP,
>> > @@ -432,10 +455,11 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
>> > 4,
>> > 0,
>> > 2,
>> > - MI_COND_BATCH_BUFFER_END | 1,
>> > + MI_COND_BATCH_BUFFER_END | MI_DO_COMPARE | 2,
>> > 0,
>> > 0,
>> > - 0
>> > + 0,
>> > + MI_ARB_CHECK,
>> > };
>> > const uint32_t batch_footer[] = {
>> > MI_BATCH_BUFFER_START | MI_BATCH_NON_SECURE_I965 | 1,
>> > @@ -443,13 +467,10 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
>> > 0,
>> > MI_BATCH_BUFFER_END,
>> > };
>> > - uint32_t batch[1024];
>> > + uint32_t *dst;
>> >
>> > igt_require(gem_can_store_dword(i915, I915_EXEC_BLT));
>> >
>> > - memset(batch, 0, sizeof(batch));
>> > - memcpy(batch, batch_header, sizeof(batch_header));
>> > -
>> > switch (test) {
>> > case BB_START_PARAM:
>> > jump_off = 5 * sizeof(uint32_t);
>> > @@ -460,12 +481,13 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
>> > break;
>> > default:
>> > jump_off = 0xf00d0000;
>> > + break;
>> > }
>> >
>> > if (test == BB_START_FAR)
>> > - footer_pos = (sizeof(batch) - sizeof(batch_footer));
>> > + footer_pos = sizeof(batch) - sizeof(batch_footer);
>> > else
>> > - footer_pos = sizeof(batch_header);
>> > + footer_pos = 17 * sizeof(uint32_t);
>> >
>> > memcpy(batch + footer_pos / sizeof(uint32_t),
>> > batch_footer, sizeof(batch_footer));
>> > @@ -481,24 +503,28 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
>> > reloc[0].delta = 0;
>> > reloc[0].read_domains = I915_GEM_DOMAIN_COMMAND;
>> > reloc[0].write_domain = I915_GEM_DOMAIN_COMMAND;
>> > - reloc[0].presumed_offset = -1;
>> >
>> > reloc[1].offset = 9 * sizeof(uint32_t);
>> > reloc[1].target_handle = obj[0].handle;
>> > reloc[1].delta = 1 * sizeof(uint32_t);
>> > reloc[1].read_domains = I915_GEM_DOMAIN_COMMAND;
>> > reloc[1].write_domain = I915_GEM_DOMAIN_COMMAND;
>> > - reloc[1].presumed_offset = -1;
>> >
>> > - reloc[2].offset = footer_pos + 1 * sizeof(uint32_t);
>> > - reloc[2].target_handle = obj[1].handle;
>> > - reloc[2].delta = jump_off;
>> > + reloc[2].offset = 14 * sizeof(uint32_t);
>> > + reloc[2].target_handle = obj[0].handle;
>> > + reloc[2].delta = 0;
>> > reloc[2].read_domains = I915_GEM_DOMAIN_COMMAND;
>> > reloc[2].write_domain = 0;
>> > - reloc[2].presumed_offset = -1;
>> > +
>> > + reloc[3].offset = footer_pos + 1 * sizeof(uint32_t);
>> > + reloc[3].target_handle = obj[1].handle;
>> > + reloc[3].delta = jump_off;
>> > + reloc[3].read_domains = I915_GEM_DOMAIN_COMMAND;
>> > + reloc[3].write_domain = 0;
>> > + reloc[3].presumed_offset = -1;
>>
>> Why we need to set the presumed only in this last reloc?
>
> It's the only one that is _not_ preset to presumed.offset + delta.
Makes sense.
Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] i915/gem_exec_parse_blt: Fix COND_BBEND used by bb-start-(cmd|far)
@ 2019-12-05 10:57 ` Mika Kuoppala
0 siblings, 0 replies; 9+ messages in thread
From: Mika Kuoppala @ 2019-12-05 10:57 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: igt-dev
Chris Wilson <chris@chris-wilson.co.uk> writes:
> Quoting Mika Kuoppala (2019-12-05 10:18:34)
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>>
>> > Correct the COND_BBEND instruction to perform the compare and apply the
>> > relocation so that it looks at the correct address. In the process,
>> > prepare for pipelined failures.
>> >
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> > ---
>> > tests/i915/gem_exec_parse_blt.c | 116 +++++++++++++++++---------------
>> > 1 file changed, 61 insertions(+), 55 deletions(-)
>> >
>> > diff --git a/tests/i915/gem_exec_parse_blt.c b/tests/i915/gem_exec_parse_blt.c
>> > index 58d1b2b32..854c59863 100644
>> > --- a/tests/i915/gem_exec_parse_blt.c
>> > +++ b/tests/i915/gem_exec_parse_blt.c
>> > @@ -30,6 +30,7 @@
>> >
>> > #include "igt.h"
>> > #include "i915/gem_submission.h"
>> > +#include "sw_sync.h"
>> >
>> > /* To help craft commands known to be invalid across all engines */
>> > #define INSTR_CLIENT_SHIFT 29
>> > @@ -53,6 +54,30 @@
>> >
>> > #define HANDLE_SIZE 4096
>> >
>> > +static int
>> > +__checked_execbuf(int i915, struct drm_i915_gem_execbuffer2 *eb)
>> > +{
>> > + int fence;
>> > + int err;
>> > +
>> > + igt_assert(!(eb->flags & I915_EXEC_FENCE_OUT));
>>
>> s/!// ?
>
> It's not a BUG_ON :)
Could be the exact thought pattern of why I did got it
wrong...spooky!
>
> We insist that the caller isn't expecting to use the out-fence
> themselves.
>
>> > + eb->flags |= I915_EXEC_FENCE_OUT;
>> > + err = __gem_execbuf_wr(i915, eb);
>> > + eb->flags &= ~I915_EXEC_FENCE_OUT;
>> > + if (err)
>> > + return err;
>> > +
>> > + fence = eb->rsvd2 >> 32;
>> > +
>> > + sync_fence_wait(fence, -1);
>> > + err = sync_fence_status(fence);
>> > + close(fence);
>> > + if (err < 0)
>> > + return err;
>> > +
>> > + return 0;
>> > +}
>> > +
>> > static int
>> > __exec_batch_patched(int i915, int engine,
>> > uint32_t cmd_bo, const uint32_t *cmds, int size,
>> > @@ -85,7 +110,7 @@ __exec_batch_patched(int i915, int engine,
>> > execbuf.batch_len = size;
>> > execbuf.flags = engine;
>> >
>> > - return __gem_execbuf(i915, &execbuf);
>> > + return __checked_execbuf(i915, &execbuf);
>> > }
>> >
>> > static void exec_batch_patched(int i915, int engine,
>> > @@ -129,7 +154,7 @@ static int __exec_batch(int i915, int engine, uint32_t cmd_bo,
>> > execbuf.batch_len = size;
>> > execbuf.flags = engine;
>> >
>> > - return __gem_execbuf(i915, &execbuf);
>> > + return __checked_execbuf(i915, &execbuf);
>> > }
>> >
>> > #if 0
>> > @@ -188,7 +213,7 @@ static void exec_split_batch(int i915, int engine, const uint32_t *cmds,
>> > 0x8);
>> > execbuf.flags = engine;
>> >
>> > - igt_assert_eq(__gem_execbuf(i915, &execbuf), expected_ret);
>> > + igt_assert_eq(__checked_execbuf(i915, &execbuf), expected_ret);
>> >
>> > gem_close(i915, cmd_bo);
>> > }
>> > @@ -251,7 +276,7 @@ static void exec_batch_chained(int i915, int engine,
>> > execbuf.batch_len = sizeof(first_level_cmds);
>> > execbuf.flags = engine;
>> >
>> > - ret = __gem_execbuf(i915, &execbuf);
>> > + ret = __checked_execbuf(i915, &execbuf);
>> > if (expected_return && ret == expected_return)
>> > goto out;
>> >
>> > @@ -402,7 +427,7 @@ static void test_bb_secure(const int i915, const uint32_t handle)
>> > execbuf.batch_len = sizeof(batch_secure);
>> > execbuf.flags = I915_EXEC_BLT;
>> >
>> > - igt_assert_eq(__gem_execbuf(i915, &execbuf), -EACCES);
>> > + igt_assert_eq(__checked_execbuf(i915, &execbuf), -EACCES);
>> > }
>> >
>> > #define BB_START_PARAM 0
>> > @@ -414,12 +439,10 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
>> > {
>> > struct drm_i915_gem_execbuffer2 execbuf;
>> > struct drm_i915_gem_exec_object2 obj[2];
>> > - struct drm_i915_gem_relocation_entry reloc[3];
>> > + struct drm_i915_gem_relocation_entry reloc[4];
>> > const uint32_t target_bo = gem_create(i915, 4096);
>> > - uint32_t *dst;
>> > - int ret;
>> > unsigned int jump_off, footer_pos;
>> > - const uint32_t batch_header[] = {
>> > + uint32_t batch[1024] = {
>> > MI_NOOP,
>> > MI_NOOP,
>> > MI_NOOP,
>> > @@ -432,10 +455,11 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
>> > 4,
>> > 0,
>> > 2,
>> > - MI_COND_BATCH_BUFFER_END | 1,
>> > + MI_COND_BATCH_BUFFER_END | MI_DO_COMPARE | 2,
>> > 0,
>> > 0,
>> > - 0
>> > + 0,
>> > + MI_ARB_CHECK,
>> > };
>> > const uint32_t batch_footer[] = {
>> > MI_BATCH_BUFFER_START | MI_BATCH_NON_SECURE_I965 | 1,
>> > @@ -443,13 +467,10 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
>> > 0,
>> > MI_BATCH_BUFFER_END,
>> > };
>> > - uint32_t batch[1024];
>> > + uint32_t *dst;
>> >
>> > igt_require(gem_can_store_dword(i915, I915_EXEC_BLT));
>> >
>> > - memset(batch, 0, sizeof(batch));
>> > - memcpy(batch, batch_header, sizeof(batch_header));
>> > -
>> > switch (test) {
>> > case BB_START_PARAM:
>> > jump_off = 5 * sizeof(uint32_t);
>> > @@ -460,12 +481,13 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
>> > break;
>> > default:
>> > jump_off = 0xf00d0000;
>> > + break;
>> > }
>> >
>> > if (test == BB_START_FAR)
>> > - footer_pos = (sizeof(batch) - sizeof(batch_footer));
>> > + footer_pos = sizeof(batch) - sizeof(batch_footer);
>> > else
>> > - footer_pos = sizeof(batch_header);
>> > + footer_pos = 17 * sizeof(uint32_t);
>> >
>> > memcpy(batch + footer_pos / sizeof(uint32_t),
>> > batch_footer, sizeof(batch_footer));
>> > @@ -481,24 +503,28 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
>> > reloc[0].delta = 0;
>> > reloc[0].read_domains = I915_GEM_DOMAIN_COMMAND;
>> > reloc[0].write_domain = I915_GEM_DOMAIN_COMMAND;
>> > - reloc[0].presumed_offset = -1;
>> >
>> > reloc[1].offset = 9 * sizeof(uint32_t);
>> > reloc[1].target_handle = obj[0].handle;
>> > reloc[1].delta = 1 * sizeof(uint32_t);
>> > reloc[1].read_domains = I915_GEM_DOMAIN_COMMAND;
>> > reloc[1].write_domain = I915_GEM_DOMAIN_COMMAND;
>> > - reloc[1].presumed_offset = -1;
>> >
>> > - reloc[2].offset = footer_pos + 1 * sizeof(uint32_t);
>> > - reloc[2].target_handle = obj[1].handle;
>> > - reloc[2].delta = jump_off;
>> > + reloc[2].offset = 14 * sizeof(uint32_t);
>> > + reloc[2].target_handle = obj[0].handle;
>> > + reloc[2].delta = 0;
>> > reloc[2].read_domains = I915_GEM_DOMAIN_COMMAND;
>> > reloc[2].write_domain = 0;
>> > - reloc[2].presumed_offset = -1;
>> > +
>> > + reloc[3].offset = footer_pos + 1 * sizeof(uint32_t);
>> > + reloc[3].target_handle = obj[1].handle;
>> > + reloc[3].delta = jump_off;
>> > + reloc[3].read_domains = I915_GEM_DOMAIN_COMMAND;
>> > + reloc[3].write_domain = 0;
>> > + reloc[3].presumed_offset = -1;
>>
>> Why we need to set the presumed only in this last reloc?
>
> It's the only one that is _not_ preset to presumed.offset + delta.
Makes sense.
Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-12-05 10:57 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-05 1:22 [Intel-gfx] [PATCH i-g-t] i915/gem_exec_parse_blt: Fix COND_BBEND used by bb-start-(cmd|far) Chris Wilson
2019-12-05 1:22 ` [igt-dev] " Chris Wilson
2019-12-05 2:44 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
2019-12-05 10:18 ` [Intel-gfx] [PATCH i-g-t] " Mika Kuoppala
2019-12-05 10:18 ` [igt-dev] " Mika Kuoppala
2019-12-05 10:33 ` [Intel-gfx] " Chris Wilson
2019-12-05 10:33 ` [igt-dev] " Chris Wilson
2019-12-05 10:57 ` [Intel-gfx] " Mika Kuoppala
2019-12-05 10:57 ` [igt-dev] " Mika Kuoppala
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.