On 15/09/2021 17:58, Matthew Brost wrote:
On Wed, Sep 15, 2021 at 09:24:15AM +0100, Tvrtko Ursulin wrote:
On 14/09/2021 19:04, Matthew Brost wrote:
On Tue, Sep 14, 2021 at 09:34:08AM +0100, Tvrtko Ursulin wrote:
8<
Today we have:
for_each intel_engines: // intel_engines is a flat list of all engines
intel_engine_setup()
You propose to change it to:
for_each engine_class:
for 0..max_global_engine_instance:
for_each intel_engines:
skip engine not present
skip class not matching
count logical instance
for_each intel_engines:
skip engine not present
skip wrong class
intel_engine_setup()
I propose:
// Leave as is:
for_each intel_engines:
intel_engine_setup()
// Add:
for_each engine_class:
logical = 0
for_each gt->engine_class[class]:
skip engine not present
engine->logical_instance = logical++
When code which actually needs a preturbed "map" arrives you add that in to
this second loop.
See above, why introduce an algorithm that doesn't work for future parts
+ future patches are land imminently? It makes zero sense whatsoever.
With your proposal we would literally land code to just throw it away a
couple of months from now + break patches we intend to land soon. This
It sure works, it just walks the per class list instead of walking the flat
list skipping one class at the time.
Just add the map based transformation to the second pass later, when it
becomes required.
I can flatten the algorithm if that helps alleviate your concerns but
with that being said, I've played around this locally and IMO makes the
code way more ugly. Sure it eliminates some iterations of the loop but
who really cares about that in a one time setup function?
algorithm works and has no reason whatsoever to be optimal as it a one
time setup call. I really don't understand why we are still talking
about this paint color.
I don't think bike shedding is not an appropriate term when complaint is how
proposed algorithm is needlessly complicated.
Are you just ignoring the fact that the algorithm (map) is needed in
pending patches? IMO it is more complicated to write throw away code
when the proper algorithm is already written. If the logical mapping was
straight forward on all platforms as the ones currently upstream I would
100% agree with your suggestion, but it isn't on unembargoed platforms
eminently going upstream. The algorithm I have works for the current
platforms + the pending platforms. IMO is 100% acceptable to merge
something looking towards a known future.
FWIW my 2c is that unused bits detract from review. And my gut feeling
still is that code can be written in a simpler way and that the map
transform can still plug in easily on top in a later series.
I said FWIW since even if I am right you can still view my comments as
external/community inputs at this point and proceed however you wish.
Regards,
Tvrtko
