[Intel-gfx] [PATCH] igt/gem_request_retire: Provoke context destruction with active VMAs
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Thu Nov 19 09:24:03 PST 2015
Hi,
On 19/11/15 16:58, Chris Wilson wrote:
> On Thu, Nov 19, 2015 at 03:06:05PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>
>> Test designed to trigger the
>> WARN_ON(!list_empty(&ppgtt->base.active_list))
>> in i915_gem_context_clean.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>
> Wouldn't this be a better fit in gem_ctx_somethingorother
>
> gem_ctx/close-active
Can move it, don't really mind either way.
>> ---
>> Some potentially hairy batch buffer building code since I don't have
>> a lot of experience in that area. :)
>> ---
>> tests/.gitignore | 1 +
>> tests/Makefile.sources | 1 +
>> tests/gem_request_retire.c | 279 +++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 281 insertions(+)
>> create mode 100644 tests/gem_request_retire.c
>>
>> diff --git a/tests/.gitignore b/tests/.gitignore
>> index 80af9a718f06..85936ea45c9f 100644
>> --- a/tests/.gitignore
>> +++ b/tests/.gitignore
>> @@ -91,6 +91,7 @@ gem_render_copy
>> gem_render_copy_redux
>> gem_render_linear_blits
>> gem_render_tiled_blits
>> +gem_request_retire
>> gem_reset_stats
>> gem_ring_sync_copy
>> gem_ring_sync_loop
>> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
>> index 8fb2de8b4665..ff178f7a2df4 100644
>> --- a/tests/Makefile.sources
>> +++ b/tests/Makefile.sources
>> @@ -53,6 +53,7 @@ TESTS_progs_M = \
>> gem_reloc_overflow \
>> gem_reloc_vs_gpu \
>> gem_render_copy_redux \
>> + gem_request_retire \
>> gem_reset_stats \
>> gem_ringfill \
>> gem_set_tiling_vs_blt \
>> diff --git a/tests/gem_request_retire.c b/tests/gem_request_retire.c
>> new file mode 100644
>> index 000000000000..710a95f7b6ed
>> --- /dev/null
>> +++ b/tests/gem_request_retire.c
>> @@ -0,0 +1,279 @@
>> +/*
>> + * Copyright © 2015 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the next
>> + * paragraph) shall be included in all copies or substantial portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + *
>> + * Authors:
>> + * Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>> + *
>> + */
>> +
>> +/** @file gem_request_retire
>> + *
>> + * Collection of tests targeting request retirement code paths.
>> + */
>> +
>> +#include "igt.h"
>> +#include <stdlib.h>
>> +#include <stdio.h>
>> +#include <string.h>
>> +#include <fcntl.h>
>> +#include <inttypes.h>
>> +#include <errno.h>
>> +#include <sys/stat.h>
>> +#include <sys/time.h>
>> +#include <sys/mman.h>
>> +#include <signal.h>
>> +#include <pthread.h>
>> +#include <time.h>
>> +
>> +#include "drm.h"
>> +#include "i915_drm.h"
>> +
>> +#include "intel_bufmgr.h"
>> +
>> +#define WIDTH 4096
>> +#define HEIGHT 4096
>> +#define BO_SIZE (WIDTH * HEIGHT * sizeof(uint32_t))
>> +
>> +static uint32_t
>> +blit(int fd, uint32_t dst, uint32_t src, uint32_t ctx_id)
>> +{
>> + const unsigned int copies = 1000;
>> + uint32_t batch[12 * copies + 2];
>> + struct drm_i915_gem_relocation_entry reloc[2 * copies];
>> + struct drm_i915_gem_exec_object2 obj[3];
>> + struct drm_i915_gem_execbuffer2 exec;
>> + uint32_t handle;
>> + int ret;
>> + unsigned int i = 0, j;
>> + unsigned int blit_len = 0;
>> +
>> + for (j = 0; j < copies; j++) {
>> + batch[i++] = XY_SRC_COPY_BLT_CMD |
>> + XY_SRC_COPY_BLT_WRITE_ALPHA |
>> + XY_SRC_COPY_BLT_WRITE_RGB;
>> + if (intel_gen(intel_get_drm_devid(fd)) >= 8)
>> + batch[i - 1] |= 8;
>> + else
>> + batch[i - 1] |= 6;
>> +
>> + batch[i++] = (3 << 24) | /* 32 bits */
>> + (0xcc << 16) | /* copy ROP */
>> + WIDTH*4;
>> + batch[i++] = 0; /* dst x1,y1 */
>> + batch[i++] = (HEIGHT << 16) | WIDTH; /* dst x2,y2 */
>> + batch[i++] = 0; /* dst reloc */
>> + if (intel_gen(intel_get_drm_devid(fd)) >= 8)
>> + batch[i++] = 0;
>> + batch[i++] = 0; /* src x1,y1 */
>> + batch[i++] = WIDTH*4;
>> + batch[i++] = 0; /* src reloc */
>> + if (intel_gen(intel_get_drm_devid(fd)) >= 8)
>> + batch[i++] = 0;
>> +
>> + while (i % 4)
>> + batch[i++] = MI_NOOP;
Is this ok? I arrived at it by experimentation, kernel was complaining
that relocations are not 4-byte aligned without it.
Although now I'm thinking offsets are in bytes and my calculation below
is completely wrong.. but it did not crash the GPU. Hm.
>> +
>> + if (blit_len == 0)
>> + blit_len = i;
>> + }
>> +
>> + batch[i++] = MI_BATCH_BUFFER_END;
>> + batch[i++] = MI_NOOP;
>> +
>> + handle = gem_create(fd, sizeof(batch));
>> + gem_write(fd, handle, 0, batch, sizeof(batch));
>> +
>> + for (j = 0; j < copies; j++) {
>> + unsigned int r0 = j * 2;
>> + unsigned int r1 = r0 + 1;
>> +
>> + reloc[r0].target_handle = dst;
>> + reloc[r0].delta = 0;
>> + reloc[r0].offset = j * blit_len + 4 * sizeof(uint32_t);
>> + reloc[r0].presumed_offset = 0;
>> + reloc[r0].read_domains = I915_GEM_DOMAIN_RENDER;
>> + reloc[r0].write_domain = I915_GEM_DOMAIN_RENDER;
>> +
>> + reloc[r1].target_handle = src;
>> + reloc[r1].delta = 0;
>> + reloc[r1].offset = j * blit_len + 7 * sizeof(uint32_t);
>> + if (intel_gen(intel_get_drm_devid(fd)) >= 8)
>> + reloc[r1].offset += sizeof(uint32_t);
>> + reloc[r1].presumed_offset = 0;
>> + reloc[r1].read_domains = I915_GEM_DOMAIN_RENDER;
>> + reloc[r1].write_domain = 0;
>
> Just fill the relocation array as you generate the batch.
To save one loop? :)
>> + }
>> +
>> + memset(obj, 0, sizeof(obj));
>> + exec.buffer_count = 0;
>> + obj[exec.buffer_count++].handle = dst;
>> + if (src != dst)
>> + obj[exec.buffer_count++].handle = src;
>> + obj[exec.buffer_count].handle = handle;
>> + obj[exec.buffer_count].relocation_count = 2 * copies;
>> + obj[exec.buffer_count].relocs_ptr = (uintptr_t)reloc;
>> + exec.buffer_count++;
>> + exec.buffers_ptr = (uintptr_t)obj;
>> +
>
> memset(&exec, 0, sizeof(exec));
> instead of:
>> + exec.batch_start_offset = 0;
>> + exec.batch_len = i * sizeof(uint32_t);
>> + exec.DR1 = exec.DR4 = 0;
>> + exec.num_cliprects = 0;
>> + exec.cliprects_ptr = 0;
>> + exec.flags = I915_EXEC_BLT;
>> + i915_execbuffer2_set_context_id(exec, ctx_id);
>> + exec.rsvd2 = 0;
>> +
>> + ret = drmIoctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &exec);
>> + igt_assert_eq(ret, 0);
> gem_execbuf(fd, &exec);
Okay, it was copy&paste. :)
>> +
>> + return handle;
>> +}
>> +
>> +static uint32_t
>> +store(int fd, uint32_t dst, uint32_t src, uint32_t ctx_id)
>> +{
>> + uint32_t batch[6];
>> + struct drm_i915_gem_relocation_entry reloc[2];
>> + struct drm_i915_gem_exec_object2 obj[3];
>> + struct drm_i915_gem_execbuffer2 exec;
>> + uint32_t handle;
>> + int ret, i = 0;
>> +
>> + batch[i++] = MI_STORE_DWORD_IMM;
>> + batch[i++] = 0; /* addrhigh */
>> + batch[i++] = 0; /* addrlow */
>> + batch[i++] = 0; /* val */
>> + batch[i++] = MI_BATCH_BUFFER_END;
>> + batch[i++] = MI_BATCH_BUFFER_END;
>> +
>> + handle = gem_create(fd, 4096);
>> + gem_write(fd, handle, 0, batch, sizeof(batch));
>> +
>> + reloc[0].target_handle = dst;
>> + reloc[0].delta = 0;
>> + reloc[0].offset = 2 * sizeof(batch[0]);
>> + reloc[0].presumed_offset = 0;
>> + reloc[0].read_domains = I915_GEM_DOMAIN_RENDER;
>> + reloc[0].write_domain = I915_GEM_DOMAIN_RENDER;
>> +
>> + reloc[1].target_handle = src;
>> + reloc[1].delta = 0;
>> + reloc[1].offset = 3 * sizeof(batch[0]);
>> + reloc[1].presumed_offset = 0;
>> + reloc[1].read_domains = I915_GEM_DOMAIN_RENDER;
>> + reloc[1].write_domain = 0;
>> +
>> + memset(obj, 0, sizeof(obj));
>> + exec.buffer_count = 0;
>> + obj[exec.buffer_count++].handle = dst;
>> + if (src != dst)
>> + obj[exec.buffer_count++].handle = src;
>> + obj[exec.buffer_count].handle = handle;
>> + obj[exec.buffer_count].relocation_count = 2;
>> + obj[exec.buffer_count].relocs_ptr = (uintptr_t)reloc;
>> + exec.buffer_count++;
>> + exec.buffers_ptr = (uintptr_t)obj;
>> +
>> + exec.batch_start_offset = 0;
>> + exec.batch_len = i * sizeof(uint32_t);
>> + exec.DR1 = exec.DR4 = 0;
>> + exec.num_cliprects = 0;
>> + exec.cliprects_ptr = 0;
>> + exec.flags = I915_EXEC_RENDER;
>> + i915_execbuffer2_set_context_id(exec, ctx_id);
>> + exec.rsvd2 = 0;
>> +
>> + ret = drmIoctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &exec);
>> + igt_assert_eq(ret, 0);
>
> For the purposes of your test, you don't even need to execute anything,
> just associate the dst handle with an empty batch on the render ring.
> You don't even need a fake relocation since the test already requires a
> fairly recent kernel.
I thought so but couldn't get it to work. Kept getting EINVAL which is a
pig to find the real reason with execbuf. I'll try some more.
>
>> +
>> + return handle;
>> +}
>> +
>> +/*
>> + * A single bo is operated from batchbuffers submitted from two contexts and on
>> + * different rings.
>> + * One execbuf finishes way ahead of the other at which point the respective
>> + * context is destroyed.
>> + */
>> +static void
>> +test_retire_vma_not_inactive(int fd)
>> +{
>> + uint32_t ctx_id;
>> + uint32_t src, dst, dst2;
>> + uint32_t blit_bb, store_bb;
>> +
>> + igt_require(HAS_BLT_RING(intel_get_drm_devid(fd)));
>> +
>> + ctx_id = gem_context_create(fd);
>> + igt_assert(ctx_id);
>> +
>> + /* Create some bos batch buffers will operate on. */
>> + src = gem_create(fd, BO_SIZE);
>> + igt_assert(src);
>> + dst = gem_create(fd, BO_SIZE);
>> + igt_assert(dst);
>> + dst2 = gem_create(fd, 4096);
>> + igt_assert(dst2);
>> +
>> + /* Submit a long running batch. */
>> + blit_bb = blit(fd, dst, src, 0);
>> + /* Submit a quick batch referencing the same object. */
>> + store_bb = store(fd, dst2, src, ctx_id);
>> + /* Wait for the quick batch to complete. */
>> + gem_sync(fd, store_bb);
>> + gem_sync(fd, dst2);
>> + gem_close(fd, store_bb);
>
>> + /* Do it again to ensure in kernel retirement is triggered. */
> Circumstantial!
Yes I don't think it is actually required but a leftover from development.
Key is that retire work handler runs before the context destruction,
since execlist retire is only runs from there, and it is keeping
references to requests which have been processed long ago.
So I was also thinking - should we call the execlist retire requests
more often in general? Execbuf path calls the normal ring retirement so
maybe add it there?
>> + store_bb = store(fd, dst2, src, ctx_id);
>> + gem_sync(fd, store_bb);
>> + gem_sync(fd, dst2);
>> + gem_close(fd, store_bb);
>> + gem_close(fd, dst2);
>> +
>> + /* Now destroy the context in which the quick batch was submitted. */
>> + gem_context_destroy(fd, ctx_id);
>
> Feels very fragile. Isn't the bug something like
Well my blit runs for 6-7 seconds on a fast SKL so only fragility is
that retire work handler manages to get scheduled during that window. So
I think it is fine.
> batch = big_render_copy(ctx);
> context_close(ctx);
> gem_sync(batch);
I don't think so. It needs two contexts so that object can be active on
two rings and we can destroy one context with another one still active.
Regards,
Tvrtko
More information about the Intel-gfx
mailing list