Re: [PATCH v2 04/32] drm/i915/dram: Add field ecc_impacting_de_bw

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

On Wed, 22 Oct 2025, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
> Quoting Gustavo Sousa (2025-10-21 21:28:29-03:00)
>>Starting with Xe3p_LPD, we now have a new field in MEM_SS_INFO_GLOBAL
>>that indicates whether the memory has enabled ECC that limits display
>>bandwidth.  Add the field ecc_impacting_de_bw to struct dram_info to
>>contain that information and set it appropriately when probing for
>>memory info.
>>
>>Currently there are no instructions in Bspec on how to handle that case,
>>so let's throw a warning if we ever find such a scenario.
>>
>>v2:
>>  - s/ecc_impacting_de/ecc_impacting_de_bw/ to be more specific. (Matt
>>    Atwood)
>>  - Add warning if ecc_impacting_de_bw is true, since we currently do
>>    not have instructions on how to handle it. (Matt Roper)
>>
>>Bspec: 69131
>>Cc: Jani Nikula <jani.nikula@linux.intel.com>
>>Cc: Matt Atwood <matthew.s.atwood@intel.com>
>>Cc: Matt Roper <matthew.d.roper@intel.com>
>>Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>>---
>> drivers/gpu/drm/i915/display/intel_bw.c | 8 ++++++++
>> drivers/gpu/drm/i915/i915_reg.h         | 1 +
>> drivers/gpu/drm/i915/soc/intel_dram.c   | 4 ++++
>> drivers/gpu/drm/i915/soc/intel_dram.h   | 1 +
>> 4 files changed, 14 insertions(+)
>>
>>diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
>>index fc173b2a1ad9..57d65e6e5429 100644
>>--- a/drivers/gpu/drm/i915/display/intel_bw.c
>>+++ b/drivers/gpu/drm/i915/display/intel_bw.c
>>@@ -802,6 +802,14 @@ void intel_bw_init_hw(struct intel_display *display)
>>         if (!HAS_DISPLAY(display))
>>                 return;
>> 
>>+        /*
>>+         * Starting with Xe3p_LPD, the hardware tells us whether memory has ECC
>>+         * enabled that would impact display bandwidth.  However, so far there
>>+         * are no instructions in Bspec on how to handle that case.  Let's
>>+         * complain if we ever find such a scenario.
>>+         */
>>+        drm_WARN_ON_ONCE(display->drm, dram_info->ecc_impacting_de_bw);
>
> Oops.  Just realized that DG2 does not have dram_info.  Thanks, CI!
>
> I'll fix this on the next version, probably with:
>
>     drm_WARN_ON_ONCE(display->drm, dram_info &&
>     dram_info->ecc_impacting_de_bw);

The comment I added above intel_dram_info():

/*
 * Returns NULL for platforms that don't have dram info. Avoid overzealous NULL
 * checks, and prefer not dereferencing on platforms that shouldn't look at dram
 * info, to catch accidental and incorrect dram info checks.
 */

You caught the whole thing on CI *because* there was no NULL check. With
the NULL check, you'll ignore missing dram info on future platforms that
should have it.


BR,
Jani.




>
> --
> Gustavo Sousa
>
>>+
>>         if (DISPLAY_VERx100(display) >= 3002) {
>>                 tgl_get_bw_info(display, dram_info, &xe3lpd_3002_sa_info);
>>         } else if (DISPLAY_VER(display) >= 30) {
>>diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>index 354ef75ef6a5..5bf3b4ab2baa 100644
>>--- a/drivers/gpu/drm/i915/i915_reg.h
>>+++ b/drivers/gpu/drm/i915/i915_reg.h
>>@@ -1233,6 +1233,7 @@
>> #define   OROM_OFFSET_MASK                        REG_GENMASK(20, 16)
>> 
>> #define MTL_MEM_SS_INFO_GLOBAL                        _MMIO(0x45700)
>>+#define   XE3P_ECC_IMPACTING_DE                        REG_BIT(12)
>> #define   MTL_N_OF_ENABLED_QGV_POINTS_MASK        REG_GENMASK(11, 8)
>> #define   MTL_N_OF_POPULATED_CH_MASK                REG_GENMASK(7, 4)
>> #define   MTL_DDR_TYPE_MASK                        REG_GENMASK(3, 0)
>>diff --git a/drivers/gpu/drm/i915/soc/intel_dram.c b/drivers/gpu/drm/i915/soc/intel_dram.c
>>index 8841cfe1cac8..73e8ad1a28e0 100644
>>--- a/drivers/gpu/drm/i915/soc/intel_dram.c
>>+++ b/drivers/gpu/drm/i915/soc/intel_dram.c
>>@@ -685,6 +685,7 @@ static int gen12_get_dram_info(struct drm_i915_private *i915, struct dram_info *
>> 
>> static int xelpdp_get_dram_info(struct drm_i915_private *i915, struct dram_info *dram_info)
>> {
>>+        struct intel_display *display = i915->display;
>>         u32 val = intel_uncore_read(&i915->uncore, MTL_MEM_SS_INFO_GLOBAL);
>> 
>>         switch (REG_FIELD_GET(MTL_DDR_TYPE_MASK, val)) {
>>@@ -723,6 +724,9 @@ static int xelpdp_get_dram_info(struct drm_i915_private *i915, struct dram_info
>>         dram_info->num_qgv_points = REG_FIELD_GET(MTL_N_OF_ENABLED_QGV_POINTS_MASK, val);
>>         /* PSF GV points not supported in D14+ */
>> 
>>+        if (DISPLAY_VER(display) >= 35)
>>+                dram_info->ecc_impacting_de_bw = REG_FIELD_GET(XE3P_ECC_IMPACTING_DE, val);
>>+
>>         return 0;
>> }
>> 
>>diff --git a/drivers/gpu/drm/i915/soc/intel_dram.h b/drivers/gpu/drm/i915/soc/intel_dram.h
>>index 03a973f1c941..8475ee379daa 100644
>>--- a/drivers/gpu/drm/i915/soc/intel_dram.h
>>+++ b/drivers/gpu/drm/i915/soc/intel_dram.h
>>@@ -30,6 +30,7 @@ struct dram_info {
>>         u8 num_channels;
>>         u8 num_qgv_points;
>>         u8 num_psf_gv_points;
>>+        bool ecc_impacting_de_bw; /* Only valid from Xe3p_LPD onward. */
>>         bool symmetric_memory;
>>         bool has_16gb_dimms;
>> };
>>
>>-- 
>>2.51.0
>>

-- 
Jani Nikula, Intel