Re: [Intel-gfx] [PATCH v2 7/8] drm/i915: Unconfuses QGV vs. PSF point masks

["Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>]

On Wed, Mar 09, 2022 at 06:49:47PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Use separate bitmasks for QGV vs. PSF GV points during
> the computation. Makes the whole thing a lot less confusing.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_bw.c | 35 ++++++++++++-------------
>  drivers/gpu/drm/i915/i915_reg.h         |  3 ++-
>  2 files changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> index adf58c58513b..b794545ff81d 100644
> --- a/drivers/gpu/drm/i915/display/intel_bw.c
> +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> @@ -820,7 +820,7 @@ static u16 icl_qgv_points_mask(struct drm_i915_private *i915)
>  {
>  	unsigned int num_psf_gv_points = i915->max_bw[0].num_psf_gv_points;
>  	unsigned int num_qgv_points = i915->max_bw[0].num_qgv_points;
> -	u16 mask = 0;
> +	u16 qgv_points = 0, psf_points = 0;
>  
>  	/*
>  	 * We can _not_ use the whole ADLS_QGV_PT_MASK here, as PCode rejects
> @@ -828,12 +828,12 @@ static u16 icl_qgv_points_mask(struct drm_i915_private *i915)
>  	 * So need to operate only with those returned from PCode.
>  	 */
>  	if (num_qgv_points > 0)
> -		mask |= REG_GENMASK(num_qgv_points - 1, 0);
> +		qgv_points = GENMASK(num_qgv_points - 1, 0);
>  
>  	if (num_psf_gv_points > 0)
> -		mask |= REG_GENMASK(num_psf_gv_points - 1, 0) << ADLS_PSF_PT_SHIFT;
> +		psf_points = GENMASK(num_psf_gv_points - 1, 0);
>  
> -	return mask;
> +	return ADLS_QGV_PT(qgv_points) | ADLS_PSF_PT(psf_points);
>  }
>  
>  static int intel_bw_check_data_rate(struct intel_atomic_state *state, bool *changed)
> @@ -890,7 +890,7 @@ int intel_bw_atomic_check(struct intel_atomic_state *state)
>  	unsigned int data_rate;
>  	unsigned int num_active_planes;
>  	int i, ret;
> -	u32 allowed_points = 0;
> +	u16 qgv_points = 0, psf_points = 0;
>  	unsigned int max_bw_point = 0, max_bw = 0;
>  	unsigned int num_qgv_points = dev_priv->max_bw[0].num_qgv_points;
>  	unsigned int num_psf_gv_points = dev_priv->max_bw[0].num_psf_gv_points;
> @@ -948,7 +948,7 @@ int intel_bw_atomic_check(struct intel_atomic_state *state)
>  			max_bw = max_data_rate;
>  		}
>  		if (max_data_rate >= data_rate)
> -			allowed_points |= REG_FIELD_PREP(ADLS_QGV_PT_MASK, BIT(i));
> +			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);
> @@ -958,7 +958,7 @@ int intel_bw_atomic_check(struct intel_atomic_state *state)
>  		unsigned int max_data_rate = adl_psf_bw(dev_priv, i);
>  
>  		if (max_data_rate >= data_rate)
> -			allowed_points |= REG_FIELD_PREP(ADLS_PSF_PT_MASK, BIT(i));
> +			psf_points |= BIT(i);
>  
>  		drm_dbg_kms(&dev_priv->drm, "PSF GV point %d: max bw %d"
>  			    " required %d\n",
> @@ -970,20 +970,18 @@ int intel_bw_atomic_check(struct intel_atomic_state *state)
>  	 * left, so if we couldn't - simply reject the configuration for obvious
>  	 * reasons.
>  	 */
> -	if ((allowed_points & ADLS_QGV_PT_MASK) == 0) {
> +	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) {
> -		if ((allowed_points & ADLS_PSF_PT_MASK) == 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;
> -		}
> +	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;
>  	}
>  
>  	/*
> @@ -992,16 +990,17 @@ int intel_bw_atomic_check(struct intel_atomic_state *state)
>  	 * cause.
>  	 */
>  	if (!intel_can_enable_sagv(dev_priv, new_bw_state)) {
> -		allowed_points &= ADLS_PSF_PT_MASK;
> -		allowed_points |= BIT(max_bw_point);
> +		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 = ~allowed_points &
> +	new_bw_state->qgv_points_mask =
> +		~(ADLS_QGV_PT(qgv_points) | ADLS_PSF_PT(psf_points)) &
>  		icl_qgv_points_mask(dev_priv);
>  
>  	/*
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 70484f6f2b8b..48a12f6c19b4 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6722,9 +6722,10 @@
>  #define   ICL_PCODE_SAGV_DE_MEM_SS_CONFIG	0xe
>  #define     ICL_PCODE_POINTS_RESTRICTED		0x0
>  #define     ICL_PCODE_POINTS_RESTRICTED_MASK	0xf
> -#define   ADLS_PSF_PT_SHIFT			8
>  #define   ADLS_QGV_PT_MASK			REG_GENMASK(7, 0)
> +#define   ADLS_QGV_PT(x)			REG_FIELD_PREP(ADLS_QGV_PT_MASK, (x))
>  #define   ADLS_PSF_PT_MASK			REG_GENMASK(10, 8)
> +#define   ADLS_PSF_PT(x)			REG_FIELD_PREP(ADLS_PSF_PT_MASK, (x))
>  #define   GEN6_PCODE_READ_D_COMP		0x10
>  #define   GEN6_PCODE_WRITE_D_COMP		0x11
>  #define   ICL_PCODE_EXIT_TCCOLD			0x12
> -- 
> 2.34.1
>