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. -- Ville Syrjälä Intel
