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

[Lucas De Marchi <lucas.demarchi@intel.com>]

On Wed, Mar 17, 2021 at 07:57:32PM +0200, Jani Nikula wrote:
>On Mon, 15 Mar 2021, Lucas De Marchi <lucas.demarchi@intel.com> 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@intel.com>
>>
>> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx