Quoting Jani Nikula (2025-10-22 08:53:34-03:00)
>On Wed, 22 Oct 2025, Gustavo Sousa <[email protected]> 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 <[email protected]>
>>>Cc: Matt Atwood <[email protected]>
>>>Cc: Matt Roper <[email protected]>
>>>Signed-off-by: Gustavo Sousa <[email protected]>
>>>---
>>> 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.
Okay. I see your point. So, perhaps I should keep the
drm_WARN_ON_ONCE() inside the display version >= 35 case?
--
Gustavo Sousa
>
>
>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
