Re: [PATCH 1/4] drm/i915/bw: Extract platform-specific parameters

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

Matt Roper <matthew.d.roper@intel.com> writes:

> On Fri, Apr 10, 2026 at 11:39:10AM -0300, Gustavo Sousa wrote:
>> Matt Roper <matthew.d.roper@intel.com> writes:
>> 
>> > On Wed, Apr 08, 2026 at 10:16:42PM +0300, Jani Nikula wrote:
>> >> On Wed, 08 Apr 2026, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
>> >> > We got confirmation from the hardware team that the bandwidth parameters
>> >> > deprogbwlimit and derating are platform-specific and not tied to the
>> >> > display IP.  As such, let's make sure that we use platform checks for
>> >> > those.
>> >> >
>> >> > The rest of the members of struct intel_sa_info are tied to the display
>> >> > IP and we will deal with them as a follow-up.
>> >> >
>> >> > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> >> > ---
>> >> >  drivers/gpu/drm/i915/display/intel_bw.c | 174 ++++++++++++++++++++++++--------
>> >> >  1 file changed, 133 insertions(+), 41 deletions(-)
>> >> >
>> >> > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
>> >> > index 474438fc1ebc..ed840b592eff 100644
>> >> > --- a/drivers/gpu/drm/i915/display/intel_bw.c
>> >> > +++ b/drivers/gpu/drm/i915/display/intel_bw.c
>> >> > @@ -375,77 +375,170 @@ static int icl_sagv_max_dclk(const struct intel_qgv_info *qi)
>> >> >  	return dclk;
>> >> >  }
>> >> >  
>> >> > +struct intel_platform_bw_params {
>> >> > +	u8 deprogbwlimit;
>> >> > +	u8 derating;
>> >> > +};
>> >> > +
>> >> > +static const struct intel_platform_bw_params icl_plat_bw_params = {
>> >> > +	.deprogbwlimit = 25,
>> >> > +	.derating = 10,
>> >> > +};
>> >> > +
>> >> > +static const struct intel_platform_bw_params tgl_plat_bw_params = {
>> >> > +	.deprogbwlimit = 34,
>> >> > +	.derating = 10,
>> >> > +};
>> >> > +
>> >> > +static const struct intel_platform_bw_params rkl_plat_bw_params = {
>> >> > +	.deprogbwlimit = 20,
>> >> > +	.derating = 10,
>> >> > +};
>> >> > +
>> >> > +static const struct intel_platform_bw_params adl_s_plat_bw_params = {
>> >> > +	.deprogbwlimit = 38,
>> >> > +	.derating = 10,
>> >> > +};
>> >> > +
>> >> > +static const struct intel_platform_bw_params adl_p_plat_bw_params = {
>> >> > +	.deprogbwlimit = 38,
>> >> > +	.derating = 20,
>> >> > +};
>> >> > +
>> >> > +static const struct intel_platform_bw_params bmg_plat_bw_params = {
>> >> > +	.deprogbwlimit = 53,
>> >> > +	.derating = 30,
>> >> > +};
>> >> > +
>> >> > +static const struct intel_platform_bw_params bmg_ecc_plat_bw_params = {
>> >> > +	.deprogbwlimit = 53,
>> >> > +	.derating = 45,
>> >> > +};
>> >> > +
>> >> > +static const struct intel_platform_bw_params ptl_plat_bw_params = {
>> >> > +	.deprogbwlimit = 65,
>> >> > +	.derating = 10,
>> >> > +};
>> >> > +
>> >> > +static const struct intel_platform_bw_params wcl_plat_bw_params = {
>> >> > +	.deprogbwlimit = 22,
>> >> > +	.derating = 10,
>> >> > +};
>> >> 
>> >> In the above, "plat" feels like tautology, since they're all prefixed by
>> >> platform acronyms.
>> >
>> > "soc" might be more consistent with what we (and various hardware docs)
>> > do to refer to "stuff that's outside the graphics/media/display IP and
>> > doesn't relate to GMD_ID version numbers."  Technically "soc" is a bit
>> > of a misnomer too since a lot of our recent platforms are multi-chip and
>> > not truly SoC's anymore, but the intent is still understandable.
>> 
>> Yeah. I intetionally prefered to use "platform" as a general term to
>> refer to either SoC or multi-chip package.  Do you prefer that we name
>> the struct type intel_soc_bw_params?
>> 
>> What about intel_display_bw_params that is added later? Is that a good
>> name? I thought intel_ip_bw_params would be a bit vague, since we have
>> different types of IPs (display being one of them) in the platform.
>
> Personally I think "soc_bw_params" vs "display_bw_params" seems like a
> similar distinction to what we have elsewhere in the driver (especially
> if we add a 1-sentence comment above the structure clarifying what the
> origin/source of those parameters is.  But I'll leave it up to Jani and
> the other display experts to make the call since they're the ones who
> work with this code the most.
>
>> 
>> >
>> >> 
>> >> > +
>> >> > +static const struct intel_platform_bw_params *get_platform_bw_params(struct intel_display *display)
>> >> > +{
>> >> > +	const struct intel_platform_bw_params *ret;
>> >> > +
>> >> > +	if (display->platform.dgfx)
>> >> > +		goto dgfx;
>> >> > +
>> >> > +	ret = &icl_plat_bw_params;
>> >> > +	if (display->platform.icelake ||
>> >> > +	    display->platform.jasperlake ||
>> >> > +	    display->platform.elkhartlake)
>> >> > +		return ret;
>> >> 
>> >> What's the point of assigning and returning ret?
>> >> 
>> >> Why not just return &icl_plat_bw_params; directly?
>> >> 
>> >
>> > It looks like the intent might have been to let people keep copy/pasting
>> > the same pattern and have the fallback at the end always default back to
>> > whatever the "newest" one was if a proper match wasn't found.  But I
>> > agree that the handling here feels awkward and a simple if/else ladder
>> > would be preferable.
>> 
>> Yeah, allowing developers to easily add new platforms without too much
>> churn was the intention here.  I knew this style was unconventional, but
>> I thought the intent justified it (and IMO the code is still readable,
>> although admittedly a bit weird).
>> 
>> If that's not acceptable, would something along the lines of below be
>> accepted?
>> 
>>     if (display->platformOB.dgfx) {
>>         if (...)
>>              return platform_a_params;
>>         else if (...)
>>              return platform_b_params;
>>         else if (...)
>>              return platform_c_params;
>>         
>>         default_params = platform_c_params;
>>     } else {
>>         if (...)
>>              return platform_d_params;
>>         else if (...)
>>              return platform_e_params;
>>         else if (...)
>>              return platform_f_params;
>>         
>>         default_params = platform_f_params;
>>     }
>>     
>>     do_warning();
>>     return default_params;
>
> Yeah, I think a traditional if/else ladder like this is best.  I don't
> think we even need to track a 'default_params' variable; we can just
> directly return some recent platform as a fallback at the end too.  If
> the fallback winds up not getting updated when we add new platforms, I
> don't think that really matters since there's no real guarantee that
> falling back to incorrect n-1 platform numbers is better than falling
> back to incorrect n-2 platform numbers.

Well, I would like to make a distinction between client and discrete
(e.g. there is a significant difference between BMG's and
PTL's derating param).  That would be the reason for the default_params
variable, which would allow a single place to raise the warning.

>
> If we've screwed up and forgotten to add the parameters for a new
> platform, then that's going to be something that's flagged almost
> immediately by CI and will be quickly fixed long before the platform
> ever leaves force_probe.

Yeah, true.  Another possibility is to just return NULL and purposefully
cause anull-pointer dereference; or even have the default being all
zeros, so that we don't worry about updating the default from time to
time.

--
Gustavo Sousa

>
>
> Matt
>
>> --
>> Gustavo Sousa
>> 
>> >
>> >
>> > Matt
>> >
>> >> > +
>> >> > +	ret = &tgl_plat_bw_params;
>> >> > +	if (display->platform.tigerlake)
>> >> > +		return ret;
>> >> > +
>> >> > +	ret = &rkl_plat_bw_params;
>> >> > +	if (display->platform.rocketlake)
>> >> > +		return ret;
>> >> > +
>> >> > +	ret = &adl_s_plat_bw_params;
>> >> > +	if (display->platform.alderlake_s)
>> >> > +		return ret;
>> >> > +
>> >> > +	ret = &adl_p_plat_bw_params;
>> >> > +	if (display->platform.alderlake_p)
>> >> > +		return ret;
>> >> > +
>> >> > +	ret = &adl_s_plat_bw_params;
>> >> > +	if (display->platform.meteorlake ||
>> >> > +	    display->platform.lunarlake)
>> >> > +		return ret;
>> >> > +
>> >> > +	ret = &ptl_plat_bw_params;
>> >> > +	if (display->platform.pantherlake ||
>> >> > +	    display->platform.novalake) {
>> >> > +		if (display->platform.pantherlake_wildcatlake)
>> >> > +			ret = &wcl_plat_bw_params;
>> >> > +
>> >> > +		return ret;
>> >> > +	}
>> >> > +
>> >> > +	goto missing;
>> >> > +
>> >> > +dgfx:
>> >> > +	ret = &tgl_plat_bw_params;
>> >> > +	if (display->platform.dg1)
>> >> > +		return ret;
>> >> > +
>> >> > +	ret = &bmg_plat_bw_params;
>> >> > +	if (display->platform.battlemage) {
>> >> > +		const struct dram_info *dram_info = intel_dram_info(display);
>> >> > +
>> >> > +		if (dram_info->type == INTEL_DRAM_GDDR_ECC)
>> >> > +			ret = &bmg_ecc_plat_bw_params;
>> >> > +
>> >> > +		return ret;
>> >> > +	}
>> >> > +
>> >> > +missing:
>> >> > +	/*
>> >> > +	 * Use parameters from the most recent platform,
>> >> > +	 * but raise a warning.
>> >> > +	 */
>> >> > +	drm_WARN(display->drm, 1,
>> >> > +		 "Platform-specific bandwidth parameters not found, using possibly incompatible default values\n");
>> >> > +
>> >> > +	return ret;
>> >> 
>> >> I don't understand at all why the function is written the way it
>> >> is. Seems like it should be a regular if-ladder like we have, with zero
>> >> gotos.
>> >> 
>> >> > +}
>> >> > +
>> >> >  struct intel_sa_info {
>> >> >  	u16 displayrtids;
>> >> > -	u8 deburst, deprogbwlimit, derating;
>> >> > +	u8 deburst;
>> >> >  };
>> >> >  
>> >> >  static const struct intel_sa_info icl_sa_info = {
>> >> >  	.deburst = 8,
>> >> > -	.deprogbwlimit = 25, /* GB/s */
>> >> >  	.displayrtids = 128,
>> >> > -	.derating = 10,
>> >> >  };
>> >> >  
>> >> >  static const struct intel_sa_info tgl_sa_info = {
>> >> >  	.deburst = 16,
>> >> > -	.deprogbwlimit = 34, /* GB/s */
>> >> >  	.displayrtids = 256,
>> >> > -	.derating = 10,
>> >> >  };
>> >> >  
>> >> >  static const struct intel_sa_info rkl_sa_info = {
>> >> >  	.deburst = 8,
>> >> > -	.deprogbwlimit = 20, /* GB/s */
>> >> >  	.displayrtids = 128,
>> >> > -	.derating = 10,
>> >> >  };
>> >> >  
>> >> >  static const struct intel_sa_info adls_sa_info = {
>> >> >  	.deburst = 16,
>> >> > -	.deprogbwlimit = 38, /* GB/s */
>> >> >  	.displayrtids = 256,
>> >> > -	.derating = 10,
>> >> >  };
>> >> >  
>> >> >  static const struct intel_sa_info adlp_sa_info = {
>> >> >  	.deburst = 16,
>> >> > -	.deprogbwlimit = 38, /* GB/s */
>> >> >  	.displayrtids = 256,
>> >> > -	.derating = 20,
>> >> >  };
>> >> >  
>> >> >  static const struct intel_sa_info mtl_sa_info = {
>> >> >  	.deburst = 32,
>> >> > -	.deprogbwlimit = 38, /* GB/s */
>> >> >  	.displayrtids = 256,
>> >> > -	.derating = 10,
>> >> > -};
>> >> > -
>> >> > -static const struct intel_sa_info xe2_hpd_sa_info = {
>> >> > -	.derating = 30,
>> >> > -	.deprogbwlimit = 53,
>> >> > -	/* Other values not used by simplified algorithm */
>> >> > -};
>> >> > -
>> >> > -static const struct intel_sa_info xe2_hpd_ecc_sa_info = {
>> >> > -	.derating = 45,
>> >> > -	.deprogbwlimit = 53,
>> >> > -	/* Other values not used by simplified algorithm */
>> >> >  };
>> >> >  
>> >> >  static const struct intel_sa_info xe3lpd_sa_info = {
>> >> >  	.deburst = 32,
>> >> > -	.deprogbwlimit = 65, /* GB/s */
>> >> >  	.displayrtids = 256,
>> >> > -	.derating = 10,
>> >> >  };
>> >> >  
>> >> >  static const struct intel_sa_info xe3lpd_3002_sa_info = {
>> >> >  	.deburst = 32,
>> >> > -	.deprogbwlimit = 22, /* GB/s */
>> >> >  	.displayrtids = 256,
>> >> > -	.derating = 10,
>> >> >  };
>> >> >  
>> >> >  static int icl_get_bw_info(struct intel_display *display,
>> >> > @@ -453,6 +546,7 @@ static int icl_get_bw_info(struct intel_display *display,
>> >> >  			   const struct intel_sa_info *sa)
>> >> >  {
>> >> >  	struct intel_qgv_info qi = {};
>> >> > +	const struct intel_platform_bw_params *plat_bw_params = get_platform_bw_params(display);
>> >> 
>> >> Perhaps it would be better to pass this in instead of every function
>> >> having the call.
>> >> 
>> >> Nitpick, "plat" is not an abbreviation I'm fond of.
>> >> 
>> >> >  	bool is_y_tile = true; /* assume y tile may be used */
>> >> >  	int num_channels = max_t(u8, 1, dram_info->num_channels);
>> >> >  	int ipqdepth, ipqdepthpch = 16;
>> >> > @@ -469,7 +563,7 @@ static int icl_get_bw_info(struct intel_display *display,
>> >> >  	}
>> >> >  
>> >> >  	dclk_max = icl_sagv_max_dclk(&qi);
>> >> > -	maxdebw = min(sa->deprogbwlimit * 1000, dclk_max * 16 * 6 / 10);
>> >> > +	maxdebw = min(plat_bw_params->deprogbwlimit * 1000, dclk_max * 16 * 6 / 10);
>> >> >  	ipqdepth = min(ipqdepthpch, sa->displayrtids / num_channels);
>> >> >  	qi.deinterleave = DIV_ROUND_UP(num_channels, is_y_tile ? 4 : 2);
>> >> >  
>> >> > @@ -499,7 +593,7 @@ static int icl_get_bw_info(struct intel_display *display,
>> >> >  			bw = DIV_ROUND_UP(sp->dclk * clpchgroup * 32 * num_channels, ct);
>> >> >  
>> >> >  			bi->deratedbw[j] = min(maxdebw,
>> >> > -					       bw * (100 - sa->derating) / 100);
>> >> > +					       bw * (100 - plat_bw_params->derating) / 100);
>> >> >  
>> >> >  			drm_dbg_kms(display->drm,
>> >> >  				    "BW%d / QGV %d: num_planes=%d deratedbw=%u\n",
>> >> > @@ -524,6 +618,7 @@ static int tgl_get_bw_info(struct intel_display *display,
>> >> >  			   const struct intel_sa_info *sa)
>> >> >  {
>> >> >  	struct intel_qgv_info qi = {};
>> >> > +	const struct intel_platform_bw_params *plat_bw_params = get_platform_bw_params(display);
>> >> >  	bool is_y_tile = true; /* assume y tile may be used */
>> >> >  	int num_channels = max_t(u8, 1, dram_info->num_channels);
>> >> >  	int ipqdepth, ipqdepthpch = 16;
>> >> > @@ -557,7 +652,7 @@ static int tgl_get_bw_info(struct intel_display *display,
>> >> >  	dclk_max = icl_sagv_max_dclk(&qi);
>> >> >  
>> >> >  	peakbw = num_channels * DIV_ROUND_UP(qi.channel_width, 8) * dclk_max;
>> >> > -	maxdebw = min(sa->deprogbwlimit * 1000, peakbw * DEPROGBWPCLIMIT / 100);
>> >> > +	maxdebw = min(plat_bw_params->deprogbwlimit * 1000, peakbw * DEPROGBWPCLIMIT / 100);
>> >> >  
>> >> >  	ipqdepth = min(ipqdepthpch, sa->displayrtids / num_channels);
>> >> >  	/*
>> >> > @@ -602,7 +697,7 @@ static int tgl_get_bw_info(struct intel_display *display,
>> >> >  			bw = DIV_ROUND_UP(sp->dclk * clpchgroup * 32 * num_channels, ct);
>> >> >  
>> >> >  			bi->deratedbw[j] = min(maxdebw,
>> >> > -					       bw * (100 - sa->derating) / 100);
>> >> > +					       bw * (100 - plat_bw_params->derating) / 100);
>> >> >  			bi->peakbw[j] = DIV_ROUND_CLOSEST(sp->dclk *
>> >> >  							  num_channels *
>> >> >  							  qi.channel_width, 8);
>> >> > @@ -663,10 +758,10 @@ static void dg2_get_bw_info(struct intel_display *display)
>> >> >  }
>> >> >  
>> >> >  static int xe2_hpd_get_bw_info(struct intel_display *display,
>> >> > -			       const struct dram_info *dram_info,
>> >> > -			       const struct intel_sa_info *sa)
>> >> > +			       const struct dram_info *dram_info)
>> >> >  {
>> >> >  	struct intel_qgv_info qi = {};
>> >> > +	const struct intel_platform_bw_params *plat_bw_params = get_platform_bw_params(display);
>> >> >  	int num_channels = dram_info->num_channels;
>> >> >  	int peakbw, maxdebw;
>> >> >  	int ret, i;
>> >> > @@ -679,14 +774,14 @@ static int xe2_hpd_get_bw_info(struct intel_display *display,
>> >> >  	}
>> >> >  
>> >> >  	peakbw = num_channels * qi.channel_width / 8 * icl_sagv_max_dclk(&qi);
>> >> > -	maxdebw = min(sa->deprogbwlimit * 1000, peakbw * DEPROGBWPCLIMIT / 10);
>> >> > +	maxdebw = min(plat_bw_params->deprogbwlimit * 1000, peakbw * DEPROGBWPCLIMIT / 10);
>> >> >  
>> >> >  	for (i = 0; i < qi.num_points; i++) {
>> >> >  		const struct intel_qgv_point *point = &qi.points[i];
>> >> >  		int bw = num_channels * (qi.channel_width / 8) * point->dclk;
>> >> >  
>> >> >  		display->bw.max[0].deratedbw[i] =
>> >> > -			min(maxdebw, (100 - sa->derating) * bw / 100);
>> >> > +			min(maxdebw, (100 - plat_bw_params->derating) * bw / 100);
>> >> >  		display->bw.max[0].peakbw[i] = bw;
>> >> >  
>> >> >  		drm_dbg_kms(display->drm, "QGV %d: deratedbw=%u peakbw: %u\n",
>> >> > @@ -814,10 +909,7 @@ void intel_bw_init_hw(struct intel_display *display)
>> >> >  		else
>> >> >  			tgl_get_bw_info(display, dram_info, &xe3lpd_sa_info);
>> >> >  	} else if (DISPLAY_VERx100(display) >= 1401 && display->platform.dgfx) {
>> >> > -		if (dram_info->type == INTEL_DRAM_GDDR_ECC)
>> >> > -			xe2_hpd_get_bw_info(display, dram_info, &xe2_hpd_ecc_sa_info);
>> >> > -		else
>> >> > -			xe2_hpd_get_bw_info(display, dram_info, &xe2_hpd_sa_info);
>> >> > +		xe2_hpd_get_bw_info(display, dram_info);
>> >> >  	} else if (DISPLAY_VER(display) >= 14) {
>> >> >  		tgl_get_bw_info(display, dram_info, &mtl_sa_info);
>> >> >  	} else if (display->platform.dg2) {
>> >> 
>> >> -- 
>> >> Jani Nikula, Intel
>> >
>> > -- 
>> > Matt Roper
>> > Graphics Software Engineer
>> > Linux GPU Platform Enablement
>> > Intel Corporation
>
> -- 
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation