Attention is currently required from: Dinesh Gehlot, Jérémy Compostella, Nick Vaccaro, Tarun Tuli, Tim Wawrzynczak.
Angel Pons 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 32: Code-Review+1
(10 comments)
File common/hw-gfx-gma-config.ads.template:
https://review.coreboot.org/c/libgfxinit/+/67494/comment/15a6b2d6_84f9ee75 : PS32, Line 217: Need_Pipe_Arb_Slots : <tglbool> := Alderlake_On; For reference, this is `Wa_22012358565:adl-p`
https://github.com/torvalds/linux/commit/0b86952d15ceae275f685f9bb571fea3090...
https://review.coreboot.org/c/libgfxinit/+/67494/comment/ac1b0b0f_6b89ea0d : PS32, Line 222: Has_TGL_Plane_Control : <genbool> := Tigerlake_On; Looks like the `PLANE_COLOR_CTL` registers were first introduced with GLK (N.B. `DISPLAY_VER(i915)` is 10 for GLK), and a few bits from `PLANE_CTL` got moved into the new register. Maybe `Has_Plane_Color_Control` would be a more precise name (this flag indicates whether the register is supported).
https://github.com/torvalds/linux/commit/47f9ea8b9143890c9b98ad9e16e8ad793be... https://github.com/torvalds/linux/blob/ed30a4a51bb196781c8058073ea720133a655...
File common/hw-gfx-gma-pipe_setup.adb:
https://review.coreboot.org/c/libgfxinit/+/67494/comment/9c009339_af59e087 : PS32, Line 46: PLANE_COLOR_CTL_GAMMA_DISABLE : constant := 1 * 2 ** 13; nit: Apparently this bit is *plane* gamma disable. There's a *pipe* gamma **enable** bit (30) in the same register (even though it's deprecated), which might lead to confusion. How about renaming this to `PLANE_COLOR_PLANE_GAMMA_DISABLE` (same name as i915)?
https://review.coreboot.org/c/libgfxinit/+/67494/comment/5187ab92_28c9fae4 : PS32, Line 228: function PLANE_CTL_ARB_SLOTS (N : Word32) return Word32 : is (Shift_Left (N, 28)); nit: format function expressions so that the first line ends with `is` and the second line contains the expression value indented one level:
``` function PLANE_CTL_ARB_SLOTS (N : Word32) return Word32 is (Shift_Left (N, 28)); ```
https://review.coreboot.org/c/libgfxinit/+/67494/comment/5df62fbe_60450553 : PS32, Line 254: then PLANE_CTL_ARB_SLOTS (1) Looks like the value used here is dependent on plane state:
https://github.com/torvalds/linux/blob/ed30a4a51bb196781c8058073ea720133a655... https://github.com/torvalds/linux/blob/ed30a4a51bb196781c8058073ea720133a655...
Is it OK to hardcode `1` here? If so, why?
https://review.coreboot.org/c/libgfxinit/+/67494/comment/49cdf2b3_0ee23aa1 : PS32, Line 326: type BW_Credit is new Natural range 0 .. 3; : function MBUS_DBOX_BW_CREDIT (C : BW_Credit) return Word32 : is (Shift_Left (Word32 (C), 14)); : : type B_Credit is new Natural range 0 .. 31; : function MBUS_DBOX_B_CREDIT (C : B_Credit) return Word32 : is (Shift_Left (Word32 (C), 8)); : : type A_Credit is new Natural range 0 .. 15; : function MBUS_DBOX_A_CREDIT (C : A_Credit) return Word32 : is (Word32 (C)); : : type B2B_Trans_Max is new Natural range 0 .. 31; : function MBUS_DBOX_B2B_TRANSACTIONS_MAX (B : B2B_Trans_Max) return Word32 : is (Shift_Left (Word32 (B), 20)); : : type B2B_Trans_Delay is new Natural range 0 .. 7; : function MBUS_DBOX_B2B_TRANSACTIONS_DELAY (B : B2B_Trans_Delay) return Word32 : is (Shift_Left (Word32 (B), 17)); : MBUS_DBOX_REGULATE_B2B_TRANSACTIONS_EN : constant := 1 * 2 ** 16; : nit: format for function expressions (see previous comment)
https://review.coreboot.org/c/libgfxinit/+/67494/comment/7ae070d5_e67fda80 : PS32, Line 354: if Config.Has_New_Mbus_Dbox_Credits then Where is this procedure described? The only procedure in i915 that seems to use these registers is this: https://github.com/torvalds/linux/blob/ed30a4a51bb196781c8058073ea720133a655...
Looks like these steps are only applied when some conditions are met. I guess it doesn't matter much for us; we can set these bits once and call it a day.
Given the WA name (`Wa_22010947358:adl-p`), one may think this is only for ADL-P. But i915 only distinguishes between ADL-P and ADL-S, so I believe `Has_New_Mbus_Dbox_Credits` has the right value.
What I don't know is whether we can forget about `joined_mbus`.
https://review.coreboot.org/c/libgfxinit/+/67494/comment/517561ae_c2641f98 : PS32, Line 380: -- ADL_P requires that we disable underrun recovery when : -- downscaling (or using the scaler for YUV420 pipe output), : -- using DSC, or using PSR2. From i915 sources:
Underrun recovery must always be disabled on display 13+. DG2 chicken bit meaning is inverted compared to other platforms.
Looks like this got updated in this i915 commit: https://github.com/torvalds/linux/commit/4fe7907f3775034140a518d1582580926da...
https://review.coreboot.org/c/libgfxinit/+/67494/comment/a5f28f08_10a836d5 : PS32, Line 387: if Config.Has_Type_C_Ports then Note to self: equivalent to `if (DISPLAY_VER(dev_priv) >= 11)`
https://review.coreboot.org/c/libgfxinit/+/67494/comment/848adc5a_36bd5eeb : PS32, Line 441: Has_TGL_Plane_Control AIUI, this is TGL+ (technically `PIPE_MISC_HDR_MODE_PRECISION` is ICL+ and only when `is_hdr_mode(crtc_state)`). But I feel using `Has_TGL_Plane_Control` here is not accurate (the `PIPE_MISC` register is not about *plane* control).