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

[Matt Roper]

On Tue, Mar 23, 2021 at 07:40:58PM +0200, Ville Syrjälä wrote:
> 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.

That macro is used throughout the driver (including non-display code),
so I don't think we'd want to re-define it.  But you're right that we
probably should eliminate the places where it gets used in the display
code (since the graphics gen shouldn't matter); I'll do that as a
follow-up series.


Matt

> 
> -- 
> Ville Syrjälä
> Intel

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795