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