Re: [Intel-gfx] [PATCH v6 4/7] drm/i915: extract intel_bw_check_qgv_points()

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 23 May 2023, Vinod Govindapillai <vinod.govindapillai@intel.com> wrote:
> Extract intel_bw_check_qgv_points() from intel_bw_atomic_check
> to facilitate future platform variations in handling SAGV
> configurations.
>
> Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bw.c | 235 +++++++++++++-----------
>  1 file changed, 130 insertions(+), 105 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> index db117638d23b..d83aafd0cc2b 100644
> --- a/drivers/gpu/drm/i915/display/intel_bw.c
> +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> @@ -803,6 +803,128 @@ intel_atomic_get_bw_state(struct intel_atomic_state *state)
>  	return to_intel_bw_state(bw_state);
>  }
>  
> +static int icl_find_qgv_points(struct drm_i915_private *i915,
> +			       unsigned int data_rate,
> +			       unsigned int num_active_planes,
> +			       const struct intel_bw_state *old_bw_state,
> +			       struct intel_bw_state *new_bw_state)
> +{
> +	unsigned int max_bw_point = 0;
> +	unsigned int max_bw = 0;
> +	unsigned int num_psf_gv_points = i915->display.bw.max[0].num_psf_gv_points;
> +	unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points;

Please just use signed ints whenever you don't have a specific reason
not to. Ditto for parameters being passed. Mixing signed and unsigned
will lead to trouble.

> +	u16 psf_points = 0;
> +	u16 qgv_points = 0;
> +	int i;
> +	int ret;
> +
> +	ret = intel_atomic_lock_global_state(&new_bw_state->base);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < num_qgv_points; i++) {

This is signed and unsigned comparison.

> +		unsigned int max_data_rate;
> +
> +		if (DISPLAY_VER(i915) > 11)
> +			max_data_rate = tgl_max_bw(i915, num_active_planes, i);
> +		else
> +			max_data_rate = icl_max_bw(i915, num_active_planes, i);
> +		/*
> +		 * We need to know which qgv point gives us
> +		 * maximum bandwidth in order to disable SAGV
> +		 * if we find that we exceed SAGV block time
> +		 * with watermarks. By that moment we already
> +		 * have those, as it is calculated earlier in
> +		 * intel_atomic_check,
> +		 */
> +		if (max_data_rate > max_bw) {
> +			max_bw_point = i;
> +			max_bw = max_data_rate;
> +		}
> +		if (max_data_rate >= data_rate)
> +			qgv_points |= BIT(i);
> +
> +		drm_dbg_kms(&i915->drm, "QGV point %d: max bw %d required %d\n",
> +			    i, max_data_rate, data_rate);
> +	}
> +
> +	for (i = 0; i < num_psf_gv_points; i++) {
> +		unsigned int max_data_rate = adl_psf_bw(i915, i);
> +
> +		if (max_data_rate >= data_rate)
> +			psf_points |= BIT(i);
> +
> +		drm_dbg_kms(&i915->drm, "PSF GV point %d: max bw %d"
> +			    " required %d\n",
> +			    i, max_data_rate, data_rate);
> +	}
> +
> +	/*
> +	 * BSpec states that we always should have at least one allowed point
> +	 * left, so if we couldn't - simply reject the configuration for obvious
> +	 * reasons.
> +	 */
> +	if (qgv_points == 0) {
> +		drm_dbg_kms(&i915->drm, "No QGV points provide sufficient memory"
> +			    " bandwidth %d for display configuration(%d active planes).\n",
> +			    data_rate, num_active_planes);
> +		return -EINVAL;
> +	}
> +
> +	if (num_psf_gv_points > 0 && psf_points == 0) {
> +		drm_dbg_kms(&i915->drm, "No PSF GV points provide sufficient memory"
> +			    " bandwidth %d for display configuration(%d active planes).\n",
> +			    data_rate, num_active_planes);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Leave only single point with highest bandwidth, if
> +	 * we can't enable SAGV due to the increased memory latency it may
> +	 * cause.
> +	 */
> +	if (!intel_can_enable_sagv(i915, new_bw_state)) {
> +		qgv_points = BIT(max_bw_point);
> +		drm_dbg_kms(&i915->drm, "No SAGV, using single QGV point %d\n",
> +			    max_bw_point);
> +	}
> +
> +	/*
> +	 * We store the ones which need to be masked as that is what PCode
> +	 * actually accepts as a parameter.
> +	 */
> +	new_bw_state->qgv_points_mask =
> +		~(ICL_PCODE_REQ_QGV_PT(qgv_points) |
> +		  ADLS_PCODE_REQ_PSF_PT(psf_points)) &
> +		icl_qgv_points_mask(i915);
> +
> +	/*
> +	 * If the actual mask had changed we need to make sure that
> +	 * the commits are serialized(in case this is a nomodeset, nonblocking)
> +	 */
> +	if (new_bw_state->qgv_points_mask != old_bw_state->qgv_points_mask) {
> +		ret = intel_atomic_serialize_global_state(&new_bw_state->base);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int intel_bw_check_qgv_points(struct drm_i915_private *i915,
> +				     const struct intel_bw_state *old_bw_state,
> +				     struct intel_bw_state *new_bw_state)
> +{
> +	unsigned int data_rate = intel_bw_data_rate(i915, new_bw_state);
> +	unsigned int num_active_planes =
> +			intel_bw_num_active_planes(i915, new_bw_state);
> +
> +	data_rate = DIV_ROUND_UP(data_rate, 1000);
> +
> +	return icl_find_qgv_points(i915, data_rate, num_active_planes,
> +				   old_bw_state, new_bw_state);
> +}
> +
>  static bool intel_bw_state_changed(struct drm_i915_private *i915,
>  				   const struct intel_bw_state *old_bw_state,
>  				   const struct intel_bw_state *new_bw_state)
> @@ -1049,20 +1171,14 @@ static int intel_bw_check_data_rate(struct intel_atomic_state *state, bool *chan
>  
>  int intel_bw_atomic_check(struct intel_atomic_state *state)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> -	const struct intel_bw_state *old_bw_state;
> -	struct intel_bw_state *new_bw_state;
> -	unsigned int data_rate;
> -	unsigned int num_active_planes;
> -	int i, ret;
> -	u16 qgv_points = 0, psf_points = 0;
> -	unsigned int max_bw_point = 0, max_bw = 0;
> -	unsigned int num_qgv_points = dev_priv->display.bw.max[0].num_qgv_points;
> -	unsigned int num_psf_gv_points = dev_priv->display.bw.max[0].num_psf_gv_points;
>  	bool changed = false;
> +	struct drm_i915_private *i915 = to_i915(state->base.dev);
> +	struct intel_bw_state *new_bw_state;
> +	const struct intel_bw_state *old_bw_state;
> +	int ret;
>  
>  	/* FIXME earlier gens need some checks too */
> -	if (DISPLAY_VER(dev_priv) < 11)
> +	if (DISPLAY_VER(i915) < 11)
>  		return 0;
>  
>  	ret = intel_bw_check_data_rate(state, &changed);
> @@ -1073,8 +1189,8 @@ int intel_bw_atomic_check(struct intel_atomic_state *state)
>  	new_bw_state = intel_atomic_get_new_bw_state(state);
>  
>  	if (new_bw_state &&
> -	    intel_can_enable_sagv(dev_priv, old_bw_state) !=
> -	    intel_can_enable_sagv(dev_priv, new_bw_state))
> +	    intel_can_enable_sagv(i915, old_bw_state) !=
> +	    intel_can_enable_sagv(i915, new_bw_state))
>  		changed = true;
>  
>  	/*
> @@ -1084,101 +1200,10 @@ int intel_bw_atomic_check(struct intel_atomic_state *state)
>  	if (!changed)
>  		return 0;
>  
> -	ret = intel_atomic_lock_global_state(&new_bw_state->base);
> +	ret = intel_bw_check_qgv_points(i915, old_bw_state, new_bw_state);
>  	if (ret)
>  		return ret;
>  
> -	data_rate = intel_bw_data_rate(dev_priv, new_bw_state);
> -	data_rate = DIV_ROUND_UP(data_rate, 1000);
> -
> -	num_active_planes = intel_bw_num_active_planes(dev_priv, new_bw_state);
> -
> -	for (i = 0; i < num_qgv_points; i++) {
> -		unsigned int max_data_rate;
> -
> -		if (DISPLAY_VER(dev_priv) > 11)
> -			max_data_rate = tgl_max_bw(dev_priv, num_active_planes, i);
> -		else
> -			max_data_rate = icl_max_bw(dev_priv, num_active_planes, i);
> -		/*
> -		 * We need to know which qgv point gives us
> -		 * maximum bandwidth in order to disable SAGV
> -		 * if we find that we exceed SAGV block time
> -		 * with watermarks. By that moment we already
> -		 * have those, as it is calculated earlier in
> -		 * intel_atomic_check,
> -		 */
> -		if (max_data_rate > max_bw) {
> -			max_bw_point = i;
> -			max_bw = max_data_rate;
> -		}
> -		if (max_data_rate >= data_rate)
> -			qgv_points |= BIT(i);
> -
> -		drm_dbg_kms(&dev_priv->drm, "QGV point %d: max bw %d required %d\n",
> -			    i, max_data_rate, data_rate);
> -	}
> -
> -	for (i = 0; i < num_psf_gv_points; i++) {
> -		unsigned int max_data_rate = adl_psf_bw(dev_priv, i);
> -
> -		if (max_data_rate >= data_rate)
> -			psf_points |= BIT(i);
> -
> -		drm_dbg_kms(&dev_priv->drm, "PSF GV point %d: max bw %d"
> -			    " required %d\n",
> -			    i, max_data_rate, data_rate);
> -	}
> -
> -	/*
> -	 * BSpec states that we always should have at least one allowed point
> -	 * left, so if we couldn't - simply reject the configuration for obvious
> -	 * reasons.
> -	 */
> -	if (qgv_points == 0) {
> -		drm_dbg_kms(&dev_priv->drm, "No QGV points provide sufficient memory"
> -			    " bandwidth %d for display configuration(%d active planes).\n",
> -			    data_rate, num_active_planes);
> -		return -EINVAL;
> -	}
> -
> -	if (num_psf_gv_points > 0 && psf_points == 0) {
> -		drm_dbg_kms(&dev_priv->drm, "No PSF GV points provide sufficient memory"
> -			    " bandwidth %d for display configuration(%d active planes).\n",
> -			    data_rate, num_active_planes);
> -		return -EINVAL;
> -	}
> -
> -	/*
> -	 * Leave only single point with highest bandwidth, if
> -	 * we can't enable SAGV due to the increased memory latency it may
> -	 * cause.
> -	 */
> -	if (!intel_can_enable_sagv(dev_priv, new_bw_state)) {
> -		qgv_points = BIT(max_bw_point);
> -		drm_dbg_kms(&dev_priv->drm, "No SAGV, using single QGV point %d\n",
> -			    max_bw_point);
> -	}
> -
> -	/*
> -	 * We store the ones which need to be masked as that is what PCode
> -	 * actually accepts as a parameter.
> -	 */
> -	new_bw_state->qgv_points_mask =
> -		~(ICL_PCODE_REQ_QGV_PT(qgv_points) |
> -		  ADLS_PCODE_REQ_PSF_PT(psf_points)) &
> -		icl_qgv_points_mask(dev_priv);
> -
> -	/*
> -	 * If the actual mask had changed we need to make sure that
> -	 * the commits are serialized(in case this is a nomodeset, nonblocking)
> -	 */
> -	if (new_bw_state->qgv_points_mask != old_bw_state->qgv_points_mask) {
> -		ret = intel_atomic_serialize_global_state(&new_bw_state->base);
> -		if (ret)
> -			return ret;
> -	}
> -
>  	return 0;
>  }

-- 
Jani Nikula, Intel Open Source Graphics Center