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

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

Gustavo Sousa <gustavo.sousa@intel.com> writes:

> Matt Roper <matthew.d.roper@intel.com> writes:
>
>> On Fri, Apr 10, 2026 at 11:39:10AM -0300, Gustavo Sousa wrote:
>>> 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.
>>
>> Personally I think "soc_bw_params" vs "display_bw_params" seems like a
>> similar distinction to what we have elsewhere in the driver (especially
>> if we add a 1-sentence comment above the structure clarifying what the
>> origin/source of those parameters is.  But I'll leave it up to Jani and
>> the other display experts to make the call since they're the ones who
>> work with this code the most.
>>
>>> 
>>> >
>>> >> 
>>> >> > +
>>> >> > +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;
>>
>> Yeah, I think a traditional if/else ladder like this is best.  I don't
>> think we even need to track a 'default_params' variable; we can just
>> directly return some recent platform as a fallback at the end too.  If
>> the fallback winds up not getting updated when we add new platforms, I
>> don't think that really matters since there's no real guarantee that
>> falling back to incorrect n-1 platform numbers is better than falling
>> back to incorrect n-2 platform numbers.
>
> Well, I would like to make a distinction between client and discrete
> (e.g. there is a significant difference between BMG's and
> PTL's derating param).  That would be the reason for the default_params
> variable, which would allow a single place to raise the warning.
>
>>
>> If we've screwed up and forgotten to add the parameters for a new
>> platform, then that's going to be something that's flagged almost
>> immediately by CI and will be quickly fixed long before the platform
>> ever leaves force_probe.
>
> Yeah, true.  Another possibility is to just return NULL and purposefully
> cause anull-pointer dereference; or even have the default being all
> zeros, so that we don't worry about updating the default from time to
> time.

Jani, I would like to hear your thoughts on these alternatives.  Do you
think it is a sensible way of approaching this?

In general, if this function is called and a platform check is missing
here, it should be considered a bug anyway, which should be caught early
on during the initial enabling for the platform.

So I'm inclined to raise a warning and let the null-pointer dereference
happen.

--
Gustavo Sousa

>
> --
> Gustavo Sousa
>
>>
>>
>> Matt
>>
>>> --
>>> 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
>>
>> -- 
>> Matt Roper
>> Graphics Software Engineer
>> Linux GPU Platform Enablement
>> Intel Corporation