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

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

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. 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.

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