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

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel OTC