Re: [PATCH v2 0/7] Make GEN macros more similar

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 26 Nov 2018, Rodrigo Vivi <rodrigo.vivi@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.

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx