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 by Tim Wawrzynczak. ( https://review.coreboot.org/c/libgfxinit/+/67494?usp=email )
Change subject: gma pipe_setup: Update for TGL & ADL ......................................................................
Patch Set 35:
(4 comments)
File common/hw-gfx-gma-pipe_setup.adb:
https://review.coreboot.org/c/libgfxinit/+/67494/comment/e2fdae7b_1de98711?u... : PS32, Line 254: then PLANE_CTL_ARB_SLOTS (1)
IIRC the PRMs say that 0 is the default, but it's late and I don't trust myself.
We also hard code the 32-bit pixel format, which matches the `1`. Will add a comment.
https://review.coreboot.org/c/libgfxinit/+/67494/comment/6a1e1d00_ac2ef4a0?u... : 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 t […]
I doubt we have that. I couldn't find much about it, but it's something that i915 uses when only pipe A or B is enabled. Probably something to increase bandwidth for a single very-high resolution pipe.
So I could put the `!joined_mbus` value here?
Or split ADL out of this patch? Might be the best as the huge amount of untested code in this review chain is only slowing us down.
https://review.coreboot.org/c/libgfxinit/+/67494/comment/6c07c912_f6274547?u... : 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.
Besides updating the comment, we should unconditionally disable underrun recovery for `DISPLAY_VER(i […]
Done
https://review.coreboot.org/c/libgfxinit/+/67494/comment/1fde794e_94672f07?u... : PS32, Line 441: Has_TGL_Plane_Control
IIRC i915 sets them depending on the display (engine) version, so I wouldn't use a named config for […]
So the register description says we want this to allow pixels to pass the pipe unaltered (e.g. w/o scaling that is possible). Linux sets the HDR bit if only the HDR planes (first 3) are used, that should also be the case for us.
What's left is to bikeshed the `Config` name. I wonder if we shouldn't start moving such register bits into the `Config` package. That would make things easier, i.e. no more `(if Config... then ... else 0)` here in the implementation.
-
Nico Huber (Code Review)