Re: [PATCH 3/4] drm/i915/bw: Rename struct intel_sa_info to intel_display_bw_params

[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:
>> To align with struct intel_platform_bw_params, rename struct
>> intel_sa_info to intel_display_bw_params.  Also add comments to contrast
>> their purposes.
>>
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_bw.c | 38 ++++++++++++++++++++-------------
>>  1 file changed, 23 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
>> index 654876215ace..64c6f18346bb 100644
>> --- a/drivers/gpu/drm/i915/display/intel_bw.c
>> +++ b/drivers/gpu/drm/i915/display/intel_bw.c
>> @@ -375,6 +375,10 @@ static int icl_sagv_max_dclk(const struct intel_qgv_info *qi)
>>  	return dclk;
>>  }
>>  
>> +/*
>> + * Bandwidth parameters that are tied to the platform (as opposed to struct
>> + * intel_display_bw_params).
>> + */
>>  struct intel_platform_bw_params {
>>  	u8 deprogbwlimit;
>>  	u8 derating;
>> @@ -496,34 +500,38 @@ static const struct intel_platform_bw_params *get_platform_bw_params(struct inte
>>  	return ret;
>>  }
>>  
>> -struct intel_sa_info {
>> +/*
>> + * Bandwidth parameters that are tied to the display IP (as opposed to struct
>> + * intel_platform_bw_params).
>> + */
>> +struct intel_display_bw_params {
>>  	u16 displayrtids;
>>  	u8 deburst;
>>  };
>>  
>> -static const struct intel_sa_info icl_sa_info = {
>> +static const struct intel_display_bw_params icl_disp_bw_params = {
>>  	.deburst = 8,
>>  	.displayrtids = 128,
>>  };
>>  
>> -static const struct intel_sa_info tgl_sa_info = {
>> +static const struct intel_display_bw_params tgl_disp_bw_params = {
>>  	.deburst = 16,
>>  	.displayrtids = 256,
>>  };
>>  
>> -static const struct intel_sa_info rkl_sa_info = {
>> +static const struct intel_display_bw_params rkl_disp_bw_params = {
>>  	.deburst = 8,
>>  	.displayrtids = 128,
>>  };
>>  
>> -static const struct intel_sa_info xelpdp_sa_info = {
>> +static const struct intel_display_bw_params xelpdp_disp_bw_params = {
>>  	.deburst = 32,
>>  	.displayrtids = 256,
>>  };
>
> So if these are tied to IP, why are they still named after platforms?

Decided to use xelpdp for the one above and platform names for the other
ones because xelpdp is where we trully have IP disaggregation.

That said, I could try to use IP release names for the other ones as
well. So, something like below?

s/tgl_disp_bw_params/xe_d_bw_params/
s/rkl_disp_bw_params/xe_d_rkl_bw_params/
s/xelpdp_disp_bw_params/xe_lpdp_bw_params/

And what to do with icl_disp_bw_params? Do we have a suitable display
release name for it?

--
Gustavo Sousa

>
> Nitpick, you'll never see me use "disp" abbreviation. It just doesn't
> abbreviate enough, only makes stuff harder to read.
>
>>  
>>  static int icl_get_bw_info(struct intel_display *display,
>>  			   const struct dram_info *dram_info,
>> -			   const struct intel_sa_info *sa)
>> +			   const struct intel_display_bw_params *disp_bw_params)
>>  {
>>  	struct intel_qgv_info qi = {};
>>  	const struct intel_platform_bw_params *plat_bw_params = get_platform_bw_params(display);
>> @@ -544,7 +552,7 @@ static int icl_get_bw_info(struct intel_display *display,
>>  
>>  	dclk_max = icl_sagv_max_dclk(&qi);
>>  	maxdebw = min(plat_bw_params->deprogbwlimit * 1000, dclk_max * 16 * 6 / 10);
>> -	ipqdepth = min(ipqdepthpch, sa->displayrtids / num_channels);
>> +	ipqdepth = min(ipqdepthpch, disp_bw_params->displayrtids / num_channels);
>>  	qi.deinterleave = DIV_ROUND_UP(num_channels, is_y_tile ? 4 : 2);
>>  
>>  	for (i = 0; i < num_groups; i++) {
>> @@ -552,7 +560,7 @@ static int icl_get_bw_info(struct intel_display *display,
>>  		int clpchgroup;
>>  		int j;
>>  
>> -		clpchgroup = (sa->deburst * qi.deinterleave / num_channels) << i;
>> +		clpchgroup = (disp_bw_params->deburst * qi.deinterleave / num_channels) << i;
>>  		bi->num_planes = (ipqdepth - clpchgroup) / clpchgroup + 1;
>>  
>>  		bi->num_qgv_points = qi.num_points;
>> @@ -595,7 +603,7 @@ static int icl_get_bw_info(struct intel_display *display,
>>  
>>  static int tgl_get_bw_info(struct intel_display *display,
>>  			   const struct dram_info *dram_info,
>> -			   const struct intel_sa_info *sa)
>> +			   const struct intel_display_bw_params *disp_bw_params)
>>  {
>>  	struct intel_qgv_info qi = {};
>>  	const struct intel_platform_bw_params *plat_bw_params = get_platform_bw_params(display);
>> @@ -634,7 +642,7 @@ static int tgl_get_bw_info(struct intel_display *display,
>>  	peakbw = num_channels * DIV_ROUND_UP(qi.channel_width, 8) * dclk_max;
>>  	maxdebw = min(plat_bw_params->deprogbwlimit * 1000, peakbw * DEPROGBWPCLIMIT / 100);
>>  
>> -	ipqdepth = min(ipqdepthpch, sa->displayrtids / num_channels);
>> +	ipqdepth = min(ipqdepthpch, disp_bw_params->displayrtids / num_channels);
>>  	/*
>>  	 * clperchgroup = 4kpagespermempage * clperchperblock,
>>  	 * clperchperblock = 8 / num_channels * interleave
>> @@ -647,7 +655,7 @@ static int tgl_get_bw_info(struct intel_display *display,
>>  		int clpchgroup;
>>  		int j;
>>  
>> -		clpchgroup = (sa->deburst * qi.deinterleave / num_channels) << i;
>> +		clpchgroup = (disp_bw_params->deburst * qi.deinterleave / num_channels) << i;
>>  
>>  		if (i < num_groups - 1) {
>>  			bi_next = &display->bw.max[i + 1];
>> @@ -886,16 +894,16 @@ void intel_bw_init_hw(struct intel_display *display)
>>  	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, &xelpdp_sa_info);
>> +		tgl_get_bw_info(display, dram_info, &xelpdp_disp_bw_params);
>>  	} else if (display->platform.dg2) {
>>  		dg2_get_bw_info(display);
>>  	} else if (DISPLAY_VER(display) >= 12) {
>>  		if (display->platform.rocketlake)
>> -			tgl_get_bw_info(display, dram_info, &rkl_sa_info);
>> +			tgl_get_bw_info(display, dram_info, &rkl_disp_bw_params);
>>  		else
>> -			tgl_get_bw_info(display, dram_info, &tgl_sa_info);
>> +			tgl_get_bw_info(display, dram_info, &tgl_disp_bw_params);
>>  	} else if (DISPLAY_VER(display) == 11) {
>> -		icl_get_bw_info(display, dram_info, &icl_sa_info);
>> +		icl_get_bw_info(display, dram_info, &icl_disp_bw_params);
>>  	}
>>  }
>
> -- 
> Jani Nikula, Intel