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

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

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?


BR,
Jani.


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