[Intel-gfx] [RFC 14/15] drm/i915: Use new IS_GEN range helpers

[Chris Wilson]

Quoting Ville Syrjälä (2018-02-09 15:12:58)
> On Fri, Feb 09, 2018 at 04:48:43PM +0200, Ville Syrjälä wrote:
> > On Fri, Feb 09, 2018 at 01:18:49PM +0200, Jani Nikula wrote:
> > > On Thu, 08 Feb 2018, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
> > > > On Thu, Feb 08, 2018 at 05:13:02PM +0200, Mika Kuoppala wrote:
> > > >> Chris Wilson <chris at chris-wilson.co.uk> writes:
> > > >> 
> > > >> > Quoting Tvrtko Ursulin (2018-02-08 14:34:38)
> > > >> >> 
> > > >> >> On 08/02/2018 14:22, Ville Syrjälä wrote:
> > > >> >> > On Thu, Feb 08, 2018 at 01:06:05PM +0000, Tvrtko Ursulin wrote:
> > > >> >> >> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > > >> >> >>
> > > >> >> >> Coccinelle transformation:
> > > >> >> >>
> > > >> >> >>   @@
> > > >> >> >>   expression p, g;
> > > >> >> >>   @@
> > > >> >> >>   (
> > > >> >> >>   -INTEL_GEN(p) > g
> > > >> >> >>   +IS_GEN_GT(p, g)
> > > >> >> > 
> > > >> >> > I think this stuff makes the code pretty close to illegible.
> > > >> >> > In this particular case even more so because "GT" actually
> > > >> >> > means something very different to us.
> > > >> >> 
> > > >> >> Oh how true! And I did not realize it at all while writing it! :)
> > > >> >> 
> > > >> >> Anyway, something like this, regardless of a name, is needed if people 
> > > >> >> want this to be effective. Since the checks have to be moved to known at 
> > > >> >> compile time. Or a completely different approach will be needed.
> > > >> >
> > > >> > IS_GEN_RANGE() doesn't cut it?
> > > >> >
> > > >> 
> > > >> IS_GEN_RANGE(8,9);
> > > >> 
> > > >> short and readable
> > > >
> > > > 'if (IS_GEN_RANGE(...))' reads funny. IS_GEN_IN_RANGE() would be more
> > > > englishy perhaps, but it looks a bit off to me for whatever reason.
> > > 
> > > We already have IS_GEN(dev_priv, start, end) for inclusive ranges with
> > > GEN_FOREVER as unbound start/end. There's no need to bikeshed the naming
> > > further. (Except perhaps the GEN_FOREVER part.)
> > > 
> > > With some macro vararg hacking, we could probably make that work for
> > > IS_GEN(dev_priv, exact) too, to replace say IS_GEN5() with one macro if
> > > we like. Just to make more code use similar constructs.
> > > 
> > > > And it still doesn't tell you anything about inclusive vs. exlusive.
> > > > So it just forces you to waste brain cells on mundane details when
> > > > reading the code. IMO that's a fairly bad tradeoff.
> > > 
> > > Agreed. I think this patch is by far the worst part in the
> > > series. Especially the _GT, _GTE, _LT, _LTE macros are IMO unacceptable.
> > > 
> > > Quick greping shows that we have much more inclusive than exclusive
> > > range checks.
> > 
> > I think the usual pattern has been inclusive start, exclusive end. That
> > can help you think in terms of "this is when the feature appeared, and
> > this is when it disappeared". But if it's hidden in a macro then I
> > think exclusive might end up being rather confusing.
> > 
> > > We could help our brains a bit by switching, uh,
> > > exclusively to inclusive ranges. That's not perfect, by far, as some
> > > checks naturally work better with < and >, but I think I'd rather have
> > > that with IS_GEN() than a bunch of macros you have to stop to think
> > > about.
> > 
> > If only the compiler would be smart and be able to see that something
> > like
> > 
> > #define INTEL_GEN(dev_priv) (ilog2((dev_priv)->gen_mask & CONFIG_GEN_MASK))
> > 
> > if (INTEL_GEN(dev_priv) < 8)
> >       ...
> > 
> > can never be true...
> > 
> > Not sure it can't actually. But I assume people have actually tried
> > that.
> 
> Hmm...
> 
> #define IS_GEN(dev_priv, expr) ((ilog2(GEN_MASK) expr) && \
>                                 (ilog2((dev_priv)->gen_mask expr)))
> 
> if (IS_GEN(dev_priv, >= 8))
>         ...
> 
> Would something like that actually work?

There are a few ways in which we could map gen to 0 if the platform
wasn't enabled at build time, but I haven't a better approach than
Tvrtko's replacement of INTEL_GEN() with IS_GENx() or IS_GEN(x, y). The
biggest problem with just mapping gen to 0 comes in then we need to add
the 0 case to many if-else chains and switches (it makes writing
IS_GEN(i915, < 8) hard as well) but worse it's hard to do it in a way
that doesn't result in the compiler emitting instructions rather than
resolving it at compile time.

Still doesn't look pretty, and we need some more surgery (some static
tables pulling in function references need pruning) and LTO.

Doing build tests is easy enough, I hope we can get CI to do
per-platform builds (as I think those are more likely to show errors).
-Chris