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

[Kamil Konieczny]

Hi Ville,
On 2024-10-10 at 21:49:55 +0300, Ville Syrjälä wrote:
> 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 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()...
> 
> 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.

I agree, this is not a blocker, with a fix in description

Reviewed-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>


> 
> -- 
> Ville Syrjälä
> Intel