[PATCH i-g-t 2/4] lib/intel: Unify MI_STORE_DWORD secure batch checks
Ville Syrjälä
ville.syrjala at linux.intel.com
Tue Oct 8 21:27:46 UTC 2024
On Wed, Oct 02, 2024 at 04:43:48PM +0200, Kamil Konieczny wrote:
> Hi Ville,
> On 2024-10-02 at 13:45:13 +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >
> > A bunch of tests use a 'gen<6' check to determine if
> > MI_STORE_DWORD needs a secure batch or not. Other places
> > have a more correct check written in different styles.
> > Unify all of it to use a common helper.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> > lib/igt_dummyload.c | 2 +-
> > lib/igt_gt.c | 20 ++++++++++++++++++++
> > lib/igt_gt.h | 1 +
> > lib/igt_store.c | 2 +-
> > tests/intel/gem_ctx_shared.c | 4 ++--
> > tests/intel/gem_exec_async.c | 2 +-
> > tests/intel/gem_exec_capture.c | 9 +++------
> > tests/intel/gem_exec_fence.c | 4 ++--
> > tests/intel/gem_exec_flush.c | 4 ++--
> > tests/intel/gem_exec_gttfill.c | 2 +-
> > tests/intel/gem_exec_nop.c | 4 ++--
> > tests/intel/gem_exec_parallel.c | 2 +-
> > tests/intel/gem_exec_params.c | 2 +-
> > tests/intel/gem_exec_reloc.c | 8 +++++---
> > tests/intel/gem_exec_schedule.c | 4 ++--
> > tests/intel/gem_exec_store.c | 16 ++++++----------
> > tests/intel/gem_exec_suspend.c | 2 +-
> > tests/intel/gem_exec_whisper.c | 2 +-
> > tests/intel/gem_ringfill.c | 2 +-
> > tests/intel/gem_softpin.c | 4 ++--
> > tests/intel/gem_sync.c | 8 ++++----
> > tests/intel/gem_userptr_blits.c | 4 ++--
> > tests/intel/i915_module_load.c | 2 +-
> > tests/prime_vgem.c | 2 +-
> > 24 files changed, 64 insertions(+), 48 deletions(-)
> >
> > diff --git a/lib/igt_dummyload.c b/lib/igt_dummyload.c
> > index a9a2de077698..3cf80b762921 100644
> > --- a/lib/igt_dummyload.c
> > +++ b/lib/igt_dummyload.c
> > @@ -217,7 +217,7 @@ emit_recursive_batch(igt_spin_t *spin,
> > } else if (opts->flags & IGT_SPIN_POLL_RUN) {
> > igt_assert(!opts->dependency);
> >
> > - if (gen == 4 || gen == 5) {
> > + if (gem_store_dword_needs_secure(fd)) {
> > execbuf->flags |= I915_EXEC_SECURE;
> > igt_require(__igt_device_set_master(fd) == 0);
> > }
> > diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> > index 331783e49acf..a1075e8bbed1 100644
> > --- a/lib/igt_gt.c
> > +++ b/lib/igt_gt.c
> > @@ -691,6 +691,26 @@ bool gem_can_store_dword(int fd, unsigned int engine)
> > gem_execbuf_flags_to_engine_class(engine));
> > }
> >
> > +/**
> > + * gem_store_dword_needs_secure:
> > + * @fd: open i915 drm file descriptor
> > + *
> > + * Does the MI_STORE_DWORD need to be executed from a secure batch?
>
> Imho better write a statement, not a question, so:
>
> * True if the MI_STORE_DWORD needs to be executed from a secure batch
>
> > + */
> > +bool gem_store_dword_needs_secure(int fd)
> > +{
> > + const struct intel_device_info *info =
> > + intel_get_device_info(intel_get_drm_devid(fd));
> > +
> > + switch (info->graphics_ver) {
> > + case 4:
> > + case 5:
> > + return true;
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > const struct intel_execution_engine2 intel_execution_engines2[] = {
> > { "rcs0", I915_ENGINE_CLASS_RENDER, 0, I915_EXEC_RENDER },
> > { "bcs0", I915_ENGINE_CLASS_COPY, 0, I915_EXEC_BLT },
> > diff --git a/lib/igt_gt.h b/lib/igt_gt.h
> > index 4a67cd9f038e..d3213123d6ac 100644
> > --- a/lib/igt_gt.h
> > +++ b/lib/igt_gt.h
> > @@ -76,6 +76,7 @@ unsigned intel_detect_and_clear_missed_interrupts(int fd);
> >
> > bool gem_can_store_dword(int fd, unsigned int engine);
> > bool gem_class_can_store_dword(int fd, int class);
> > +bool gem_store_dword_needs_secure(int fd);
> >
> > extern const struct intel_execution_engine2 {
> > char name[16];
> > diff --git a/lib/igt_store.c b/lib/igt_store.c
> > index 538405e7f594..42ffdc5cdbe4 100644
> > --- a/lib/igt_store.c
> > +++ b/lib/igt_store.c
> > @@ -48,7 +48,7 @@ void igt_store_word(int fd, uint64_t ahnd, const intel_ctx_t *ctx,
> > execbuf.flags |= I915_EXEC_FENCE_IN;
> > execbuf.rsvd2 = fence;
> > }
> > - if (gen < 6)
> > + if (gem_store_dword_needs_secure(fd))
> > execbuf.flags |= I915_EXEC_SECURE;
>
> What about just one function returning this flag or zero?
> So it would be used like:
>
> execbuf.flags |= gem_store_dword_secure_batch(fd);
>
> Even less code.
I suppose that'd work. Thought the name would probably need
more work. gem_store_dword_exec_secure() or maybe even
gem_store_dword_execbuf_flags()...
>
> >
> > memset(obj, 0, sizeof(obj));
> > diff --git a/tests/intel/gem_ctx_shared.c b/tests/intel/gem_ctx_shared.c
> > index de06f2d54876..4b9d604f6732 100644
> > --- a/tests/intel/gem_ctx_shared.c
> > +++ b/tests/intel/gem_ctx_shared.c
> > @@ -366,7 +366,7 @@ static void exec_shared_gtt(int i915, const intel_ctx_cfg_t *cfg,
> > obj.handle = batch;
> > obj.offset += 8192; /* make sure we don't cause an eviction! */
> > execbuf.rsvd1 = ctx[1]->id;
> > - if (gen > 3 && gen < 6)
> > + if (gem_store_dword_needs_secure(i915))
> > execbuf.flags |= I915_EXEC_SECURE;
> > gem_execbuf(i915, &execbuf);
> >
> > @@ -567,7 +567,7 @@ static void store_dword(int i915, uint64_t ahnd, const intel_ctx_t *ctx,
> > execbuf.buffers_ptr = to_user_pointer(obj + !cork);
> > execbuf.buffer_count = 2 + !!cork;
> > execbuf.flags = ring;
> > - if (gen < 6)
> > + if (gem_store_dword_needs_secure(i915))
> > execbuf.flags |= I915_EXEC_SECURE;
> > execbuf.rsvd1 = ctx->id;
> >
> > diff --git a/tests/intel/gem_exec_async.c b/tests/intel/gem_exec_async.c
> > index 573157add648..6d739307cfe6 100644
> > --- a/tests/intel/gem_exec_async.c
> > +++ b/tests/intel/gem_exec_async.c
> > @@ -56,7 +56,7 @@ static void store_dword(int fd, int id, const intel_ctx_t *ctx,
> > execbuf.buffers_ptr = to_user_pointer(obj);
> > execbuf.buffer_count = 2;
> > execbuf.flags = ring;
> > - if (gen < 6)
> > + if (gem_store_dword_needs_secure(fd))
> > execbuf.flags |= I915_EXEC_SECURE;
> > execbuf.rsvd1 = ctx->id;
> >
> > diff --git a/tests/intel/gem_exec_capture.c b/tests/intel/gem_exec_capture.c
> > index 2340ad49520e..37c1f7255046 100644
> > --- a/tests/intel/gem_exec_capture.c
> > +++ b/tests/intel/gem_exec_capture.c
> > @@ -396,7 +396,7 @@ static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
> > execbuf.buffers_ptr = (uintptr_t)obj;
> > execbuf.buffer_count = ARRAY_SIZE(obj);
> > execbuf.flags = e->flags;
> > - if (gen > 3 && gen < 6)
> > + if (gem_store_dword_needs_secure(fd))
> > execbuf.flags |= I915_EXEC_SECURE;
> > execbuf.flags |= I915_EXEC_FENCE_OUT;
> > execbuf.rsvd1 = ctx->id;
> > @@ -586,7 +586,7 @@ __captureN(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
> > execbuf.buffers_ptr = (uintptr_t)obj;
> > execbuf.buffer_count = count + 2;
> > execbuf.flags = e->flags;
> > - if (gen > 3 && gen < 6)
> > + if (gem_store_dword_needs_secure(fd))
> > execbuf.flags |= I915_EXEC_SECURE;
> > execbuf.flags |= I915_EXEC_FENCE_OUT;
> > execbuf.rsvd1 = ctx->id;
> > @@ -968,12 +968,9 @@ igt_main
> > uint32_t region;
> >
> > igt_fixture {
> > - int gen;
> > -
> > fd = drm_open_driver(DRIVER_INTEL);
> >
> > - gen = intel_gen(intel_get_drm_devid(fd));
> > - if (gen > 3 && gen < 6) /* ctg and ilk need secure batches */
> > + if (gem_store_dword_needs_secure(fd))
> > igt_device_set_master(fd);
> >
> > igt_require_gem(fd);
> > diff --git a/tests/intel/gem_exec_fence.c b/tests/intel/gem_exec_fence.c
> > index 7f39c73d72fa..c3c462b77dc4 100644
> > --- a/tests/intel/gem_exec_fence.c
> > +++ b/tests/intel/gem_exec_fence.c
> > @@ -796,7 +796,7 @@ static void test_parallel(int i915, const intel_ctx_t *ctx,
> > batch[++i] = MI_BATCH_BUFFER_END;
> > gem_write(i915, obj[1].handle, 0, batch, sizeof(batch));
> >
> > - if (gen < 6)
> > + if (gem_store_dword_needs_secure(i915))
> > execbuf.flags |= I915_EXEC_SECURE;
> >
> > gem_execbuf(i915, &execbuf);
> > @@ -919,7 +919,7 @@ static void test_concurrent(int i915, const intel_ctx_t *ctx,
> > tmp_ctx = intel_ctx_create(i915, &ctx->cfg);
> > execbuf.rsvd1 = tmp_ctx->id;
> > execbuf.rsvd2 = spin->out_fence;
> > - if (gen < 6)
> > + if (gem_store_dword_needs_secure(i915))
> > execbuf.flags |= I915_EXEC_SECURE;
> >
> > gem_execbuf(i915, &execbuf);
> > diff --git a/tests/intel/gem_exec_flush.c b/tests/intel/gem_exec_flush.c
> > index 795be929c2e2..027510424b06 100644
> > --- a/tests/intel/gem_exec_flush.c
> > +++ b/tests/intel/gem_exec_flush.c
> > @@ -1668,7 +1668,7 @@ static void run(int fd, unsigned ring, int nchild, int timeout,
> > execbuf.buffers_ptr = to_user_pointer(obj);
> > execbuf.buffer_count = 3;
> > execbuf.flags = ring | (1 << 12);
> > - if (gen < 6)
> > + if (gem_store_dword_needs_secure(fd))
> > execbuf.flags |= I915_EXEC_SECURE;
> >
> > obj[1].handle = gem_create(fd, 1024*64);
> > @@ -1915,7 +1915,7 @@ static void batch(int fd, unsigned ring, int nchild, int timeout,
> > execbuf.buffers_ptr = to_user_pointer(obj);
> > execbuf.buffer_count = 2;
> > execbuf.flags = ring | (1 << 12);
> > - if (gen < 6)
> > + if (gem_store_dword_needs_secure(fd))
> > execbuf.flags |= I915_EXEC_SECURE;
> >
> > obj[1].handle = gem_create(fd, 64<<10);
> > diff --git a/tests/intel/gem_exec_gttfill.c b/tests/intel/gem_exec_gttfill.c
> > index 3f05017772af..59a3679fec07 100644
> > --- a/tests/intel/gem_exec_gttfill.c
> > +++ b/tests/intel/gem_exec_gttfill.c
> > @@ -187,7 +187,7 @@ static void fillgtt(int fd, const intel_ctx_t *ctx, unsigned ring, int timeout)
> >
> > memset(&execbuf, 0, sizeof(execbuf));
> > execbuf.buffer_count = 1;
> > - if (gen < 6)
> > + if (gem_store_dword_needs_secure(fd))
> > execbuf.flags |= I915_EXEC_SECURE;
> > execbuf.rsvd1 = ctx->id;
> >
> > diff --git a/tests/intel/gem_exec_nop.c b/tests/intel/gem_exec_nop.c
> > index 1b20cc870187..652f8deffc3d 100644
> > --- a/tests/intel/gem_exec_nop.c
> > +++ b/tests/intel/gem_exec_nop.c
> > @@ -165,7 +165,7 @@ static void poll_ring(int fd, const intel_ctx_t *ctx,
> > uint64_t elapsed;
> >
> > flags = I915_EXEC_NO_RELOC;
> > - if (gen == 4 || gen == 5)
> > + if (gem_store_dword_needs_secure(fd))
> > flags |= I915_EXEC_SECURE;
> >
> > igt_require(gem_class_can_store_dword(fd, e->class));
> > @@ -278,7 +278,7 @@ static void poll_sequential(int fd, const intel_ctx_t *ctx,
> > bool cached;
> >
> > flags = I915_EXEC_NO_RELOC;
> > - if (gen == 4 || gen == 5)
> > + if (gem_store_dword_needs_secure(fd))
> > flags |= I915_EXEC_SECURE;
> >
> > nengine = 0;
> > diff --git a/tests/intel/gem_exec_parallel.c b/tests/intel/gem_exec_parallel.c
> > index 90c46bf0cb17..9817bcd0ff33 100644
> > --- a/tests/intel/gem_exec_parallel.c
> > +++ b/tests/intel/gem_exec_parallel.c
> > @@ -147,7 +147,7 @@ static void *thread(void *data)
> > execbuf.flags = t->engine;
> > execbuf.flags |= I915_EXEC_HANDLE_LUT;
> > execbuf.flags |= I915_EXEC_NO_RELOC;
> > - if (t->gen < 6)
> > + if (gem_store_dword_needs_secure(fd))
> > execbuf.flags |= I915_EXEC_SECURE;
> > if (t->flags & (CONTEXTS | FDS)) {
> > tmp_ctx = intel_ctx_create(fd, &t->ctx->cfg);
> > diff --git a/tests/intel/gem_exec_params.c b/tests/intel/gem_exec_params.c
> > index d0260d5f1ae6..382d4f6dc39d 100644
> > --- a/tests/intel/gem_exec_params.c
> > +++ b/tests/intel/gem_exec_params.c
> > @@ -248,7 +248,7 @@ static void test_batch_first(int fd)
> > memset(&execbuf, 0, sizeof(execbuf));
> > execbuf.buffers_ptr = to_user_pointer(obj);
> > execbuf.buffer_count = ARRAY_SIZE(obj);
> > - if (gen > 3 && gen < 6)
> > + if (gem_store_dword_needs_secure(fd))
> > execbuf.flags |= I915_EXEC_SECURE;
> >
> > /* Normal mode */
> > diff --git a/tests/intel/gem_exec_reloc.c b/tests/intel/gem_exec_reloc.c
> > index 44c09c3e2941..d9afbe23a337 100644
> > --- a/tests/intel/gem_exec_reloc.c
> > +++ b/tests/intel/gem_exec_reloc.c
> > @@ -903,7 +903,7 @@ static void active(int fd, const intel_ctx_t *ctx, unsigned engine)
> > memset(&execbuf, 0, sizeof(execbuf));
> > execbuf.buffers_ptr = to_user_pointer(obj);
> > execbuf.buffer_count = 2;
> > - if (gen < 6)
> > + if (gem_store_dword_needs_secure(fd))
> > execbuf.flags |= I915_EXEC_SECURE;
> > execbuf.rsvd1 = ctx->id;
> >
> > @@ -1310,14 +1310,13 @@ static void concurrent_child(int i915, const intel_ctx_t *ctx,
> > uint32_t *common, int num_common,
> > int in, int out)
> > {
> > - const unsigned int gen = intel_gen(intel_get_drm_devid(i915));
> > int idx = flags_to_index(e);
> > uint64_t relocs = concurrent_relocs(i915, idx, CONCURRENT);
> > struct drm_i915_gem_exec_object2 obj[num_common + 2];
> > struct drm_i915_gem_execbuffer2 execbuf = {
> > .buffers_ptr = to_user_pointer(obj),
> > .buffer_count = ARRAY_SIZE(obj),
> > - .flags = e->flags | I915_EXEC_HANDLE_LUT | (gen < 6 ? I915_EXEC_SECURE : 0),
> > + .flags = e->flags | I915_EXEC_HANDLE_LUT,
> > .rsvd1 = ctx->id,
> > };
> > uint32_t *batch = &obj[num_common + 1].handle;
> > @@ -1325,6 +1324,9 @@ static void concurrent_child(int i915, const intel_ctx_t *ctx,
> > uint32_t *x;
> > int err = 0;
> >
> > + if (gem_store_dword_needs_secure(i915))
> > + execbuf.flags |= I915_EXEC_SECURE;
> > +
> > memset(obj, 0, sizeof(obj));
> > obj[0].handle = gem_create(i915, 64 * CONCURRENT * 4);
> >
> > diff --git a/tests/intel/gem_exec_schedule.c b/tests/intel/gem_exec_schedule.c
> > index fdb7ebd702cd..84219b4cf5a9 100644
> > --- a/tests/intel/gem_exec_schedule.c
> > +++ b/tests/intel/gem_exec_schedule.c
> > @@ -187,7 +187,7 @@ static uint32_t __store_dword(int fd, uint64_t ahnd, const intel_ctx_t *ctx,
> > execbuf.buffers_ptr = to_user_pointer(obj + !cork);
> > execbuf.buffer_count = 2 + !!cork;
> > execbuf.flags = ring;
> > - if (gen < 6)
> > + if (gem_store_dword_needs_secure(fd))
> > execbuf.flags |= I915_EXEC_SECURE;
> > execbuf.rsvd1 = ctx->id;
> >
> > @@ -2342,7 +2342,7 @@ static void reorder_wide(int fd, const intel_ctx_cfg_t *cfg, unsigned ring)
> > execbuf.buffers_ptr = to_user_pointer(obj);
> > execbuf.buffer_count = ARRAY_SIZE(obj);
> > execbuf.flags = ring;
> > - if (gen < 6)
> > + if (gem_store_dword_needs_secure(fd))
> > execbuf.flags |= I915_EXEC_SECURE;
> >
> > execbuf.flags |= I915_EXEC_FENCE_IN;
> > diff --git a/tests/intel/gem_exec_store.c b/tests/intel/gem_exec_store.c
> > index 3b1319519107..d64cdeb6d435 100644
> > --- a/tests/intel/gem_exec_store.c
> > +++ b/tests/intel/gem_exec_store.c
> > @@ -84,7 +84,7 @@ static void store_dword(int fd, const intel_ctx_t *ctx,
> > execbuf.buffers_ptr = to_user_pointer(obj);
> > execbuf.buffer_count = 2;
> > execbuf.flags = e->flags;
> > - if (gen > 3 && gen < 6)
> > + if (gem_store_dword_needs_secure(fd))
> > execbuf.flags |= I915_EXEC_SECURE;
> > execbuf.rsvd1 = ctx->id;
> >
> > @@ -169,7 +169,7 @@ static void store_cachelines(int fd, const intel_ctx_t *ctx,
> > memset(&execbuf, 0, sizeof(execbuf));
> > execbuf.buffer_count = flags & PAGES ? NCACHELINES + 1 : 2;
> > execbuf.flags = e->flags;
> > - if (gen > 3 && gen < 6)
> > + if (gem_store_dword_needs_secure(fd))
> > execbuf.flags |= I915_EXEC_SECURE;
> > execbuf.rsvd1 = ctx->id;
> >
> > @@ -281,7 +281,7 @@ static void store_all(int fd, const intel_ctx_t *ctx)
> > memset(&execbuf, 0, sizeof(execbuf));
> > execbuf.buffers_ptr = to_user_pointer(obj);
> > execbuf.buffer_count = 2;
> > - if (gen < 6)
> > + if (gem_store_dword_needs_secure(fd))
> > execbuf.flags |= I915_EXEC_SECURE;
> > execbuf.rsvd1 = ctx->id;
> >
> > @@ -414,7 +414,7 @@ static void store_all(int fd, const intel_ctx_t *ctx)
> > free(reloc);
> > }
> >
> > -static int print_welcome(int fd)
> > +static void print_welcome(int fd)
> > {
> > uint16_t devid = intel_get_drm_devid(fd);
> > const struct intel_device_info *info = intel_get_device_info(devid);
> > @@ -430,8 +430,6 @@ static int print_welcome(int fd)
> > err = -errno;
> > igt_info("GPU operation? %s [errno=%d]\n",
> > err == 0 ? "yes" : "no", err);
> > -
> > - return info->graphics_ver;
> > }
>
> Why do you changed this? Seems unrelated.
>
> >
> > #define test_each_engine(T, i915, ctx, e) \
> > @@ -446,12 +444,10 @@ igt_main
> > int fd;
> >
> > igt_fixture {
> > - int gen;
> > -
> > fd = drm_open_driver(DRIVER_INTEL);
> >
> > - gen = print_welcome(fd);
>
> Even if you do not need it here it will look cleaner
> in a follow up patch.
The compiler isn't happy about the unused variable. If we
want to split this part it'd have to be a prep patch.
--
Ville Syrjälä
Intel
More information about the igt-dev
mailing list