Re: [PATCH 06/32] drm/i915/xe3p_lpd: Update bandwidth parameters

[Gustavo Sousa <gustavo.sousa@intel.com>]

Quoting Matt Roper (2025-10-15 16:12:41-03:00)
>On Wed, Oct 15, 2025 at 03:12:34PM -0300, Gustavo Sousa wrote:
>> Quoting Matt Roper (2025-10-15 14:48:30-03:00)
>> >On Wed, Oct 15, 2025 at 12:15:06AM -0300, Gustavo Sousa wrote:
>> >> From: Matt Atwood <matthew.s.atwood@intel.com>
>> >> 
>> >> Bandwidth parameters for Xe3p_LPD are basically the same as for Xe3_LPD.
>> >> However, now Xe3p_LPD has the ecc_impacting_de field, which could impact
>> >> how the derating is defined.
>> >> 
>> >> For the cases where that field is true, we use xe3p_lpd_ecc_sa_info,
>> >> similarly to what was done for Xe2_HPD.  Note, however, that Bspec
>> >> specifies the ECC derating value only for GDDR memory.  For now, we just
>> >> re-use the value that was defined for Xe2_HPD, namely 45.  We need to
>> >> confirm with the hardware team what would be the correct value(s) to use
>> >> for the ECC case.
>> >
>> >I think we need to use .derating = 10.  This specific block of the bspec
>> >is a shared block that applies to lots of IPs/platforms.  GDDR isn't
>> >relevant to an LPD platform and .derating = 10 is the documented value
>> >for all other types of memory.
>> 
>> In that case, do mean we should drop the patch adding the field
>> ecc_impacting_de and unconditionally use xe3lpd_sa_info?
>
>They're somewhat orthogonal.  The hardware (or rather firmware I guess?)
>now has a way to tell software that there's ECC present that would
>impact bandwidth, and in general that notification could be used with
>any kind of RAM.  Some platforms will never have a situation where ECC
>matters to bandwidth (so this new flag will never be set), some igpu
>platforms will have cases where system memory ECC impacts bandwidth, and
>some dgpu platforms will have cases where vram ECC impacts bandwidth.
>We don't have any relevant rules at the moment, but real details may get
>added to the spec later as we get closer to supporting the specific
>platform(s) that these IP versions will be incorporated into.  But
>adding the general ability to read out the new flag and have it ready
>for when platform-specific details start arriving in the future seems
>fine to me.  We could add a warning print if the flag is actually
>getting set on some platform before we have any rules documenting what
>we're supposed to do about it.

Yep.  Adding a warning sounds good to me.  I think that would be better
as part of the previous patch.

>
>In general, I'm wondering if the memory bandwidth numbers are something
>that we should consider moving back to platform-based checks.  The
>hardware teams tie these kinds of changes to tickets associated with
>specific IPs, but that's mostly just because of how the databases for
>hardware changes are organized these days.  The reality is that the
>details for memory characteristics are something that's more defined by
>the underlying platform rather than the IP (and that's especially true
>for igpu platforms where where we're talking about system memory that's
>used by the CPU and all the other devices on the platform as well).

Hm... Probably.  As an example that illustrates your point, we have PTL
and WCL, which contain the same display architecture, but one different
instance of struct intel_sa_info for each.

--
Gustavo Sousa

>
>
>Matt
>
>> 
>> In the meantime I'll try to get clarifications from HW team on this.
>> 
>> --
>> Gustavo Sousa
>> 
>> >
>> >
>> >Matt
>> >
>> >> 
>> >> Bspec: 68859, 69131
>> >> Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com>
>> >> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> >> ---
>> >>  drivers/gpu/drm/i915/display/intel_bw.c | 21 ++++++++++++++++++++-
>> >>  1 file changed, 20 insertions(+), 1 deletion(-)
>> >> 
>> >> diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
>> >> index 8f5b86cd91b6..f0940ff9d19b 100644
>> >> --- a/drivers/gpu/drm/i915/display/intel_bw.c
>> >> +++ b/drivers/gpu/drm/i915/display/intel_bw.c
>> >> @@ -461,6 +461,20 @@ static const struct intel_sa_info xe3lpd_3002_sa_info = {
>> >>          .derating = 10,
>> >>  };
>> >>  
>> >> +static const struct intel_sa_info xe3p_lpd_ecc_sa_info = {
>> >> +        .deburst = 32,
>> >> +        .deprogbwlimit = 65, /* GB/s */
>> >> +        .displayrtids = 256,
>> >> +        /*
>> >> +         * FIXME: The Bspec only shows that derating for ECC should be 45 for
>> >> +         * GDDR memory and does not mention other types of memory.  For now, we
>> >> +         * just re-use that value, but we need to confirm whether that is
>> >> +         * correct or if there are different values depending on the memory
>> >> +         * type.
>> >> +         */
>> >> +        .derating = 45,
>> >> +};
>> >> +
>> >>  static int icl_get_bw_info(struct intel_display *display,
>> >>                             const struct dram_info *dram_info,
>> >>                             const struct intel_sa_info *sa)
>> >> @@ -812,7 +826,12 @@ void intel_bw_init_hw(struct intel_display *display)
>> >>          if (!HAS_DISPLAY(display))
>> >>                  return;
>> >>  
>> >> -        if (DISPLAY_VERx100(display) >= 3002) {
>> >> +        if (DISPLAY_VER(display) >= 35) {
>> >> +                if (dram_info->ecc_impacting_de)
>> >> +                        tgl_get_bw_info(display, dram_info, &xe3p_lpd_ecc_sa_info);
>> >> +                else
>> >> +                        tgl_get_bw_info(display, dram_info, &xe3lpd_sa_info);
>> >> +        } else if (DISPLAY_VERx100(display) >= 3002) {
>> >>                  tgl_get_bw_info(display, dram_info, &xe3lpd_3002_sa_info);
>> >>          } else if (DISPLAY_VER(display) >= 30) {
>> >>                  tgl_get_bw_info(display, dram_info, &xe3lpd_sa_info);
>> >> 
>> >> -- 
>> >> 2.51.0
>> >> 
>> >
>> >-- 
>> >Matt Roper
>> >Graphics Software Engineer
>> >Linux GPU Platform Enablement
>> >Intel Corporation
>
>-- 
>Matt Roper
>Graphics Software Engineer
>Linux GPU Platform Enablement
>Intel Corporation