On Fri, 07 Nov 2025, Ville Syrjälä <[email protected]> wrote: > On Wed, Nov 05, 2025 at 04:25:49PM -0600, Lucas De Marchi wrote: >> On Wed, Oct 29, 2025 at 10:42:15PM +0200, Ville Syrjälä wrote: >> >From: Ville Syrjälä <[email protected]> >> > >> >Unfortunately the MAD_DIMM DIMM_S and DIMM_L bits on ICL are >> >not idential, so we are currently decoding DIMM_S incorrectly. >> > >> >Fix the problem by defining the DIMM_S and DIMM_L bits separately. >> >And for consistency do that same for SKL, even though there the >> >bits do match between the two DIMMs. The result is rather >> >repetitive in places, but I didn't feel like obfuscatign things >> >with cpp macros/etc. >> > >> >Broken decoding on Dell XPS 13 7390 2-in-1: >> > CH0 DIMM L size: 32 Gb, width: X16, ranks: 2, 16Gb+ DIMMs: no >> > CH0 DIMM S size: 32 Gb, width: X32, ranks: 3, 16Gb+ DIMMs: no >> > CH0 ranks: 2, 16Gb+ DIMMs: no >> > CH1 DIMM L size: 32 Gb, width: X16, ranks: 2, 16Gb+ DIMMs: no >> > CH1 DIMM S size: 32 Gb, width: X32, ranks: 3, 16Gb+ DIMMs: no >> > CH1 ranks: 2, 16Gb+ DIMMs: no >> > Memory configuration is symmetric? no >> > >> >Fixed decoding on Dell XPS 13 7390 2-in-1: >> > CH0 DIMM L size: 32 Gb, width: X16, ranks: 2, 16Gb+ DIMMs: no >> > CH0 DIMM S size: 32 Gb, width: X16, ranks: 2, 16Gb+ DIMMs: no >> > CH0 ranks: 2, 16Gb+ DIMMs: no >> > CH1 DIMM L size: 32 Gb, width: X16, ranks: 2, 16Gb+ DIMMs: no >> > CH1 DIMM S size: 32 Gb, width: X16, ranks: 2, 16Gb+ DIMMs: no >> > CH1 ranks: 2, 16Gb+ DIMMs: no >> > Memory configuration is symmetric? yes >> > >> >Signed-off-by: Ville Syrjälä <[email protected]> >> >--- >> > drivers/gpu/drm/i915/intel_mchbar_regs.h | 53 +++++--- >> > drivers/gpu/drm/i915/soc/intel_dram.c | 166 +++++++++++++++++------ >> > 2 files changed, 155 insertions(+), 64 deletions(-) >> > >> >diff --git a/drivers/gpu/drm/i915/intel_mchbar_regs.h >> >b/drivers/gpu/drm/i915/intel_mchbar_regs.h >> >index a46a45b9d2e1..614d4017b57b 100644 >> >--- a/drivers/gpu/drm/i915/intel_mchbar_regs.h >> >+++ b/drivers/gpu/drm/i915/intel_mchbar_regs.h >> >@@ -160,25 +160,40 @@ >> > >> > #define SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN >> > _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x500C) >> > #define SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN >> > _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5010) >> >-#define SKL_DRAM_S_SHIFT 16 >> >-#define SKL_DRAM_RANK_MASK REG_GENMASK(10, 10) >> >-#define SKL_DRAM_RANK_1 >> >REG_FIELD_PREP(SKL_DRAM_RANK_MASK, 0) >> >-#define SKL_DRAM_RANK_2 >> >REG_FIELD_PREP(SKL_DRAM_RANK_MASK, 1) >> >-#define SKL_DRAM_WIDTH_MASK REG_GENMASK(9, 8) >> >-#define SKL_DRAM_WIDTH_X8 >> >REG_FIELD_PREP(SKL_DRAM_WIDTH_MASK, 0) >> >-#define SKL_DRAM_WIDTH_X16 >> >REG_FIELD_PREP(SKL_DRAM_WIDTH_MASK, 1) >> >-#define SKL_DRAM_WIDTH_X32 >> >REG_FIELD_PREP(SKL_DRAM_WIDTH_MASK, 2) >> >-#define SKL_DRAM_SIZE_MASK REG_GENMASK(5, 0) >> >-#define ICL_DRAM_RANK_MASK REG_GENMASK(10, 9) >> >-#define ICL_DRAM_RANK_1 >> >REG_FIELD_PREP(ICL_DRAM_RANK_MASK, 0) >> >-#define ICL_DRAM_RANK_2 >> >REG_FIELD_PREP(ICL_DRAM_RANK_MASK, 1) >> >-#define ICL_DRAM_RANK_3 >> >REG_FIELD_PREP(ICL_DRAM_RANK_MASK, 2) >> >-#define ICL_DRAM_RANK_4 >> >REG_FIELD_PREP(ICL_DRAM_RANK_MASK, 3) >> >-#define ICL_DRAM_WIDTH_MASK REG_GENMASK(8, 7) >> >-#define ICL_DRAM_WIDTH_X8 >> >REG_FIELD_PREP(ICL_DRAM_WIDTH_MASK, 0) >> >-#define ICL_DRAM_WIDTH_X16 >> >REG_FIELD_PREP(ICL_DRAM_WIDTH_MASK, 1) >> >-#define ICL_DRAM_WIDTH_X32 >> >REG_FIELD_PREP(ICL_DRAM_WIDTH_MASK, 2) >> >-#define ICL_DRAM_SIZE_MASK REG_GENMASK(6, 0) >> >+#define SKL_DIMM_S_RANK_MASK REG_GENMASK(26, 26) >> >+#define SKL_DIMM_S_RANK_1 >> >REG_FIELD_PREP(SKL_DIMM_S_RANK_MASK, 0) >> >+#define SKL_DIMM_S_RANK_2 >> >REG_FIELD_PREP(SKL_DIMM_S_RANK_MASK, 1) >> >+#define SKL_DIMM_S_WIDTH_MASK REG_GENMASK(25, 24) >> >+#define SKL_DIMM_S_WIDTH_X8 >> >REG_FIELD_PREP(SKL_DIMM_S_WIDTH_MASK, 0) >> >+#define SKL_DIMM_S_WIDTH_X16 >> >REG_FIELD_PREP(SKL_DIMM_S_WIDTH_MASK, 1) >> >+#define SKL_DIMM_S_WIDTH_X32 >> >REG_FIELD_PREP(SKL_DIMM_S_WIDTH_MASK, 2) >> >+#define SKL_DIMM_S_SIZE_MASK REG_GENMASK(21, 16) >> >+#define SKL_DIMM_L_RANK_MASK REG_GENMASK(10, 10) >> >+#define SKL_DIMM_L_RANK_1 >> >REG_FIELD_PREP(SKL_DIMM_L_RANK_MASK, 0) >> >+#define SKL_DIMM_L_RANK_2 >> >REG_FIELD_PREP(SKL_DIMM_L_RANK_MASK, 1) >> >+#define SKL_DIMM_L_WIDTH_MASK REG_GENMASK(9, 8) >> >+#define SKL_DIMM_L_WIDTH_X8 >> >REG_FIELD_PREP(SKL_DIMM_L_WIDTH_MASK, 0) >> >+#define SKL_DIMM_L_WIDTH_X16 >> >REG_FIELD_PREP(SKL_DIMM_L_WIDTH_MASK, 1) >> >+#define SKL_DIMM_L_WIDTH_X32 >> >REG_FIELD_PREP(SKL_DIMM_L_WIDTH_MASK, 2) >> >+#define SKL_DIMM_L_SIZE_MASK REG_GENMASK(5, 0) >> >+#define ICL_DIMM_S_RANK_MASK REG_GENMASK(27, 26) >> >+#define ICL_DIMM_S_RANK_1 >> >REG_FIELD_PREP(ICL_DIMM_S_RANK_MASK, 0) >> >+#define ICL_DIMM_S_RANK_2 >> >REG_FIELD_PREP(ICL_DIMM_S_RANK_MASK, 1) >> >+#define ICL_DIMM_S_WIDTH_MASK REG_GENMASK(25, 24) >> >+#define ICL_DIMM_S_WIDTH_X8 >> >REG_FIELD_PREP(ICL_DIMM_S_WIDTH_MASK, 0) >> >+#define ICL_DIMM_S_WIDTH_X16 >> >REG_FIELD_PREP(ICL_DIMM_S_WIDTH_MASK, 1) >> >+#define ICL_DIMM_S_WIDTH_X32 >> >REG_FIELD_PREP(ICL_DIMM_S_WIDTH_MASK, 2) >> >+#define ICL_DIMM_S_SIZE_MASK REG_GENMASK(22, 16) >> >+#define ICL_DIMM_L_RANK_MASK REG_GENMASK(10, 9) >> >+#define ICL_DIMM_L_RANK_1 >> >REG_FIELD_PREP(ICL_DIMM_L_RANK_MASK, 0) >> >+#define ICL_DIMM_L_RANK_2 >> >REG_FIELD_PREP(ICL_DIMM_L_RANK_MASK, 1) >> >+#define ICL_DIMM_L_RANK_3 >> >REG_FIELD_PREP(ICL_DIMM_L_RANK_MASK, 2) >> >+#define ICL_DIMM_L_RANK_4 >> >REG_FIELD_PREP(ICL_DIMM_L_RANK_MASK, 3) >> >+#define ICL_DIMM_L_WIDTH_MASK REG_GENMASK(8, 7) >> >+#define ICL_DIMM_L_WIDTH_X8 >> >REG_FIELD_PREP(ICL_DIMM_L_WIDTH_MASK, 0) >> >+#define ICL_DIMM_L_WIDTH_X16 >> >REG_FIELD_PREP(ICL_DIMM_L_WIDTH_MASK, 1) >> >+#define ICL_DIMM_L_WIDTH_X32 >> >REG_FIELD_PREP(ICL_DIMM_L_WIDTH_MASK, 2) >> >+#define ICL_DIMM_L_SIZE_MASK REG_GENMASK(6, 0) >> >> so we have: >> >> ICL_DIMM_L_RANK_MASK REG_GENMASK(10 , 9) >> ICL_DIMM_L_WIDTH_MASK REG_GENMASK(8 , 7) >> ICL_DIMM_L_SIZE_MASK REG_GENMASK(6 , 0) >> >> vs >> >> ICL_DIMM_S_RANK_MASK REG_GENMASK(11 + 16, 10 + 16) >> ICL_DIMM_S_WIDTH_MASK REG_GENMASK(9 + 16, 8 + 16) >> ICL_DIMM_S_SIZE_MASK REG_GENMASK(6 + 16, 0 + 16) >> >> >> I can't really check the soc spec right now, but this fix seems very >> verbose. Maybe the first patch could fix the bug in the simple way and >> then have these refactors on top? >> >> u16 lval = val & 0xffff; >> u16 sval = val >> 16; >> >> /* >> * Some cursing here for format change - "fix" it up by making it compatible >> * with the lower bits by doing shr where appropriate >> */ >> sval = (sval & ICL_DIMM_L_SIZE_MASK) | >> ((sval >> 1) & ICL_DIMM_L_WIDTH_MASK) | >> ((sval >> 1) & ICL_DIMM_L_RANK_MASK); >> >> ... >> >> > static int >> > skl_dram_get_channel_info(struct drm_i915_private *i915, >> > struct dram_channel_info *ch, >> > int channel, u32 val) >> > { >> >- skl_dram_get_dimm_info(i915, &ch->dimm_l, >> >- channel, 'L', val & 0xffff); >> >- skl_dram_get_dimm_info(i915, &ch->dimm_s, >> >- channel, 'S', val >> 16); >> >> Which would mean basically changing this function to derive sval and >> lval as above (untested). This could easily propagate to through stable. > > Meh. We have no know issues that this fixes so wasn't planning on > a stable backport anyway.
I'm fine with backporting the deps if we end up having to backport this. It's a hard diff to read, but I couldn't spot any issues. Reviewed-by: Jani Nikula <[email protected]> -- Jani Nikula, Intel
