[igt-dev] [PATCH i-g-t 1/1] tests/i915/gem_ctx_persistence: Set context with supported engines
Chris Wilson
chris at chris-wilson.co.uk
Fri Jan 10 11:03:43 UTC 2020
Quoting Tvrtko Ursulin (2020-01-10 10:56:59)
>
> These were my suggestions so I'll reply. :)
>
> On 09/01/2020 09:18, Chris Wilson wrote:
> > Quoting Bommu Krishnaiah (2020-01-09 08:50:23)
> >> Update the context with supported engines on the platform with set_property
> >> I915_CONTEXT_PARAM_ENGINES to make sure the work load is submitted to
> >> the available engines only.
> >>
> >> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >> Signed-off-by: Bommu Krishnaiah <krishnaiah.bommu at intel.com>
> >> ---
> >> tests/i915/gem_ctx_persistence.c | 34 ++++++++++++++++++--------------
> >> 1 file changed, 19 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/tests/i915/gem_ctx_persistence.c b/tests/i915/gem_ctx_persistence.c
> >> index d68431ae..e410e85f 100644
> >> --- a/tests/i915/gem_ctx_persistence.c
> >> +++ b/tests/i915/gem_ctx_persistence.c
> >> @@ -145,6 +145,15 @@ static void test_clone(int i915)
> >> gem_context_destroy(i915, clone);
> >> }
> >>
> >> +static uint32_t create_context(int i915, bool persistent)
> >> +{
> >> + uint32_t ctx;
> >> +
> >> + ctx = gem_context_create(i915);
> >> + gem_context_set_persistence(i915, ctx, persistent);
> >> + gem_context_set_all_engines(i915, ctx);
> >
> > Really? We only need a single engine, could you not be a little more
> > precise.
>
> We have a general situation where as soon as we call
> __for_each_physical_engine, all code/subtests that follow and which
> create new contexts, which are then submitted mixing the flags from the
> iterated engines, needs to have a matching engine map. This stateful
> behaviour causes problems but it's what we have. I don't have any better
> ideas on how to universally go about this.
>
> >
> >> + return ctx;
> >> +}
> >> static void test_persistence(int i915, unsigned int engine)
> >> {
> >> igt_spin_t *spin;
> >> @@ -156,8 +165,7 @@ static void test_persistence(int i915, unsigned int engine)
> >> * request is retired -- no early termination.
> >> */
> >>
> >> - ctx = gem_context_create(i915);
> >> - gem_context_set_persistence(i915, ctx, true);
> >> + ctx = create_context(i915, true);
> >>
> >> spin = igt_spin_new(i915, ctx,
> >> .engine = engine,
> >> @@ -188,8 +196,7 @@ static void test_nonpersistent_cleanup(int i915, unsigned int engine)
> >> * any inflight request is cancelled.
> >> */
> >>
> >> - ctx = gem_context_create(i915);
> >> - gem_context_set_persistence(i915, ctx, false);
> >> + ctx = create_context(i915, false);
> >>
> >> spin = igt_spin_new(i915, ctx,
> >> .engine = engine,
> >> @@ -217,8 +224,7 @@ static void test_nonpersistent_mixed(int i915, unsigned int engine)
> >> igt_spin_t *spin;
> >> uint32_t ctx;
> >>
> >> - ctx = gem_context_create(i915);
> >> - gem_context_set_persistence(i915, ctx, i & 1);
> >> + ctx = create_context(i915, i & 1);
> >>
> >> spin = igt_spin_new(i915, ctx,
> >> .engine = engine,
> >> @@ -253,8 +259,7 @@ static void test_nonpersistent_hostile(int i915, unsigned int engine)
> >> * the requests and cleanup the context.
> >> */
> >>
> >> - ctx = gem_context_create(i915);
> >> - gem_context_set_persistence(i915, ctx, false);
> >> + ctx = create_context(i915, false);
> >>
> >> spin = igt_spin_new(i915, ctx,
> >> .engine = engine,
> >> @@ -284,8 +289,8 @@ static void test_nonpersistent_hostile_preempt(int i915, unsigned int engine)
> >>
> >> igt_require(gem_scheduler_has_preemption(i915));
> >>
> >> - ctx = gem_context_create(i915);
> >> - gem_context_set_persistence(i915, ctx, true);
> >> + ctx = create_context(i915, true);
> >> +
> >> gem_context_set_priority(i915, ctx, 0);
> >> spin[0] = igt_spin_new(i915, ctx,
> >> .engine = engine,
> >> @@ -385,8 +390,8 @@ static void test_nonpersistent_queued(int i915, unsigned int engine)
> >>
> >> gem_quiescent_gpu(i915);
> >>
> >> - ctx = gem_context_create(i915);
> >> - gem_context_set_persistence(i915, ctx, false);
> >> + ctx = create_context(i915, false);
> >> +
> >> spin = igt_spin_new(i915, ctx,
> >> .engine = engine,
> >> .flags = IGT_SPIN_FENCE_OUT);
> >> @@ -512,8 +517,7 @@ static void test_process_mixed(int i915, unsigned int engine)
> >> igt_spin_t *spin;
> >> uint32_t ctx;
> >>
> >> - ctx = gem_context_create(i915);
> >> - gem_context_set_persistence(i915, ctx, persists);
> >> + ctx = create_context(i915, persists);
> >>
> >> spin = igt_spin_new(i915, ctx,
> >> .engine = engine,
> >> @@ -727,7 +731,7 @@ igt_main
> >> igt_subtest("hangcheck")
> >> test_nohangcheck_hostile(i915);
> >>
> >> - __for_each_static_engine(e) {
> >> + __for_each_physical_engine(i915, e) {
> >
> > Please note this has to check ABI. We have to confirm that no matter how
> > the user tries to fool us, they cannot. So the gem_context_set_engines()
> > should be considered a separate step, or a very, very persuasive
> > argument made as to why we consider the multiple ABI paths equivalent,
> > bearing in mind the security implications of getting it wrong.
>
> So duplicate the subtest loop, first do ABI, then physical?
>
> for_each_engine(..) {
> ...
>
> igt_subtest_f("legacy-%s-persistence")
> test_persistence(..)
> ...
> }
>
> __for_each_physical_engine(..) {
> ...
>
> igt_subtest_f("%s-persistence")
> test_persistence(..)
> ...
> }
>
> Then in create_context helper we need to query the default context (
> gem_context_has_engine_map) to establish if we need to call
> gem_context_set_all_engines or not.
>
> Acceptable?
Along the lines (I don't like gem_context_set_all_engines very much, and
so will keep nagging.). However, talking about engines[] vs persistence
opened up a can of worms, in that we need to keep silly/abusive
engines[] to make sure a hostile user can not circumvent the DoS
prevention. So the task got a bit bigger :-p
-Chris
More information about the igt-dev
mailing list