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

[Tvrtko Ursulin]

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