Re: [Intel-gfx] [PATCH 08/27] drm/i915: Add logical engine mapping

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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