Attention is currently required from: Dinesh Gehlot, Jérémy Compostella, Nick Vaccaro, Tarun Tuli, Tim Wawrzynczak.
Patch set 32:Code-Review +1
10 comments:
File common/hw-gfx-gma-config.ads.template:
Patch Set #32, Line 217: Need_Pipe_Arb_Slots : <tglbool> := Alderlake_On;
For reference, this is `Wa_22012358565:adl-p`
https://github.com/torvalds/linux/commit/0b86952d15ceae275f685f9bb571fea30904147f
Patch Set #32, 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/47f9ea8b9143890c9b98ad9e16e8ad793be2d9e3
https://github.com/torvalds/linux/blob/ed30a4a51bb196781c8058073ea720133a65596f/drivers/gpu/drm/i915/display/skl_universal_plane.c#L1195
File common/hw-gfx-gma-pipe_setup.adb:
Patch Set #32, 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)?
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));
```
Patch Set #32, 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/ed30a4a51bb196781c8058073ea720133a65596f/drivers/gpu/drm/i915/display/skl_universal_plane.c#L871
https://github.com/torvalds/linux/blob/ed30a4a51bb196781c8058073ea720133a65596f/include/drm/drm_fourcc.h#L87
Is it OK to hardcode `1` here? If so, why?
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)
Patch Set #32, 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/ed30a4a51bb196781c8058073ea720133a65596f/drivers/gpu/drm/i915/display/skl_watermark.c#L3631
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`.
-- 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/4fe7907f3775034140a518d1582580926da98ee2
Patch Set #32, Line 387: if Config.Has_Type_C_Ports then
Note to self: equivalent to `if (DISPLAY_VER(dev_priv) >= 11)`
Patch Set #32, 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).
To view, visit change 67494. To unsubscribe, or for help writing mail filters, visit settings.