From: Mika Kuoppala <mika.kuoppala@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Cc: igt-dev@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH i-g-t] i915/gem_exec_parse_blt: Fix COND_BBEND used by bb-start-(cmd|far)
Date: Thu, 05 Dec 2019 12:57:17 +0200 [thread overview]
Message-ID: <87y2vrrqeq.fsf@gaia.fi.intel.com> (raw)
In-Reply-To: <157554202607.22727.15086790736335326296@skylake-alporthouse-com>
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
WARNING: multiple messages have this Message-ID (diff)
From: Mika Kuoppala <mika.kuoppala@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Cc: igt-dev@lists.freedesktop.org
Subject: Re: [igt-dev] [PATCH i-g-t] i915/gem_exec_parse_blt: Fix COND_BBEND used by bb-start-(cmd|far)
Date: Thu, 05 Dec 2019 12:57:17 +0200 [thread overview]
Message-ID: <87y2vrrqeq.fsf@gaia.fi.intel.com> (raw)
In-Reply-To: <157554202607.22727.15086790736335326296@skylake-alporthouse-com>
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
next prev parent reply other threads:[~2019-12-05 10:57 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
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 ` Mika Kuoppala [this message]
2019-12-05 10:57 ` Mika Kuoppala
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87y2vrrqeq.fsf@gaia.fi.intel.com \
--to=mika.kuoppala@linux.intel.com \
--cc=chris@chris-wilson.co.uk \
--cc=igt-dev@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.