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

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

Jani Nikula <jani.nikula@linux.intel.com> writes:

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

Yeah, true.  In an upcoming patch I also introduce display-IP-specific
structs, and I decided to use plat vs disp to differentiate
(e.g. tgl_plat_bw_params vs tgl_disp_bw_params).

Any suggestion of how to make it better?

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

I would like that we always have a default to be returned if somehow we
miss adding a new case for a new platform (hence the "missing:"
label).  The idea of assigning to ret is for it to have a valid value
when hitting the missing: point.

Arguably we could also only use this pattern only in the last case, but that
would feel like an inconsistency and prone to someone forgetting to
"push" it to the next new platform when the time comes.

I would be much happier with a switch-case statement that would have a
"default:" combined with "fallthrough", but I think that's not possible
with the current way we handle platform matching in the display code.

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

Just wanted the default for the missing case to be the most recent
platform in an "automated" way, with separate ordering for dgfx and
integrated.

Do you suggest we do not have such a default and cause a null-pointer
deref (on purpose) if we forget to add a new platform?

Or always "manually" update the default when we add a new
platform-specific instance?

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

Okay. I think I'll have to rework patch #4 as well
then :-)

>
> Nitpick, "plat" is not an abbreviation I'm fond of.

Noted. 

Out of curiosity: because it is a noun of its own? Or is it something
else?

--
Gustavo Sousa

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