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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Apr 10, 2026 at 10:33:55AM -0700, Matt Roper wrote:
> 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.

Well, I don't believe this is true. 'SoC' is still the glue of the multi-chips.

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

Well, both terms still exist. SoC is the glue. Platform is the overall, but
sometimes platform is more about the whole group of components working together:
CPU + PCH + FW + board level integration (BMC, etc).

I'm really bad with naming... perhaps intel_product_bw_params ?!

we could perhaps live with SoC because that is perhaps the cause of the bw
limitation, but not exactly a soc 'param'.

perhaps platform is also acceptable like we used in other places.

so, no strong feeling from my side.

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