On 19/11/2018 22:20, Lucas De Marchi wrote:
On Thu, Nov 08, 2018 at 11:23:46AM +0000, Tvrtko Ursulin wrote:
[snip]
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.
I forgot about the per-SKU builds when replying on this point. So I
think we need to add them as the benefit of using range masks
throughout. Question of whether people can stomach the ugliness still
remains. But at least for me personally, I think if we want to get to
per-SKU builds one day, then we'll have to do this.
But not sure how many of those there are.
What definitely exists in large-ish numbers are:
if (gen >= 11 || IS_PLATFORM)
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).
We agreed there that gen in runtime should be avoided if at all possible.
It seems that the consensus is to have macros to access device info,
whether the current, or two flavours of it in the future. So I think
only the question of naming remains on this item.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
