> > > +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@xxxxxxxxx>
> >
> > Thanks, this patch clarifies a few more things as well,
>
> Thanks!
Thank you,
Andi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
