[igt-dev] [PATCH i-g-t 2/2] tests/i915/gem_concurrent_all: Add no-reloc capability
Kamil Konieczny
kamil.konieczny at linux.intel.com
Tue Mar 29 12:56:48 UTC 2022
Hi Zbigniew,
Dnia 2022-03-29 at 10:10:14 +0200, Zbigniew Kempczyński napisał(a):
> On Mon, Mar 28, 2022 at 06:55:45PM +0200, Kamil Konieczny wrote:
> > Add noreloc mode for GPU gens without relocations. Also
> > while at this, add some caching for required properties.
> > Change also snoop function so it will work on DG1.
> >
> > Tested with ./gem_concurrent_blit --run '4KiB-tiny-gpu-*'
> > and 256KiB with modified drm-tip to allow softpinning.
> >
> > v7: rebase, cleanup bit17 caching (Zbigniew comments)
> > v6: correct comment, rewrite bit17 caching (Zbigniew)
> > v5: rebase, fix caching in bit17_require, changes according
> > to Zbigniew review: simplify cache of !gem_has_llc, drop
> > multiprocess start/stop, use ALLOC_STRATEGY_HIGH_TO_LOW,
> > correct offset and flags
> > v4: corrected alloc_open and first ahnd setting
> >
> > Signed-off-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> > Cc: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > ---
> > tests/i915/gem_concurrent_all.c | 176 ++++++++++++++++++++++++++------
> > 1 file changed, 145 insertions(+), 31 deletions(-)
> >
> > diff --git a/tests/i915/gem_concurrent_all.c b/tests/i915/gem_concurrent_all.c
> > index d0f9b62e..7a91434c 100644
> > --- a/tests/i915/gem_concurrent_all.c
> > +++ b/tests/i915/gem_concurrent_all.c
> > @@ -60,6 +60,7 @@ int fd, devid, gen;
> > int vgem_drv = -1;
> > int all;
> > int pass;
> > +uint64_t ahnd;
> >
> > struct create {
> > const char *name;
> > @@ -239,8 +240,16 @@ unmapped_create_bo(const struct buffers *b)
> >
> > static void create_snoop_require(const struct create *create, unsigned count)
> > {
> > + static bool check_llc = true;
> > + static bool has_snoop;
> > +
> > create_cpu_require(create, count);
> > - igt_require(!gem_has_llc(fd));
> > + if (check_llc) {
> > + has_snoop = !gem_has_llc(fd);
> > + check_llc = false;
> > + }
> > +
> > + igt_require(has_snoop);
> > }
> >
> > static struct intel_buf *
> > @@ -249,7 +258,7 @@ snoop_create_bo(const struct buffers *b)
> > struct intel_buf *buf;
> >
> > buf = unmapped_create_bo(b);
> > - gem_set_caching(fd, buf->handle, I915_CACHING_CACHED);
> > + __gem_set_caching(fd, buf->handle, I915_CACHING_CACHED);
> >
> > return buf;
> > }
> > @@ -572,22 +581,33 @@ gttX_create_bo(const struct buffers *b)
> >
> > static void bit17_require(void)
> > {
> > - static struct drm_i915_gem_get_tiling2 {
> > - uint32_t handle;
> > - uint32_t tiling_mode;
> > - uint32_t swizzle_mode;
> > - uint32_t phys_swizzle_mode;
> > - } arg;
> > + static bool has_tiling2, checked;
> > +
> > #define DRM_IOCTL_I915_GEM_GET_TILING2 DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_GET_TILING, struct drm_i915_gem_get_tiling2)
> >
> > - if (arg.handle == 0) {
> > + if (!checked) {
> > + struct drm_i915_gem_get_tiling2 {
> > + uint32_t handle;
> > + uint32_t tiling_mode;
> > + uint32_t swizzle_mode;
> > + uint32_t phys_swizzle_mode;
> > + } arg = {};
> > + int err;
> > +
> > + checked = true;
> > arg.handle = gem_create(fd, 4096);
> > - gem_set_tiling(fd, arg.handle, I915_TILING_X, 512);
> > + err = __gem_set_tiling(fd, arg.handle, I915_TILING_X, 512);
> > + if (!err) {
> > + igt_ioctl(fd, DRM_IOCTL_I915_GEM_GET_TILING2, &arg);
> > + if (!errno && arg.phys_swizzle_mode == arg.swizzle_mode)
> > + has_tiling2 = true;
> > + }
> >
> > - do_ioctl(fd, DRM_IOCTL_I915_GEM_GET_TILING2, &arg);
> > + errno = 0;
> > gem_close(fd, arg.handle);
> > }
> > - igt_require(arg.phys_swizzle_mode == arg.swizzle_mode);
> > +
> > + igt_require(has_tiling2);
> > }
>
> This looks much better.
>
> >
> > static void wc_require(void)
> > @@ -670,11 +690,21 @@ gpu_set_bo(struct buffers *buffers, struct intel_buf *buf, uint32_t val)
> > struct drm_i915_gem_exec_object2 gem_exec[2];
> > struct drm_i915_gem_execbuffer2 execbuf;
> > uint32_t tmp[10], *b;
> > + uint64_t addr = 0;
> >
> > memset(reloc, 0, sizeof(reloc));
> > memset(gem_exec, 0, sizeof(gem_exec));
> > memset(&execbuf, 0, sizeof(execbuf));
> >
> > + if (ahnd) {
> > + addr = buf->addr.offset;
> > + if (INVALID_ADDR(addr)) {
> > + addr = intel_allocator_alloc(buffers->ibb->allocator_handle,
> > + buf->handle, buf->size, 0);
> > + buf->addr.offset = addr;
> > + }
> > + }
> > +
> > b = tmp;
> > *b++ = XY_COLOR_BLT_CMD_NOLEN |
> > ((gen >= 8) ? 5 : 4) |
> > @@ -691,9 +721,9 @@ gpu_set_bo(struct buffers *buffers, struct intel_buf *buf, uint32_t val)
> > reloc[0].target_handle = buf->handle;
> > reloc[0].read_domains = I915_GEM_DOMAIN_RENDER;
> > reloc[0].write_domain = I915_GEM_DOMAIN_RENDER;
> > - *b++ = 0;
> > + *b++ = addr;
> > if (gen >= 8)
> > - *b++ = 0;
> > + *b++ = addr >> 32;
> > *b++ = val;
> > *b++ = MI_BATCH_BUFFER_END;
> > if ((b - tmp) & 1)
> > @@ -703,8 +733,19 @@ gpu_set_bo(struct buffers *buffers, struct intel_buf *buf, uint32_t val)
> > gem_exec[0].flags = EXEC_OBJECT_NEEDS_FENCE;
> >
> > gem_exec[1].handle = gem_create(fd, 4096);
> > - gem_exec[1].relocation_count = 1;
> > - gem_exec[1].relocs_ptr = to_user_pointer(reloc);
> > + if (!ahnd) {
> > + gem_exec[1].relocation_count = 1;
> > + gem_exec[1].relocs_ptr = to_user_pointer(reloc);
> > + } else {
> > + gem_exec[1].offset = CANONICAL(intel_allocator_alloc(ahnd,
> > + gem_exec[1].handle,
> > + 4096, 0));
> > + gem_exec[1].flags |= EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> > +
> > + gem_exec[0].offset = CANONICAL(buf->addr.offset);
> > + gem_exec[0].flags |= EXEC_OBJECT_PINNED | EXEC_OBJECT_WRITE |
> > + EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> > + }
> >
> > execbuf.buffers_ptr = to_user_pointer(gem_exec);
> > execbuf.buffer_count = 2;
> > @@ -716,6 +757,7 @@ gpu_set_bo(struct buffers *buffers, struct intel_buf *buf, uint32_t val)
> > gem_execbuf(fd, &execbuf);
> >
> > gem_close(fd, gem_exec[1].handle);
> > + put_offset(ahnd, gem_exec[1].handle);
> > }
> >
> > static void
> > @@ -766,6 +808,18 @@ static bool set_max_map_count(int num_buffers)
> > return max > num_buffers;
> > }
> >
> > +static uint64_t alloc_open(void)
> > +{
> > + return ahnd ? intel_allocator_open_full(fd, 0, 0, 0, INTEL_ALLOCATOR_SIMPLE,
> > + ALLOC_STRATEGY_HIGH_TO_LOW, 0) : 0;
>
> Should be:
>
> return ahnd ? get_simple_h2l_ahnd(fd, 0) : 0;
>
> Explanation below.
I would like to keep it here, get_simple checks for relocs
before actually doing anything and that runs execbuf. I like to
avoid any syscall here, if possible.
> > +}
> > +
> > +static struct intel_bb *bb_create(int i915, uint32_t size)
> > +{
> > + return ahnd ? intel_bb_create_no_relocs(i915, size) :
> > + intel_bb_create_with_relocs(i915, size);
> > +}
> > +
> > static void buffers_init(struct buffers *b,
> > const char *name,
> > const struct create *create,
> > @@ -796,7 +850,7 @@ static void buffers_init(struct buffers *b,
> > igt_assert(b->src);
> > b->dst = b->src + num_buffers;
> >
> > - b->ibb = intel_bb_create(_fd, 4096);
> > + b->ibb = bb_create(_fd, 4096);
> > }
> >
> > static void buffers_destroy(struct buffers *b)
> > @@ -829,6 +883,27 @@ static void buffers_destroy(struct buffers *b)
> > }
> > }
> >
> > +static void bb_destroy(struct buffers *b)
> > +{
> > + if (b->ibb) {
> > + intel_bb_destroy(b->ibb);
> > + b->ibb = NULL;
> > + }
> > +}
> > +
> > +static void __bufs_destroy(struct buffers *b)
> > +{
> > + buffers_destroy(b);
> > + if (b->ibb) {
> > + intel_bb_destroy(b->ibb);
> > + b->ibb = NULL;
> > + }
> > + if (b->bops) {
> > + buf_ops_destroy(b->bops);
> > + b->bops = NULL;
> > + }
> > +}
> > +
> > static void buffers_create(struct buffers *b)
> > {
> > int count = b->num_buffers;
> > @@ -838,32 +913,57 @@ static void buffers_create(struct buffers *b)
> > igt_assert(b->count == 0);
> > b->count = count;
> >
> > + ahnd = alloc_open();
> > for (int i = 0; i < count; i++) {
> > b->src[i] = b->mode->create_bo(b);
> > b->dst[i] = b->mode->create_bo(b);
> > }
> > b->spare = b->mode->create_bo(b);
> > b->snoop = snoop_create_bo(b);
> > + if (b->ibb)
> > + intel_bb_destroy(b->ibb);
> > +
> > + b->ibb = bb_create(fd, 4096);
> > }
> >
> > static void buffers_reset(struct buffers *b)
> > {
> > b->bops = buf_ops_create(fd);
> > - b->ibb = intel_bb_create(fd, 4096);
> > + b->ibb = bb_create(fd, 4096);
> > +}
> > +
> > +static void __buffers_create(struct buffers *b)
> > +{
> > + b->bops = buf_ops_create(fd);
> > + igt_assert(b->bops);
> > + igt_assert(b->num_buffers > 0);
> > + igt_assert(b->mode);
> > + igt_assert(b->mode->create_bo);
> > +
> > + b->count = 0;
> > + for (int i = 0; i < b->num_buffers; i++) {
> > + b->src[i] = b->mode->create_bo(b);
> > + b->dst[i] = b->mode->create_bo(b);
> > + }
> > + b->count = b->num_buffers;
> > + b->spare = b->mode->create_bo(b);
> > + b->snoop = snoop_create_bo(b);
> > + ahnd = alloc_open();
> > + b->ibb = bb_create(fd, 4096);
> > }
> >
> > static void buffers_fini(struct buffers *b)
> > {
> > if (b->bops == NULL)
> > return;
> > -
> > buffers_destroy(b);
> >
> > free(b->tmp);
> > free(b->src);
> > -
> > - intel_bb_destroy(b->ibb);
> > - buf_ops_destroy(b->bops);
> > + if (b->ibb)
> > + intel_bb_destroy(b->ibb);
> > + if (b->bops)
> > + buf_ops_destroy(b->bops);
> >
> > memset(b, 0, sizeof(*b));
> > }
> > @@ -1306,6 +1406,8 @@ static void run_single(struct buffers *buffers,
> > do_hang do_hang_func)
> > {
> > pass = 0;
> > + bb_destroy(buffers);
> > + buffers->ibb = bb_create(fd, 4096);
> > do_test_func(buffers, do_copy_func, do_hang_func);
> > igt_assert_eq(intel_detect_and_clear_missed_interrupts(fd), 0);
> > }
> > @@ -1316,6 +1418,8 @@ static void run_interruptible(struct buffers *buffers,
> > do_hang do_hang_func)
> > {
> > pass = 0;
> > + bb_destroy(buffers);
> > + buffers->ibb = bb_create(fd, 4096);
> > igt_while_interruptible(true)
> > do_test_func(buffers, do_copy_func, do_hang_func);
> > igt_assert_eq(intel_detect_and_clear_missed_interrupts(fd), 0);
> > @@ -1332,10 +1436,18 @@ static void run_child(struct buffers *buffers,
> > * leading to the child closing an object without the parent knowing.
> > */
> > pass = 0;
> > - igt_fork(child, 1)
> > + __bufs_destroy(buffers);
> > +
> > + igt_fork(child, 1) {
> > + /* recreate process local variables */
> > + intel_allocator_init();
> > + __buffers_create(buffers);
> > do_test_func(buffers, do_copy_func, do_hang_func);
> > + }
> > igt_waitchildren();
> > +
> > igt_assert_eq(intel_detect_and_clear_missed_interrupts(fd), 0);
> > + buffers_reset(buffers);
> > }
> >
> > static void __run_forked(struct buffers *buffers,
> > @@ -1346,24 +1458,20 @@ static void __run_forked(struct buffers *buffers,
> >
> > {
> > /* purge the caches before cloing the process */
> > - buffers_destroy(buffers);
> > - intel_bb_destroy(buffers->ibb);
> > - buf_ops_destroy(buffers->bops);
> > + __bufs_destroy(buffers);
> >
> > igt_fork(child, num_children) {
> > int num_buffers;
> >
> > /* recreate process local variables */
> > fd = gem_reopen_driver(fd);
> > -
> > + intel_allocator_init(); /* detach from thread */
> > num_buffers = buffers->num_buffers / num_children;
> > num_buffers += MIN_BUFFERS;
> > if (num_buffers < buffers->num_buffers)
> > buffers->num_buffers = num_buffers;
> >
> > - buffers_reset(buffers);
> > - buffers_create(buffers);
> > -
> > + __buffers_create(buffers);
> > igt_while_interruptible(interrupt) {
> > for (pass = 0; pass < loops; pass++)
> > do_test_func(buffers,
> > @@ -1773,6 +1881,7 @@ igt_main
> > { "16MiB", 2048, 2048 },
> > { NULL}
> > };
> > +
> > uint64_t pin_sz = 0;
> > void *pinned = NULL;
> > char name[80];
> > @@ -1792,6 +1901,12 @@ igt_main
> > rendercopy = igt_get_render_copyfunc(devid);
> >
> > vgem_drv = __drm_open_driver(DRIVER_VGEM);
> > +
> > + ahnd = intel_allocator_open_full(fd, 0, 0, 0, INTEL_ALLOCATOR_SIMPLE,
> > + ALLOC_STRATEGY_HIGH_TO_LOW, 0);
>
> You will use allocator always, even on gens < 8 where offsets could be
> altered by relocations. This should look:
>
> ahnd = get_simple_h2l_ahnd(fd, 0);
I agree that here it can be used once.
--
Kamil
> > + put_ahnd(ahnd);
> > + if (ahnd)
> > + intel_bb_track(true);
> > }
> >
> > for (const struct create *c = create; c->name; c++) {
> > @@ -1864,7 +1979,6 @@ igt_main
> > igt_fixture
> > igt_stop_shrink_helper();
> > }
> > -
> > /* Use the entire mappable aperture, force swapping */
> > snprintf(name, sizeof(name), "%s%s-%s",
> > c->name, s->name, "swap");
> > --
> > 2.32.0
> >
More information about the igt-dev
mailing list