Re: [PATCH v2 03/23] drm/i915/display: Eliminate most usage of INTEL_GEN()

[Lucas De Marchi <lucas.demarchi@xxxxxxxxx>]

On Wed, Mar 17, 2021 at 07:57:32PM +0200, Jani Nikula wrote:
> On Mon, 15 Mar 2021, Lucas De Marchi <lucas.demarchi@xxxxxxxxx> wrote:
> > On Thu, Mar 11, 2021 at 07:33:55AM -0800, Matt Roper wrote:
> > > Use Coccinelle to convert most of the usage of INTEL_GEN() and IS_GEN()
> > > in the display code to use DISPLAY_VER() comparisons instead.  The
> > > following semantic patch was used:
> > >
> > >        @@ expression dev_priv; @@
> > >        - INTEL_GEN(dev_priv)
> > >        + DISPLAY_VER(dev_priv)
> > >
> > >        @@ expression dev_priv; expression E; @@
> > >        - !IS_GEN(dev_priv, E)
> > >        + DISPLAY_VER(dev_priv) != E
> > >
> > >        @@ expression dev_priv; expression E; @@
> > >        - IS_GEN(dev_priv, E)
> > >        + DISPLAY_VER(dev_priv) == E
> > >
> > >        @@
> > >        expression dev_priv;
> > >        expression from, until;
> > >        @@
> > >        - !IS_GEN_RANGE(dev_priv, from, until)
> > >        + DISPLAY_VER(dev_priv) < from || DISPLAY_VER(dev_priv) > until
> > >
> > >        @@
> > >        expression dev_priv;
> > >        expression from, until;
> > >        statement S;
> > >        @@
> > >        - if (IS_GEN_RANGE(dev_priv, from, until)) S
> > >        + if (DISPLAY_VER(dev_priv) >= from && DISPLAY_VER(dev_priv) <= until) S
> > >
> > >        @@
> > >        expression dev_priv;
> > >        expression from, until;
> > >        @@
> > >        - IS_GEN_RANGE(dev_priv, from, until)
> > >        + (DISPLAY_VER(dev_priv) >= from && DISPLAY_VER(dev_priv) <= until)
> > >
> > > There are still some display-related uses of INTEL_GEN() in intel_pm.c
> > > (watermark code) and i915_irq.c.  Those will be updated separately.
> > >
> > > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> >
> > The reason why we had macros like IS_GEN() and IS_GEN_RANGE() is because
> > this allows the compiler to short-circuit it into a single
> > bitwise AND + comparison check.
>
> Well, just looking at the code, I think IS_GEN() and IS_GEN_RANGE() also
> look cleaner, at least once you've grown used to them.
>
> Stuff like this gets slower to read IMO:
>
> > > -	if (!IS_GEN_RANGE(dev_priv, 3, 7)) {
> > > +	if (DISPLAY_VER(dev_priv) < 3 || DISPLAY_VER(dev_priv) > 7) {
>
> In the past we had all combinations of <, <=, >, >= with && and ||, and,
> while it's pretty regular stuff, I think it benefited from the
> unification readability wise.
>
> Sure we can add IS_DISPLAY_VER() and _RANGE() later on, but with a bunch
> of churn that I find unlikely to happen again soon.
>
> So I guess I need more convincing this is indeed the path we want to
> take.

I don't disagree. I think the case you cited is the worst case, where we
have to invert the check. But looking at the code there are just 4 cases
in which this is used:

$ git grep \!IS_GEN_RANGE
drivers/gpu/drm/i915/display/intel_bios.c:      if (!IS_GEN_RANGE(dev_priv, 3, 7)) {
drivers/gpu/drm/i915/gt/gen8_ppgtt.c:   ppgtt->vm.has_read_only = !IS_GEN_RANGE(gt->i915, 11, 12);
drivers/gpu/drm/i915/gt/intel_ring_submission.c:        if (!IS_GEN_RANGE(engine->i915, 6, 7))
drivers/gpu/drm/i915/i915_cmd_parser.c:                         GEM_BUG_ON(!IS_GEN_RANGE(engine->i915, 6, 7));

Checking also the positive checks:

	$ git grep IS_GEN_RANGE | wc -l 
	37

So, not that many.  But to avoid this kind of issue I think we could add a
`DISPLAY_VER_RANGE(i915, from, until)` already and get rid of this problem.

Lucas De Marchi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx