On Wed, Sep 27, 2017 at 12:16:20PM +0000, Daniel Vetter wrote:
> On Tue, Sep 26, 2017 at 01:06:22PM -0700, Rodrigo Vivi wrote:
> > On Fri, Sep 22, 2017 at 06:32:04PM +0000, Rodrigo Vivi wrote:
> > > On Fri, Sep 22, 2017 at 04:44:38PM +0000, Oscar Mateo wrote:
> > > >
> > > >
> > > > On 09/22/2017 06:15 AM, Rodrigo Vivi wrote:
> > > > > CNL adds an extra register for slice/subslice information.
> > > > > Although no SKU is planed with an extra slice let's already
> > > > > handle this extra piece of information so we don't have the
> > > > > risk in future of getting a part that might have chosen this
> > > > > part of the die instead of other slices or anything like that.
> > > > >
> > > > > Also if subslice is disabled the information of eu ack for that
> > > > > is garbage, so let's skip checks for eu if subslice is disabled
> > > > > as we skip the subslice if slice is disabled.
> > > > >
> > > > > The rest is pretty much like gen9.
> > > > >
> > > > > v2: Remove IS_CANNONLAKE from gen9 status function.
> > > > >
> > > > > v3: Consider s_max = 6 and ss_max=4 to run over all possible
> > > > > slices and subslices possible by spec. Although no real
> > > > > hardware will have that many slices/subslices.
> > > > > To match with sseu info init.
> > > > > v4: Fix offset calculation for slices 4 and 5.
> > > > > Removed Oscar's rv-b since this change also needs review.
> > > > >
> > > > > Cc: Oscar Mateo <oscar.mateo@xxxxxxxxx>
> > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > > > > ---
> > > > > drivers/gpu/drm/i915/i915_debugfs.c | 54 +++++++++++++++++++++++++++++++++++--
> > > > > drivers/gpu/drm/i915/i915_reg.h | 6 +++++
> > > > > 2 files changed, 58 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > index ca6fa6d122c6..e197e5d99277 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > @@ -4575,6 +4575,54 @@ static void cherryview_sseu_device_status(struct drm_i915_private *dev_priv,
> > > > > }
> > > > > }
> > > > > +static void gen10_sseu_device_status(struct drm_i915_private *dev_priv,
> > > > > + struct sseu_dev_info *sseu)
> > > > > +{
> > > > > + const struct intel_device_info *info = INTEL_INFO(dev_priv);
> > > > > + int s_max = 6, ss_max = 4;
> > > > > + int s, ss;
> > > > > + u32 s_reg[s_max], eu_reg[2 * s_max], eu_mask[2];
> > > > > +
> > > > > + for (s = 0; s < s_max; s++) {
> > > > > + s_reg[s] = I915_READ(GEN10_SLICE_PGCTL_ACK(s));
> > > > > + eu_reg[2 * s] = I915_READ(GEN10_SS01_EU_PGCTL_ACK(s));
> > > > > + eu_reg[2 * s + 1] = I915_READ(GEN10_SS23_EU_PGCTL_ACK(s));
> > > > > + }
> > > > > +
> > > > > + eu_mask[0] = GEN9_PGCTL_SSA_EU08_ACK |
> > > > > + GEN9_PGCTL_SSA_EU19_ACK |
> > > > > + GEN9_PGCTL_SSA_EU210_ACK |
> > > > > + GEN9_PGCTL_SSA_EU311_ACK;
> > > > > + eu_mask[1] = GEN9_PGCTL_SSB_EU08_ACK |
> > > > > + GEN9_PGCTL_SSB_EU19_ACK |
> > > > > + GEN9_PGCTL_SSB_EU210_ACK |
> > > > > + GEN9_PGCTL_SSB_EU311_ACK;
> > > > > +
> > > > > + for (s = 0; s < s_max; s++) {
> > > > > + if ((s_reg[s] & GEN9_PGCTL_SLICE_ACK) == 0)
> > > > > + /* skip disabled slice */
> > > > > + continue;
> > > > > +
> > > > > + sseu->slice_mask |= BIT(s);
> > > > > + sseu->subslice_mask = info->sseu.subslice_mask;
> > > > > +
> > > > > + for (ss = 0; ss < ss_max; ss++) {
> > > > > + unsigned int eu_cnt;
> > > > > +
> > > > > + if (!(s_reg[s] & (GEN9_PGCTL_SS_ACK(ss))))
> > > > > + /* skip disabled subslice */
> > > > > + continue;
> > > >
> > > > You are going to hate me, but I found something else:
> > >
> > > Should I hate you for being a good reviewer? ;)
> > > You should hate me for not noticing that before...
> > > Thanks a lot for the patience
> > >
> > > >
> > > > SLICE0_PGCTL_ACK has powergate acknowledge bits for subslices 0, 1 & 2, but
> > > > not for subslice 3
> > > > SLICEn_PGCTL_ACK (where n = 1-5) has powergate acknowledge bits for
> > > > subslices 0 & 1, but not for subslices 2 & 3
> > >
> > > hmmm... :(
> > > I will check...
> > >
> > > >
> > > > I have no idea where the missing bits went (maybe the BSpec is wrong?).
> > >
> > > Do you know anyone at your end that could help us to clarify that?
> >
> > Based on the emails it seems that it is a spec bug. But we are
> > not going to merge this patch 2/2 while we don't get the official confirmation.
> >
> > Meanwhile we need the first patch for userspace, so
> > I merged the first patch on dinq while we solve this mistery.
>
> Not sure I remember correctly, but iirc the userspace for this was
> media/compute. If that's correct, do we have this open-sourced? Otherwise
> we need to add this to the list of things we might need to revert :-/
I think initially was only media, but nowadays Mesa is using it.
This was the last think blocking them to fully move from the
internal to drm-tip.
Thanks,
Rodrigo.
> -Daniel
>
> >
> > Thanks for all the reviews.
> >
> > >
> > > Thanks,
> > > Rodrigo.
> > >
> > > >
> > > > > +
> > > > > + eu_cnt = 2 * hweight32(eu_reg[2 * s + ss / 2] &
> > > > > + eu_mask[ss % 2]);
> > > > > + sseu->eu_total += eu_cnt;
> > > > > + sseu->eu_per_subslice = max_t(unsigned int,
> > > > > + sseu->eu_per_subslice,
> > > > > + eu_cnt);
> > > > > + }
> > > > > + }
> > > > > +}
> > > > > +
> > > > > static void gen9_sseu_device_status(struct drm_i915_private *dev_priv,
> > > > > struct sseu_dev_info *sseu)
> > > > > {
> > > > > @@ -4610,7 +4658,7 @@ static void gen9_sseu_device_status(struct drm_i915_private *dev_priv,
> > > > > sseu->slice_mask |= BIT(s);
> > > > > - if (IS_GEN9_BC(dev_priv) || IS_CANNONLAKE(dev_priv))
> > > > > + if (IS_GEN9_BC(dev_priv))
> > > > > sseu->subslice_mask =
> > > > > INTEL_INFO(dev_priv)->sseu.subslice_mask;
> > > > > @@ -4716,8 +4764,10 @@ static int i915_sseu_status(struct seq_file *m, void *unused)
> > > > > cherryview_sseu_device_status(dev_priv, &sseu);
> > > > > } else if (IS_BROADWELL(dev_priv)) {
> > > > > broadwell_sseu_device_status(dev_priv, &sseu);
> > > > > - } else if (INTEL_GEN(dev_priv) >= 9) {
> > > > > + } else if (IS_GEN9(dev_priv)) {
> > > > > gen9_sseu_device_status(dev_priv, &sseu);
> > > > > + } else if (INTEL_GEN(dev_priv) >= 10) {
> > > > > + gen10_sseu_device_status(dev_priv, &sseu);
> > > > > }
> > > > > intel_runtime_pm_put(dev_priv);
> > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > > index 1c257797c583..9729145e6c03 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > @@ -8020,11 +8020,17 @@ enum {
> > > > > #define CHV_EU311_PG_ENABLE (1<<1)
> > > > > #define GEN9_SLICE_PGCTL_ACK(slice) _MMIO(0x804c + (slice)*0x4)
> > > > > +#define GEN10_SLICE_PGCTL_ACK(slice) _MMIO(0x804c + ((slice) / 3) * 0x34 + \
> > > > > + ((slice) % 3) * 0x4)
> > > > > #define GEN9_PGCTL_SLICE_ACK (1 << 0)
> > > > > #define GEN9_PGCTL_SS_ACK(subslice) (1 << (2 + (subslice)*2))
> > > > > #define GEN9_SS01_EU_PGCTL_ACK(slice) _MMIO(0x805c + (slice)*0x8)
> > > > > +#define GEN10_SS01_EU_PGCTL_ACK(slice) _MMIO(0x805c + ((slice) / 3) * 0x30 + \
> > > > > + ((slice) % 3) * 0x8)
> > > > > #define GEN9_SS23_EU_PGCTL_ACK(slice) _MMIO(0x8060 + (slice)*0x8)
> > > > > +#define GEN10_SS23_EU_PGCTL_ACK(slice) _MMIO(0x8060 + ((slice) / 3) * 0x30 + \
> > > > > + ((slice) % 3) * 0x8)
> > > > > #define GEN9_PGCTL_SSA_EU08_ACK (1 << 0)
> > > > > #define GEN9_PGCTL_SSA_EU19_ACK (1 << 2)
> > > > > #define GEN9_PGCTL_SSA_EU210_ACK (1 << 4)
> > > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
