Re: [Intel-gfx] [PATCH v3 6/6] drm/i915/display: Simplify GLK display version tests

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Matt Roper <matthew.d.roper@intel.com>
Cc: intel-gfx@lists.freedesktop.org,
	"Ville Syrjälä" <ville.syrjala@intel.com>
Subject: Re: [Intel-gfx] [PATCH v3 6/6] drm/i915/display: Simplify GLK display version tests
Date: Tue, 23 Mar 2021 19:40:58 +0200	[thread overview]
Message-ID: <YFooKuVqPeZrLWM3@intel.com> (raw)
In-Reply-To: <20210323172734.GU3422723@mdroper-desk1.amr.corp.intel.com>

On Tue, Mar 23, 2021 at 10:27:34AM -0700, Matt Roper wrote:
> On Tue, Mar 23, 2021 at 07:25:57PM +0200, Ville Syrjälä wrote:
> > On Mon, Mar 22, 2021 at 04:38:40PM -0700, Matt Roper wrote:
> > > GLK has always been a bit of a special case since it reports INTEL_GEN()
> > > as 9, but has version 10 display IP.  Now we can properly represent the
> > > display version as 10 and simplify the display generation tests
> > > throughout the display code.
> > > 
> > > Aside from manually adding the version to the glk_info structure, the
> > > rest of this patch is generated with a Coccinelle semantic patch.  Note
> > > that we also need to switch any code that matches gen10 today but *not*
> > > GLK to be CNL-specific:
> > > 
> > >         @@ expression dev_priv; @@
> > >         - DISPLAY_VER(dev_priv) > 9
> > >         + DISPLAY_VER(dev_priv) >= 10
> > > 
> > >         @@ expression dev_priv, E; @@
> > >         (
> > >         - DISPLAY_VER(dev_priv) >= 10 && E
> > >         + (DISPLAY_VER(dev_priv) >= 11 || IS_CANNONLAKE(dev_priv)) && E
> > >         |
> > >         - DISPLAY_VER(dev_priv) >= 10
> > >         + DISPLAY_VER(dev_priv) >= 11 || IS_CANNONLAKE(dev_priv)
> > >         |
> > >         - IS_DISPLAY_RANGE(dev_priv, 10, E)
> > >         + IS_DISPLAY_RANGE(dev_priv, 11, E) || IS_CANNONLAKE(dev_priv)
> > >         )
> > > 
> > >         @@ expression dev_priv, E, E2; @@
> > >         (
> > >         - (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> > >         + IS_DISPLAY_VER(dev_priv, 10)
> > >         |
> > >         - E || IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)
> > >         + E || IS_DISPLAY_VER(dev_priv, 10)
> > >         |
> > >         - (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv))
> > >         + IS_DISPLAY_VER(dev_priv, 10)
> > >         |
> > >         - IS_GEMINILAKE(dev_priv) || E || IS_CANNONLAKE(dev_priv)
> > >         + E || IS_DISPLAY_VER(dev_priv, 10)
> > >         |
> > >         - E || IS_GEMINILAKE(dev_priv) || E2 || IS_CANNONLAKE(dev_priv)
> > >         + E || E2 || IS_DISPLAY_VER(dev_priv, 10)
> > >         |
> > >         - (IS_DISPLAY_VER(dev_priv, 10) || IS_GEMINILAKE(dev_priv))
> > >         + IS_DISPLAY_VER(dev_priv, 10)
> > >         |
> > >         - (IS_GEMINILAKE(dev_priv) || IS_DISPLAY_VER(dev_priv, 10))
> > >         + IS_DISPLAY_VER(dev_priv, 10)
> > >         )
> > > 
> > >         @@ expression dev_priv; @@
> > >         - (IS_DISPLAY_VER(dev_priv, 9) && !IS_GEMINILAKE(dev_priv))
> > >         + IS_DISPLAY_VER(dev_priv, 9)
> > > 
> > >         @@ expression dev_priv; @@
> > >         (
> > >         - !(DISPLAY_VER(dev_priv) >= 11 || IS_DISPLAY_VER(dev_priv, 10))
> > >         + DISPLAY_VER(dev_priv) < 10
> > >         |
> > >         - (DISPLAY_VER(dev_priv) >= 11 || IS_DISPLAY_VER(dev_priv, 10))
> > >         + DISPLAY_VER(dev_priv) >= 10
> > >         )
> > > 
> > >         @@ expression dev_priv, E; @@
> > >         - E || DISPLAY_VER(dev_priv) >= 11 || IS_DISPLAY_VER(dev_priv, 10)
> > >         + E || DISPLAY_VER(dev_priv) >= 10
> > > 
> > >         @@ expression dev_priv, E; @@
> > >         - (IS_DISPLAY_RANGE(dev_priv, 11, E) || IS_DISPLAY_VER(dev_priv, 10))
> > >         + IS_DISPLAY_RANGE(dev_priv, 10, E)
> > > 
> > >         @@ expression dev_priv; @@
> > >         (
> > >         - DISPLAY_VER(dev_priv) >= 11 || IS_CANNONLAKE(dev_priv) || IS_GEN9_LP(dev_priv)
> > >         + DISPLAY_VER(dev_priv) >= 10 || IS_GEN9_LP(dev_priv)
> > >         |
> > >         - IS_GEN9_LP(dev_priv) || DISPLAY_VER(dev_priv) >= 11 || IS_CANNONLAKE(dev_priv)
> > >         + IS_GEN9_LP(dev_priv) || DISPLAY_VER(dev_priv) >= 10
> > >         )
> > > 
> > >         @@ expression dev_priv, E; @@
> > >         - !(DISPLAY_VER(dev_priv) >= E)
> > >         + DISPLAY_VER(dev_priv) < E
> > > 
> > > v2:
> > >  - Convert gen10 conditions that don't include GLK into CNL conditions.
> > >    (Ville)
> > > 
> > > v3:
> > >  - Rework coccinelle rules so that "ver>=10" turns into "ver>=11||is_cnl." (Ville)
> > > 
> > > v3.1:
> > >  - Manually re-add the ".display.version = 10" to glk_info after
> > >    regenerating patch via Coccinelle.
> > > 
> > > v4:
> > >  - Also apply cocci rules to intel_pm.c and i915_irq.c!  (CI)
> > 
> > Ugh. One thing that occurred to me when looking at i915_irq.c is that
> > IS_GEN9_LP() is now maybe broken on glk? So seems to me all uses of
> > IS_GEN9_LP() need to be reviewed and potentially changed.
> 
> Broken how?  That macro still uses the gen/gt version instead of the
> display number, so I think it still behaves the same as before?

Oh you're not changng it to to use display ver? I guess it still kinda
works then. But it's going to be pretty confusing to use that for
display stuff now. Ie. we should probably stop using it.

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