> -----Original Message-----
> From: Arun Siluvery [mailto:[email protected]]
> Sent: Wednesday, September 2, 2015 17:08
> To: Daniluk, Lukasz; [email protected]
> 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 <[email protected]>
> > Signed-off-by: Łukasz Daniluk <[email protected]>
> > ---
> > 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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
