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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 08/12/2016 14:00, Jani Nikula wrote:
On Thu, 08 Dec 2016, Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
On 08/12/2016 13:37, Jani Nikula wrote:
On Thu, 08 Dec 2016, Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
On 08/12/2016 10:46, Jani Nikula wrote:
On Thu, 08 Dec 2016, Tvrtko Ursulin <tursulin@xxxxxxxxxxx> wrote:
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>

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@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>

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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux