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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 07 Nov 2025, Ville Syrjälä <ville.syrjala@linux.intel.com> 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ä <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.

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 <jani.nikula@intel.com>


-- 
Jani Nikula, Intel