Re: [PATCH v3 2/5] drm/i915/bw: Extract platform-specific parameters

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

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

> 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.
>
> v2:
>   - Use good old if-ladder instead of weird-looking pattern "assign ret,
>     check platform, then return ret". (Jani, Matt)
>   - Have a single call site for get_platform_bw_params() and pass the
>     result as parameter to the *_get_bw_info() functions. (Jani)
>   - Avoid using "plat" as abbreviation for "platform". (Jani)
>   - s/_plat_bw_params/_bw_params/, since all of the instances are
>     prefixed with platform names. (Jani)
>   - s/struct intel_platform_bw_params/struct intel_soc_bw_params/.
>     (Matt)
>   - Do not return a default value; prefer to return NULL and
>     intentionally cause a NULL pointer dereference if a platform is
>     missing. (Gustavo)
>
> v3:
>   - Call get_soc_bw_params() only after the check on
>     HAS_DISPLAY(display). (Jani)
>   - Combine if-ladder branches for adl_s_bw_params into a single one.
>     (Matt)
>   - Flatten if-ladder by checking for WCL before PTL (as opposed to
>     checking for WCL inside the brace for PTL). (Matt)
>   - Bail out of intel_bw_init_hw() if display version is below 11.
>     (Gustavo)
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bw.c | 162 ++++++++++++++++++++++----------
>  1 file changed, 114 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> index 7eef693b51ad..351ecf741b54 100644
> --- a/drivers/gpu/drm/i915/display/intel_bw.c
> +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> @@ -372,81 +372,144 @@ static int icl_sagv_max_dclk(const struct intel_qgv_info *qi)
>  	return dclk;
>  }
>  
> +struct intel_soc_bw_params {
> +	u8 deprogbwlimit;
> +	u8 derating;
> +};
> +
> +static const struct intel_soc_bw_params icl_bw_params = {
> +	.deprogbwlimit = 25,
> +	.derating = 10,
> +};
> +
> +static const struct intel_soc_bw_params tgl_bw_params = {
> +	.deprogbwlimit = 34,
> +	.derating = 10,
> +};
> +
> +static const struct intel_soc_bw_params rkl_bw_params = {
> +	.deprogbwlimit = 20,
> +	.derating = 10,
> +};
> +
> +static const struct intel_soc_bw_params adl_s_bw_params = {
> +	.deprogbwlimit = 38,
> +	.derating = 10,
> +};
> +
> +static const struct intel_soc_bw_params adl_p_bw_params = {
> +	.deprogbwlimit = 38,
> +	.derating = 20,
> +};
> +
> +static const struct intel_soc_bw_params bmg_bw_params = {
> +	.deprogbwlimit = 53,
> +	.derating = 30,
> +};
> +
> +static const struct intel_soc_bw_params bmg_ecc_bw_params = {
> +	.deprogbwlimit = 53,
> +	.derating = 45,
> +};
> +
> +static const struct intel_soc_bw_params ptl_bw_params = {
> +	.deprogbwlimit = 65,
> +	.derating = 10,
> +};
> +
> +static const struct intel_soc_bw_params wcl_bw_params = {
> +	.deprogbwlimit = 22,
> +	.derating = 10,
> +};
> +
> +static const struct intel_soc_bw_params *get_soc_bw_params(struct intel_display *display)
> +{
> +	if (display->platform.dgfx) {
> +		if (display->platform.dg1) {
> +			return &tgl_bw_params;
> +		} else if (display->platform.battlemage) {
> +			const struct dram_info *dram_info = intel_dram_info(display);
> +
> +			if (dram_info->type == INTEL_DRAM_GDDR_ECC)
> +				return &bmg_ecc_bw_params;
> +			else
> +				return &bmg_bw_params;
> +		}
> +	} else {
> +		if (display->platform.icelake ||
> +		    display->platform.jasperlake ||
> +		    display->platform.elkhartlake)
> +			return &icl_bw_params;
> +		else if (display->platform.tigerlake)
> +			return &tgl_bw_params;
> +		else if (display->platform.rocketlake)
> +			return &rkl_bw_params;
> +		else if (display->platform.alderlake_s ||
> +			 display->platform.meteorlake ||
> +			 display->platform.lunarlake)
> +			return &adl_s_bw_params;
> +		else if (display->platform.alderlake_p)
> +			return &adl_p_bw_params;
> +		else if (display->platform.pantherlake_wildcatlake)
> +			return &wcl_bw_params;
> +		else if (display->platform.pantherlake ||
> +			 display->platform.novalake)
> +			return &ptl_bw_params;
> +	}
> +
> +	drm_WARN(display->drm, 1, "Platform-specific bandwidth parameters not found!\n");

CI shows the warning because DG2 does not use bandwidth
parameters and get_soc_bw_params() rightfully does not cover DG2.

We could just get rid of the warnings in get_soc_bw_params() and
get_display_bw_params() all together.  The idea of the warning was to
serve as an aid to the developer, but I guess tracing the null pointer
dereference back to those functions shouldn't be too hard?

Another idea is to make sure that only the functions that use those
parameters make the call to get_{soc,display}_bw_params().

Jani, I know you preferred the other way around, but maybe this is a
compelling reason for moving the call to the direct users?

--
Gustavo Sousa

> +
> +	return NULL;
> +}
> +
>  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,
>  			   const struct dram_info *dram_info,
> +			   const struct intel_soc_bw_params *soc_bw_params,
>  			   const struct intel_sa_info *sa)
>  {
>  	struct intel_qgv_info qi = {};
> @@ -466,7 +529,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(soc_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);
>  
> @@ -496,7 +559,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 - soc_bw_params->derating) / 100);
>  
>  			drm_dbg_kms(display->drm,
>  				    "BW%d / QGV %d: num_planes=%d deratedbw=%u\n",
> @@ -518,6 +581,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_soc_bw_params *soc_bw_params,
>  			   const struct intel_sa_info *sa)
>  {
>  	struct intel_qgv_info qi = {};
> @@ -554,7 +618,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(soc_bw_params->deprogbwlimit * 1000, peakbw * DEPROGBWPCLIMIT / 100);
>  
>  	ipqdepth = min(ipqdepthpch, sa->displayrtids / num_channels);
>  	/*
> @@ -599,7 +663,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 - soc_bw_params->derating) / 100);
>  			bi->peakbw[j] = DIV_ROUND_CLOSEST(sp->dclk *
>  							  num_channels *
>  							  qi.channel_width, 8);
> @@ -661,7 +725,7 @@ 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 intel_soc_bw_params *soc_bw_params)
>  {
>  	struct intel_qgv_info qi = {};
>  	int num_channels = dram_info->num_channels;
> @@ -676,14 +740,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(soc_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 - soc_bw_params->derating) * bw / 100);
>  		display->bw.max[0].peakbw[i] = bw;
>  
>  		drm_dbg_kms(display->drm, "QGV %d: deratedbw=%u peakbw: %u\n",
> @@ -792,11 +856,16 @@ static unsigned int icl_qgv_bw(struct intel_display *display,
>  void intel_bw_init_hw(struct intel_display *display)
>  {
>  	const struct dram_info *dram_info;
> +	const struct intel_soc_bw_params *soc_bw_params;
>  
>  	if (!HAS_DISPLAY(display))
>  		return;
>  
> +	if (DISPLAY_VER(display) < 11)
> +		return;
> +
>  	dram_info = intel_dram_info(display);
> +	soc_bw_params = get_soc_bw_params(display);
>  
>  	/*
>  	 * Starting with Xe3p_LPD, the hardware tells us whether memory has ECC
> @@ -809,28 +878,25 @@ void intel_bw_init_hw(struct intel_display *display)
>  
>  	if (DISPLAY_VER(display) >= 30) {
>  		if (DISPLAY_VERx100(display) == 3002)
> -			tgl_get_bw_info(display, dram_info, &xe3lpd_3002_sa_info);
> +			tgl_get_bw_info(display, dram_info, soc_bw_params, &xe3lpd_3002_sa_info);
>  		else
> -			tgl_get_bw_info(display, dram_info, &xe3lpd_sa_info);
> +			tgl_get_bw_info(display, dram_info, soc_bw_params, &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, soc_bw_params);
>  	} else if (DISPLAY_VER(display) >= 14) {
> -		tgl_get_bw_info(display, dram_info, &mtl_sa_info);
> +		tgl_get_bw_info(display, dram_info, soc_bw_params, &mtl_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);
> +		tgl_get_bw_info(display, dram_info, soc_bw_params, &adlp_sa_info);
>  	} else if (display->platform.alderlake_s) {
> -		tgl_get_bw_info(display, dram_info, &adls_sa_info);
> +		tgl_get_bw_info(display, dram_info, soc_bw_params, &adls_sa_info);
>  	} else if (display->platform.rocketlake) {
> -		tgl_get_bw_info(display, dram_info, &rkl_sa_info);
> +		tgl_get_bw_info(display, dram_info, soc_bw_params, &rkl_sa_info);
>  	} else if (DISPLAY_VER(display) == 12) {
> -		tgl_get_bw_info(display, dram_info, &tgl_sa_info);
> +		tgl_get_bw_info(display, dram_info, soc_bw_params, &tgl_sa_info);
>  	} else if (DISPLAY_VER(display) == 11) {
> -		icl_get_bw_info(display, dram_info, &icl_sa_info);
> +		icl_get_bw_info(display, dram_info, soc_bw_params, &icl_sa_info);
>  	}
>  }
>  
>
> -- 
> 2.53.0