[Intel-gfx] [PATCH v2 0/7] Make GEN macros more similar

[Tvrtko Ursulin]

On 21/11/2018 22:19, Rodrigo Vivi wrote:
> On Mon, Nov 19, 2018 at 02:20:55PM -0800, Lucas De Marchi wrote:
>> On Thu, Nov 08, 2018 at 11:23:46AM +0000, Tvrtko Ursulin wrote:
>>>
>>> On 08/11/2018 00:57, Lucas De Marchi wrote:
>>>> On Wed, Nov 07, 2018 at 10:05:19AM +0000, Tvrtko Ursulin wrote:
>>>>>
>>>>> On 06/11/2018 21:51, Lucas De Marchi wrote:
>>>>>> This is the second version of the series trying to make GEN checks
>>>>>> more similar. These or roughly the changes from v1:
>>>>>>
>>>>>> - We don't have a single macro receiving 2 or 3 parameters. Now there
>>>>>>      is GT_GEN_RANGE(), and GT_GEN(). The firs is the conversion from
>>>>>>      IS_GEN() while the second is the conversion from IS_GEN<N>()
>>>>>> - Bring GEN_FOREVER back to be used with above macros
>>>>>> - Patch converting <, <=, ==, >, >=  checks using INTEL_GEN() to
>>>>>>      use the macros above was added
>>>>>> - INTEL_GEN() is removed to avoid it being used when we should prefer
>>>>>>      the new macros
>>>>>>
>>>>>> The idea of the names is to pave the way for checks of the display version,
>>>>>> which would be named DISPLAY_GEN(), DISPLAY_GEN_RANGE().
>>>>>>
>>>>>> In the commit messages we have the scripts to regenerate the patch to make
>>>>>> it easier to apply after the discussions and also to be able to convert
>>>>>> inflight patches.
>>>>>>
>>>>>> Sorry in advance for the noise this causes in the codebase, but hopefully
>>>>>> it is for the greater good.
>>>>>>
>>>>>>
>>>>>> Lucas De Marchi (6):
>>>>>>      Revert "drm/i915: Kill GEN_FOREVER"
>>>>>>      drm/i915: replace IS_GEN<N> with GT_GEN(..., N)
>>>>>>      drm/i915: rename IS_GEN9_* to GT_GEN9_*
>>>>>>      drm/i915: replace gen checks using operators by GT_GEN/GT_GEN_RANGE
>>>>>
>>>>> I have reservations about this patch, where I think forcing only one flavour
>>>>> maybe is not the best thing. Because for plain if-ladder cases it feels more
>>>>> readable to stick with the current scheme of arithmetic comparisons. And it
>>>>> is more efficient in code as well.
>>>>>
>>>>> Range checks are on the other hand useful either when combined in the same
>>>>> conditional as some other bitmask based test, or when both ends of the
>>>>> comparison edge are bound.
>>>>
>>>> So are you against changing the == to use the macros, changing the >=, >, <, <=,
>>>> or all of them?
>>>
>>> Definitely not all of them. Just plain if ladders I think are definitely
>>> more readable in source and result in better code in the normal fashion of:
>>>
>>>     if (gen >= 11)
>>>     else if (gen >= 9)
>>>     else if (gen >= 7)
>>>     ... etc ...
>>>
>>> Where I think it makes sense is when either both edges are bound, like:
>>>
>>>    if (gen < 11 || gen >= 8)
>>>    if (gen >= 10 || gen == 8)
>>
>> ok, I will take a look before respinning this.
>>
>>>
>>> But not sure how many of those there are.
>>>
>>> What definitely exists in large-ish numbers are:
> 
> specially on display side...
> 
>>>
>>>     if (gen >= 11 ||  IS_PLATFORM)
> 
> My goal is exactly to organize the gen numbers in a way that
> we minimize this mix as much as possible.
> 
>>>
>>> At some point I had a prototype which puts the gen and platform masks into a
>>> bag of bits and then, depending on bits locality, this too can be compressed
>>> to a single bitmask test. However I felt that was going too far, and the
>>> issue is achieving interesting bits locality for it too work. But I digress.
>>>
>>>> Looking at the changes to ==, they seem very reasonable and in a few cases it allowed
>>>> the next patch to merge them in a GT_GEN_RANGE() -- yes the patch ordering was on
>>>> purpose to allow that.
>>>
>>> Yep those are the good ones.
>>>
>>>> The others are indeed debatable. However IMO for the cases it makes sense,
>>>> there's always the fallback
>>>>
>>>> if (dev_priv->info.gen == 10)
>>>>       ...
>>>> else if (dev_priv->info.gen == 11)
>>>>       ...
>>>> else if (dev_priv->info.gen < 5)
>>>>       ...
>>>>
>>>> We can go on a case by case manner in this patch rather than the mass conversion
>>>> for these.
>>>>
>>>>>
>>>>>>      drm/i915: merge gen checks to use range
>>>>>>      drm/i915: remove INTEL_GEN macro
>>>>>
>>>>> I have reservations about this as as well, especially considering the
>>>>> previous paragraph. But even on it's own I am not sure we want to go back to
>>>>> more verbose.
>>>>
>>>> see above. IMO it's not actually so verbose as one would think.
>>>>
>>>>       if (INTEL_GEN(dev_priv) == 11)
>>>>       vs
>>>>       if (dev_priv->info.gen == 11)
>>>
>>> I think it should be:
>>>
>>>         if (INTEL_INFO(dev_priv)->gen == 11)
>>>
>>> Which is a tiny bit longer..
>>
>> If it's longer, why bother? We could just as well do for the if ladders:
>>
>> gen = dev_priv->info.gen;
>> or
>> gen = INTEL_INFO(dev_priv)->gen
>>
>> In your other series you would be moving gen to a runtime info, so this
>> would cause the same amount of churn (although I disagree with moving gen to a runtime
>> info just because of the mock struct).
>>
>>
>>>
>>>> The "verbose" version is actually one character less and one lookup less
>>>> to what the macro is doing underneath. Of course that means a lot of churn
>>>> to the codebase when/if we change where the gen number is located, but that
>>>> number is at the same place since its introduction in 2010
>>>> (commit c96c3a8cb7fadcb33d9a5ebe35fcee8b7d0a7946)
>>>
>>> I am pretty sure we went through patches to a) move towards INTEL_INFO and
>>> b) replace INTEL_INFO(dev_priv)->gen with INTEL_GEN. So I don't see an
>>> advantage of reverting that, just so that we can lose the IS_ prefix below.
>>>
>>>>> Perhaps in the new scheme of things it should be renamed to INTEL_GT_GEN? I
>>>>> know it doesn't fit nicely with the naming scheme of GT/DISPLAY_GEN.. so
>>>>> maybe:
>>>>>
>>>>> GT_GEN -> IS_GT_GEN
>>>>> GT_GEN_RANGE -> IS_GT_GEN_RANGE
>>>>> INTEL_GEN -> GT_GEN (but churn!?)
>>>>
>>>> I still think INTEL_GEN() is not bringing much clarity and forcing
>>>> the other macros to have the IS_ prefix.
>>>
>>> Is the IS_ prefix that bad?
> 
> I personally don't like it... but maybe it is just my bad english?!
> 
> 1. if gen 9
> 2. if is gen 9
> 3. if this is a gen 9 platform
> 
> I like more the option 1...
> 
>>>
>>> I agree my idea does not decrease the amount of churn, since it said to
>>> replace INTEL_GEN with INTEL_GT_GEN. But in the light of the GT/DISPLAY
>>> split, and if we agree to leave a lot of the arithmetic comparison in
>>> (dialing down of "drm/i915: replace gen checks using operators by
>>> GT_GEN/GT_GEN_RANGE"), then it feels going back to INTEL_INFO(dev_priv)->gen
>>> throughout the code is undoing some work, just so you can remove the
>>> INTEL_GEN macro instead of renaming it INTEL_GT_GEN.
>>>
>>> Don't know, it's my opinion at least and more people are welcome to chime in
>>> with theirs.
>>
>> Any others to chime in on this? Jani, Ville, Rodrigo?
> 
> I don't like mixed styles much. If we don't kill the macro we will continue
> having mixed cases.
> 
> So I'm in favor of the approach of this series here.

Including the removal of INTEL_GEN macro? Or what do you propose for 
that one? Can't be called GT_GEN now since that has been used up...

Regards,

Tvrtko