Re: [PATCH 4/5] drm/i915: add helper IS_GEN_* macros

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

On Thu, 01 Nov 2018, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> On 01/11/2018 08:35, Lucas De Marchi wrote:
>> This will allow us to make all gen comparisons simple gen comparisons
>> with the same kind of macro rather than resorting to a mix of IS_GEN(),
>> INTEL_GEN() >=, >, <=, <.
>> 
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h | 5 +++++
>>   1 file changed, 5 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 8d22353a3da6..eb636dfe5228 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2370,6 +2370,11 @@ intel_info(const struct drm_i915_private *dev_priv)
>>   #define IS_GEN(dev_priv, ...) \
>>   	CONCATENATE(_IS_GEN_ARG, COUNT_ARGS(__VA_ARGS__))((dev_priv), ##__VA_ARGS__)
>>   
>> +#define IS_GEN_GT(dev_priv, g)	(g > (dev_priv)->info.gen)
>
> Interesting :), I have at some point floated a similar patch and there 
> was overwhelming dislike of the GT suffix, since we have GT2 GT3 etc.. 
> as established acronyms for GPU configurations.

To be honest even *without* the GPU GT clash, I think GT/GE/LT/LE are a
cognitive burden. Not a big one, but still less natural than >/>=/</<=.
Feels like assembly.

Not that I have better alternatives. IS_GEN_AFTER for GT,
IS_GEN_OR_LATER for GE? Feels a bit contrived?

> Furthermore, I am pretty sure I've done it using the gen bitmask so 
> hypothetical checks like IS_GEN_LT(5) || IS_GEN_RANGE(8, 9) could still 
> be compile time optimized to a single conditional. That would also now 
> become more difficult due removal of GEN_FOREVER.
>
> To summarise, I don't see a advantage of hiding this in macros if we are 
> not getting the above benefit.

Agreed. I didn't realize the GEN_FOREVER removal was merged already, I
wish you'd chimed in there. Of course, we can bring it back.

BR,
Jani.


>
> Regards,
>
> Tvrtko
>
>> +#define IS_GEN_GE(dev_priv, g)	(g >= (dev_priv)->info.gen)
>> +#define IS_GEN_LT(dev_priv, g)	(g < (dev_priv)->info.gen)
>> +#define IS_GEN_LE(dev_priv, g)	(g <= (dev_priv)->info.gen)
>> +
>>   /*
>>    * Return true if revision is in range [since,until] inclusive.
>>    *
>> 

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx