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

Tue Nov 27 11:35:54 UTC 2018

On 27/11/2018 09:36, Lucas De Marchi wrote:
> On Tue, Nov 27, 2018 at 10:37:21AM +0200, Jani Nikula wrote:
>> On Mon, 26 Nov 2018, Rodrigo Vivi <rodrigo.vivi at intel.com> wrote:
>>> On Thu, Nov 22, 2018 at 08:54:30AM +0000, Tvrtko Ursulin wrote:
>>>>
>>>>
>>>> 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...
>>>
>>> Yes, including the removal of INTEL_GEN macro.
>>>
>>> I don't like the mixed styles like using INTEL_GEN(d) > 7 in one place
>>> and INTEL_GEN_RANGE(d, 7, FOREVER) in another place.
>>>
>>> The meaning is the same so we should stick with one of the usages.
>>>
>>> I see that there were many cases where having the info->gen number
>>> directly is useful. But I'd prefer to use that on a direct fashion
>>> rather than keeping this macro.
>>>
>>> Because if we keep the macro developers will eventually end up
>>> adding the mixed style back.
>>>
>>> Using direct info->gen as Lucas already showed has almost same number
>>> of chars than the macro, but requires more attention what is a good
>>> thing.
>>
>> I prefer using INTEL_GEN() because it hides where the gen comes from,
>> and we can change it on a whim. If we keep using dev_priv->info.gen
>> we'll need to change that already when info becomes a pointer,
>> i.e. dev_priv->info->gen. Throughout the codebase. With INTEL_GEN() it's
>> just a couple of places.
> 
> patch 7 actually does:
> 
> - INTEL_GEN(E)
> + INTEL_INFO(E)->gen
> 
> So still using the macro and if info becomes a pointer it would still be
> correct. My main motivation for removing it is not INTEL_GEN() itself,
> but because that would force us to keep the IS_ prefix in the other
> macros. IS_GT_GEN_RANGE is too big and ugly IMO.  If my
> previous proposal of using a single macro for both range and single gen
> would be accepted, I think keeping the IS_ prefix would not be so ugly.
> 
> Anyway, considering the addition of display macros, do you think it should
> be like below?
> 
> IS_GEN -> IS_GT_GEN()
>         `> IS_GT_GEN_RANGE()  (dialing down on the changes as suggested by
>                                Tvrtko to preserve plain if ladders and
>                                simple unbound ranges)

More range macro usage will help future per SKU builds work. So as said 
in my previous reply, if people can stomach this, I am okay with having 
this conversion as well. (From arithmetic gen comparison to ...GEN_RANGE 
everywhere.)

That would also makes the question of INTEL_GEN mostly irrelevant. So I 
think above is the main point to clarify first.

Then on the question of IS_ prefix or not, I don't feel very strongly 
about it. IS_ has a nice parallel with HAS_ and IS_platform, but I agree 
it doesn't look the prettiest (IS_GT_GEN). So don't know, whatever the 
vote ends up being.

Regards,

Tvrtko

> IS_GEN<N> -> IS_GT_GEN(..., N)
> INTEL_GEN -> GT_GEN()
> 
> IS_DISPLAY_GEN()
> IS_DISPLAY_GEN_RANGE()
> DISPLAY_GEN()
> 
> 
> Other option: just do the IS_GEN<N> IS_GEN(..., N),
> that was the original motivation for this series, and remember that
> gen here refers to GT.
> 
> Lucas De Marchi
> 
>>
>> If mixed use is a problem, then rename gen to __gen. I even sent a patch
>> for it, but didn't get it merged.
>>
>> BR,
>> Jani.
>>
>>
>> -- 
>> Jani Nikula, Intel Open Source Graphics Center
> 