Attention is currently required from: Angel Pons, Dinesh Gehlot, Jérémy Compostella, Nick Vaccaro, 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 & ADL ......................................................................
Patch Set 33:
(9 comments)
File common/hw-gfx-gma-config.ads.template:
https://review.coreboot.org/c/libgfxinit/+/67494/comment/e395dfd3_ce089faf : PS32, Line 222: Has_TGL_Plane_Control : <genbool> := Tigerlake_On;
Looks like the `PLANE_COLOR_CTL` registers were first introduced with GLK (N.B. […]
Done
File common/hw-gfx-gma-pipe_setup.adb:
https://review.coreboot.org/c/libgfxinit/+/67494/comment/7ca81fae_ea661482 : PS28, Line 221: Config.Variable)),
GNATprove stumbles here because it's only true for the new platforms. […]
Done
File common/hw-gfx-gma-pipe_setup.adb:
https://review.coreboot.org/c/libgfxinit/+/67494/comment/2e96bbe8_f6535985 : PS32, Line 46: PLANE_COLOR_CTL_GAMMA_DISABLE : constant := 1 * 2 ** 13;
nit: Apparently this bit is *plane* gamma disable. […]
I didn't want to drop the CTL_ (part of the register name), but added the PLANE_.
https://review.coreboot.org/c/libgfxinit/+/67494/comment/e7639896_deab49ec : 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 […]
Done
https://review.coreboot.org/c/libgfxinit/+/67494/comment/de720e00_fff707e0 : PS32, Line 254: then PLANE_CTL_ARB_SLOTS (1)
Looks like the value used here is dependent on plane state: […]
On TGL we use the hardware defaults, i.e. don't set USE_PROGRAMMED_SLOTS. I assume the hardcoded 1 is what used to be the default, but not sure anymore.
https://review.coreboot.org/c/libgfxinit/+/67494/comment/64955cad_20a3f01d : PS32, Line 266: Value => 0); Moving this out of the if, because PLANE_AUX_DIST seems to exist since `Has_Plane_Control` and should default to 0.
https://review.coreboot.org/c/libgfxinit/+/67494/comment/2eda732d_915f1be3 : 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)
Done
https://review.coreboot.org/c/libgfxinit/+/67494/comment/08810fdf_f5fd19ad : 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: […]
Anything to do beside updating the comment?
https://review.coreboot.org/c/libgfxinit/+/67494/comment/f95f9b25_ea6f8603 : 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(cr […]
I also stumbled over this. And I actually see no reason to set these bits. I'll try to do some tests.