Re: [igt-dev] [PATCH i-g-t v2] tests/i915/gem_ctx_switch: Update with engine discovery

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

> > > +gem_eb_flags_to_engine(unsigned int flags)
> > > +{
> > > +	const struct intel_execution_engine2 *e2;
> > > +
> > > +	__for_each_static_engine(e2) {
> > > +		if (e2->flags == flags)
> > > +			return e2;
> > > +	}
> > > +
> > > +	return NULL;
> > > +}
> > 
> > the amount of "helpers" is getting almost unbearable for a simple
> > mind like mine.
> > 
> > This means that we can get rid of
> > 
> >   gem_execbuf_flags_to_engine_class
> >   gem_ring_is_physical_engine
> >   gem_ring_has_physical_engine
> > 
> > in igt_gt.c, right?
> 
> If/when there are no callers left we can like everything. I dont' know if
> that is the case right now.

No, not yet, but this replaces the logical meaning of the
function above... but... there is still lots of legacy involved :(

> > > +	return param.size;
> > 
> > a small nitpick: bool to me means '0' or '1'.
> > 
> > So, if you do 'return param.size', I would call the function
> > gem_context_get_param_size, otherwise I would return
> > '!!param.size' and keep it bool.
> 
> Some people would then complain !! was not needed. ~o~
> 
> And actually I think I want to remove the distinction of no map and map with
> no engines for the callers of this helpers. So returning size would not work
> for that.
> 
> > (We are also somehow abusing on the size definition of bool in
> > C99/C17 or previous.)
> > 
> > I'm not asking you to change it, but it would make me happier :)
> 
> I don't understand the issue with size definition. Size is u32 - will not
> implicit conversion to bool just work?

This is fine, it's just some philosophical thinking that for me
bool should be true false.

-----------------
(If we want to be purists, this would rather be

	return param.size ? true : false;

which basically changes nothing, but sticks to the meaining of
"bool" and it would be to much anyway)

> > > -		q = ARRAY_SIZE(ctx) * timeout * 1e9 / igt_nsec_elapsed(&tv) / 8 + 1;
> > > +		q = ARRAY_SIZE(ctx) * timeout * 1e9 / igt_nsec_elapsed(&tv) /
> > > +		    8 + 1;
> > 
> > I don't know whether it's me who is paranoic, but the change
> > above doesn't match the commit log.
> 
> What do you mean:
> 
> """
> Also beware of drive-by formatting changes.
> """
> 
> ;)
> 
> File to many lines falling of 80 char so I tidied it alongside.

I'm not saying that this change is wrong, just that it's
out of the context of the patch and it should lay in a different
change (I'm not very strong in this case, though, but I've seen
such cases too many times in this list).

> > The rest of the patch I trust you it works :)
> > (however the devotion to whatever is legacy doesn't leave me with
> > a good taste after all the work done)
> > 
> > You can add my Reviewed-by: Andi Shyti <andi.shyti@intel.com>
> > 
> > Thanks, this patch clarifies a few more things as well,
> 
> Thanks!

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