[igt-dev] [PATCH i-g-t v3 2/2] i915/gem_ctx_isolation:: change semantics of ctx isolation uAPI
Adrian Larumbe
adrian.larumbe at collabora.com
Thu Sep 1 16:46:45 UTC 2022
On 01.09.2022 15:33, Petri Latvala wrote:
>>>On Fri, Aug 19, 2022 at 08:52:18PM +0100, Adrian Larumbe wrote:
>>>> ioctl I915_PARAM_HAS_CONTEXT_ISOLATION param is meant to report whether all
>>>> the engines in the system support context isolation, but the way the return
>>>> value was being used did not respect the contract on the uAPI.
>>>>
>>>> Skip all engine tests for which context isolation is not supported. Also skip
>>>> tests that involve an engine reset if both RCS and CCS engines are present in
>>>> the system. This is because they belong to the same reset domain.
>>>>
>>>> Signed-Off-By: Adrian Larumbe <adrian.larumbe at collabora.com>
>>>> ---
>>>> include/drm-uapi/i915_drm.h | 6 +-
>>>> tests/i915/gem_ctx_isolation.c | 100 +++++++++++++++++++++++----------
>>>> 2 files changed, 72 insertions(+), 34 deletions(-)
>>>>
>>>> diff --git a/include/drm-uapi/i915_drm.h b/include/drm-uapi/i915_drm.h
>>>> index b4efc96c2edc..0b8fa57b52f9 100644
>>>> --- a/include/drm-uapi/i915_drm.h
>>>> +++ b/include/drm-uapi/i915_drm.h
>>>> @@ -691,11 +691,7 @@ typedef struct drm_i915_irq_wait {
>>>> * rather than default HW values. If true, it also ensures (insofar as HW
>>>> * supports) that all state set by this context will not leak to any other
>>>> * context.
>>>> - *
>>>> - * As not every engine across every gen support contexts, the returned
>>>> - * value reports the support of context isolation for individual engines by
>>>> - * returning a bitmask of each engine class set to true if that class supports
>>>> - * isolation.
>>>> + *
>>>
>>>Don't make edits to the drm-uapi files. They need to be direct copies from the kernel.
The issue here is the parallel kernel patch also makes this change. I guess I'll
wait until the relevant kernel patch is accepted and then copy it verbatim from
the kernel sources instead.
>>--
>>Petri Latvala
>>
>>
>>
>>> */
>>> #define I915_PARAM_HAS_CONTEXT_ISOLATION 50
>>>
>>> diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c
>>> index 4233ea5784dc..273a0c9708bb 100644
>>> --- a/tests/i915/gem_ctx_isolation.c
>>> +++ b/tests/i915/gem_ctx_isolation.c
>>> @@ -45,6 +45,7 @@ enum {
>>> VCS2 = ENGINE(I915_ENGINE_CLASS_VIDEO, 2),
>>> VCS3 = ENGINE(I915_ENGINE_CLASS_VIDEO, 3),
>>> VECS0 = ENGINE(I915_ENGINE_CLASS_VIDEO_ENHANCE, 0),
>>> + CCS0 = ENGINE(I915_ENGINE_CLASS_COMPUTE, 0),
>>> };
>>>
>>> #define ALL ~0u
>>> @@ -627,9 +628,40 @@ static void compare_regs(int fd, const struct intel_execution_engine2 *e,
>>> num_errors, who);
>>> }
>>>
>>> +static bool
>>> +engine_has_context_isolation(const struct intel_execution_engine2 *e,
>>> + int gen)
>>> +{
>>> + if (gen > 8)
>>> + return true;
>>> +
>>> + if (gen >= 6 && gen <= 8 && e->class == I915_ENGINE_CLASS_RENDER)
>>> + return true;
>>> +
>>> + return false;
>>> +}
>>> +
>>> +static bool
>>> +has_engine_class(const intel_ctx_cfg_t *cfg, unsigned int class)
>>> +{
>>> + const struct i915_engine_class_instance *eci;
>>> + unsigned int i;
>>> +
>>> + igt_require(class <= I915_ENGINE_CLASS_COMPUTE);
>>> +
>>> + for (i = 0; i < cfg->num_engines; i++) {
>>> + eci = &cfg->engines[i];
>>> + if (eci->engine_class == class)
>>> + return true;
>>> + }
>>> +
>>> + return false;
>>> +}
>>> +
>>> static void nonpriv(int fd, const intel_ctx_cfg_t *cfg,
>>> const struct intel_execution_engine2 *e,
>>> - unsigned int flags)
>>> + unsigned int flags,
>>> + int gen)
>>> {
>>> static const uint32_t values[] = {
>>> 0x0,
>>> @@ -647,6 +679,7 @@ static void nonpriv(int fd, const intel_ctx_cfg_t *cfg,
>>>
>>> /* Sigh -- hsw: we need cmdparser access to our own registers! */
>>> igt_skip_on(intel_gen(intel_get_drm_devid(fd)) < 8);
>>> + igt_require(engine_has_context_isolation(e, gen));
>>>
>>> gem_quiescent_gpu(fd);
>>>
>>> @@ -729,7 +762,8 @@ static void nonpriv(int fd, const intel_ctx_cfg_t *cfg,
>>>
>>> static void isolation(int fd, const intel_ctx_cfg_t *cfg,
>>> const struct intel_execution_engine2 *e,
>>> - unsigned int flags)
>>> + unsigned int flags,
>>> + int gen)
>>> {
>>> static const uint32_t values[] = {
>>> 0x0,
>>> @@ -743,6 +777,8 @@ static void isolation(int fd, const intel_ctx_cfg_t *cfg,
>>> unsigned int num_values =
>>> flags & (DIRTY1 | DIRTY2) ? ARRAY_SIZE(values) : 1;
>>>
>>> + igt_require(engine_has_context_isolation(e, gen));
>>> +
>>> gem_quiescent_gpu(fd);
>>>
>>> for (int v = 0; v < num_values; v++) {
>>> @@ -865,7 +901,8 @@ static void inject_reset_context(int fd, const intel_ctx_cfg_t *cfg,
>>>
>>> static void preservation(int fd, const intel_ctx_cfg_t *cfg,
>>> const struct intel_execution_engine2 *e,
>>> - unsigned int flags)
>>> + unsigned int flags,
>>> + int gen)
>>> {
>>> static const uint32_t values[] = {
>>> 0x0,
>>> @@ -882,6 +919,8 @@ static void preservation(int fd, const intel_ctx_cfg_t *cfg,
>>> uint64_t ahnd[num_values + 1];
>>> igt_spin_t *spin;
>>>
>>> + igt_require(engine_has_context_isolation(e, gen));
>>> +
>>> gem_quiescent_gpu(fd);
>>>
>>> ctx[num_values] = intel_ctx_create(fd, cfg);
>>> @@ -902,8 +941,12 @@ static void preservation(int fd, const intel_ctx_cfg_t *cfg,
>>> gem_close(fd, read_regs(fd, ahnd[num_values], ctx[num_values], e, flags));
>>> igt_spin_free(fd, spin);
>>>
>>> - if (flags & RESET)
>>> + if (flags & RESET) {
>>> + igt_skip_on(has_engine_class(cfg, I915_ENGINE_CLASS_RENDER) &&
>>> + has_engine_class(cfg, I915_ENGINE_CLASS_COMPUTE));
>>> +
>>> inject_reset_context(fd, cfg, e);
>>> + }
>>>
>>> switch (flags & SLEEP_MASK) {
>>> case NOSLEEP:
>>> @@ -962,7 +1005,7 @@ static unsigned int __has_context_isolation(int fd)
>>> int value = 0;
>>>
>>> memset(&gp, 0, sizeof(gp));
>>> - gp.param = 50; /* I915_PARAM_HAS_CONTEXT_ISOLATION */
>>> + gp.param = I915_PARAM_HAS_CONTEXT_ISOLATION;
>>> gp.value = &value;
>>>
>>> igt_ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
>>> @@ -971,21 +1014,19 @@ static unsigned int __has_context_isolation(int fd)
>>> return value;
>>> }
>>>
>>> -#define test_each_engine(e, i915, cfg, mask) \
>>> +#define test_each_engine(e, i915, cfg) \
>>> for_each_ctx_cfg_engine(i915, cfg, e) \
>>> - for_each_if(mask & (1 << (e)->class)) \
>>> - igt_dynamic_f("%s", (e)->name)
>>> + igt_dynamic_f("%s", (e)->name)
>>>
>>> igt_main
>>> {
>>> - unsigned int has_context_isolation = 0;
>>> + bool has_context_isolation = 0;
>>> const struct intel_execution_engine2 *e;
>>> intel_ctx_cfg_t cfg;
>>> int i915 = -1;
>>> + int gen;
>>>
>>> igt_fixture {
>>> - int gen;
>>> -
>>> i915 = drm_open_driver(DRIVER_INTEL);
>>> igt_require_gem(i915);
>>> igt_require(gem_has_contexts(i915));
>>> @@ -995,6 +1036,7 @@ igt_main
>>> igt_require(has_context_isolation);
>>>
>>> gen = intel_gen(intel_get_drm_devid(i915));
>>> + igt_require(gen >= 2);
>>>
>>> igt_warn_on_f(gen > LAST_KNOWN_GEN,
>>> "GEN not recognized! Test needs to be updated to run.\n");
>>> @@ -1006,43 +1048,43 @@ igt_main
>>> }
>>>
>>> igt_subtest_with_dynamic("nonpriv") {
>>> - test_each_engine(e, i915, &cfg, has_context_isolation)
>>> - nonpriv(i915, &cfg, e, 0);
>>> + test_each_engine(e, i915, &cfg)
>>> + nonpriv(i915, &cfg, e, 0, gen);
>>> }
>>>
>>> igt_subtest_with_dynamic("nonpriv-switch") {
>>> - test_each_engine(e, i915, &cfg, has_context_isolation)
>>> - nonpriv(i915, &cfg, e, DIRTY2);
>>> + test_each_engine(e, i915, &cfg)
>>> + nonpriv(i915, &cfg, e, DIRTY2, gen);
>>> }
>>>
>>> igt_subtest_with_dynamic("clean") {
>>> - test_each_engine(e, i915, &cfg, has_context_isolation)
>>> - isolation(i915, &cfg, e, 0);
>>> + test_each_engine(e, i915, &cfg)
>>> + isolation(i915, &cfg, e, 0, gen);
>>> }
>>>
>>> igt_subtest_with_dynamic("dirty-create") {
>>> - test_each_engine(e, i915, &cfg, has_context_isolation)
>>> - isolation(i915, &cfg, e, DIRTY1);
>>> + test_each_engine(e, i915, &cfg)
>>> + isolation(i915, &cfg, e, DIRTY1, gen);
>>> }
>>>
>>> igt_subtest_with_dynamic("dirty-switch") {
>>> - test_each_engine(e, i915, &cfg, has_context_isolation)
>>> - isolation(i915, &cfg, e, DIRTY2);
>>> + test_each_engine(e, i915, &cfg)
>>> + isolation(i915, &cfg, e, DIRTY2, gen);
>>> }
>>>
>>> igt_subtest_with_dynamic("preservation") {
>>> - test_each_engine(e, i915, &cfg, has_context_isolation)
>>> - preservation(i915, &cfg, e, 0);
>>> + test_each_engine(e, i915, &cfg)
>>> + preservation(i915, &cfg, e, 0, gen);
>>> }
>>>
>>> igt_subtest_with_dynamic("preservation-S3") {
>>> - test_each_engine(e, i915, &cfg, has_context_isolation)
>>> - preservation(i915, &cfg, e, S3);
>>> + test_each_engine(e, i915, &cfg)
>>> + preservation(i915, &cfg, e, S3, gen);
>>> }
>>>
>>> igt_subtest_with_dynamic("preservation-S4") {
>>> - test_each_engine(e, i915, &cfg, has_context_isolation)
>>> - preservation(i915, &cfg, e, S4);
>>> + test_each_engine(e, i915, &cfg)
>>> + preservation(i915, &cfg, e, S4, gen);
>>> }
>>>
>>> igt_fixture {
>>> @@ -1052,8 +1094,8 @@ igt_main
>>> igt_subtest_with_dynamic("preservation-reset") {
>>> igt_hang_t hang = igt_allow_hang(i915, 0, 0);
>>>
>>> - test_each_engine(e, i915, &cfg, has_context_isolation)
>>> - preservation(i915, &cfg, e, RESET);
>>> + test_each_engine(e, i915, &cfg)
>>> + preservation(i915, &cfg, e, RESET, gen);
>>>
>>> igt_disallow_hang(i915, hang);
>>> }
>>> --
>>> 2.37.0
>>>
More information about the igt-dev
mailing list