Re: [PATCH 3/3] drm/i915/dram: Fix ICL DIMM_S decoding

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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ä <ville.syrjala@linux.intel.com>
> >
> >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ä <ville.syrjala@linux.intel.com>
> >---
> > 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