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

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