Re: [PATCH 4/7] drm/i915: sseu: convert slice count field to mask

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

On Wed, Oct 21, 2015 at 06:40:34PM +0300, Imre Deak wrote:
> In an upcoming patch we'll need the actual mask of slices in addition to
> their count, so replace the count field with a mask.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 14 +++++++-------
>  drivers/gpu/drm/i915/i915_dma.c     | 37 +++++++++++++++++++------------------
>  drivers/gpu/drm/i915/i915_drv.h     |  2 +-
>  drivers/gpu/drm/i915/intel_lrc.c    |  2 +-
>  4 files changed, 28 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 183c1f2..390dc59 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4965,7 +4965,7 @@ static void cherryview_sseu_device_status(struct drm_device *dev,
>  			/* skip disabled subslice */
>  			continue;
>  
> -		stat->slice_total = 1;
> +		stat->slice_mask = BIT(0);
>  		stat->subslice_per_slice++;
>  		eu_cnt = ((sig1[ss] & CHV_EU08_PG_ENABLE) ? 0 : 2) +
>  			 ((sig1[ss] & CHV_EU19_PG_ENABLE) ? 0 : 2) +
> @@ -5014,7 +5014,7 @@ static void gen9_sseu_device_status(struct drm_device *dev,
>  			/* skip disabled slice */
>  			continue;
>  
> -		stat->slice_total++;
> +		stat->slice_mask |= BIT(s);
>  
>  		if (IS_SKYLAKE(dev))
>  			ss_cnt = INTEL_INFO(dev)->sseu.subslice_per_slice;
> @@ -5052,18 +5052,18 @@ static void broadwell_sseu_device_status(struct drm_device *dev,
>  	int s;
>  	u32 slice_info = I915_READ(GEN8_GT_SLICE_INFO);
>  
> -	stat->slice_total = hweight32(slice_info & GEN8_LSLICESTAT_MASK);
> +	stat->slice_mask = slice_info & GEN8_LSLICESTAT_MASK;
>  
> -	if (stat->slice_total) {
> +	if (stat->slice_mask) {
>  		stat->subslice_per_slice =
>  				INTEL_INFO(dev)->sseu.subslice_per_slice;
> -		stat->subslice_total = stat->slice_total *
> +		stat->subslice_total = hweight32(stat->slice_mask) *
					^ hweight8?

>  				       stat->subslice_per_slice;
>  		stat->eu_per_subslice = INTEL_INFO(dev)->sseu.eu_per_subslice;
>  		stat->eu_total = stat->eu_per_subslice * stat->subslice_total;
>  
>  		/* subtract fused off EU(s) from enabled slice(s) */
> -		for (s = 0; s < stat->slice_total; s++) {
> +		for (s = 0; s < hweight32(stat->slice_mask); s++) {
				^ hweight8?

how about for_each_set_bit(s, stat->slice_mask, 8) {} ?

>  			u8 subslice_7eu = INTEL_INFO(dev)->sseu.subslice_7eu[s];
>  
>  			stat->eu_total -= hweight8(subslice_7eu);
> @@ -5077,7 +5077,7 @@ static void i915_print_sseu_info(struct seq_file *m, bool is_available_info,
>  	const char *type = is_available_info ? "Available" : "Enabled";
>  
>  	seq_printf(m, "  %s Slice Total: %u\n", type,
> -		   sseu->slice_total);
> +		   hweight32(sseu->slice_mask));
			^ hweight8?

>  	seq_printf(m, "  %s Subslice Total: %u\n", type,
>  		   sseu->subslice_total);
>  	seq_printf(m, "  %s Subslice Per Slice: %u\n", type,
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index be8e141..1f5f3a7d 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -559,7 +559,7 @@ static void cherryview_sseu_info_init(struct drm_device *dev)
>  	info = (struct sseu_dev_info *)&INTEL_INFO(dev_priv)->sseu;
>  	fuse = I915_READ(CHV_FUSE_GT);
>  
> -	info->slice_total = 1;
> +	info->slice_mask = BIT(0);
>  
>  	if (!(fuse & CHV_FGT_DISABLE_SS0)) {
>  		info->subslice_per_slice++;
> @@ -599,23 +599,21 @@ 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, s_enable, ss_disable, eu_disable;
> +	u32 fuse2, ss_disable, eu_disable;
>  	u8 eu_mask = 0xff;
>  
>  	info = (struct sseu_dev_info *)&INTEL_INFO(dev_priv)->sseu;
>  	fuse2 = I915_READ(GEN8_FUSE2);
> -	s_enable = (fuse2 & GEN8_F2_S_ENA_MASK) >>
> -		   GEN8_F2_S_ENA_SHIFT;
> +	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;
>  
> -	info->slice_total = hweight32(s_enable);
>  	/*
>  	 * 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 = info->slice_total *
> +	info->subslice_total = hweight32(info->slice_mask) *
				^ hweight8?
>  			       info->subslice_per_slice;
>  
>  	/*
> @@ -623,7 +621,7 @@ static void gen9_sseu_info_init(struct drm_device *dev)
>  	 * count the total enabled EU.
>  	*/
>  	for (s = 0; s < s_max; s++) {
> -		if (!(s_enable & (0x1 << s)))
> +		if (!(info->slice_mask & BIT(s)))

You could do for_each_set_bit again here too if you like.

>  			/* skip disabled slice */
>  			continue;
>  
> @@ -668,7 +666,8 @@ static void gen9_sseu_info_init(struct drm_device *dev)
>  	 * supports EU power gating on devices with more than one EU
>  	 * pair per subslice.
>  	*/
> -	info->has_slice_pg = (IS_SKYLAKE(dev) && (info->slice_total > 1));
> +	info->has_slice_pg = IS_SKYLAKE(dev) &&
> +			     hweight32(info->slice_mask) > 1;
				^ hweight8?

>  	info->has_subslice_pg = (IS_BROXTON(dev) && (info->subslice_total > 1));
>  	info->has_eu_pg = (info->eu_per_subslice > 2);
>  }
> @@ -679,10 +678,14 @@ 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], s_enable, ss_disable;
> +	u32 fuse2, eu_disable[s_max], ss_disable;
> +
> +	info = (struct sseu_dev_info *)&dev_priv->info.sseu;
>  
>  	fuse2 = I915_READ(GEN8_FUSE2);
> -	s_enable = (fuse2 & GEN8_F2_S_ENA_MASK) >> GEN8_F2_S_ENA_SHIFT;
> +
> +	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;
>  
>  	eu_disable[0] = I915_READ(GEN8_EU_DISABLE0) & GEN8_EU_DIS0_S0_MASK;
> @@ -694,23 +697,20 @@ static void broadwell_sseu_info_init(struct drm_device *dev)
>  			 (32 - GEN8_EU_DIS1_S2_SHIFT));
>  
>  
> -	info = (struct sseu_dev_info *)&INTEL_INFO(dev_priv)->sseu;
> -
> -	info->slice_total = hweight32(s_enable);
> -
>  	/*
>  	 * 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 = info->slice_total * info->subslice_per_slice;
> +	info->subslice_total = hweight32(info->slice_mask) *
				^ hweight8?
> +			       info->subslice_per_slice;
>  
>  	/*
>  	 * Iterate through enabled slices and subslices to
>  	 * count the total enabled EU.
>  	 */
>  	for (s = 0; s < s_max; s++) {
> -		if (!(s_enable & (0x1 << s)))
> +		if (!(info->slice_mask & BIT(s)))

(same about for_each set_bit)

>  			/* skip disabled slice */
>  			continue;
>  
> @@ -745,7 +745,7 @@ static void broadwell_sseu_info_init(struct drm_device *dev)
>  	 * BDW supports slice power gating on devices with more than
>  	 * one slice.
>  	 */
> -	info->has_slice_pg = (info->slice_total > 1);
> +	info->has_slice_pg = hweight32(info->slice_mask) > 1;
				^ hweight8?
>  	info->has_subslice_pg = 0;
>  	info->has_eu_pg = 0;
>  }
> @@ -825,7 +825,8 @@ static void intel_device_info_runtime_init(struct drm_device *dev)
>  	else if (INTEL_INFO(dev)->gen >= 9)
>  		gen9_sseu_info_init(dev);
>  
> -	DRM_DEBUG_DRIVER("slice total: %u\n", info->sseu.slice_total);
> +	DRM_DEBUG_DRIVER("slice total: %u\n",
> +			 hweight32(info->sseu.slice_mask));
				^ hweight8?
>  	DRM_DEBUG_DRIVER("subslice total: %u\n", info->sseu.subslice_total);
>  	DRM_DEBUG_DRIVER("subslice per slice: %u\n",
>  						info->sseu.subslice_per_slice);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index cb0427d..7cc9ec8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -783,7 +783,7 @@ struct intel_csr {
>  #define SEP_SEMICOLON ;
>  
>  struct sseu_dev_info {
> -	u8 slice_total;
> +	u8 slice_mask;
>  	u8 subslice_total;
>  	u8 subslice_per_slice;
>  	u8 eu_total;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 8a55f8a..4130ff1 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -2202,7 +2202,7 @@ make_rpcs(struct drm_device *dev)
>  	*/
>  	if (INTEL_INFO(dev)->sseu.has_slice_pg) {
>  		rpcs |= GEN8_RPCS_S_CNT_ENABLE;
> -		rpcs |= INTEL_INFO(dev)->sseu.slice_total <<
> +		rpcs |= hweight32(INTEL_INFO(dev)->sseu.slice_mask) <<
			^ hweight8?

>  			GEN8_RPCS_S_CNT_SHIFT;
>  		rpcs |= GEN8_RPCS_ENABLE;
>  	}

I'm not positive if hweight32 is actually okay on an 8bit type. I remember Ville
correcting me once on this, but I can't remember it's correct. Assuming
hweight32 is fine to use, with or without my recommendations, this is:
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