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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----Original Message-----
> From: Arun Siluvery [mailto:arun.siluvery@xxxxxxxxxxxxxxx]
> Sent: Wednesday, September 2, 2015 17:08
> To: Daniluk, Lukasz; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re:  [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@xxxxxxxxx>
> > Signed-off-by: Łukasz Daniluk <lukasz.daniluk@xxxxxxxxx>
> > ---
> >   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.

> 
> 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux