Re: [igt-dev] [PATCH v16 6/8] lib: igt_gt: make gem_engine_can_store_dword() check engine class

[Andi Shyti <andi.shyti@intel.com>]

> > +bool gem_can_store_dword(int fd, uint64_t engine)
> 
> Yeah eb->flags is u64, although we don't need it all here. Okay, I don't mind.

here flags (u64), class (u16) and engine (u8 is enough) are
somehow confusingly mixed... need to check better the type range
I need.

> > +bool gem_engine_can_store_dword(int fd, const struct intel_execution_engine2 *e)
> > +{
> > +	if (!gem_has_engine_topology(fd))
> > +		return gem_can_store_dword(fd, e->flags);
> > +
> > +	return gem_class_can_store_dword(fd, e->class);
> > +}
> 
> Couldn't you always use class?

How do you distinguish between eb flags as engine and context
mapping, then?

In theory this function should not work in all the cases, because
it assumes that e->flags has always a meningful value.

While if this is called during a __for_each_static_engine (which
should never be used in my dream world), flags is always '0'. In
the next patches I tried to be careful to this case (_tried_, eh?  :) )

(BTW, this is a proposal, because the legacy usage of
intel_execution_engines2 list is used a lot)

> bool gem_can_store_dword(int fd, uint64_t engine)
> {
> 	u16 class = eb_engine_to_class(engine);

Yes, I need this extra step, assuming that engine is always an
eb_flag.

In the last patchset you recommended the following:

  // ioctl and stuff.. bummer... :(
  ret = gem_context_get_engine_map_class_instance(fd, opts->ctx, opts->engine,
  &class, &instance);     
  if (ret) // error = no map = means opts->engine is eb flags
          check legacy gem_can_store_dword
  else
          check using new gem_engine_can_store_dword(class, instance)

which I did here (still it works only out from
__for_each_static_engine).

Andi
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev