Re: [PATCH 2/4] drm/i915/bw: Deduplicate intel_sa_info instances

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

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

> On Wed, Apr 08, 2026 at 03:53:00PM -0300, Gustavo Sousa wrote:
>> Now that intel_sa_info contains bandwidth parameters specific to the
>> display IP, we can drop many duplicates and reuse from previous
>> releases.
>> 
>> Let's do that and also simplify intel_bw_init_hw() while at it.
>> 
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_bw.c | 44 ++++++---------------------------
>>  1 file changed, 8 insertions(+), 36 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
>> index ed840b592eff..654876215ace 100644
>> --- a/drivers/gpu/drm/i915/display/intel_bw.c
>> +++ b/drivers/gpu/drm/i915/display/intel_bw.c
>> @@ -516,27 +516,7 @@ static const struct intel_sa_info rkl_sa_info = {
>>  	.displayrtids = 128,
>>  };
>>  
>> -static const struct intel_sa_info adls_sa_info = {
>> -	.deburst = 16,
>> -	.displayrtids = 256,
>> -};
>> -
>> -static const struct intel_sa_info adlp_sa_info = {
>> -	.deburst = 16,
>> -	.displayrtids = 256,
>> -};
>> -
>> -static const struct intel_sa_info mtl_sa_info = {
>> -	.deburst = 32,
>> -	.displayrtids = 256,
>> -};
>> -
>> -static const struct intel_sa_info xe3lpd_sa_info = {
>> -	.deburst = 32,
>> -	.displayrtids = 256,
>> -};
>> -
>> -static const struct intel_sa_info xe3lpd_3002_sa_info = {
>> +static const struct intel_sa_info xelpdp_sa_info = {
>>  	.deburst = 32,
>>  	.displayrtids = 256,
>>  };
>> @@ -903,25 +883,17 @@ void intel_bw_init_hw(struct intel_display *display)
>>  	if (DISPLAY_VER(display) >= 35)
>>  		drm_WARN_ON(display->drm, dram_info->ecc_impacting_de_bw);
>>  
>> -	if (DISPLAY_VER(display) >= 30) {
>> -		if (DISPLAY_VERx100(display) == 3002)
>> -			tgl_get_bw_info(display, dram_info, &xe3lpd_3002_sa_info);
>> -		else
>> -			tgl_get_bw_info(display, dram_info, &xe3lpd_sa_info);
>> -	} else if (DISPLAY_VERx100(display) >= 1401 && display->platform.dgfx) {
>> +	if (DISPLAY_VERx100(display) >= 1401 && display->platform.dgfx) {
>>  		xe2_hpd_get_bw_info(display, dram_info);
>>  	} else if (DISPLAY_VER(display) >= 14) {
>> -		tgl_get_bw_info(display, dram_info, &mtl_sa_info);
>> +		tgl_get_bw_info(display, dram_info, &xelpdp_sa_info);
>>  	} else if (display->platform.dg2) {
>>  		dg2_get_bw_info(display);
>> -	} else if (display->platform.alderlake_p) {
>> -		tgl_get_bw_info(display, dram_info, &adlp_sa_info);
>> -	} else if (display->platform.alderlake_s) {
>> -		tgl_get_bw_info(display, dram_info, &adls_sa_info);
>> -	} else if (display->platform.rocketlake) {
>> -		tgl_get_bw_info(display, dram_info, &rkl_sa_info);
>> -	} else if (DISPLAY_VER(display) == 12) {
>> -		tgl_get_bw_info(display, dram_info, &tgl_sa_info);
>> +	} else if (DISPLAY_VER(display) >= 12) {
>> +		if (display->platform.rocketlake)
>> +			tgl_get_bw_info(display, dram_info, &rkl_sa_info);
>> +		else
>> +			tgl_get_bw_info(display, dram_info, &tgl_sa_info);
>
> It seems strange to need to need to drop back to a platform check here
> on something that's supposed to be tied to IP version.  But if I recall
> correctly, RKL was a bit of a strange frankenstein platform where TGL's
> "gen12" IP got backported onto an ICL-style chassis, which caused it to
> inherit various ICL traits despite the new IP.  It might actually be
> more clear to just re-use the icl_sa_info for that one and leave a
> comment admitting that yeah, RKL was an oddball platform that didn't
> really follow the rules.

Huh.  Yep, using icl_sa_info sounds good; I totally missed the fact that
rkl_sa_info matched icl_sa_info.

>
> We might also want to rename tgl_sa_info to "gen12_sa_info" since that's
> a more accurate description (and is the last version where we're allowed
> to use the "gen" terminology rather than the new marketing names for the
> IP).  By similar logic, "icl_sa_info" should become "gen11_sa_info"
> since it does indeed get used on the other gen11 platforms too
> (jsl/ehl).

Sounds good.  Since I'm renaming the struct type name to
intel_display_bw_params (to indicate that those are tied to the display
IP) in a separate patch, I think it makes sense to do the instances
renaming there as well.  Ack on moving back to mtl_sa_info here and then
doing all renaming in the patch that also renames the struct type?

--
Gustavo Sousa

>
>
> Matt
>
>>  	} else if (DISPLAY_VER(display) == 11) {
>>  		icl_get_bw_info(display, dram_info, &icl_sa_info);
>>  	}
>> 
>> -- 
>> 2.53.0
>> 
>
> -- 
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation