Re: [PATCH v2] drm/i915/bdw: Check for slice, subslice and EU count for BDW

[Arun Siluvery <arun.siluvery@linux.intel.com>]

On 08/09/2015 13:53, Daniluk, Lukasz wrote:
>> -----Original Message-----
>> From: Arun Siluvery [mailto:arun.siluvery@linux.intel.com]
>> Sent: Wednesday, September 2, 2015 17:08
>> To: Daniluk, Lukasz; intel-gfx@lists.freedesktop.org
>> Subject: Re: [Intel-gfx] [PATCH v2] drm/i915/bdw: Check for slice, subslice and
>> EU count for BDW
>>
>> On 02/09/2015 16:47, Łukasz Daniluk wrote:
>>> Added checks for available slices, subslices and EUs for Broadwell.
>>> This information is filled in intel_device_info and is available to
>>> user with GET_PARAM.
>>> Added checks for enabled slices, subslices and EU for Broadwell. This
>>> information is based on available counts but takes power gated slices
>>> into account. It can be read in debugfs.
>>> Introduce new register defines that contain information on slices on
>>> Broadwell.
>>>
>>> v2:
>>> - Introduce GT_SLICE_INFO register
>>> - Change Broadwell sseu_device_status function to use GT_SLICE_INFO
>>>     register instead of RPCS register
>>> - Undo removal of dev_priv variables in Cherryview and Gen9
>>>     sseu_device_satus functions
>>>
>>> Cc: Jeff Mcgee <jeff.mcgee@intel.com>
>>> Signed-off-by: Łukasz Daniluk <lukasz.daniluk@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/i915_debugfs.c | 29 +++++++++++-
>>>    drivers/gpu/drm/i915/i915_dma.c     | 89
>> +++++++++++++++++++++++++++++++++++++
>>>    drivers/gpu/drm/i915/i915_reg.h     | 22 ++++++++-
>>>    3 files changed, 137 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>>> b/drivers/gpu/drm/i915/i915_debugfs.c
>>> index 23a69307..e8a62ef 100644
>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>> @@ -4932,13 +4932,38 @@ static void gen9_sseu_device_status(struct
>> drm_device *dev,
>>>    	}
>>>    }
>>>
>>> +static void broadwell_sseu_device_status(struct drm_device *dev,
>>> +					 struct sseu_dev_status *stat)
>>> +{
>>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>> +	int s;
>>> +	u32 slice_info = I915_READ(GEN8_GT_SLICE_INFO);
>> I don't see this register (0x138064 as defined below) in bdw spec.
>>
>
> Yes, this register isn't listed in spec. I am unsure when the full spec for this register
> will be published.
>
>>> +
>>> +	stat->slice_total =
>>> +		hweight32(slice_info & GEN8_LSLICESTAT_MASK);
>>
>> why not in a single line, seems to fit under 80 chars.
>>
>
> I was using longer define name and didn't recheck if this line fits under 80. Thanks for catch.
>
>>> +
>>> +	if (stat->slice_total)
>>> +	{
>>> +		stat->subslice_per_slice = INTEL_INFO(dev)->subslice_per_slice;
>>> +		stat->eu_per_subslice = INTEL_INFO(dev)->eu_per_subslice;
>>> +		stat->subslice_total = stat->slice_total * stat-
>>> subslice_per_slice;
>>> +		stat->eu_total = stat->eu_per_subslice * stat->subslice_total;
>>> +
>> Please reorder such that all subslice values are populated first followed by EUs
>> to follow an order.
>>
>
> Okay. My idea was to order them based on where value comes from (is it computed here or
> simply read with INTEL_INFO).
>
>>> +		/* subtract fused off EU(s) from enabled slice(s) */
>>> +		for (s = 0; s < stat->slice_total; s++) {
>>> +			u8 subslice_7eu = INTEL_INFO(dev)->subslice_7eu[s];
>>> +			stat->eu_total -= hweight8(subslice_7eu);
>>> +		}
>>> +	}
>>> +}
>>> +
>>>    static int i915_sseu_status(struct seq_file *m, void *unused)
>>>    {
>>>    	struct drm_info_node *node = (struct drm_info_node *) m->private;
>>>    	struct drm_device *dev = node->minor->dev;
>>>    	struct sseu_dev_status stat;
>>>
>>> -	if ((INTEL_INFO(dev)->gen < 8) || IS_BROADWELL(dev))
>>> +	if (INTEL_INFO(dev)->gen < 8)
>>>    		return -ENODEV;
>>>
>>>    	seq_puts(m, "SSEU Device Info\n");
>>> @@ -4963,6 +4988,8 @@ static int i915_sseu_status(struct seq_file *m, void
>> *unused)
>>>    	memset(&stat, 0, sizeof(stat));
>>>    	if (IS_CHERRYVIEW(dev)) {
>>>    		cherryview_sseu_device_status(dev, &stat);
>>> +	} else if (IS_BROADWELL(dev)) {
>>> +		broadwell_sseu_device_status(dev, &stat);
>>>    	} else if (INTEL_INFO(dev)->gen >= 9) {
>>>    		gen9_sseu_device_status(dev, &stat);
>>>    	}
>>> diff --git a/drivers/gpu/drm/i915/i915_dma.c
>>> b/drivers/gpu/drm/i915/i915_dma.c index ab37d11..2d52b1e 100644
>>> --- a/drivers/gpu/drm/i915/i915_dma.c
>>> +++ b/drivers/gpu/drm/i915/i915_dma.c
>>> @@ -705,6 +705,93 @@ static void gen9_sseu_info_init(struct drm_device
>> *dev)
>>>    	info->has_eu_pg = (info->eu_per_subslice > 2);
>>>    }
>>>
>>> +static void broadwell_sseu_info_init(struct drm_device *dev) {
>>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>> +	struct intel_device_info *info;
>>> +	const int s_max = 3, ss_max = 3, eu_max = 8;
>> I only see max of 2 slices for bdw in spec.
>>
>
> Register values assume that there can be 3 slices (slice 0,1 and 2) and that’s why
> I used 3 here as max slices count.
>
>>> +	int s, ss;
>>> +	u32 fuse2, eu_disable[s_max], s_enable, ss_disable;
>>> +
>>> +	fuse2 = I915_READ(GEN8_FUSE2);
>>> +	s_enable = (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;
>>> +	eu_disable[1] = (I915_READ(GEN8_EU_DISABLE0) >>
>>> +			GEN8_EU_DIS0_S1_SHIFT) |
>>> +		((I915_READ(GEN8_EU_DISABLE1) &
>>> +		  GEN8_EU_DIS1_S1_MASK) <<
>>> +		 (32 - GEN8_EU_DIS0_S1_SHIFT));
>>> +	eu_disable[2] = (I915_READ(GEN8_EU_DISABLE1) >>
>>> +			GEN8_EU_DIS1_S2_SHIFT) |
>>> +		((I915_READ(GEN8_EU_DISABLE2) &
>>> +		  GEN8_EU_DIS2_S2_MASK) <<
>>> +		 (32 - GEN8_EU_DIS1_S2_SHIFT));
>>> +
>>
>> GEN9_EU_DISABLE(slice) is already available, since the register addresses are
>> same maybe it can be reused after renaming it to GEN8 instead of creating new
>> definitions? ofcourse shift/mask are different.
>
> I wouldn't use renaming of register from other generation. Since contents of this register
> are read in different way I treated these as different registers and that’s why I choose
> to create new define without any references to GEN9 register. However, if the renaming
> (or even using existing define) is preferred approach I will modify the patch.

Normally we create a new define for newer Gen if the existing one cannot 
be reused, in this case it is the opposite, I am not sure what is best 
way to handle this, better to check with Daniel.

regards
Arun

>
>>
>> Indentation is off, in general please use () to force indentation.
>> first two statements can fit in single line.
>>
>> regards
>> Arun
>>
>
> Thank you for questions and style suggestions. I will reformat the lines before submitting
> updated patch.
>
> ---
> Łukasz Daniluk
>
>>> +
>>> +	info = (struct intel_device_info *)&dev_priv->info;
>>> +	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;
>>> +
>>> +	/*
>>> +	 * Iterate through enabled slices and subslices to
>>> +	 * count the total enabled EU.
>>> +	 */
>>> +	for (s = 0; s < s_max; s++) {
>>> +		if (!(s_enable & (0x1 << s)))
>>> +			/* skip disabled slice */
>>> +			continue;
>>> +
>>> +		for (ss = 0; ss < ss_max; ss++) {
>>> +			u32 n_disabled;
>>> +
>>> +			if (ss_disable & (0x1 << ss))
>>> +				/* skip disabled subslice */
>>> +				continue;
>>> +
>>> +			n_disabled = hweight8(eu_disable[s] >>
>>> +					(ss * eu_max));
>>> +
>>> +			/*
>>> +			 * Record which subslice(s) has(have) 7 EUs. we
>>> +			 * can tune the hash used to spread work among
>>> +			 * subslices if they are unbalanced.
>>> +			 */
>>> +			if (eu_max - n_disabled == 7)
>>> +				info->subslice_7eu[s] |= 1 << ss;
>>> +
>>> +			info->eu_total += eu_max - n_disabled;
>>> +		}
>>> +	}
>>> +
>>> +	/*
>>> +	 * BDW is expected to always have a uniform distribution of EU across
>>> +	 * 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;
>>> +
>>> +	/*
>>> +	 * BDW supports slice power gating on devices with more than
>>> +	 * one slice.
>>> +	 */
>>> +	info->has_slice_pg = (info->slice_total > 1);
>>> +	info->has_subslice_pg = 0;
>>> +	info->has_eu_pg = 0;
>>> +}
>>> +
>>>    /*
>>>     * Determine various intel_device_info fields at runtime.
>>>     *
>>> @@ -775,6 +862,8 @@ static void intel_device_info_runtime_init(struct
>> drm_device *dev)
>>>    	/* Initialize slice/subslice/EU info */
>>>    	if (IS_CHERRYVIEW(dev))
>>>    		cherryview_sseu_info_init(dev);
>>> +	else if (IS_BROADWELL(dev))
>>> +		broadwell_sseu_info_init(dev);
>>>    	else if (INTEL_INFO(dev)->gen >= 9)
>>>    		gen9_sseu_info_init(dev);
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>>> b/drivers/gpu/drm/i915/i915_reg.h index be87e3b..37f658f 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -1841,11 +1841,26 @@ enum skl_disp_power_wells {
>>>    #define   CHV_FGT_EU_DIS_SS1_R1_MASK	(0xf <<
>> CHV_FGT_EU_DIS_SS1_R1_SHIFT)
>>>
>>>    #define GEN8_FUSE2			0x9120
>>> +#define   GEN8_F2_SS_DIS_SHIFT		21
>>> +#define   GEN8_F2_SS_DIS_MASK		(0x7 <<
>> GEN8_F2_SS_DIS_SHIFT)
>>>    #define   GEN8_F2_S_ENA_SHIFT		25
>>>    #define   GEN8_F2_S_ENA_MASK		(0x7 <<
>> GEN8_F2_S_ENA_SHIFT)
>>>
>>> -#define   GEN9_F2_SS_DIS_SHIFT		20
>>> -#define   GEN9_F2_SS_DIS_MASK		(0xf <<
>> GEN9_F2_SS_DIS_SHIFT)
>>> +#define GEN8_EU_DISABLE0		0x9134
>>> +#define   GEN8_EU_DIS0_S0_MASK		0xffffff
>>> +#define   GEN8_EU_DIS0_S1_SHIFT		24
>>> +#define   GEN8_EU_DIS0_S1_MASK		(0xff <<
>> GEN8_EU_DIS0_S1_SHIFT)
>>> +
>>> +#define GEN8_EU_DISABLE1		0x9138
>>> +#define   GEN8_EU_DIS1_S1_MASK		0xffff
>>> +#define   GEN8_EU_DIS1_S2_SHIFT		16
>>> +#define   GEN8_EU_DIS1_S2_MASK		(0xffff <<
>> GEN8_EU_DIS1_S2_SHIFT)
>>> +
>>> +#define GEN8_EU_DISABLE2		0x913c
>>> +#define   GEN8_EU_DIS2_S2_MASK		0xff
>>> +
>>> +#define GEN9_F2_SS_DIS_SHIFT		20
>>> +#define GEN9_F2_SS_DIS_MASK		(0xf <<
>> GEN9_F2_SS_DIS_SHIFT)
>>>
>>>    #define GEN9_EU_DISABLE(slice)		(0x9134 + (slice)*0x4)
>>>
>>> @@ -6822,6 +6837,9 @@ enum skl_disp_power_wells {
>>>    #define   GEN6_RC6			3
>>>    #define   GEN6_RC7			4
>>>
>>> +#define GEN8_GT_SLICE_INFO		0x138064
>>> +#define   GEN8_LSLICESTAT_MASK		0x7
>>> +
>>>    #define CHV_POWER_SS0_SIG1		0xa720
>>>    #define CHV_POWER_SS1_SIG1		0xa728
>>>    #define   CHV_SS_PG_ENABLE		(1<<1)
>>>
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx