Re: [RFC 14/15] drm/i915: Use new IS_GEN range helpers

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@linux.intel.com> wrote:
> > > On Thu, Feb 08, 2018 at 05:13:02PM +0200, Mika Kuoppala wrote:
> > >> Chris Wilson <chris@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@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?

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx