Hi Janusz, On 2023-02-13 at 10:31:32 +0100, Janusz Krzysztofik wrote: > Users reported oopses on list corruptions when using i915 perf with a > number of concurrently running graphics applications. That indicates we > are currently missing some important tests for such scenarios. Cover > that gap. > > Root cause analysis pointed out to an issue in barrier processing code and > its interaction with perf replacing kernel contexts' active barriers with > its own requests. > > Add a new test intended for exercising intentionally racy barrier tasks > list processing and its interaction with other i915 subsystems. As a > first subtest, add one that exercises the interaction of remote requests > with barrier tasks list handling, especially barrier preallocate / acquire > operations performed during context first pin / last unpin. > > The code is partially inspired by Chris Wilson's igt@perf@open-race > subtest, which I was not able to get an Ack for from upstream. > > v3: don't add the new subtest to gem_ctx_exec which occurred blocklisted, > create a new test hosting the new subtest, update commit descripion, > - prepare parameters for perf open still in the main thread to avoid > test failures on platforms with no perf support (will skip now), > - call perf open with OA buffer reports disabled, this will make sure > that the perf API doesn't unnecessarily enable the OA unit, while the > test still runs the targeted code (Umesh), > - replace additional code for OA exponent calculations with a reasonable > hardcoded value (Umesh). > v2: convert to a separate subtest, not a variant of another one (that has > been dropped from the series), > - move the subtest out of tests/i915/perf.c (Ashutosh), add it to > tests/i915/gem_ctx_exec.c, > - don't touch lib/i915/perf.c (Umesh, Ashutosh), duplicate reused code > from tests/i915/perf.c in tests/i915/gem_ctx_exec.c. > > References: https://gitlab.freedesktop.org/drm/intel/-/issues/6333 > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@xxxxxxxxxxxxxxx> > Cc: Chris Wilson <chris.p.wilson@xxxxxxxxx> > Cc: Kamil Konieczny <kamil.konieczny@xxxxxxxxxxxxxxx> > Cc: Ashutosh Dixit <ashutosh.dixit@xxxxxxxxx> > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@xxxxxxxxx> > --- > tests/i915/gem_barrier_race.c | 159 ++++++++++++++++++++++++++++++++++ > tests/meson.build | 8 ++ > 2 files changed, 167 insertions(+) > create mode 100644 tests/i915/gem_barrier_race.c > > diff --git a/tests/i915/gem_barrier_race.c b/tests/i915/gem_barrier_race.c > new file mode 100644 > index 0000000000..fd0c7bdf1c > --- /dev/null > +++ b/tests/i915/gem_barrier_race.c > @@ -0,0 +1,159 @@ > +// SPDX-License-Identifier: MIT > +/* > + * Copyright(c) 2023 Intel Corporation. All rights reserved. > + */ > + > +#include <stdint.h> > + > +#include "drmtest.h" > +#include "igt_aux.h" > +#include "igt_core.h" > +#include "igt_gt.h" > +#include "intel_chipset.h" > +#include "intel_reg.h" > +#include "ioctl_wrappers.h" > + > +#include "i915/gem.h" > +#include "i915/gem_create.h" > +#include "i915/gem_engine_topology.h" > +#include "i915/perf.h" > + > +IGT_TEST_DESCRIPTION("Exercise barrier tasks list handling and its interation with other i915 subsystems"); ----------------------------------------------- ^^^^^^^^^^^^^ --------------^-------------- ^ s/interation/interaction/ Please make it generic so it will not need to be changed soon, for example: IGT_TEST_DESCRIPTION("Exercise barrier tasks and its interaction with other subsystems"); > + > +/* Based on code patterns found in tests/i915/perf.c */ > +static void perf_open_close_workload(int fd, int *done) > +{ > + struct intel_perf_metric_set *metric_set = NULL, *metric_set_iter; > + struct intel_perf *intel_perf = intel_perf_for_fd(fd); > + uint64_t properties[] = { > + DRM_I915_PERF_PROP_SAMPLE_OA, true, > + DRM_I915_PERF_PROP_OA_METRICS_SET, 0, > + DRM_I915_PERF_PROP_OA_FORMAT, 0, > + DRM_I915_PERF_PROP_OA_EXPONENT, 5, > + }; > + struct drm_i915_perf_open_param param = { > + .flags = I915_PERF_FLAG_FD_CLOEXEC | I915_PERF_FLAG_DISABLED, > + .num_properties = sizeof(properties) / 16, > + .properties_ptr = to_user_pointer(properties), > + }; > + uint32_t devid = intel_get_drm_devid(fd); > + > + igt_require(intel_perf); > + intel_perf_load_perf_configs(intel_perf, fd); > + > + igt_require(devid); > + igt_list_for_each_entry(metric_set_iter, &intel_perf->metric_sets, link) { > + if (!strcmp(metric_set_iter->symbol_name, > + IS_HASWELL(devid) ? "RenderBasic" : "TestOa")) { > + metric_set = metric_set_iter; > + break; > + } > + } > + igt_require(metric_set); > + igt_require(metric_set->perf_oa_metrics_set); > + properties[3] = metric_set->perf_oa_metrics_set; > + properties[5] = metric_set->perf_oa_format; > + > + intel_perf_free(intel_perf); > + > + igt_fork(child, 1) { > + do { > + int stream = igt_ioctl(fd, DRM_IOCTL_I915_PERF_OPEN, ¶m); > + > + igt_assert_fd(stream); > + close(stream); > + > + } while (!READ_ONCE(*done)); > + } > +} > + > +static void remote_request_workload(int fd, int *done) -------------- ^ > +{ > + /* > + * Use DRM_IOCTL_I915_PERF_OPEN / close as > + * intel_context_prepare_remote_request() workload > + */ > + perf_open_close_workload(fd, done); ------- ^ > +} These is just calling one function as another name, imho just rename perf_open_close_workload() into remote_request_workload() > + > +/* Copied from tests/i915/gem_ctx_exec.c */ > +static int exec(int fd, uint32_t handle, int ring, int ctx_id) > +{ > + struct drm_i915_gem_exec_object2 obj = { .handle = handle }; > + struct drm_i915_gem_execbuffer2 execbuf = { > + .buffers_ptr = to_user_pointer(&obj), > + .buffer_count = 1, > + .flags = ring, > + }; > + > + i915_execbuffer2_set_context_id(execbuf, ctx_id); > + > + return __gem_execbuf(fd, &execbuf); > +} > + > +static void gem_create_nop_exec_sync_close_loop(int fd, uint64_t engine, int *done) > +{ > + const uint32_t batch[2] = { 0, MI_BATCH_BUFFER_END }; > + > + fd = gem_reopen_driver(fd); > + > + do { > + uint32_t handle = gem_create(fd, 4096); > + > + gem_write(fd, handle, 0, batch, sizeof(batch)); > + igt_assert_eq(exec(fd, handle, engine, 0), 0); > + > + gem_sync(fd, handle); > + gem_close(fd, handle); > + > + } while (!READ_ONCE(*done)); > +} > + > +static void intel_context_first_pin_last_unpin_loop(int fd, uint64_t engine, int *done) -------------- ^ > +{ > + /* > + * Use gem_create -> gem_write -> gem_execbuf -> gem_sync -> gem_close > + * as intel context first pin / last unpin intensive workload > + */ > + gem_create_nop_exec_sync_close_loop(fd, engine, done); ------- ^ > +} Same here, just rename original function, maybe make it a little shorter and put longer explanations in comments. > + > +static void test_remote_request(int fd, uint64_t engine, unsigned int timeout) > +{ > + int *done = mmap(0, 4096, PROT_WRITE, MAP_SHARED | MAP_ANON, -1, 0); > + > + igt_assert(done != MAP_FAILED); > + > + remote_request_workload(fd, done); > + > + igt_fork(child, sysconf(_SC_NPROCESSORS_ONLN)) > + intel_context_first_pin_last_unpin_loop(fd, engine, done); > + > + sleep(timeout); > + *done = 1; > + igt_waitchildren(); > + munmap(done, 4096); > +} > + > +igt_main > +{ > + int fd; > + > + igt_fixture { > + fd = drm_open_driver_render(DRIVER_INTEL); > + igt_require_gem(fd); > + } > + > + igt_describe("Race intel_context_prepare_remote_request against intel_context_active_acquire/release"); > + igt_subtest_with_dynamic("remote-request") { > + struct intel_execution_engine2 *e; > + > + for_each_physical_engine(fd, e) { > + if (e->class != I915_ENGINE_CLASS_RENDER) > + continue; > + > + igt_dynamic(e->name) > + test_remote_request(fd, e->flags, 5); Do we need all physical engines to be tested ? Regards, Kamil > + } > + } > +} > diff --git a/tests/meson.build b/tests/meson.build > index 6fb1bb86c9..5670712ae8 100644 > --- a/tests/meson.build > +++ b/tests/meson.build > @@ -389,6 +389,14 @@ test_executables += executable('i915_pm_rc6_residency', > install : true) > test_list += 'i915_pm_rc6_residency' > > +test_executables += executable('gem_barrier_race', > + join_paths('i915', 'gem_barrier_race.c'), > + dependencies : test_deps + [ lib_igt_i915_perf ], > + install_dir : libexecdir, > + install_rpath : libexecdir_rpathdir, > + install : true) > +test_list += 'gem_barrier_race' > + > test_executables += executable('perf_pmu', > join_paths('i915', 'perf_pmu.c'), > dependencies : test_deps + [ lib_igt_perf ], > -- > 2.25.1 >
