Quoting Matt Roper (2025-10-29 18:22:15-03:00) >On Tue, Oct 21, 2025 at 09:28:38PM -0300, Gustavo Sousa wrote: >> From: Ravi Kumar Vodapalli <[email protected]> >> >> Some of the register fields of MBUS_CTL and DBUF_CTL register are >> changed for Xe3p_LPD platforms. Update the changed fields in the driver. >> Below are the changes: >> >> MBUS_CTL: >> Translation Throttle Min >> It changed from BIT[15:13] to BIT[16:13] >> >> DBUF_CTL: >> Min Tracker State Service >> It changed from BIT[18:16] to BIT[20:16] >> Max Tracker State Service >> It changed to from BIT[23:19] to BIT[14:10] >> but using default value, so no need to define >> in code. > >In a lot of cases when a register field picks up extra high bit(s), the >extra bits were previously reserved, so it's fine to just adjust the >existing definition (since reserved bits are required to always read out >of hardware as zeroes). However in these cases, the new bits these >fields are extending into were actively used by the hardware for other >purposes on previous platforms, which is why it's necessary to keep the >existing pre-Xe3p definitions unchanged and create separate Xe3p ones >that can be used only on the newer Xe3p platforms. You should make some >mention of that in the commit message so it's clear why we're handling >these a bit differently than a lot of other registers.
Agreed. Just updated the local v3 to make that clear. > >> >> v2: >> - Keep definitions in the same line (i.e. without line continuation >> breaks) for better readability. (Jani) >> >> Bspec: 68868, 68872 >> Cc: Jani Nikula <[email protected]> >> Signed-off-by: Ravi Kumar Vodapalli <[email protected]> >> Signed-off-by: Gustavo Sousa <[email protected]> >> --- >> drivers/gpu/drm/i915/display/skl_watermark.c | 16 +++++-- >> drivers/gpu/drm/i915/display/skl_watermark_regs.h | 52 >> ++++++++++++----------- >> 2 files changed, 40 insertions(+), 28 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c >> b/drivers/gpu/drm/i915/display/skl_watermark.c >> index 256162da9afc..c141d575009f 100644 >> --- a/drivers/gpu/drm/i915/display/skl_watermark.c >> +++ b/drivers/gpu/drm/i915/display/skl_watermark.c >> @@ -3477,7 +3477,10 @@ void intel_dbuf_mdclk_cdclk_ratio_update(struct >> intel_display *display, >> if (!HAS_MBUS_JOINING(display)) >> return; >> >> - if (DISPLAY_VER(display) >= 20) >> + if (DISPLAY_VER(display) >= 35) >> + intel_de_rmw(display, MBUS_CTL, >> XE3P_MBUS_TRANSLATION_THROTTLE_MIN_MASK, >> + XE3P_MBUS_TRANSLATION_THROTTLE_MIN(ratio - 1)); >> + else if (DISPLAY_VER(display) >= 20) >> intel_de_rmw(display, MBUS_CTL, >> MBUS_TRANSLATION_THROTTLE_MIN_MASK, >> MBUS_TRANSLATION_THROTTLE_MIN(ratio - 1)); >> >> @@ -3488,9 +3491,14 @@ void intel_dbuf_mdclk_cdclk_ratio_update(struct >> intel_display *display, >> ratio, str_yes_no(joined_mbus)); >> >> for_each_dbuf_slice(display, slice) >> - intel_de_rmw(display, DBUF_CTL_S(slice), >> - DBUF_MIN_TRACKER_STATE_SERVICE_MASK, >> - DBUF_MIN_TRACKER_STATE_SERVICE(ratio - 1)); >> + if (DISPLAY_VER(display) >= 35) >> + intel_de_rmw(display, DBUF_CTL_S(slice), >> + >> XE3P_DBUF_MIN_TRACKER_STATE_SERVICE_MASK, >> + >> XE3P_DBUF_MIN_TRACKER_STATE_SERVICE(ratio - 1)); >> + else >> + intel_de_rmw(display, DBUF_CTL_S(slice), >> + DBUF_MIN_TRACKER_STATE_SERVICE_MASK, >> + DBUF_MIN_TRACKER_STATE_SERVICE(ratio - >> 1)); >> } >> >> static void intel_dbuf_mdclk_min_tracker_update(struct intel_atomic_state >> *state) >> diff --git a/drivers/gpu/drm/i915/display/skl_watermark_regs.h >> b/drivers/gpu/drm/i915/display/skl_watermark_regs.h >> index c5572fc0e847..94915afc6b0b 100644 >> --- a/drivers/gpu/drm/i915/display/skl_watermark_regs.h >> +++ b/drivers/gpu/drm/i915/display/skl_watermark_regs.h >> @@ -32,16 +32,18 @@ >> #define MBUS_BBOX_CTL_S1 _MMIO(0x45040) >> #define MBUS_BBOX_CTL_S2 _MMIO(0x45044) >> >> -#define MBUS_CTL _MMIO(0x4438C) >> -#define MBUS_JOIN REG_BIT(31) >> -#define MBUS_HASHING_MODE_MASK REG_BIT(30) >> -#define MBUS_HASHING_MODE_2x2 >> REG_FIELD_PREP(MBUS_HASHING_MODE_MASK, 0) >> -#define MBUS_HASHING_MODE_1x4 >> REG_FIELD_PREP(MBUS_HASHING_MODE_MASK, 1) >> -#define MBUS_JOIN_PIPE_SELECT_MASK REG_GENMASK(28, 26) >> -#define MBUS_JOIN_PIPE_SELECT(pipe) >> REG_FIELD_PREP(MBUS_JOIN_PIPE_SELECT_MASK, pipe) >> -#define MBUS_JOIN_PIPE_SELECT_NONE MBUS_JOIN_PIPE_SELECT(7) >> -#define MBUS_TRANSLATION_THROTTLE_MIN_MASK REG_GENMASK(15, 13) >> -#define MBUS_TRANSLATION_THROTTLE_MIN(val) >> REG_FIELD_PREP(MBUS_TRANSLATION_THROTTLE_MIN_MASK, val) >> +#define MBUS_CTL _MMIO(0x4438C) >> +#define MBUS_JOIN REG_BIT(31) >> +#define MBUS_HASHING_MODE_MASK REG_BIT(30) >> +#define MBUS_HASHING_MODE_2x2 >> REG_FIELD_PREP(MBUS_HASHING_MODE_MASK, 0) >> +#define MBUS_HASHING_MODE_1x4 >> REG_FIELD_PREP(MBUS_HASHING_MODE_MASK, 1) >> +#define MBUS_JOIN_PIPE_SELECT_MASK REG_GENMASK(28, >> 26) >> +#define MBUS_JOIN_PIPE_SELECT(pipe) >> REG_FIELD_PREP(MBUS_JOIN_PIPE_SELECT_MASK, pipe) >> +#define MBUS_JOIN_PIPE_SELECT_NONE >> MBUS_JOIN_PIPE_SELECT(7) >> +#define MBUS_TRANSLATION_THROTTLE_MIN_MASK REG_GENMASK(15, >> 13) >> +#define MBUS_TRANSLATION_THROTTLE_MIN(val) >> REG_FIELD_PREP(MBUS_TRANSLATION_THROTTLE_MIN_MASK, val) >> +#define XE3P_MBUS_TRANSLATION_THROTTLE_MIN_MASK REG_GENMASK(16, 13) >> +#define XE3P_MBUS_TRANSLATION_THROTTLE_MIN(val) >> REG_FIELD_PREP(XE3P_MBUS_TRANSLATION_THROTTLE_MIN_MASK, val) > >Nitpick: I'm not sure if we're 100% consistent, but I feel like we >usually sort bitfields based on their upper bound rather than their >lower bound. So even though xe3p and pre-xe3p start at the same bit 13, >the xe3p should probably be sorted first since it ends at a higher bit >(16 vs 15). Ack. Thanks! -- Gustavo Sousa > >> >> /* >> * The below are numbered starting from "S1" on gen11/gen12, but starting >> @@ -51,21 +53,23 @@ >> * way things will be named by the hardware team going forward, plus it's >> more >> * consistent with how most of the rest of our registers are named. >> */ >> -#define _DBUF_CTL_S0 0x45008 >> -#define _DBUF_CTL_S1 0x44FE8 >> -#define _DBUF_CTL_S2 0x44300 >> -#define _DBUF_CTL_S3 0x44304 >> -#define DBUF_CTL_S(slice) _MMIO(_PICK(slice, \ >> - _DBUF_CTL_S0, \ >> - _DBUF_CTL_S1, \ >> - _DBUF_CTL_S2, \ >> - _DBUF_CTL_S3)) >> -#define DBUF_POWER_REQUEST REG_BIT(31) >> -#define DBUF_POWER_STATE REG_BIT(30) >> -#define DBUF_TRACKER_STATE_SERVICE_MASK REG_GENMASK(23, 19) >> -#define DBUF_TRACKER_STATE_SERVICE(x) >> REG_FIELD_PREP(DBUF_TRACKER_STATE_SERVICE_MASK, x) >> -#define DBUF_MIN_TRACKER_STATE_SERVICE_MASK REG_GENMASK(18, 16) /* >> ADL-P+ */ >> +#define _DBUF_CTL_S0 0x45008 >> +#define _DBUF_CTL_S1 0x44FE8 >> +#define _DBUF_CTL_S2 0x44300 >> +#define _DBUF_CTL_S3 0x44304 >> +#define DBUF_CTL_S(slice) _MMIO(_PICK(slice, >> \ >> + >> _DBUF_CTL_S0, \ >> + >> _DBUF_CTL_S1, \ >> + >> _DBUF_CTL_S2, \ >> + >> _DBUF_CTL_S3)) >> +#define DBUF_POWER_REQUEST REG_BIT(31) >> +#define DBUF_POWER_STATE REG_BIT(30) >> +#define DBUF_TRACKER_STATE_SERVICE_MASK REG_GENMASK(23, 19) >> +#define DBUF_TRACKER_STATE_SERVICE(x) >> REG_FIELD_PREP(DBUF_TRACKER_STATE_SERVICE_MASK, x) >> +#define DBUF_MIN_TRACKER_STATE_SERVICE_MASK REG_GENMASK(18, >> 16) /* ADL-P+ */ >> #define DBUF_MIN_TRACKER_STATE_SERVICE(x) >> REG_FIELD_PREP(DBUF_MIN_TRACKER_STATE_SERVICE_MASK, x) /* ADL-P+ */ >> +#define XE3P_DBUF_MIN_TRACKER_STATE_SERVICE_MASK REG_GENMASK(20, 16) >> +#define XE3P_DBUF_MIN_TRACKER_STATE_SERVICE(x) >> REG_FIELD_PREP(XE3P_DBUF_MIN_TRACKER_STATE_SERVICE_MASK, x) > >Same here. > > >Matt > >> >> #define MTL_LATENCY_LP0_LP1 _MMIO(0x45780) >> #define MTL_LATENCY_LP2_LP3 _MMIO(0x45784) >> >> -- >> 2.51.0 >> > >-- >Matt Roper >Graphics Software Engineer >Linux GPU Platform Enablement >Intel Corporation
