> -----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
