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