Re: [PATCH 1/2] drm/i915: Introduce INTEL_GEN_MASK

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

On Fri, 08 Sep 2017, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> On 08/09/2017 10:39, Jani Nikula wrote:
>> On Fri, 08 Sep 2017, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote:
>>> Split INTEL_GEN_MASK out of IS_GEN macro, and make it usable
>>> within static declarations (unlike combound statements).
>>>
>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_drv.h | 22 ++++++++++------------
>>>   1 file changed, 10 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 63ca2ffcafef..c3f9d7d7b146 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -2873,23 +2873,21 @@ intel_info(const struct drm_i915_private *dev_priv)
>>>   #define INTEL_REVID(dev_priv)	((dev_priv)->drm.pdev->revision)
>>>   
>>>   #define GEN_FOREVER (0)
>>> +
>>> +#define INTEL_GEN_MASK(s, e) ( \
>>> +        BUILD_BUG_ON_ZERO(!__builtin_constant_p(s)) + \
>>> +        BUILD_BUG_ON_ZERO(!__builtin_constant_p(e)) + \
>>> +        GENMASK((e) != GEN_FOREVER ? (e) - 1 : BITS_PER_LONG - 1, \
>>> +	        (s) != GEN_FOREVER ? (s) - 1 : 0) \
>> 
>> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>> 
>> but I'd really like a patch on top to remove the -1 from here and
>> info->gen_mask.
>
> What is the objection on -1? It is hidden in the macros so you can only 
> see it if looking at disassembly or even lower level.

IMO it's unnecessary optimization that makes reviewing the patches at
hand a tiny bit harder. Multiply that by everyone who ever looks at the
guts of the macros, and wonders why gen_mask is really
gen_off_by_one_mask, and for what reason. Indeed I thought there must be
a reason, until I realized that in reality, there is none.

BR,
Jani.

>
> We could even make it -2, but AFAIR Chris wanted to keep the option for 
> supporting gen1 open. :)
>
> Regards,
>
> Tvrtko

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