[Intel-gfx] [PATCH 5/5] drm/i915: Try to print INSTDONE bits for all slice/subslice
Ben Widawsky
benjamin.widawsky at intel.com
Fri Oct 2 08:52:48 PDT 2015
On Fri, Oct 02, 2015 at 01:58:13PM +0300, Imre Deak wrote:
> On Thu, 2015-10-01 at 16:00 -0700, Ben Widawsky wrote:
> > On Wed, Sep 30, 2015 at 11:00:46PM +0300, Imre Deak wrote:
> > > From: Ben Widawsky <benjamin.widawsky at intel.com>
> > >
> > > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> > >
> > > ---
> > > Changes (Imre):
> > > - use the new INSTDONE capturing by default on new GENs (On Ben's request)
> > > - keep printing the render ring INSTDONE to dmesg
> > > - don't hard code the extra_instdone array sizes
> > > - fix typo in GEN8_MCR_SLICE/GEN8_MCR_SUBSLICE
> > > - fix typo when capturing to extra->row
> > > - warn if the MCR selectors are non-zero
> > > ---
> > > drivers/gpu/drm/i915/i915_drv.h | 6 ++--
> > > drivers/gpu/drm/i915/i915_gpu_error.c | 62 +++++++++++++++++++++++++++++++----
> > > drivers/gpu/drm/i915/i915_irq.c | 4 +--
> > > drivers/gpu/drm/i915/i915_reg.h | 5 +++
> > > 4 files changed, 66 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 621acf1..d1b4011 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -513,10 +513,12 @@ struct drm_i915_error_state {
> > > u32 gam_ecochk;
> > > u32 gab_ctl;
> > > u32 gfx_mode;
> > > +#define INSTDONE_SLICE_NUM 3
> > > +#define INSTDONE_SUBSLICE_NUM 3
> > > struct extra_instdone {
> > > u32 slice_common;
> > > - u32 sampler;
> > > - u32 row;
> > > + u32 sampler[INSTDONE_SLICE_NUM][INSTDONE_SUBSLICE_NUM];
> > > + u32 row[INSTDONE_SLICE_NUM][INSTDONE_SUBSLICE_NUM];
> > > } extra_instdone;
> > >
> > > u64 fence[I915_MAX_NUM_FENCES];
> > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> > > index e78e512..c6d1cbc 100644
> > > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > > @@ -336,7 +336,7 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
> > > struct drm_i915_private *dev_priv = dev->dev_private;
> > > struct drm_i915_error_state *error = error_priv->error;
> > > struct drm_i915_error_object *obj;
> > > - int i, j, offset, elt;
> > > + int i, j, slice, subslice, offset, elt;
> > > int max_hangcheck_score;
> > >
> > > if (!error) {
> > > @@ -385,9 +385,17 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
> > >
> > > err_printf(m, " SC_INSTDONE (slice common): 0x%08x\n",
> > > error->extra_instdone.slice_common);
> > > - err_printf(m, " SAMPLER_INTSDONE: 0x%08x\n",
> > > - error->extra_instdone.sampler);
> > > - err_printf(m, " ROW_INSTDONE: 0x%08x\n", error->extra_instdone.row);
> > > + for (slice = 0; slice < INSTDONE_SLICE_NUM; slice++) {
> >
> > Ideally we should only be doing this for the active slices.
>
> Looking at it now the spec is not too clear to me:
> """
> When slice 0 is disabled (when fuse reflection MMADR 0x9120[25] = 0),
> this field must be set to a valid slice (slice 1 or slice 2) before
> issuing a read to a register in a slice unit.
> """
>
The default is to go to slice 0. So if you have slice 0 disabled (which I
believe only exists in special fused configs) you will end up reading garbage on
the MMIO. This is true of all MMIO, not just INSTDONE. In other words, if we
actually have such things shipping, they surely aren't working today.
> Why is slice 0 special? I would understand the above if it just said
> "the field must be set to a valid slice", without mentioning specific
> slices.
>
> But yes, I think it would be safer to access only active slices. I would
> also limit the readout to active subslices, although there is no similar
> note about it in bspec.
>
> > I was hoping you'd have a chance to look into that.
>
> Ok, I'll resend this patch with the above changed.
>
> > Do we know if this works on SKUs with fewer slices?
>
> It worked on CHV and BXT.
>
> > > + for (subslice = 0; subslice < INSTDONE_SUBSLICE_NUM;
> > > + subslice++) {
> > > + struct extra_instdone *extra = &error->extra_instdone;
> > > +
> > > + err_printf(m, " SAMPLER_INTSDONE: 0x%08x\n",
> > > + extra->sampler[slice][subslice]);
> > > + err_printf(m, " ROW_INSTDONE: 0x%08x\n",
> > > + extra->row[slice][subslice]);
> > > + }
> > > + }
> > >
> > > if (INTEL_INFO(dev)->gen >= 6) {
> > > err_printf(m, "ERROR: 0x%08x\n", error->error);
> > > @@ -1383,11 +1391,35 @@ const char *i915_cache_level_str(struct drm_i915_private *i915, int type)
> > > }
> > > }
> > >
> > > -/* NB: please notice the memset */
> > > +static inline uint32_t instdone_read(struct drm_i915_private *dev_priv, int
> > > + slice, int subslice, uint32_t offset) {
> > > + /*
> > > + * XXX: The MCR register should be locked, but since we are only using
> > > + * it for debug/error state, it's not terribly important to
> > > + * synchronize it properly.
> > > + */
> > > + uint32_t tmp = I915_READ(GEN8_MCR_SELECTOR);
> > > + uint32_t ret;
> > > +
> > > + /*
> > > + * The HW expects the slice and sublice selectors to be reset to 0
> > > + * after reading out the registers.
> > > + */
> > > + WARN_ON_ONCE(tmp & (GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK));
> > > +
> > > + I915_WRITE(GEN8_MCR_SELECTOR, tmp | GEN8_MCR_SLICE(slice) |
> > > + GEN8_MCR_SUBSLICE(subslice));
> > > + ret = I915_READ(offset);
> > > + I915_WRITE(GEN8_MCR_SELECTOR, tmp);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > void i915_get_extra_instdone(struct drm_device *dev,
> > > struct extra_instdone *extra)
> > > {
> > > struct drm_i915_private *dev_priv = dev->dev_private;
> > > + int slice, subslice;
> > >
> > > /*
> > > * The render INSTDONE register (GEN2_INSTDONE, RING_INSTDONE) is read
> > > @@ -1396,8 +1428,24 @@ void i915_get_extra_instdone(struct drm_device *dev,
> > > switch (INTEL_INFO(dev)->gen) {
> > > default:
> > > extra->slice_common = I915_READ(GEN7_SC_INSTDONE);
> > > - extra->sampler = I915_READ(GEN7_SAMPLER_INSTDONE);
> > > - extra->row = I915_READ(GEN7_ROW_INSTDONE);
> > > + for (slice = 0; slice < INSTDONE_SLICE_NUM; slice++) {
> > > + for (subslice = 0; subslice < INSTDONE_SUBSLICE_NUM;
> > > + subslice++) {
> > > + extra->sampler[slice][subslice] =
> > > + instdone_read(dev_priv,
> > > + slice, subslice,
> > > + GEN7_SAMPLER_INSTDONE);
> > > + extra->row[slice][subslice] =
> > > + instdone_read(dev_priv,
> > > + slice, subslice,
> > > + GEN7_ROW_INSTDONE);
> > > + }
> > > + }
> > > + break;
> > > + case 7:
> > > + extra->slice_common = I915_READ(GEN7_SC_INSTDONE);
> > > + extra->sampler[0][0] = I915_READ(GEN7_SAMPLER_INSTDONE);
> > > + extra->row[0][0] = I915_READ(GEN7_ROW_INSTDONE);
> > > break;
> > > case 6:
> > > case 5:
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > index 8a3dc73..3cfcd1f 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -2478,8 +2478,8 @@ i915_err_print_instdone(uint32_t render, struct extra_instdone *extra)
> > > {
> > > pr_err(" INSTDONE (render): 0x%08x\n", render);
> > > pr_err(" INSTDONE (common): 0x%08x\n", extra->slice_common);
> > > - pr_err(" INSTDONE (sampler): 0x%08x\n", extra->sampler);
> > > - pr_err(" INSTDONE (row): 0x%08x\n", extra->row);
> > > + pr_err(" INSTDONE (sampler): 0x%08x\n", extra->sampler[0][0]);
> > > + pr_err(" INSTDONE (row): 0x%08x\n", extra->row[0][0]);
> > > }
> > >
> > > static void i915_report_and_clear_eir(struct drm_device *dev)
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index ac5f49a..2412ec7 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -1596,6 +1596,11 @@ enum skl_disp_power_wells {
> > > #define GEN7_SC_INSTDONE 0x07100
> > > #define GEN7_SAMPLER_INSTDONE 0x0e160
> > > #define GEN7_ROW_INSTDONE 0x0e164
> > > +#define GEN8_MCR_SELECTOR 0xfdc
> > > +#define GEN8_MCR_SLICE(slice) (((slice) & 3) << 26)
> > > +#define GEN8_MCR_SLICE_MASK (GEN8_MCR_SLICE(3))
> > > +#define GEN8_MCR_SUBSLICE(slice) (((slice) & 3) << 24)
> > > +#define GEN8_MCR_SUBSLICE_MASK (GEN8_MCR_SUBSLICE(3))
> > > #define RING_IPEIR(base) ((base)+0x64)
> > > #define RING_IPEHR(base) ((base)+0x68)
> > > /*
> > > --
> > > 2.1.4
> > >
> >
>
>
--
Ben Widawsky, Intel Open Source Technology Center
More information about the Intel-gfx
mailing list