Re: [PATCH i-g-t 2/4] lib/intel: Unify MI_STORE_DWORD secure batch checks

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Oct 09, 2024 at 12:27:46AM +0300, Ville Syrjälä wrote:
> 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@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@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()...

Actually it might be a bit more awkward for the cases where we
do other stuff based on the result. So I think we should just go
with the straightforward conversion for now, and maybe think more
about further simplifications afterwards.

-- 
Ville Syrjälä
Intel