Re: [PATCH 2/3] drm/i915: Generate all IS_<platform> macros

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 08/12/2016 14:00, Jani Nikula wrote:
> On Thu, 08 Dec 2016, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>> On 08/12/2016 13:37, Jani Nikula wrote:
>>> On Thu, 08 Dec 2016, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>>>> On 08/12/2016 10:46, Jani Nikula wrote:
>>>>> On Thu, 08 Dec 2016, Tvrtko Ursulin <tursulin@ursulin.net> wrote:
>>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>>
>>>>>> Instead of listing them individually we can generate them
>>>>>> using the new i915_platforms.h header.
>>>>>>
>>>>>> Also convert them to a static inline function which
>>>>>> interestingly makes the code smaller as well.
>>>>>>
>>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>>>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>>>>
>>>>> NAK. Absolutely opposed to this.
>>>>
>>>> Gee, sounds a bit to harsh to me. :) Didn't we say we are not doing NAKs
>>>> any longer?
>>>
>>> Only when dropped without rationale. I needed to make it clear in no
>>> uncertain terms how important this is to me.
>>
>> Hm ok, I'll give you a benefit of doubt here.
>
> Thanks; I hope you've observed I don't use it lightly.
>
>>>>> A large part of my work involves digging through the source tree, and a
>>>>> crucial part of that is looking up definitions and references, both for
>>>>> macros and functions. Not having the macro/function definitions breaks
>>>>> that workflow. Losing that, source code archeology gets *much*
>>>>> harder. The losses are much greater than the gains.
>>>>
>>>> Hm, I struggle to see that point on the same magnitude of a disaster
>>>> scale as you. I would have thought we all know what IS_SKYLAKE & co are
>>>> so it would be no big deal.
>>>
>>> Sure we know what they are; I want to be able to see all the
>>> *references* to them as well, using GNU global. That fails if they're
>>> not defined in the first place. And no, git grep is not the same.
>>>
>>>> Imagine if we changed it to IS_PLATFORM(SKYLAKE) for instance.
>>>
>>> Then all the things passed as parameter would have to be defined.
>>
>> They are already -> enum intel_platform?!
>
> See the other mail; they'd have to be defined directly (as they
> currently are in git) instead of via macros (as in patch 1).
>
> Hmm, how about
>
> static inline bool intel_is_platform(const struct drm_i915_private *dev_priv,
> 				     enum intel_platform platform)
> {
> 	return dev_priv->info.platform == platform;
> }
>
> and doing
>
> #define IS_FOO(dev_priv) intel_is_platform(dev_priv, INTEL_FOO)
>
> manually for the ones we actually use (we don't need them all)? If the
> function is inline, I don't see how defining N similar functions instead
> of passing in the parameter would be more efficient. And you could still
> do the optimizations of patchs 3/3 AFAICS.
>
> Suitable compromise?

Yes, in fact, I was already half way through typing that when this 
e-mail arrived.

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx