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

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

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.

> +
> +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?

> +
> +	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