* [igt-dev] [PATCH i-g-t 7/7] tests/gem_exec_schedule: Use store_dword_plug again
@ 2021-06-08 9:39 Daniel Vetter
2021-06-09 18:08 ` Jason Ekstrand
0 siblings, 1 reply; 3+ messages in thread
From: Daniel Vetter @ 2021-06-08 9:39 UTC (permalink / raw)
To: IGT development; +Cc: Daniel Vetter, Dave Airlie
In
commit 2884f91dd6d7682ea738ef6f0943cc591643dda2
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date: Fri Jul 20 12:29:30 2018 +0100
igt/gem_exec_schedule: Trim deep runtime
this was open-coded, I guess to avoid having to allocate a ton of
batchbuffers. Unfortunately this relies on being able to rewrite
batchbuffer relocations while the batch is in-flight (otherwise
everything stalls and our setup is for nothing), and we're removing
gpu relocations from upstream.
This problem was realized for the testcase in general in
commit f1e62e330a6e2de7b3cbf7cf02d71ae00cc6adcc
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date: Tue May 26 15:22:38 2020 +0100
i915/gem_exec_schedule: Reduce relocation risk for store-dword
but the fix in there is only stochastic, there's still a chance of
failure and hence unsightly noise in CI. The proper fix is to use
softpin for this testcase unconditionally (not just when relocations
aren't available), so that we clearly disentangle the concerns here
and really only test the scheduler. And not some legacy features that
are on the way out to their deserved retirement like relocations.
Zbyscek and Ashotush cc'ed so they're aware that this testcase must be
switched over to softping uncondtionally, for correctness reasons.
But that's a pile more work, so as an interim solution go back to
__store_dword helper.
Cc: Jason Ekstrand <jason@jlekstrand.net>
Cc: Dixit Ashutosh <ashutosh.dixit@intel.com>
Cc: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Dave Airlie <airlied@redhat.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
tests/i915/gem_exec_schedule.c | 52 +++-------------------------------
1 file changed, 4 insertions(+), 48 deletions(-)
diff --git a/tests/i915/gem_exec_schedule.c b/tests/i915/gem_exec_schedule.c
index 9585059d8ded..b5de1ab4eeae 100644
--- a/tests/i915/gem_exec_schedule.c
+++ b/tests/i915/gem_exec_schedule.c
@@ -1846,55 +1846,11 @@ static void deep(int fd, unsigned ring)
/* Create a deep dependency chain, with a few branches */
for (n = 0; n < nreq && igt_seconds_elapsed(&tv) < 2; n++) {
- const unsigned int gen = intel_gen(intel_get_drm_devid(fd));
- struct drm_i915_gem_exec_object2 obj[3];
- struct drm_i915_gem_relocation_entry reloc;
- struct drm_i915_gem_execbuffer2 eb = {
- .buffers_ptr = to_user_pointer(obj),
- .buffer_count = 3,
- .flags = ring | (gen < 6 ? I915_EXEC_SECURE : 0),
- .rsvd1 = ctx[n % MAX_CONTEXTS],
- };
- uint32_t batch[16];
- int i;
-
- memset(obj, 0, sizeof(obj));
- obj[0].handle = plug;
-
- memset(&reloc, 0, sizeof(reloc));
- reloc.presumed_offset = 0;
- reloc.offset = sizeof(uint32_t);
- reloc.delta = sizeof(uint32_t) * n;
- reloc.read_domains = I915_GEM_DOMAIN_RENDER;
- reloc.write_domain = I915_GEM_DOMAIN_RENDER;
- obj[2].handle = gem_create(fd, 4096);
- obj[2].relocs_ptr = to_user_pointer(&reloc);
- obj[2].relocation_count = 1;
-
- i = 0;
- batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
- if (gen >= 8) {
- batch[++i] = reloc.delta;
- batch[++i] = 0;
- } else if (gen >= 4) {
- batch[++i] = 0;
- batch[++i] = reloc.delta;
- reloc.offset += sizeof(uint32_t);
- } else {
- batch[i]--;
- batch[++i] = reloc.delta;
- }
- batch[++i] = eb.rsvd1;
- batch[++i] = MI_BATCH_BUFFER_END;
- gem_write(fd, obj[2].handle, 0, batch, sizeof(batch));
+ uint32_t context = ctx[n % MAX_CONTEXTS];
+ gem_context_set_priority(fd, context, MAX_PRIO - nreq + n);
- gem_context_set_priority(fd, eb.rsvd1, MAX_PRIO - nreq + n);
- for (int m = 0; m < XS; m++) {
- obj[1].handle = dep[m];
- reloc.target_handle = obj[1].handle;
- gem_execbuf(fd, &eb);
- }
- gem_close(fd, obj[2].handle);
+ for (int m = 0; m < XS; m++)
+ store_dword_plug(fd, context, ring, dep[m], 4*n, context, plug, I915_GEM_DOMAIN_INSTRUCTION);
}
igt_info("First deptree: %d requests [%.3fs]\n",
n * XS, 1e-9*igt_nsec_elapsed(&tv));
--
2.24.1
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 7/7] tests/gem_exec_schedule: Use store_dword_plug again
2021-06-08 9:39 [igt-dev] [PATCH i-g-t 7/7] tests/gem_exec_schedule: Use store_dword_plug again Daniel Vetter
@ 2021-06-09 18:08 ` Jason Ekstrand
0 siblings, 0 replies; 3+ messages in thread
From: Jason Ekstrand @ 2021-06-09 18:08 UTC (permalink / raw)
To: Daniel Vetter; +Cc: IGT development, Dave Airlie, Daniel Vetter
Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>
On Tue, Jun 8, 2021 at 4:39 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> In
>
> commit 2884f91dd6d7682ea738ef6f0943cc591643dda2
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date: Fri Jul 20 12:29:30 2018 +0100
>
> igt/gem_exec_schedule: Trim deep runtime
>
> this was open-coded, I guess to avoid having to allocate a ton of
> batchbuffers. Unfortunately this relies on being able to rewrite
> batchbuffer relocations while the batch is in-flight (otherwise
> everything stalls and our setup is for nothing), and we're removing
> gpu relocations from upstream.
>
> This problem was realized for the testcase in general in
>
> commit f1e62e330a6e2de7b3cbf7cf02d71ae00cc6adcc
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date: Tue May 26 15:22:38 2020 +0100
>
> i915/gem_exec_schedule: Reduce relocation risk for store-dword
>
> but the fix in there is only stochastic, there's still a chance of
> failure and hence unsightly noise in CI. The proper fix is to use
> softpin for this testcase unconditionally (not just when relocations
> aren't available), so that we clearly disentangle the concerns here
> and really only test the scheduler. And not some legacy features that
> are on the way out to their deserved retirement like relocations.
>
> Zbyscek and Ashotush cc'ed so they're aware that this testcase must be
> switched over to softping uncondtionally, for correctness reasons.
>
> But that's a pile more work, so as an interim solution go back to
> __store_dword helper.
>
> Cc: Jason Ekstrand <jason@jlekstrand.net>
> Cc: Dixit Ashutosh <ashutosh.dixit@intel.com>
> Cc: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
> tests/i915/gem_exec_schedule.c | 52 +++-------------------------------
> 1 file changed, 4 insertions(+), 48 deletions(-)
>
> diff --git a/tests/i915/gem_exec_schedule.c b/tests/i915/gem_exec_schedule.c
> index 9585059d8ded..b5de1ab4eeae 100644
> --- a/tests/i915/gem_exec_schedule.c
> +++ b/tests/i915/gem_exec_schedule.c
> @@ -1846,55 +1846,11 @@ static void deep(int fd, unsigned ring)
>
> /* Create a deep dependency chain, with a few branches */
> for (n = 0; n < nreq && igt_seconds_elapsed(&tv) < 2; n++) {
> - const unsigned int gen = intel_gen(intel_get_drm_devid(fd));
> - struct drm_i915_gem_exec_object2 obj[3];
> - struct drm_i915_gem_relocation_entry reloc;
> - struct drm_i915_gem_execbuffer2 eb = {
> - .buffers_ptr = to_user_pointer(obj),
> - .buffer_count = 3,
> - .flags = ring | (gen < 6 ? I915_EXEC_SECURE : 0),
> - .rsvd1 = ctx[n % MAX_CONTEXTS],
> - };
> - uint32_t batch[16];
> - int i;
> -
> - memset(obj, 0, sizeof(obj));
> - obj[0].handle = plug;
> -
> - memset(&reloc, 0, sizeof(reloc));
> - reloc.presumed_offset = 0;
> - reloc.offset = sizeof(uint32_t);
> - reloc.delta = sizeof(uint32_t) * n;
> - reloc.read_domains = I915_GEM_DOMAIN_RENDER;
> - reloc.write_domain = I915_GEM_DOMAIN_RENDER;
> - obj[2].handle = gem_create(fd, 4096);
> - obj[2].relocs_ptr = to_user_pointer(&reloc);
> - obj[2].relocation_count = 1;
> -
> - i = 0;
> - batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
> - if (gen >= 8) {
> - batch[++i] = reloc.delta;
> - batch[++i] = 0;
> - } else if (gen >= 4) {
> - batch[++i] = 0;
> - batch[++i] = reloc.delta;
> - reloc.offset += sizeof(uint32_t);
> - } else {
> - batch[i]--;
> - batch[++i] = reloc.delta;
> - }
> - batch[++i] = eb.rsvd1;
> - batch[++i] = MI_BATCH_BUFFER_END;
> - gem_write(fd, obj[2].handle, 0, batch, sizeof(batch));
> + uint32_t context = ctx[n % MAX_CONTEXTS];
> + gem_context_set_priority(fd, context, MAX_PRIO - nreq + n);
>
> - gem_context_set_priority(fd, eb.rsvd1, MAX_PRIO - nreq + n);
> - for (int m = 0; m < XS; m++) {
> - obj[1].handle = dep[m];
> - reloc.target_handle = obj[1].handle;
> - gem_execbuf(fd, &eb);
> - }
> - gem_close(fd, obj[2].handle);
> + for (int m = 0; m < XS; m++)
> + store_dword_plug(fd, context, ring, dep[m], 4*n, context, plug, I915_GEM_DOMAIN_INSTRUCTION);
> }
> igt_info("First deptree: %d requests [%.3fs]\n",
> n * XS, 1e-9*igt_nsec_elapsed(&tv));
> --
> 2.24.1
>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 3+ messages in thread
* [igt-dev] [PATCH i-g-t 7/7] tests/gem_exec_schedule: Use store_dword_plug again
2021-06-08 9:40 [igt-dev] [PATCH i-g-t 1/7] tests/gem_exec_reloc: Remove banned tests Daniel Vetter
@ 2021-06-08 9:40 ` Daniel Vetter
0 siblings, 0 replies; 3+ messages in thread
From: Daniel Vetter @ 2021-06-08 9:40 UTC (permalink / raw)
To: IGT development; +Cc: Daniel Vetter, Dave Airlie
In
commit 2884f91dd6d7682ea738ef6f0943cc591643dda2
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date: Fri Jul 20 12:29:30 2018 +0100
igt/gem_exec_schedule: Trim deep runtime
this was open-coded, I guess to avoid having to allocate a ton of
batchbuffers. Unfortunately this relies on being able to rewrite
batchbuffer relocations while the batch is in-flight (otherwise
everything stalls and our setup is for nothing), and we're removing
gpu relocations from upstream.
This problem was realized for the testcase in general in
commit f1e62e330a6e2de7b3cbf7cf02d71ae00cc6adcc
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date: Tue May 26 15:22:38 2020 +0100
i915/gem_exec_schedule: Reduce relocation risk for store-dword
but the fix in there is only stochastic, there's still a chance of
failure and hence unsightly noise in CI. The proper fix is to use
softpin for this testcase unconditionally (not just when relocations
aren't available), so that we clearly disentangle the concerns here
and really only test the scheduler. And not some legacy features that
are on the way out to their deserved retirement like relocations.
Zbyscek and Ashotush cc'ed so they're aware that this testcase must be
switched over to softping uncondtionally, for correctness reasons.
But that's a pile more work, so as an interim solution go back to
__store_dword helper.
Cc: Jason Ekstrand <jason@jlekstrand.net>
Cc: Dixit Ashutosh <ashutosh.dixit@intel.com>
Cc: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Dave Airlie <airlied@redhat.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
tests/i915/gem_exec_schedule.c | 52 +++-------------------------------
1 file changed, 4 insertions(+), 48 deletions(-)
diff --git a/tests/i915/gem_exec_schedule.c b/tests/i915/gem_exec_schedule.c
index 9585059d8ded..b5de1ab4eeae 100644
--- a/tests/i915/gem_exec_schedule.c
+++ b/tests/i915/gem_exec_schedule.c
@@ -1846,55 +1846,11 @@ static void deep(int fd, unsigned ring)
/* Create a deep dependency chain, with a few branches */
for (n = 0; n < nreq && igt_seconds_elapsed(&tv) < 2; n++) {
- const unsigned int gen = intel_gen(intel_get_drm_devid(fd));
- struct drm_i915_gem_exec_object2 obj[3];
- struct drm_i915_gem_relocation_entry reloc;
- struct drm_i915_gem_execbuffer2 eb = {
- .buffers_ptr = to_user_pointer(obj),
- .buffer_count = 3,
- .flags = ring | (gen < 6 ? I915_EXEC_SECURE : 0),
- .rsvd1 = ctx[n % MAX_CONTEXTS],
- };
- uint32_t batch[16];
- int i;
-
- memset(obj, 0, sizeof(obj));
- obj[0].handle = plug;
-
- memset(&reloc, 0, sizeof(reloc));
- reloc.presumed_offset = 0;
- reloc.offset = sizeof(uint32_t);
- reloc.delta = sizeof(uint32_t) * n;
- reloc.read_domains = I915_GEM_DOMAIN_RENDER;
- reloc.write_domain = I915_GEM_DOMAIN_RENDER;
- obj[2].handle = gem_create(fd, 4096);
- obj[2].relocs_ptr = to_user_pointer(&reloc);
- obj[2].relocation_count = 1;
-
- i = 0;
- batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
- if (gen >= 8) {
- batch[++i] = reloc.delta;
- batch[++i] = 0;
- } else if (gen >= 4) {
- batch[++i] = 0;
- batch[++i] = reloc.delta;
- reloc.offset += sizeof(uint32_t);
- } else {
- batch[i]--;
- batch[++i] = reloc.delta;
- }
- batch[++i] = eb.rsvd1;
- batch[++i] = MI_BATCH_BUFFER_END;
- gem_write(fd, obj[2].handle, 0, batch, sizeof(batch));
+ uint32_t context = ctx[n % MAX_CONTEXTS];
+ gem_context_set_priority(fd, context, MAX_PRIO - nreq + n);
- gem_context_set_priority(fd, eb.rsvd1, MAX_PRIO - nreq + n);
- for (int m = 0; m < XS; m++) {
- obj[1].handle = dep[m];
- reloc.target_handle = obj[1].handle;
- gem_execbuf(fd, &eb);
- }
- gem_close(fd, obj[2].handle);
+ for (int m = 0; m < XS; m++)
+ store_dword_plug(fd, context, ring, dep[m], 4*n, context, plug, I915_GEM_DOMAIN_INSTRUCTION);
}
igt_info("First deptree: %d requests [%.3fs]\n",
n * XS, 1e-9*igt_nsec_elapsed(&tv));
--
2.24.1
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-06-09 18:08 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-08 9:39 [igt-dev] [PATCH i-g-t 7/7] tests/gem_exec_schedule: Use store_dword_plug again Daniel Vetter
2021-06-09 18:08 ` Jason Ekstrand
2021-06-08 9:40 [igt-dev] [PATCH i-g-t 1/7] tests/gem_exec_reloc: Remove banned tests Daniel Vetter
2021-06-08 9:40 ` [igt-dev] [PATCH i-g-t 7/7] tests/gem_exec_schedule: Use store_dword_plug again Daniel Vetter
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.