Re: [PATCH 5/7] drm/i915: sseu: convert subslice count fields to subslice mask

[Ben Widawsky <benjamin.widawsky@intel.com>]

On Wed, Oct 21, 2015 at 06:40:35PM +0300, Imre Deak wrote:
> In an upcoming patch we'll need the actual mask of subslices in addition
> to their count, so convert the subslice_per_slice field to a mask.
> Also we can easily calculate subslice_total from the other fields, so
> instead of storing a cached version of this, add a helper to calculate
> it.

Oh good, I think I asked for this in patch 1.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 37 ++++++++-------------
>  drivers/gpu/drm/i915/i915_dma.c     | 64 +++++++++++++++++--------------------
>  drivers/gpu/drm/i915/i915_drv.h     |  8 +++--
>  drivers/gpu/drm/i915/intel_lrc.c    |  2 +-
>  4 files changed, 51 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 390dc59..3fb83ea 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4966,7 +4966,7 @@ static void cherryview_sseu_device_status(struct drm_device *dev,
>  			continue;
>  
>  		stat->slice_mask = BIT(0);
> -		stat->subslice_per_slice++;
> +		stat->subslice_mask |= BIT(ss);
>  		eu_cnt = ((sig1[ss] & CHV_EU08_PG_ENABLE) ? 0 : 2) +
>  			 ((sig1[ss] & CHV_EU19_PG_ENABLE) ? 0 : 2) +
>  			 ((sig1[ss] & CHV_EU210_PG_ENABLE) ? 0 : 2) +
> @@ -4975,7 +4975,6 @@ static void cherryview_sseu_device_status(struct drm_device *dev,
>  		stat->eu_per_subslice = max_t(unsigned int,
>  					      stat->eu_per_subslice, eu_cnt);
>  	}
> -	stat->subslice_total = stat->subslice_per_slice;
>  }
>  
>  static void gen9_sseu_device_status(struct drm_device *dev,
> @@ -5008,8 +5007,6 @@ static void gen9_sseu_device_status(struct drm_device *dev,
>  		     GEN9_PGCTL_SSB_EU311_ACK;
>  
>  	for (s = 0; s < s_max; s++) {
> -		unsigned int ss_cnt = 0;
> -
>  		if ((s_reg[s] & GEN9_PGCTL_SLICE_ACK) == 0)
>  			/* skip disabled slice */
>  			continue;
> @@ -5017,18 +5014,19 @@ static void gen9_sseu_device_status(struct drm_device *dev,
>  		stat->slice_mask |= BIT(s);
>  
>  		if (IS_SKYLAKE(dev))
> -			ss_cnt = INTEL_INFO(dev)->sseu.subslice_per_slice;
> +			stat->subslice_mask =
> +				INTEL_INFO(dev_priv)->sseu.subslice_mask;
>  
>  		for (ss = 0; ss < ss_max; ss++) {
>  			unsigned int eu_cnt;
>  
> -			if (IS_BROXTON(dev) &&
> -			    !(s_reg[s] & (GEN9_PGCTL_SS_ACK(ss))))
> -				/* skip disabled subslice */
> -				continue;
> +			if (IS_BROXTON(dev)) {
> +				if (!(s_reg[s] & (GEN9_PGCTL_SS_ACK(ss))))
> +					/* skip disabled subslice */
> +					continue;
>  
> -			if (IS_BROXTON(dev))
> -				ss_cnt++;
> +				stat->subslice_mask |= BIT(ss);
> +			}
>  
>  			eu_cnt = 2 * hweight32(eu_reg[2*s + ss/2] &
>  					       eu_mask[ss%2]);
> @@ -5037,11 +5035,6 @@ static void gen9_sseu_device_status(struct drm_device *dev,
>  						      stat->eu_per_subslice,
>  						      eu_cnt);
>  		}
> -
> -		stat->subslice_total += ss_cnt;
> -		stat->subslice_per_slice = max_t(unsigned int,
> -						 stat->subslice_per_slice,
> -						 ss_cnt);
>  	}
>  }
>  
> @@ -5055,12 +5048,10 @@ static void broadwell_sseu_device_status(struct drm_device *dev,
>  	stat->slice_mask = slice_info & GEN8_LSLICESTAT_MASK;
>  
>  	if (stat->slice_mask) {
> -		stat->subslice_per_slice =
> -				INTEL_INFO(dev)->sseu.subslice_per_slice;
> -		stat->subslice_total = hweight32(stat->slice_mask) *
> -				       stat->subslice_per_slice;
> +		stat->subslice_mask = INTEL_INFO(dev)->sseu.subslice_mask;
>  		stat->eu_per_subslice = INTEL_INFO(dev)->sseu.eu_per_subslice;
> -		stat->eu_total = stat->eu_per_subslice * stat->subslice_total;
> +		stat->eu_total = stat->eu_per_subslice *
> +				 sseu_subslice_total(stat);
>  
>  		/* subtract fused off EU(s) from enabled slice(s) */
>  		for (s = 0; s < hweight32(stat->slice_mask); s++) {
> @@ -5079,9 +5070,9 @@ static void i915_print_sseu_info(struct seq_file *m, bool is_available_info,
>  	seq_printf(m, "  %s Slice Total: %u\n", type,
>  		   hweight32(sseu->slice_mask));
>  	seq_printf(m, "  %s Subslice Total: %u\n", type,
> -		   sseu->subslice_total);
> +		   sseu_subslice_total(sseu));
>  	seq_printf(m, "  %s Subslice Per Slice: %u\n", type,
> -		   sseu->subslice_per_slice);
> +		   hweight32(sseu->subslice_mask));
>  	seq_printf(m, "  %s EU Total: %u\n", type,
>  		   sseu->eu_total);
>  	seq_printf(m, "  %s EU Per Subslice: %u\n", type,
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 1f5f3a7d..69a81ae9 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -154,7 +154,7 @@ static int i915_getparam(struct drm_device *dev, void *data,
>  		value = 1;
>  		break;
>  	case I915_PARAM_SUBSLICE_TOTAL:
> -		value = INTEL_INFO(dev)->sseu.subslice_total;
> +		value = sseu_subslice_total(&INTEL_INFO(dev)->sseu);
>  		if (!value)
>  			return -ENODEV;
>  		break;
> @@ -562,26 +562,25 @@ static void cherryview_sseu_info_init(struct drm_device *dev)
>  	info->slice_mask = BIT(0);
>  
>  	if (!(fuse & CHV_FGT_DISABLE_SS0)) {
> -		info->subslice_per_slice++;
> +		info->subslice_mask |= BIT(0);
>  		eu_dis = fuse & (CHV_FGT_EU_DIS_SS0_R0_MASK |
>  				 CHV_FGT_EU_DIS_SS0_R1_MASK);
>  		info->eu_total += 8 - hweight32(eu_dis);
>  	}
>  
>  	if (!(fuse & CHV_FGT_DISABLE_SS1)) {
> -		info->subslice_per_slice++;
> +		info->subslice_mask |= BIT(1);
>  		eu_dis = fuse & (CHV_FGT_EU_DIS_SS1_R0_MASK |
>  				 CHV_FGT_EU_DIS_SS1_R1_MASK);
>  		info->eu_total += 8 - hweight32(eu_dis);
>  	}
>  
> -	info->subslice_total = info->subslice_per_slice;
>  	/*
>  	 * CHV expected to always have a uniform distribution of EU
>  	 * across subslices.
>  	*/
> -	info->eu_per_subslice = info->subslice_total ?
> -				info->eu_total / info->subslice_total :
> +	info->eu_per_subslice = sseu_subslice_total(info) ?
> +				info->eu_total / sseu_subslice_total(info) :
>  				0;
>  	/*
>  	 * CHV supports subslice power gating on devices with more than
> @@ -589,7 +588,7 @@ static void cherryview_sseu_info_init(struct drm_device *dev)
>  	 * more than one EU pair per subslice.
>  	*/
>  	info->has_slice_pg = 0;
> -	info->has_subslice_pg = (info->subslice_total > 1);
> +	info->has_subslice_pg = sseu_subslice_total(info) > 1;
>  	info->has_eu_pg = (info->eu_per_subslice > 2);
>  }
>  
> @@ -599,22 +598,19 @@ static void gen9_sseu_info_init(struct drm_device *dev)
>  	struct sseu_dev_info *info;
>  	int s_max = 3, ss_max = 4, eu_max = 8;
>  	int s, ss;
> -	u32 fuse2, ss_disable, eu_disable;
> +	u32 fuse2, eu_disable;
>  	u8 eu_mask = 0xff;
>  
>  	info = (struct sseu_dev_info *)&INTEL_INFO(dev_priv)->sseu;
>  	fuse2 = I915_READ(GEN8_FUSE2);
>  	info->slice_mask = (fuse2 & GEN8_F2_S_ENA_MASK) >> GEN8_F2_S_ENA_SHIFT;
> -	ss_disable = (fuse2 & GEN9_F2_SS_DIS_MASK) >>
> -		     GEN9_F2_SS_DIS_SHIFT;
> -
>  	/*
>  	 * The subslice disable field is global, i.e. it applies
>  	 * to each of the enabled slices.
>  	*/
> -	info->subslice_per_slice = ss_max - hweight32(ss_disable);
> -	info->subslice_total = hweight32(info->slice_mask) *
> -			       info->subslice_per_slice;
> +	info->subslice_mask = (1 << ss_max) - 1;
> +	info->subslice_mask &= ~((fuse2 & GEN9_F2_SS_DIS_MASK) >>
> +				GEN9_F2_SS_DIS_SHIFT);
>  
>  	/*
>  	 * Iterate through enabled slices and subslices to
> @@ -629,7 +625,7 @@ static void gen9_sseu_info_init(struct drm_device *dev)
>  		for (ss = 0; ss < ss_max; ss++) {
>  			int eu_per_ss;
>  
> -			if (ss_disable & (0x1 << ss))
> +			if (!(info->subslice_mask & BIT(ss)))
>  				/* skip disabled subslice */
>  				continue;
>  
> @@ -655,9 +651,9 @@ static void gen9_sseu_info_init(struct drm_device *dev)
>  	 * recovery. BXT is expected to be perfectly uniform in EU
>  	 * distribution.
>  	*/
> -	info->eu_per_subslice = info->subslice_total ?
> +	info->eu_per_subslice = sseu_subslice_total(info) ?
>  				DIV_ROUND_UP(info->eu_total,
> -					     info->subslice_total) : 0;
> +					     sseu_subslice_total(info)) : 0;
>  	/*
>  	 * SKL supports slice power gating on devices with more than
>  	 * one slice, and supports EU power gating on devices with
> @@ -668,7 +664,8 @@ static void gen9_sseu_info_init(struct drm_device *dev)
>  	*/
>  	info->has_slice_pg = IS_SKYLAKE(dev) &&
>  			     hweight32(info->slice_mask) > 1;
> -	info->has_subslice_pg = (IS_BROXTON(dev) && (info->subslice_total > 1));
> +	info->has_subslice_pg = IS_BROXTON(dev) &&
> +				sseu_subslice_total(info) > 1;
>  	info->has_eu_pg = (info->eu_per_subslice > 2);
>  }
>  
> @@ -678,7 +675,7 @@ static void broadwell_sseu_info_init(struct drm_device *dev)
>  	struct sseu_dev_info *info;
>  	const int s_max = 3, ss_max = 3, eu_max = 8;
>  	int s, ss;
> -	u32 fuse2, eu_disable[s_max], ss_disable;
> +	u32 fuse2, eu_disable[s_max];
>  
>  	info = (struct sseu_dev_info *)&dev_priv->info.sseu;
>  
> @@ -686,7 +683,13 @@ static void broadwell_sseu_info_init(struct drm_device *dev)
>  
>  	info->slice_mask = (fuse2 & GEN8_F2_S_ENA_MASK) >>
>  				GEN8_F2_S_ENA_SHIFT;
> -	ss_disable = (fuse2 & GEN8_F2_SS_DIS_MASK) >> GEN8_F2_SS_DIS_SHIFT;
> +	/*
> +	 * The subslice disable field is global, i.e. it applies
> +	 * to each of the enabled slices.
> +	 */
> +	info->subslice_mask = (1 << ss_max) - 1;
> +	info->subslice_mask &= ~((fuse2 & GEN8_F2_SS_DIS_MASK) >>
> +				 GEN8_F2_SS_DIS_SHIFT);
>  
>  	eu_disable[0] = I915_READ(GEN8_EU_DISABLE0) & GEN8_EU_DIS0_S0_MASK;
>  	eu_disable[1] = (I915_READ(GEN8_EU_DISABLE0) >> GEN8_EU_DIS0_S1_SHIFT) |
> @@ -696,15 +699,6 @@ static void broadwell_sseu_info_init(struct drm_device *dev)
>  			((I915_READ(GEN8_EU_DISABLE2) & GEN8_EU_DIS2_S2_MASK) <<
>  			 (32 - GEN8_EU_DIS1_S2_SHIFT));
>  
> -
> -	/*
> -	 * The subslice disable field is global, i.e. it applies
> -	 * to each of the enabled slices.
> -	 */
> -	info->subslice_per_slice = ss_max - hweight32(ss_disable);
> -	info->subslice_total = hweight32(info->slice_mask) *
> -			       info->subslice_per_slice;
> -
>  	/*
>  	 * Iterate through enabled slices and subslices to
>  	 * count the total enabled EU.
> @@ -717,7 +711,7 @@ static void broadwell_sseu_info_init(struct drm_device *dev)
>  		for (ss = 0; ss < ss_max; ss++) {
>  			u32 n_disabled;
>  
> -			if (ss_disable & (0x1 << ss))
> +			if (!(info->subslice_mask & BIT(ss)))
>  				/* skip disabled subslice */
>  				continue;
>  
> @@ -738,8 +732,9 @@ static void broadwell_sseu_info_init(struct drm_device *dev)
>  	 * subslices with the exception that any one EU in any one subslice may
>  	 * be fused off for die recovery.
>  	 */
> -	info->eu_per_subslice = info->subslice_total ?
> -		DIV_ROUND_UP(info->eu_total, info->subslice_total) : 0;
> +	info->eu_per_subslice = sseu_subslice_total(info) ?
> +				DIV_ROUND_UP(info->eu_total,
> +					     sseu_subslice_total(info)) : 0;
>  
>  	/*
>  	 * BDW supports slice power gating on devices with more than
> @@ -827,9 +822,10 @@ static void intel_device_info_runtime_init(struct drm_device *dev)
>  
>  	DRM_DEBUG_DRIVER("slice total: %u\n",
>  			 hweight32(info->sseu.slice_mask));
> -	DRM_DEBUG_DRIVER("subslice total: %u\n", info->sseu.subslice_total);
> +	DRM_DEBUG_DRIVER("subslice total: %u\n",
> +			 sseu_subslice_total(&info->sseu));
>  	DRM_DEBUG_DRIVER("subslice per slice: %u\n",
> -						info->sseu.subslice_per_slice);
> +			 hweight32(info->sseu.subslice_mask));
>  	DRM_DEBUG_DRIVER("EU total: %u\n", info->sseu.eu_total);
>  	DRM_DEBUG_DRIVER("EU per subslice: %u\n", info->sseu.eu_per_subslice);
>  	DRM_DEBUG_DRIVER("has slice power gating: %s\n",
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7cc9ec8..d47e544 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -784,8 +784,7 @@ struct intel_csr {
>  
>  struct sseu_dev_info {
>  	u8 slice_mask;
> -	u8 subslice_total;
> -	u8 subslice_per_slice;
> +	u8 subslice_mask;

I know we have situations for GT1 parts where the number of subslices per slice
is less than that of the same GEN of a different SKU. AFAIK, this never carries
over into higher GT (ie. GT2 would always have 3 subslices per slice, but GT1
may have 2 subslices per slice). However. I am not certain this is the case - I
hope you've double checked that.

>  	u8 eu_total;
>  	u8 eu_per_subslice;
>  	/* For each slice, which subslice(s) has(have) 7 EUs (bitfield)? */
> @@ -795,6 +794,11 @@ struct sseu_dev_info {
>  	u8 has_eu_pg:1;
>  };
>  
> +static inline unsigned int sseu_subslice_total(const struct sseu_dev_info *sseu)
> +{
> +	return hweight32((sseu)->slice_mask) * hweight32((sseu)->subslice_mask);

hweight8

basically s/hweight32/hweight8 on the whole file

> +}
> +
>  struct intel_device_info {
>  	u32 display_mmio_offset;
>  	u16 device_id;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 4130ff1..158f008 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -2209,7 +2209,7 @@ make_rpcs(struct drm_device *dev)
>  
>  	if (INTEL_INFO(dev)->sseu.has_subslice_pg) {
>  		rpcs |= GEN8_RPCS_SS_CNT_ENABLE;
> -		rpcs |= INTEL_INFO(dev)->sseu.subslice_per_slice <<
> +		rpcs |= hweight32(INTEL_INFO(dev)->sseu.subslice_mask) <<


>  			GEN8_RPCS_SS_CNT_SHIFT;
>  		rpcs |= GEN8_RPCS_ENABLE;
>  	}

Reviewed-by: Ben Widawsky <benjamin.widawsky@intel.com>

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx