[coreboot-gerrit] [M] Change in libgfxinit[master]: gma pipe_setup: Update for TGL

[Nico Huber (Code Review)]

Attention is currently required from: Angel Pons, Jérémy Compostella, Tarun Tuli, Tim Wawrzynczak.

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/67494?usp=email )

Change subject: gma pipe_setup: Update for TGL
......................................................................

Patch Set 23: Code-Review+1

(12 comments)

Patchset:

PS23: 
Only style nits and some suggestions, the logic looks good!

I noticed that IS_ALDERLAKE_S() in `i915` includes ADL-N, so that should
cover all the IDs we have added so far. Only ADL-S would differ (at least
wrt. the DBOX credits).

NB. I'm currently following the chain as pushed on February 15th. Hope
that's right. Some changes were updated individually since then. Let
me know if you need help rebasing w/o losing such changes.

File common/hw-gfx-gma-config.ads.template:

https://review.coreboot.org/c/libgfxinit/+/67494/comment/63eb8691_8124c6b0 :
PS23, Line 217: Alderlake_On
I see the changed register interface in TGL docs already. I haven't
found much documentation about the arbitration slots. Only that it
seems optional if the hardware defaults are ok. Has it been tested
w/o this 'override'?

Update: Seems to be about `/* Wa_22012358565:adl-p */` ?

https://review.coreboot.org/c/libgfxinit/+/67494/comment/2bc6a565_c4c1a6cc :
PS23, Line 223:    Has_New_Mbus_Dbox_Credits     : <tglbool> := Alderlake_On;
`and Is_LP` AFAICT

File common/hw-gfx-gma-pipe_setup.adb:

https://review.coreboot.org/c/libgfxinit/+/67494/comment/e62ab921_c011f55b :
PS10, Line 350:                         MBUS_DBOX_A_CREDIT (2));
> I'll you close this one if you agree.

Thanks for the update. I'm still missing the ABOX configuration. But that is
probably not done per pipe setup... closing.

File common/hw-gfx-gma-pipe_setup.adb:

https://review.coreboot.org/c/libgfxinit/+/67494/comment/9fe6668b_30ff2766 :
PS23, Line 46: 16#2000#
The notation may look confusing, but it's valuable to have the
bit number visible, i.e.
```
   PLANE_COLOR_CTL_GAMMA_DISABLE : constant := 1 * 2 ** 13;
```

https://review.coreboot.org/c/libgfxinit/+/67494/comment/e736e70d_3cb1ee77 :
PS23, Line 48:    PLANE_CTL_PLANE_ENABLE                 : constant := 1 * 2 ** 31;
             :    PLANE_CTL_SRC_PIX_FMT_RGB_32B_8888     : constant := 4 * 2 ** 24;
             :    PLANE_CTL_PLANE_GAMMA_DISABLE          : constant := 1 * 2 ** 13;
             :    PLANE_CTL_TILED_SURFACE_MASK           : constant := 7 * 2 ** 10;
             :    PLANE_CTL_TILED_SURFACE_LINEAR         : constant := 0 * 2 ** 10;
             :    PLANE_CTL_TILED_SURFACE_X_TILED        : constant := 1 * 2 ** 10;
             :    PLANE_CTL_TILED_SURFACE_Y_TILED        : constant := 4 * 2 ** 10;
             :    PLANE_CTL_TILED_SURFACE_YF_TILED       : constant := 5 * 2 ** 10;
Whitespace changes are not needed (anymore).

https://review.coreboot.org/c/libgfxinit/+/67494/comment/bfa54e00_08ba5073 :
PS23, Line 95: natural
It's commonly written as `Natural`, i.e. upper-case `N` (I wonder if
we can enable a warning for this).

https://review.coreboot.org/c/libgfxinit/+/67494/comment/bb38e6d2_f6dc2df3 :
PS23, Line 95: ARB_SLOTS
No all-caps for types, please.

Same for other types (around `MBUS_DBOX_*`) below.

https://review.coreboot.org/c/libgfxinit/+/67494/comment/d0ff20f6_e2fcf94e :
PS23, Line 218:                Config.Variable)),
Would prefer to have this above `Controller` so that globals and parameters
are grouped.

https://review.coreboot.org/c/libgfxinit/+/67494/comment/8cab48a3_d15d4740 :
PS23, Line 225:       function PLANE_CTL_ARB_SLOTS (N : Word32) return Word32
              :       is (Shift_Left (N, 28));
Could reside above with the other PLANE_CTL constants.

https://review.coreboot.org/c/libgfxinit/+/67494/comment/e799aa95_6591aeee :
PS23, Line 342:       MBUS_DBOX_REGULATE_B2B_TRANSACTIONS_EN : constant := 1 * 2 ** 16;
If we are scoping them anyway, all these types and functions could go
into Program_Mbux_Dbox_Credits().

https://review.coreboot.org/c/libgfxinit/+/67494/comment/d58883e2_23902648 :
PS23, Line 438: IPEMISC_PIXEL_ROUNDING_TRUNC or HDR_MODE
Please add such constants.